diff mbox series

[RFC,05/12] mm/gup: Fix follow_devmap_p[mu]d() to return even if NULL

Message ID 20231116012908.392077-6-peterx@redhat.com
State New
Headers
Series mm/gup: Unify hugetlb, part 2 |

Commit Message

Peter Xu Nov. 16, 2023, 1:29 a.m. UTC 
  This seems to be a bug not by any report but by code observations.

When GUP sees a devpmd or devpud, it should return whatever value returned
from follow_devmap_p[mu]d().  If page==NULL returned, it means a fault is
probably required.  Skipping return the NULL should allow the code to fall
through, which can cause unexpected behavior.

It was there at least before the follow page rework (080dbb618b) in 2017,
so 6 years.  Not yet digging for a Fixes, assuming it can hardly trigger
even if the logical bug does exist.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 mm/gup.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Christoph Hellwig Nov. 23, 2023, 7:25 a.m. UTC | #1 
On Wed, Nov 15, 2023 at 08:29:01PM -0500, Peter Xu wrote:
> This seems to be a bug not by any report but by code observations.
> 
> When GUP sees a devpmd or devpud, it should return whatever value returned
> from follow_devmap_p[mu]d().  If page==NULL returned, it means a fault is
> probably required.  Skipping return the NULL should allow the code to fall
> through, which can cause unexpected behavior.
> 
> It was there at least before the follow page rework (080dbb618b) in 2017,
> so 6 years.  Not yet digging for a Fixes, assuming it can hardly trigger
> even if the logical bug does exist.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

I'd still add a fixes tag and send if off to Linux for 6.7-rc instead
of letting it linger.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Peter Xu Nov. 23, 2023, 5:59 p.m. UTC | #2 
On Wed, Nov 22, 2023 at 11:25:28PM -0800, Christoph Hellwig wrote:
> On Wed, Nov 15, 2023 at 08:29:01PM -0500, Peter Xu wrote:
> > This seems to be a bug not by any report but by code observations.
> > 
> > When GUP sees a devpmd or devpud, it should return whatever value returned
> > from follow_devmap_p[mu]d().  If page==NULL returned, it means a fault is
> > probably required.  Skipping return the NULL should allow the code to fall
> > through, which can cause unexpected behavior.
> > 
> > It was there at least before the follow page rework (080dbb618b) in 2017,
> > so 6 years.  Not yet digging for a Fixes, assuming it can hardly trigger
> > even if the logical bug does exist.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> I'd still add a fixes tag and send if off to Linux for 6.7-rc instead
> of letting it linger.

Will do.

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

When rethinking, "return page" is correct for devmap, but it should be
better to always use no_page_table() instead for page==NULL when trying to
return NULL for follow_page(), even if no_page_table() should constantly
return NULL for devmap, afaict, so it should be the same.

So I'll make it slightly different when reposting separately, please feel
free to have another look.

Thanks,
diff mbox series

Patch

diff --git a/mm/gup.c b/mm/gup.c
index a8b73a8289ad..0e00204761d2 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -708,8 +708,7 @@  static struct page *follow_pmd_mask(struct vm_area_struct *vma,
 		ptl = pmd_lock(mm, pmd);
 		page = follow_devmap_pmd(vma, address, pmd, flags, &ctx->pgmap);
 		spin_unlock(ptl);
-		if (page)
-			return page;
+		return page;
 	}
 	if (likely(!pmd_trans_huge(pmdval)))
 		return follow_page_pte(vma, address, pmd, flags, &ctx->pgmap);
@@ -756,8 +755,7 @@  static struct page *follow_pud_mask(struct vm_area_struct *vma,
 		ptl = pud_lock(mm, pud);
 		page = follow_devmap_pud(vma, address, pud, flags, &ctx->pgmap);
 		spin_unlock(ptl);
-		if (page)
-			return page;
+		return page;
 	}
 	if (unlikely(pud_bad(*pud)))
 		return no_page_table(vma, flags);