Re: [PATCH v2 09/13] mm/gup: Cache *pudp in follow_pud_mask()

[Jason Gunthorpe]

On Wed, Jan 03, 2024 at 05:14:19PM +0800, pet...@redhat.com wrote:
> From: Peter Xu 
> 
> Introduce "pud_t pud" in the function, so the code won't dereference *pudp
> multiple time.  Not only because that looks less straightforward, but also
> because if the dereference really happened, it's not clear whether there
> can be race to see different *pudp values if it's being modified at the
> same time.
> 
> Acked-by: James Houghton 
> Signed-off-by: Peter Xu 
> ---
>  mm/gup.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)

I think we have several more case like this, and I ceratinly agree
code should not access a READ_ONCE variable more than once :(

Reviewed-by: Jason Gunthorpe 

Jason

[PATCH v2 09/13] mm/gup: Cache *pudp in follow_pud_mask()

[peterx]

From: Peter Xu 

Introduce "pud_t pud" in the function, so the code won't dereference *pudp
multiple time.  Not only because that looks less straightforward, but also
because if the dereference really happened, it's not clear whether there
can be race to see different *pudp values if it's being modified at the
same time.

Acked-by: James Houghton 
Signed-off-by: Peter Xu 
---
 mm/gup.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index b8a80e2bfe08..63845b3ec44f 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -753,26 +753,27 @@ static struct page *follow_pud_mask(struct vm_area_struct 
*vma,
unsigned int flags,
struct follow_page_context *ctx)
 {
-   pud_t *pud;
+   pud_t *pudp, pud;
spinlock_t *ptl;
struct page *page;
struct mm_struct *mm = vma->vm_mm;
 
-   pud = pud_offset(p4dp, address);
-   if (pud_none(*pud))
+   pudp = pud_offset(p4dp, address);
+   pud = READ_ONCE(*pudp);
+   if (pud_none(pud))
return no_page_table(vma, flags, address);
-   if (pud_devmap(*pud)) {
-   ptl = pud_lock(mm, pud);
-   page = follow_devmap_pud(vma, address, pud, flags, >pgmap);
+   if (pud_devmap(pud)) {
+   ptl = pud_lock(mm, pudp);
+   page = follow_devmap_pud(vma, address, pudp, flags, 
>pgmap);
spin_unlock(ptl);
if (page)
return page;
return no_page_table(vma, flags, address);
}
-   if (unlikely(pud_bad(*pud)))
+   if (unlikely(pud_bad(pud)))
return no_page_table(vma, flags, address);
 
-   return follow_pmd_mask(vma, address, pud, flags, ctx);
+   return follow_pmd_mask(vma, address, pudp, flags, ctx);
 }
 
 static struct page *follow_p4d_mask(struct vm_area_struct *vma,
-- 
2.41.0