[PATCH v1 0/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[PATCH v1 0/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[David Hildenbrand]

Working on another generic page table walker that tries to avoid
special-casing hugetlb, I found a page table locking issue with hugetlb
folios that are not mapped using a single PMD/PUD.

For some hugetlb folio sizes, GUP will take different page table locks
when walking the page tables than hugetlb when modifying the page tables.

I did not actually try reproducing an issue, but looking at
follow_pmd_mask() where we might be rereading a PMD value multiple times
it's rather clear that concurrent modifications are rather unpleasant.

In follow_page_pte() we might be better in that regard -- ptep_get() does
a READ_ONCE() -- but who knows what else could happen concurrently in
some weird corner cases (e.g., hugetlb folio getting unmapped and freed).

Did some basic sanity testing with various hugetlb sizes on x86-64 and
arm64. Maybe I'll find some time to actually write a simple reproducer in
the common weeks, so this wouldn't have to be all-theoretical for now.

Only v6.10 is affected, so the #1 can be simply backported as a prereq
patch along with the real fix.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Muchun Song <muchun.song@linux.dev>
Cc: Peter Xu <peterx@redhat.com>
Cc: Oscar Salvador <osalvador@suse.de>

David Hildenbrand (2):
  mm: let pte_lockptr() consume a pte_t pointer
  mm/hugetlb: fix hugetlb vs. core-mm PT locking

 include/linux/hugetlb.h | 25 ++++++++++++++++++++++---
 include/linux/mm.h      |  7 ++++---
 mm/khugepaged.c         | 21 +++++++++++++++------
 mm/pgtable-generic.c    |  4 ++--
 4 files changed, 43 insertions(+), 14 deletions(-)


base-commit: cca1345bd26a67fc61a92ff0c6d81766c259e522
-- 
2.45.2

[PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[David Hildenbrand]

pte_lockptr() is the only *_lockptr() function that doesn't consume
what would be expected: it consumes a pmd_t pointer instead of a pte_t
pointer.

Let's change that. The two callers in pgtable-generic.c are easily
adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
pte_offset_map_nolock() to obtain the lock, even though we won't actually
be traversing the page table.

This makes the code more similar to the other variants and avoids other
hacks to make the new pte_lockptr() version happy. pte_lockptr() users
reside now only in  pgtable-generic.c.

Maybe, using pte_offset_map_nolock() is the right thing to do because
the PTE table could have been removed in the meantime? At least it sounds
more future proof if we ever have other means of page table reclaim.

It's not quite clear if holding the PTE table lock is really required:
what if someone else obtains the lock just after we unlock it? But we'll
leave that as is for now, maybe there are good reasons.

This is a preparation for adapting hugetlb page table locking logic to
take the same locks as core-mm page table walkers would.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/linux/mm.h   |  7 ++++---
 mm/khugepaged.c      | 21 +++++++++++++++------
 mm/pgtable-generic.c |  4 ++--
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 2c6ccf088c7be..0472a5090b180 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2873,9 +2873,10 @@ static inline spinlock_t *ptlock_ptr(struct ptdesc *ptdesc)
 }
 #endif /* ALLOC_SPLIT_PTLOCKS */
 
-static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pmd_t *pmd)
+static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pte_t *pte)
 {
-	return ptlock_ptr(page_ptdesc(pmd_page(*pmd)));
+	/* PTE page tables don't currently exceed a single page. */
+	return ptlock_ptr(virt_to_ptdesc(pte));
 }
 
 static inline bool ptlock_init(struct ptdesc *ptdesc)
@@ -2898,7 +2899,7 @@ static inline bool ptlock_init(struct ptdesc *ptdesc)
 /*
  * We use mm->page_table_lock to guard all pagetable pages of the mm.
  */
-static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pmd_t *pmd)
+static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pte_t *pte)
 {
 	return &mm->page_table_lock;
 }
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index cdd1d8655a76b..f3b3db1046155 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1697,12 +1697,13 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
 	i_mmap_lock_read(mapping);
 	vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) {
 		struct mmu_notifier_range range;
+		bool retracted = false;
 		struct mm_struct *mm;
 		unsigned long addr;
 		pmd_t *pmd, pgt_pmd;
 		spinlock_t *pml;
 		spinlock_t *ptl;
-		bool skipped_uffd = false;
+		pte_t *pte;
 
 		/*
 		 * Check vma->anon_vma to exclude MAP_PRIVATE mappings that
@@ -1739,9 +1740,17 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
 		mmu_notifier_invalidate_range_start(&range);
 
 		pml = pmd_lock(mm, pmd);
-		ptl = pte_lockptr(mm, pmd);
+
+		/*
+		 * No need to check the PTE table content, but we'll grab the
+		 * PTE table lock while we zap it.
+		 */
+		pte = pte_offset_map_nolock(mm, pmd, addr, &ptl);
+		if (!pte)
+			goto unlock_pmd;
 		if (ptl != pml)
 			spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
+		pte_unmap(pte);
 
 		/*
 		 * Huge page lock is still held, so normally the page table
@@ -1752,20 +1761,20 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
 		 * repeating the anon_vma check protects from one category,
 		 * and repeating the userfaultfd_wp() check from another.
 		 */
-		if (unlikely(vma->anon_vma || userfaultfd_wp(vma))) {
-			skipped_uffd = true;
-		} else {
+		if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) {
 			pgt_pmd = pmdp_collapse_flush(vma, addr, pmd);
 			pmdp_get_lockless_sync();
+			retracted = true;
 		}
 
 		if (ptl != pml)
 			spin_unlock(ptl);
+unlock_pmd:
 		spin_unlock(pml);
 
 		mmu_notifier_invalidate_range_end(&range);
 
-		if (!skipped_uffd) {
+		if (retracted) {
 			mm_dec_nr_ptes(mm);
 			page_table_check_pte_clear_range(mm, addr, pgt_pmd);
 			pte_free_defer(mm, pmd_pgtable(pgt_pmd));
diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
index a78a4adf711ac..13a7705df3f87 100644
--- a/mm/pgtable-generic.c
+++ b/mm/pgtable-generic.c
@@ -313,7 +313,7 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd,
 
 	pte = __pte_offset_map(pmd, addr, &pmdval);
 	if (likely(pte))
-		*ptlp = pte_lockptr(mm, &pmdval);
+		*ptlp = pte_lockptr(mm, pte);
 	return pte;
 }
 
@@ -371,7 +371,7 @@ pte_t *__pte_offset_map_lock(struct mm_struct *mm, pmd_t *pmd,
 	pte = __pte_offset_map(pmd, addr, &pmdval);
 	if (unlikely(!pte))
 		return pte;
-	ptl = pte_lockptr(mm, &pmdval);
+	ptl = pte_lockptr(mm, pte);
 	spin_lock(ptl);
 	if (likely(pmd_same(pmdval, pmdp_get_lockless(pmd)))) {
 		*ptlp = ptl;
-- 
2.45.2

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Peter Xu]

On Thu, Jul 25, 2024 at 08:39:54PM +0200, David Hildenbrand wrote:
> pte_lockptr() is the only *_lockptr() function that doesn't consume
> what would be expected: it consumes a pmd_t pointer instead of a pte_t
> pointer.
> 
> Let's change that. The two callers in pgtable-generic.c are easily
> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
> pte_offset_map_nolock() to obtain the lock, even though we won't actually
> be traversing the page table.
> 
> This makes the code more similar to the other variants and avoids other
> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
> reside now only in  pgtable-generic.c.
> 
> Maybe, using pte_offset_map_nolock() is the right thing to do because
> the PTE table could have been removed in the meantime? At least it sounds
> more future proof if we ever have other means of page table reclaim.

I think it can't change, because anyone who wants to race against this
should try to take the pmd lock first (which was held already)?

I wonder an open coded "ptlock_ptr(page_ptdesc(pmd_page(*pmd)))" would be
nicer here, but only if my understanding is correct.

Thanks,

> 
> It's not quite clear if holding the PTE table lock is really required:
> what if someone else obtains the lock just after we unlock it? But we'll
> leave that as is for now, maybe there are good reasons.
> 
> This is a preparation for adapting hugetlb page table locking logic to
> take the same locks as core-mm page table walkers would.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  include/linux/mm.h   |  7 ++++---
>  mm/khugepaged.c      | 21 +++++++++++++++------
>  mm/pgtable-generic.c |  4 ++--
>  3 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 2c6ccf088c7be..0472a5090b180 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2873,9 +2873,10 @@ static inline spinlock_t *ptlock_ptr(struct ptdesc *ptdesc)
>  }
>  #endif /* ALLOC_SPLIT_PTLOCKS */
>  
> -static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pmd_t *pmd)
> +static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pte_t *pte)
>  {
> -	return ptlock_ptr(page_ptdesc(pmd_page(*pmd)));
> +	/* PTE page tables don't currently exceed a single page. */
> +	return ptlock_ptr(virt_to_ptdesc(pte));
>  }
>  
>  static inline bool ptlock_init(struct ptdesc *ptdesc)
> @@ -2898,7 +2899,7 @@ static inline bool ptlock_init(struct ptdesc *ptdesc)
>  /*
>   * We use mm->page_table_lock to guard all pagetable pages of the mm.
>   */
> -static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pmd_t *pmd)
> +static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pte_t *pte)
>  {
>  	return &mm->page_table_lock;
>  }
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index cdd1d8655a76b..f3b3db1046155 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1697,12 +1697,13 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>  	i_mmap_lock_read(mapping);
>  	vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) {
>  		struct mmu_notifier_range range;
> +		bool retracted = false;
>  		struct mm_struct *mm;
>  		unsigned long addr;
>  		pmd_t *pmd, pgt_pmd;
>  		spinlock_t *pml;
>  		spinlock_t *ptl;
> -		bool skipped_uffd = false;
> +		pte_t *pte;
>  
>  		/*
>  		 * Check vma->anon_vma to exclude MAP_PRIVATE mappings that
> @@ -1739,9 +1740,17 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>  		mmu_notifier_invalidate_range_start(&range);
>  
>  		pml = pmd_lock(mm, pmd);
> -		ptl = pte_lockptr(mm, pmd);
> +
> +		/*
> +		 * No need to check the PTE table content, but we'll grab the
> +		 * PTE table lock while we zap it.
> +		 */
> +		pte = pte_offset_map_nolock(mm, pmd, addr, &ptl);
> +		if (!pte)
> +			goto unlock_pmd;
>  		if (ptl != pml)
>  			spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
> +		pte_unmap(pte);
>  
>  		/*
>  		 * Huge page lock is still held, so normally the page table
> @@ -1752,20 +1761,20 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>  		 * repeating the anon_vma check protects from one category,
>  		 * and repeating the userfaultfd_wp() check from another.
>  		 */
> -		if (unlikely(vma->anon_vma || userfaultfd_wp(vma))) {
> -			skipped_uffd = true;
> -		} else {
> +		if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) {
>  			pgt_pmd = pmdp_collapse_flush(vma, addr, pmd);
>  			pmdp_get_lockless_sync();
> +			retracted = true;
>  		}
>  
>  		if (ptl != pml)
>  			spin_unlock(ptl);
> +unlock_pmd:
>  		spin_unlock(pml);
>  
>  		mmu_notifier_invalidate_range_end(&range);
>  
> -		if (!skipped_uffd) {
> +		if (retracted) {
>  			mm_dec_nr_ptes(mm);
>  			page_table_check_pte_clear_range(mm, addr, pgt_pmd);
>  			pte_free_defer(mm, pmd_pgtable(pgt_pmd));
> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> index a78a4adf711ac..13a7705df3f87 100644
> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -313,7 +313,7 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd,
>  
>  	pte = __pte_offset_map(pmd, addr, &pmdval);
>  	if (likely(pte))
> -		*ptlp = pte_lockptr(mm, &pmdval);
> +		*ptlp = pte_lockptr(mm, pte);
>  	return pte;
>  }
>  
> @@ -371,7 +371,7 @@ pte_t *__pte_offset_map_lock(struct mm_struct *mm, pmd_t *pmd,
>  	pte = __pte_offset_map(pmd, addr, &pmdval);
>  	if (unlikely(!pte))
>  		return pte;
> -	ptl = pte_lockptr(mm, &pmdval);
> +	ptl = pte_lockptr(mm, pte);
>  	spin_lock(ptl);
>  	if (likely(pmd_same(pmdval, pmdp_get_lockless(pmd)))) {
>  		*ptlp = ptl;
> -- 
> 2.45.2
> 

-- 
Peter Xu

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[David Hildenbrand]

On 26.07.24 17:36, Peter Xu wrote:
> On Thu, Jul 25, 2024 at 08:39:54PM +0200, David Hildenbrand wrote:
>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>> pointer.
>>
>> Let's change that. The two callers in pgtable-generic.c are easily
>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>> pte_offset_map_nolock() to obtain the lock, even though we won't actually
>> be traversing the page table.
>>
>> This makes the code more similar to the other variants and avoids other
>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>> reside now only in  pgtable-generic.c.
>>
>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>> the PTE table could have been removed in the meantime? At least it sounds
>> more future proof if we ever have other means of page table reclaim.
> 
> I think it can't change, because anyone who wants to race against this
> should try to take the pmd lock first (which was held already)?

That doesn't explain why it is safe for us to assume that after we took 
the PMD lock that the PMD actually still points at a completely empty 
page table. Likely it currently works by accident, because we only have 
a single such user that makes this assumption. It might certainly be a 
different once we asynchronously reclaim page tables.

But yes, the PMD cannot get modified while we hold the PMD lock, 
otherwise we'd be in trouble

> 
> I wonder an open coded "ptlock_ptr(page_ptdesc(pmd_page(*pmd)))" would be
> nicer here, but only if my understanding is correct.

I really don't like open-coding that. Fortunately we were able to limit 
the use of ptlock_ptr to a single user outside of arch/x86/xen/mmu_pv.c 
so far.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Peter Xu]

On Fri, Jul 26, 2024 at 06:02:17PM +0200, David Hildenbrand wrote:
> On 26.07.24 17:36, Peter Xu wrote:
> > On Thu, Jul 25, 2024 at 08:39:54PM +0200, David Hildenbrand wrote:
> > > pte_lockptr() is the only *_lockptr() function that doesn't consume
> > > what would be expected: it consumes a pmd_t pointer instead of a pte_t
> > > pointer.
> > > 
> > > Let's change that. The two callers in pgtable-generic.c are easily
> > > adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
> > > pte_offset_map_nolock() to obtain the lock, even though we won't actually
> > > be traversing the page table.
> > > 
> > > This makes the code more similar to the other variants and avoids other
> > > hacks to make the new pte_lockptr() version happy. pte_lockptr() users
> > > reside now only in  pgtable-generic.c.
> > > 
> > > Maybe, using pte_offset_map_nolock() is the right thing to do because
> > > the PTE table could have been removed in the meantime? At least it sounds
> > > more future proof if we ever have other means of page table reclaim.
> > 
> > I think it can't change, because anyone who wants to race against this
> > should try to take the pmd lock first (which was held already)?
> 
> That doesn't explain why it is safe for us to assume that after we took the
> PMD lock that the PMD actually still points at a completely empty page
> table. Likely it currently works by accident, because we only have a single
> such user that makes this assumption. It might certainly be a different once
> we asynchronously reclaim page tables.

I think it's safe because find_pmd_or_thp_or_none() returned SUCCEED, and
we're holding i_mmap lock for read.  I don't see any way that this pmd can
become a non-pgtable-page.

I meant, AFAIU tearing down pgtable in whatever sane way will need to at
least take both mmap write lock and i_mmap write lock (in this case, a file
mapping), no?

> 
> But yes, the PMD cannot get modified while we hold the PMD lock, otherwise
> we'd be in trouble
> 
> > 
> > I wonder an open coded "ptlock_ptr(page_ptdesc(pmd_page(*pmd)))" would be
> > nicer here, but only if my understanding is correct.
> 
> I really don't like open-coding that. Fortunately we were able to limit the
> use of ptlock_ptr to a single user outside of arch/x86/xen/mmu_pv.c so far.

I'm fine if you prefer like that; I don't see it a huge deal to me.

Thanks,

-- 
Peter Xu

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[David Hildenbrand]

On 26.07.24 23:28, Peter Xu wrote:
> On Fri, Jul 26, 2024 at 06:02:17PM +0200, David Hildenbrand wrote:
>> On 26.07.24 17:36, Peter Xu wrote:
>>> On Thu, Jul 25, 2024 at 08:39:54PM +0200, David Hildenbrand wrote:
>>>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>>>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>>>> pointer.
>>>>
>>>> Let's change that. The two callers in pgtable-generic.c are easily
>>>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>>>> pte_offset_map_nolock() to obtain the lock, even though we won't actually
>>>> be traversing the page table.
>>>>
>>>> This makes the code more similar to the other variants and avoids other
>>>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>>>> reside now only in  pgtable-generic.c.
>>>>
>>>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>>>> the PTE table could have been removed in the meantime? At least it sounds
>>>> more future proof if we ever have other means of page table reclaim.
>>>
>>> I think it can't change, because anyone who wants to race against this
>>> should try to take the pmd lock first (which was held already)?
>>
>> That doesn't explain why it is safe for us to assume that after we took the
>> PMD lock that the PMD actually still points at a completely empty page
>> table. Likely it currently works by accident, because we only have a single
>> such user that makes this assumption. It might certainly be a different once
>> we asynchronously reclaim page tables.
> 
> I think it's safe because find_pmd_or_thp_or_none() returned SUCCEED, and
> we're holding i_mmap lock for read.  I don't see any way that this pmd can
> become a non-pgtable-page.
> 
> I meant, AFAIU tearing down pgtable in whatever sane way will need to at
> least take both mmap write lock and i_mmap write lock (in this case, a file
> mapping), no?

Skimming over [1] where I still owe a review I think we can now do it 
now purely under the read locks, with the PMD lock held.

I think this is also what collapse_pte_mapped_thp() ends up doing: 
replace a PTE table that maps a folio by a PMD (present or none, 
depends) that maps a folio only while holding the mmap lock in read 
mode. Of course, here the table is not empty but we need similar ways of 
making PT walkers aware of concurrent page table retraction.

IIRC, that was the magic added to __pte_offset_map(), such that 
pte_offset_map_nolock/pte_offset_map_lock can fail on races.


But if we hold the PMD lock, nothing should actually change (so far my 
understanding) -- we cannot suddenly rip out a page table.

[1] 
https://lkml.kernel.org/r/cover.1719570849.git.zhengqi.arch@bytedance.com

> 
>>
>> But yes, the PMD cannot get modified while we hold the PMD lock, otherwise
>> we'd be in trouble
>>
>>>
>>> I wonder an open coded "ptlock_ptr(page_ptdesc(pmd_page(*pmd)))" would be
>>> nicer here, but only if my understanding is correct.
>>
>> I really don't like open-coding that. Fortunately we were able to limit the
>> use of ptlock_ptr to a single user outside of arch/x86/xen/mmu_pv.c so far.
> 
> I'm fine if you prefer like that; I don't see it a huge deal to me.

Let's keep it like that, unless we can come up with something neater. At 
least it makes the code also more consistent with similar code in that 
file and the overhead should be  minimal.

I was briefly thinking about actually testing if the PT is full of 
pte_none(), either as a debugging check or to also handle what is 
currently handled via:

if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) {

Seems wasteful just because some part of a VMA might have a private page 
mapped / uffd-wp active to let all other parts suffer.

Will think about if that is really worth it.

... also because I still want to understand why the PTL of the PMD table 
is required at all. What if we lock it first and somebody else wants to 
lock it after us while we already ripped it out? Sure there must be some 
reason for the lock, I just don't understand it yet :/.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Qi Zheng]

Hi David,

On 2024/7/27 05:48, David Hildenbrand wrote:
> On 26.07.24 23:28, Peter Xu wrote:
>> On Fri, Jul 26, 2024 at 06:02:17PM +0200, David Hildenbrand wrote:
>>> On 26.07.24 17:36, Peter Xu wrote:
>>>> On Thu, Jul 25, 2024 at 08:39:54PM +0200, David Hildenbrand wrote:
>>>>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>>>>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>>>>> pointer.
>>>>>
>>>>> Let's change that. The two callers in pgtable-generic.c are easily
>>>>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>>>>> pte_offset_map_nolock() to obtain the lock, even though we won't 
>>>>> actually
>>>>> be traversing the page table.
>>>>>
>>>>> This makes the code more similar to the other variants and avoids 
>>>>> other
>>>>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>>>>> reside now only in  pgtable-generic.c.
>>>>>
>>>>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>>>>> the PTE table could have been removed in the meantime? At least it 
>>>>> sounds
>>>>> more future proof if we ever have other means of page table reclaim.
>>>>
>>>> I think it can't change, because anyone who wants to race against this
>>>> should try to take the pmd lock first (which was held already)?
>>>
>>> That doesn't explain why it is safe for us to assume that after we 
>>> took the
>>> PMD lock that the PMD actually still points at a completely empty page
>>> table. Likely it currently works by accident, because we only have a 
>>> single
>>> such user that makes this assumption. It might certainly be a 
>>> different once
>>> we asynchronously reclaim page tables.
>>
>> I think it's safe because find_pmd_or_thp_or_none() returned SUCCEED, and
>> we're holding i_mmap lock for read.  I don't see any way that this pmd 
>> can
>> become a non-pgtable-page.
>>
>> I meant, AFAIU tearing down pgtable in whatever sane way will need to at
>> least take both mmap write lock and i_mmap write lock (in this case, a 
>> file
>> mapping), no?
> 
> Skimming over [1] where I still owe a review I think we can now do it 
> now purely under the read locks, with the PMD lock held.

Yes.

> 
> I think this is also what collapse_pte_mapped_thp() ends up doing: 
> replace a PTE table that maps a folio by a PMD (present or none, 
> depends) that maps a folio only while holding the mmap lock in read 
> mode. Of course, here the table is not empty but we need similar ways of 
> making PT walkers aware of concurrent page table retraction.
> 
> IIRC, that was the magic added to __pte_offset_map(), such that 
> pte_offset_map_nolock/pte_offset_map_lock can fail on races.
> 
> 
> But if we hold the PMD lock, nothing should actually change (so far my 
> understanding) -- we cannot suddenly rip out a page table.
> 
> [1] 
> https://lkml.kernel.org/r/cover.1719570849.git.zhengqi.arch@bytedance.com
> 
>>
>>>
>>> But yes, the PMD cannot get modified while we hold the PMD lock, 
>>> otherwise
>>> we'd be in trouble
>>>
>>>>
>>>> I wonder an open coded "ptlock_ptr(page_ptdesc(pmd_page(*pmd)))" 
>>>> would be
>>>> nicer here, but only if my understanding is correct.
>>>
>>> I really don't like open-coding that. Fortunately we were able to 
>>> limit the
>>> use of ptlock_ptr to a single user outside of arch/x86/xen/mmu_pv.c 
>>> so far.
>>
>> I'm fine if you prefer like that; I don't see it a huge deal to me.
> 
> Let's keep it like that, unless we can come up with something neater. At 
> least it makes the code also more consistent with similar code in that 
> file and the overhead should be  minimal.
> 
> I was briefly thinking about actually testing if the PT is full of 
> pte_none(), either as a debugging check or to also handle what is > currently handled via:
> 
> if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) {
> 
> Seems wasteful just because some part of a VMA might have a private page 
> mapped / uffd-wp active to let all other parts suffer.
> 
> Will think about if that is really worth it.
> 
> ... also because I still want to understand why the PTL of the PMD table 
> is required at all. What if we lock it first and somebody else wants to 
> lock it after us while we already ripped it out? Sure there must be some 
> reason for the lock, I just don't understand it yet :/.

For pmd lock, I think this is needed to clear the pmd entry
(pmdp_collapse_flush()). For pte lock, there should be the following two
reasons:

1. release it after clearing pmd entry, then we can capture the changed
    pmd in pte_offset_map_lock() etc after holding this pte lock.
    (This is also what I did in my patchset)

2. As mentioned in the comments, we may be concurrent with
    userfaultfd_ioctl(), but we do not hold the read lock of mmap (or
    read lock of vma), so the VM_UFFD_WP may be set. Therefore, we need
    to hold the pte lock to check whether a new pte entry has been
    inserted.
    (See commit[1] for more details)

[1]. 
https://github.com/torvalds/linux/commit/a98460494b16db9c377e55bc13e5407a0eb79fe8

Thanks,
Qi

> 

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[David Hildenbrand]

>> ... also because I still want to understand why the PTL of the PMD table
>> is required at all. What if we lock it first and somebody else wants to
>> lock it after us while we already ripped it out? Sure there must be some
>> reason for the lock, I just don't understand it yet :/.
> 
> For pmd lock, I think this is needed to clear the pmd entry
> (pmdp_collapse_flush()). For pte lock, there should be the following two
> reasons:
> 

Thanks for the details.

My current understanding correct that removing *empty* page tables is
currently only possible if we can guarantee that nothing would try modifying the
page tables after we drop the PTE page table lock, but we would be happy if someone
would walk an empty page table while we remove it as long as the access is read-only.

In retract_page_tables() I thought that would be guaranteed as we prevent refaults
from the page cache and exclude any uffd + anon handling using the big hammer
(if any could be active, disallow zapping the page table).

What I am still not quite getting is what happens if someone would grab the PTE page
table lock after we released it -- and how that really protects us here.


I assume it's the

spin_lock(ptl);
if (likely(pmd_same(pmdval, pmdp_get_lockless(pmd)))) {
...

handling in __pte_offset_map_lock() that guarantees that.


That indeed means that pte_offset_map_nolock() requires similar checks after
obtaining the PTL (for the cases where we are not holding the PMD table lock
and can be sure that we are the one ripping out that table right now).


> 1. release it after clearing pmd entry, then we can capture the changed
>      pmd in pte_offset_map_lock() etc after holding this pte lock.
>      (This is also what I did in my patchset)

Yes, I get it now.

> 
> 2. As mentioned in the comments, we may be concurrent with
>      userfaultfd_ioctl(), but we do not hold the read lock of mmap (or
>      read lock of vma), so the VM_UFFD_WP may be set. Therefore, we need
>      to hold the pte lock to check whether a new pte entry has been
>      inserted.
>      (See commit[1] for more details)


Yes, I see we tried to document that magic and it is still absolutely confusing :)

But at least now it's clearer to me why retract_page_tables() uses slightly different
locking than collapse_pte_mapped_thp().

Maybe we should look into letting collapse_pte_mapped_thp() use a similar approach as
retract_page_tables() to at least make it more consistent. That topic is also touched
in a98460494b16.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Qi Zheng]

Hi David,

On 2024/7/30 16:40, David Hildenbrand wrote:
>>> ... also because I still want to understand why the PTL of the PMD table
>>> is required at all. What if we lock it first and somebody else wants to
>>> lock it after us while we already ripped it out? Sure there must be some
>>> reason for the lock, I just don't understand it yet :/.
>>
>> For pmd lock, I think this is needed to clear the pmd entry
>> (pmdp_collapse_flush()). For pte lock, there should be the following two
>> reasons:
>>
> 
> Thanks for the details.
> 
> My current understanding correct that removing *empty* page tables is
> currently only possible if we can guarantee that nothing would try 
> modifying the
> page tables after we drop the PTE page table lock, but we would be happy 
> if someone
> would walk an empty page table while we remove it as long as the access 
> is read-only.
> 
> In retract_page_tables() I thought that would be guaranteed as we 
> prevent refaults
> from the page cache and exclude any uffd + anon handling using the big 
> hammer
> (if any could be active, disallow zapping the page table).
> 
> What I am still not quite getting is what happens if someone would grab 
> the PTE page
> table lock after we released it -- and how that really protects us here.
> 
> 
> I assume it's the
> 
> spin_lock(ptl);
> if (likely(pmd_same(pmdval, pmdp_get_lockless(pmd)))) {
> ...
> 
> handling in __pte_offset_map_lock() that guarantees that.

Yes.

> 
> 
> That indeed means that pte_offset_map_nolock() requires similar checks 
> after
> obtaining the PTL (for the cases where we are not holding the PMD table 
> lock
> and can be sure that we are the one ripping out that table right now).

Yes, if the PTE page will be freed concurrently, then the pmd entry
needs to be rechecked after acquiring the PTL. So I made
pte_offset_map_nolock() return pmdval in my patch[1], and then we
can do the following:

start_pte = pte_offset_map_nolock(mm, pmd, &pmdval, addr, &ptl);
spin_lock(ptl)
if (likely(pmd_same(pmdval, pmdp_get_lockless(pmd)))) {
...


[1]. 
https://lore.kernel.org/lkml/7f5233f9f612c7f58abf218852fb1042d764940b.1719570849.git.zhengqi.arch@bytedance.com/

> 
> 
>> 1. release it after clearing pmd entry, then we can capture the changed
>>      pmd in pte_offset_map_lock() etc after holding this pte lock.
>>      (This is also what I did in my patchset)
> 
> Yes, I get it now.
> 
>>
>> 2. As mentioned in the comments, we may be concurrent with
>>      userfaultfd_ioctl(), but we do not hold the read lock of mmap (or
>>      read lock of vma), so the VM_UFFD_WP may be set. Therefore, we need
>>      to hold the pte lock to check whether a new pte entry has been
>>      inserted.
>>      (See commit[1] for more details)
> 
> 
> Yes, I see we tried to document that magic and it is still absolutely 
> confusing :)
> 
> But at least now it's clearer to me why retract_page_tables() uses 
> slightly different
> locking than collapse_pte_mapped_thp().
> 
> Maybe we should look into letting collapse_pte_mapped_thp() use a 
> similar approach as
> retract_page_tables() to at least make it more consistent. That topic is 
> also touched
> in a98460494b16.

Agree, more consistency is better.

Thanks,
Qi

> 

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Peter Xu]

On Fri, Jul 26, 2024 at 11:48:01PM +0200, David Hildenbrand wrote:
> On 26.07.24 23:28, Peter Xu wrote:
> > On Fri, Jul 26, 2024 at 06:02:17PM +0200, David Hildenbrand wrote:
> > > On 26.07.24 17:36, Peter Xu wrote:
> > > > On Thu, Jul 25, 2024 at 08:39:54PM +0200, David Hildenbrand wrote:
> > > > > pte_lockptr() is the only *_lockptr() function that doesn't consume
> > > > > what would be expected: it consumes a pmd_t pointer instead of a pte_t
> > > > > pointer.
> > > > > 
> > > > > Let's change that. The two callers in pgtable-generic.c are easily
> > > > > adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
> > > > > pte_offset_map_nolock() to obtain the lock, even though we won't actually
> > > > > be traversing the page table.
> > > > > 
> > > > > This makes the code more similar to the other variants and avoids other
> > > > > hacks to make the new pte_lockptr() version happy. pte_lockptr() users
> > > > > reside now only in  pgtable-generic.c.
> > > > > 
> > > > > Maybe, using pte_offset_map_nolock() is the right thing to do because
> > > > > the PTE table could have been removed in the meantime? At least it sounds
> > > > > more future proof if we ever have other means of page table reclaim.
> > > > 
> > > > I think it can't change, because anyone who wants to race against this
> > > > should try to take the pmd lock first (which was held already)?
> > > 
> > > That doesn't explain why it is safe for us to assume that after we took the
> > > PMD lock that the PMD actually still points at a completely empty page
> > > table. Likely it currently works by accident, because we only have a single
> > > such user that makes this assumption. It might certainly be a different once
> > > we asynchronously reclaim page tables.
> > 
> > I think it's safe because find_pmd_or_thp_or_none() returned SUCCEED, and
> > we're holding i_mmap lock for read.  I don't see any way that this pmd can
> > become a non-pgtable-page.
> > 
> > I meant, AFAIU tearing down pgtable in whatever sane way will need to at
> > least take both mmap write lock and i_mmap write lock (in this case, a file
> > mapping), no?
> 
> Skimming over [1] where I still owe a review I think we can now do it now
> purely under the read locks, with the PMD lock held.

Err, how I missed that.. yeah you're definitely right, and that's the
context here where we're collapsing.  I think I somehow forgot all Hugh's
work when I replied there, sorry.

> 
> I think this is also what collapse_pte_mapped_thp() ends up doing: replace a
> PTE table that maps a folio by a PMD (present or none, depends) that maps a
> folio only while holding the mmap lock in read mode. Of course, here the
> table is not empty but we need similar ways of making PT walkers aware of
> concurrent page table retraction.
> 
> IIRC, that was the magic added to __pte_offset_map(), such that
> pte_offset_map_nolock/pte_offset_map_lock can fail on races.

Said that, I still think current code (before this patch) is safe, same to
a hard-coded line to lock the pte pgtable lock.  Again, I'm fine if you
prefer pte_offset_map_nolock() but I just think the rcu read lock and stuff
can be avoided.

I think it's because such collapse so far can only happen in such path
where we need to hold the large folio (PMD-level) lock first.  It means
anyone who could change this pmd entry to be not a pte pgtable is blocked
already, hence it must keeping being a pte pgtable page even if we don't
take any rcu.

> 
> 
> But if we hold the PMD lock, nothing should actually change (so far my
> understanding) -- we cannot suddenly rip out a page table.
> 
> [1]
> https://lkml.kernel.org/r/cover.1719570849.git.zhengqi.arch@bytedance.com
> 
> > 
> > > 
> > > But yes, the PMD cannot get modified while we hold the PMD lock, otherwise
> > > we'd be in trouble
> > > 
> > > > 
> > > > I wonder an open coded "ptlock_ptr(page_ptdesc(pmd_page(*pmd)))" would be
> > > > nicer here, but only if my understanding is correct.
> > > 
> > > I really don't like open-coding that. Fortunately we were able to limit the
> > > use of ptlock_ptr to a single user outside of arch/x86/xen/mmu_pv.c so far.
> > 
> > I'm fine if you prefer like that; I don't see it a huge deal to me.
> 
> Let's keep it like that, unless we can come up with something neater. At
> least it makes the code also more consistent with similar code in that file
> and the overhead should be  minimal.
> 
> I was briefly thinking about actually testing if the PT is full of
> pte_none(), either as a debugging check or to also handle what is currently
> handled via:
> 
> if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) {
> 
> Seems wasteful just because some part of a VMA might have a private page
> mapped / uffd-wp active to let all other parts suffer.
> 
> Will think about if that is really worth it.
> 
> ... also because I still want to understand why the PTL of the PMD table is
> required at all. What if we lock it first and somebody else wants to lock it
> after us while we already ripped it out? Sure there must be some reason for
> the lock, I just don't understand it yet :/.

IIUC the pte pgtable lock will be needed for checking anon_vma safely.

e.g., consider if we don't take the pte pgtable lock, I think it's possible
some thread tries to inject a private pte (and prepare anon_vma before
that) concurrently with this thread trying to collapse the pgtable into a
huge pmd.  I mean, when without the pte pgtable lock held, I think it's
racy to check this line:

        if (unlikely(vma->anon_vma || userfaultfd_wp(vma))) {
                ...
        }

On the 1st condition.

Thanks,

-- 
Peter Xu

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Peter Xu]

On Mon, Jul 29, 2024 at 12:26:00PM -0400, Peter Xu wrote:
> On Fri, Jul 26, 2024 at 11:48:01PM +0200, David Hildenbrand wrote:
> > On 26.07.24 23:28, Peter Xu wrote:
> > > On Fri, Jul 26, 2024 at 06:02:17PM +0200, David Hildenbrand wrote:
> > > > On 26.07.24 17:36, Peter Xu wrote:
> > > > > On Thu, Jul 25, 2024 at 08:39:54PM +0200, David Hildenbrand wrote:
> > > > > > pte_lockptr() is the only *_lockptr() function that doesn't consume
> > > > > > what would be expected: it consumes a pmd_t pointer instead of a pte_t
> > > > > > pointer.
> > > > > > 
> > > > > > Let's change that. The two callers in pgtable-generic.c are easily
> > > > > > adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
> > > > > > pte_offset_map_nolock() to obtain the lock, even though we won't actually
> > > > > > be traversing the page table.
> > > > > > 
> > > > > > This makes the code more similar to the other variants and avoids other
> > > > > > hacks to make the new pte_lockptr() version happy. pte_lockptr() users
> > > > > > reside now only in  pgtable-generic.c.
> > > > > > 
> > > > > > Maybe, using pte_offset_map_nolock() is the right thing to do because
> > > > > > the PTE table could have been removed in the meantime? At least it sounds
> > > > > > more future proof if we ever have other means of page table reclaim.
> > > > > 
> > > > > I think it can't change, because anyone who wants to race against this
> > > > > should try to take the pmd lock first (which was held already)?
> > > > 
> > > > That doesn't explain why it is safe for us to assume that after we took the
> > > > PMD lock that the PMD actually still points at a completely empty page
> > > > table. Likely it currently works by accident, because we only have a single
> > > > such user that makes this assumption. It might certainly be a different once
> > > > we asynchronously reclaim page tables.
> > > 
> > > I think it's safe because find_pmd_or_thp_or_none() returned SUCCEED, and
> > > we're holding i_mmap lock for read.  I don't see any way that this pmd can
> > > become a non-pgtable-page.
> > > 
> > > I meant, AFAIU tearing down pgtable in whatever sane way will need to at
> > > least take both mmap write lock and i_mmap write lock (in this case, a file
> > > mapping), no?
> > 
> > Skimming over [1] where I still owe a review I think we can now do it now
> > purely under the read locks, with the PMD lock held.
> 
> Err, how I missed that.. yeah you're definitely right, and that's the
> context here where we're collapsing.  I think I somehow forgot all Hugh's
> work when I replied there, sorry.
> 
> > 
> > I think this is also what collapse_pte_mapped_thp() ends up doing: replace a
> > PTE table that maps a folio by a PMD (present or none, depends) that maps a
> > folio only while holding the mmap lock in read mode. Of course, here the
> > table is not empty but we need similar ways of making PT walkers aware of
> > concurrent page table retraction.
> > 
> > IIRC, that was the magic added to __pte_offset_map(), such that
> > pte_offset_map_nolock/pte_offset_map_lock can fail on races.
> 
> Said that, I still think current code (before this patch) is safe, same to
> a hard-coded line to lock the pte pgtable lock.  Again, I'm fine if you
> prefer pte_offset_map_nolock() but I just think the rcu read lock and stuff
> can be avoided.
> 
> I think it's because such collapse so far can only happen in such path
> where we need to hold the large folio (PMD-level) lock first.  It means
> anyone who could change this pmd entry to be not a pte pgtable is blocked
> already, hence it must keeping being a pte pgtable page even if we don't
> take any rcu.
> 
> > 
> > 
> > But if we hold the PMD lock, nothing should actually change (so far my
> > understanding) -- we cannot suddenly rip out a page table.
> > 
> > [1]
> > https://lkml.kernel.org/r/cover.1719570849.git.zhengqi.arch@bytedance.com
> > 
> > > 
> > > > 
> > > > But yes, the PMD cannot get modified while we hold the PMD lock, otherwise
> > > > we'd be in trouble
> > > > 
> > > > > 
> > > > > I wonder an open coded "ptlock_ptr(page_ptdesc(pmd_page(*pmd)))" would be
> > > > > nicer here, but only if my understanding is correct.
> > > > 
> > > > I really don't like open-coding that. Fortunately we were able to limit the
> > > > use of ptlock_ptr to a single user outside of arch/x86/xen/mmu_pv.c so far.
> > > 
> > > I'm fine if you prefer like that; I don't see it a huge deal to me.
> > 
> > Let's keep it like that, unless we can come up with something neater. At
> > least it makes the code also more consistent with similar code in that file
> > and the overhead should be  minimal.
> > 
> > I was briefly thinking about actually testing if the PT is full of
> > pte_none(), either as a debugging check or to also handle what is currently
> > handled via:
> > 
> > if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) {
> > 
> > Seems wasteful just because some part of a VMA might have a private page
> > mapped / uffd-wp active to let all other parts suffer.
> > 
> > Will think about if that is really worth it.
> > 
> > ... also because I still want to understand why the PTL of the PMD table is
> > required at all. What if we lock it first and somebody else wants to lock it
> > after us while we already ripped it out? Sure there must be some reason for
> > the lock, I just don't understand it yet :/.
> 
> IIUC the pte pgtable lock will be needed for checking anon_vma safely.
> 
> e.g., consider if we don't take the pte pgtable lock, I think it's possible
> some thread tries to inject a private pte (and prepare anon_vma before
> that) concurrently with this thread trying to collapse the pgtable into a
> huge pmd.  I mean, when without the pte pgtable lock held, I think it's
> racy to check this line:
> 
>         if (unlikely(vma->anon_vma || userfaultfd_wp(vma))) {
>                 ...
>         }
> 
> On the 1st condition.

Hmm, right after I replied, I think it also guarantees safety on the 2nd
condition..

Note that one thing I still prefer a hard-coded line over
pte_offset_map_nolock() is that, the new code seems to say we can treat the
pte pgtable page differently from the pte pgtable lock.  But I think
they're really in the same realm.

In short, AFAIU the rcu lock not only protects the pte pgtable's existance,
but also protects the pte lock.

From that POV, below new code in this patch:

-               ptl = pte_lockptr(mm, pmd);
+
+               /*
+                * No need to check the PTE table content, but we'll grab the
+                * PTE table lock while we zap it.
+                */
+               pte = pte_offset_map_nolock(mm, pmd, addr, &ptl);
+               if (!pte)
+                       goto unlock_pmd;
                if (ptl != pml)
                        spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
+               pte_unmap(pte);

Could be very misleading, where it seems to say that it's fine to use the
pte pgtable lock even after rcu unlock.  It might make the code harder to
understand.

Thanks,

-- 
Peter Xu

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[David Hildenbrand]

On 29.07.24 18:39, Peter Xu wrote:
> On Mon, Jul 29, 2024 at 12:26:00PM -0400, Peter Xu wrote:
>> On Fri, Jul 26, 2024 at 11:48:01PM +0200, David Hildenbrand wrote:
>>> On 26.07.24 23:28, Peter Xu wrote:
>>>> On Fri, Jul 26, 2024 at 06:02:17PM +0200, David Hildenbrand wrote:
>>>>> On 26.07.24 17:36, Peter Xu wrote:
>>>>>> On Thu, Jul 25, 2024 at 08:39:54PM +0200, David Hildenbrand wrote:
>>>>>>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>>>>>>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>>>>>>> pointer.
>>>>>>>
>>>>>>> Let's change that. The two callers in pgtable-generic.c are easily
>>>>>>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>>>>>>> pte_offset_map_nolock() to obtain the lock, even though we won't actually
>>>>>>> be traversing the page table.
>>>>>>>
>>>>>>> This makes the code more similar to the other variants and avoids other
>>>>>>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>>>>>>> reside now only in  pgtable-generic.c.
>>>>>>>
>>>>>>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>>>>>>> the PTE table could have been removed in the meantime? At least it sounds
>>>>>>> more future proof if we ever have other means of page table reclaim.
>>>>>>
>>>>>> I think it can't change, because anyone who wants to race against this
>>>>>> should try to take the pmd lock first (which was held already)?
>>>>>
>>>>> That doesn't explain why it is safe for us to assume that after we took the
>>>>> PMD lock that the PMD actually still points at a completely empty page
>>>>> table. Likely it currently works by accident, because we only have a single
>>>>> such user that makes this assumption. It might certainly be a different once
>>>>> we asynchronously reclaim page tables.
>>>>
>>>> I think it's safe because find_pmd_or_thp_or_none() returned SUCCEED, and
>>>> we're holding i_mmap lock for read.  I don't see any way that this pmd can
>>>> become a non-pgtable-page.
>>>>
>>>> I meant, AFAIU tearing down pgtable in whatever sane way will need to at
>>>> least take both mmap write lock and i_mmap write lock (in this case, a file
>>>> mapping), no?
>>>
>>> Skimming over [1] where I still owe a review I think we can now do it now
>>> purely under the read locks, with the PMD lock held.
>>
>> Err, how I missed that.. yeah you're definitely right, and that's the
>> context here where we're collapsing.  I think I somehow forgot all Hugh's
>> work when I replied there, sorry.
>>
>>>
>>> I think this is also what collapse_pte_mapped_thp() ends up doing: replace a
>>> PTE table that maps a folio by a PMD (present or none, depends) that maps a
>>> folio only while holding the mmap lock in read mode. Of course, here the
>>> table is not empty but we need similar ways of making PT walkers aware of
>>> concurrent page table retraction.
>>>
>>> IIRC, that was the magic added to __pte_offset_map(), such that
>>> pte_offset_map_nolock/pte_offset_map_lock can fail on races.
>>
>> Said that, I still think current code (before this patch) is safe, same to
>> a hard-coded line to lock the pte pgtable lock.  Again, I'm fine if you
>> prefer pte_offset_map_nolock() but I just think the rcu read lock and stuff
>> can be avoided.
>>
>> I think it's because such collapse so far can only happen in such path
>> where we need to hold the large folio (PMD-level) lock first.  It means
>> anyone who could change this pmd entry to be not a pte pgtable is blocked
>> already, hence it must keeping being a pte pgtable page even if we don't
>> take any rcu.
>>
>>>
>>>
>>> But if we hold the PMD lock, nothing should actually change (so far my
>>> understanding) -- we cannot suddenly rip out a page table.
>>>
>>> [1]
>>> https://lkml.kernel.org/r/cover.1719570849.git.zhengqi.arch@bytedance.com
>>>
>>>>
>>>>>
>>>>> But yes, the PMD cannot get modified while we hold the PMD lock, otherwise
>>>>> we'd be in trouble
>>>>>
>>>>>>
>>>>>> I wonder an open coded "ptlock_ptr(page_ptdesc(pmd_page(*pmd)))" would be
>>>>>> nicer here, but only if my understanding is correct.
>>>>>
>>>>> I really don't like open-coding that. Fortunately we were able to limit the
>>>>> use of ptlock_ptr to a single user outside of arch/x86/xen/mmu_pv.c so far.
>>>>
>>>> I'm fine if you prefer like that; I don't see it a huge deal to me.
>>>
>>> Let's keep it like that, unless we can come up with something neater. At
>>> least it makes the code also more consistent with similar code in that file
>>> and the overhead should be  minimal.
>>>
>>> I was briefly thinking about actually testing if the PT is full of
>>> pte_none(), either as a debugging check or to also handle what is currently
>>> handled via:
>>>
>>> if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) {
>>>
>>> Seems wasteful just because some part of a VMA might have a private page
>>> mapped / uffd-wp active to let all other parts suffer.
>>>
>>> Will think about if that is really worth it.
>>>
>>> ... also because I still want to understand why the PTL of the PMD table is
>>> required at all. What if we lock it first and somebody else wants to lock it
>>> after us while we already ripped it out? Sure there must be some reason for
>>> the lock, I just don't understand it yet :/.
>>
>> IIUC the pte pgtable lock will be needed for checking anon_vma safely.
>>
>> e.g., consider if we don't take the pte pgtable lock, I think it's possible
>> some thread tries to inject a private pte (and prepare anon_vma before
>> that) concurrently with this thread trying to collapse the pgtable into a
>> huge pmd.  I mean, when without the pte pgtable lock held, I think it's
>> racy to check this line:
>>
>>          if (unlikely(vma->anon_vma || userfaultfd_wp(vma))) {
>>                  ...
>>          }
>>
>> On the 1st condition.
> 
> Hmm, right after I replied, I think it also guarantees safety on the 2nd
> condition..
> 
> Note that one thing I still prefer a hard-coded line over
> pte_offset_map_nolock() is that, the new code seems to say we can treat the
> pte pgtable page differently from the pte pgtable lock.  But I think
> they're really in the same realm.
> 
> In short, AFAIU the rcu lock not only protects the pte pgtable's existance,
> but also protects the pte lock.
> 
>  From that POV, below new code in this patch:
> 
> -               ptl = pte_lockptr(mm, pmd);
> +
> +               /*
> +                * No need to check the PTE table content, but we'll grab the
> +                * PTE table lock while we zap it.
> +                */
> +               pte = pte_offset_map_nolock(mm, pmd, addr, &ptl);
> +               if (!pte)
> +                       goto unlock_pmd;
>                  if (ptl != pml)
>                          spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
> +               pte_unmap(pte);
> 
> Could be very misleading, where it seems to say that it's fine to use the
> pte pgtable lock even after rcu unlock.  It might make the code harder to
> understand.

I see what you mean but this is a very similar pattern as used in 
collapse_pte_mapped_thp(), no? There we have

start_pte = pte_offset_map_nolock(mm, pmd, haddr, &ptl);
...
if (!pml)
	spin_lock(ptl);
...
pte_unmap(start_pte);
if (!pml)
	spin_unlock(ptl);


Again, I don't have a strong opinion on this, but doing it more similar 
to collapse_pte_mapped_thp() to obtain locks makes it clearer to me. But 
if I am missing something obvious please shout and I'll change it.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Peter Xu]

On Mon, Jul 29, 2024 at 07:46:26PM +0200, David Hildenbrand wrote:
> I see what you mean but this is a very similar pattern as used in
> collapse_pte_mapped_thp(), no? There we have
> 
> start_pte = pte_offset_map_nolock(mm, pmd, haddr, &ptl);
> ...
> if (!pml)
> 	spin_lock(ptl);
> ...
> pte_unmap(start_pte);
> if (!pml)
> 	spin_unlock(ptl);
> 
> 
> Again, I don't have a strong opinion on this, but doing it more similar to
> collapse_pte_mapped_thp() to obtain locks makes it clearer to me. But if I
> am missing something obvious please shout and I'll change it.

Right.. I don't think that path can change the pte pgtable either, and
there is even the line Hugh left showing it's impossible:

	if (!start_pte)		/* mmap_lock + page lock should prevent this */
		goto abort;

I was thinking maybe the page lock is the critical one, irrelevant of mmap
lock.

No strong opinion either.  Not sure whether Hugh has some thoughts.  But
maybe if we stick with pte_offset_map_nolock() and if there'll be a repost
anyway, we could add a similar comment like this one showing that the pte
pgtable should be actually as stable as the ptlock.

Thanks,

-- 
Peter Xu

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[David Hildenbrand]

On 30.07.24 20:44, Peter Xu wrote:
> On Mon, Jul 29, 2024 at 07:46:26PM +0200, David Hildenbrand wrote:
>> I see what you mean but this is a very similar pattern as used in
>> collapse_pte_mapped_thp(), no? There we have
>>
>> start_pte = pte_offset_map_nolock(mm, pmd, haddr, &ptl);
>> ...
>> if (!pml)
>> 	spin_lock(ptl);
>> ...
>> pte_unmap(start_pte);
>> if (!pml)
>> 	spin_unlock(ptl);
>>
>>
>> Again, I don't have a strong opinion on this, but doing it more similar to
>> collapse_pte_mapped_thp() to obtain locks makes it clearer to me. But if I
>> am missing something obvious please shout and I'll change it.
> 
> Right.. I don't think that path can change the pte pgtable either, and
> there is even the line Hugh left showing it's impossible:
> 
> 	if (!start_pte)		/* mmap_lock + page lock should prevent this */
> 		goto abort;
> 
> I was thinking maybe the page lock is the critical one, irrelevant of mmap
> lock.
> 
> No strong opinion either.  Not sure whether Hugh has some thoughts.  But
> maybe if we stick with pte_offset_map_nolock() and if there'll be a repost
> anyway, we could add a similar comment like this one showing that the pte
> pgtable should be actually as stable as the ptlock.

I think this all deserves some future cleanups :)

... for the time being I'll have to drop this patch completely. I should 
have known that virt_to_page() does not work on kmap'ed pages, but part 
of me didn't want to believe it.

CONFIG_HIGHPTE wants to make my life challenging :)

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Qi Zheng]

On 2024/7/26 02:39, David Hildenbrand wrote:
> pte_lockptr() is the only *_lockptr() function that doesn't consume
> what would be expected: it consumes a pmd_t pointer instead of a pte_t
> pointer.
> 
> Let's change that. The two callers in pgtable-generic.c are easily
> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
> pte_offset_map_nolock() to obtain the lock, even though we won't actually
> be traversing the page table.
> 
> This makes the code more similar to the other variants and avoids other
> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
> reside now only in  pgtable-generic.c.
> 
> Maybe, using pte_offset_map_nolock() is the right thing to do because
> the PTE table could have been removed in the meantime? At least it sounds
> more future proof if we ever have other means of page table reclaim.

Agree, this helps us recheck the pmd entry.

> 
> It's not quite clear if holding the PTE table lock is really required:
> what if someone else obtains the lock just after we unlock it? But we'll
> leave that as is for now, maybe there are good reasons.
> 
> This is a preparation for adapting hugetlb page table locking logic to
> take the same locks as core-mm page table walkers would.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   include/linux/mm.h   |  7 ++++---
>   mm/khugepaged.c      | 21 +++++++++++++++------
>   mm/pgtable-generic.c |  4 ++--
>   3 files changed, 21 insertions(+), 11 deletions(-)

Since pte_lockptr() no longer has a pmd parameter, it is best to modify
the comments above __pte_offset_map_lock() as well:

```
This helps the caller to avoid a later pte_lockptr(mm, *pmd), which 
might by that time act on a changed *pmd ...
```

Otherwise:
Reviewed-by: Qi Zheng <zhengqi.arch@bytedance.com>

> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 2c6ccf088c7be..0472a5090b180 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2873,9 +2873,10 @@ static inline spinlock_t *ptlock_ptr(struct ptdesc *ptdesc)
>   }
>   #endif /* ALLOC_SPLIT_PTLOCKS */
>   
> -static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pmd_t *pmd)
> +static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pte_t *pte)
>   {
> -	return ptlock_ptr(page_ptdesc(pmd_page(*pmd)));
> +	/* PTE page tables don't currently exceed a single page. */
> +	return ptlock_ptr(virt_to_ptdesc(pte));
>   }
>   
>   static inline bool ptlock_init(struct ptdesc *ptdesc)
> @@ -2898,7 +2899,7 @@ static inline bool ptlock_init(struct ptdesc *ptdesc)
>   /*
>    * We use mm->page_table_lock to guard all pagetable pages of the mm.
>    */
> -static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pmd_t *pmd)
> +static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pte_t *pte)
>   {
>   	return &mm->page_table_lock;
>   }
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index cdd1d8655a76b..f3b3db1046155 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1697,12 +1697,13 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>   	i_mmap_lock_read(mapping);
>   	vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) {
>   		struct mmu_notifier_range range;
> +		bool retracted = false;
>   		struct mm_struct *mm;
>   		unsigned long addr;
>   		pmd_t *pmd, pgt_pmd;
>   		spinlock_t *pml;
>   		spinlock_t *ptl;
> -		bool skipped_uffd = false;
> +		pte_t *pte;
>   
>   		/*
>   		 * Check vma->anon_vma to exclude MAP_PRIVATE mappings that
> @@ -1739,9 +1740,17 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>   		mmu_notifier_invalidate_range_start(&range);
>   
>   		pml = pmd_lock(mm, pmd);
> -		ptl = pte_lockptr(mm, pmd);
> +
> +		/*
> +		 * No need to check the PTE table content, but we'll grab the
> +		 * PTE table lock while we zap it.
> +		 */
> +		pte = pte_offset_map_nolock(mm, pmd, addr, &ptl);
> +		if (!pte)
> +			goto unlock_pmd;
>   		if (ptl != pml)
>   			spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
> +		pte_unmap(pte);
>   
>   		/*
>   		 * Huge page lock is still held, so normally the page table
> @@ -1752,20 +1761,20 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>   		 * repeating the anon_vma check protects from one category,
>   		 * and repeating the userfaultfd_wp() check from another.
>   		 */
> -		if (unlikely(vma->anon_vma || userfaultfd_wp(vma))) {
> -			skipped_uffd = true;
> -		} else {
> +		if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) {
>   			pgt_pmd = pmdp_collapse_flush(vma, addr, pmd);
>   			pmdp_get_lockless_sync();
> +			retracted = true;
>   		}
>   
>   		if (ptl != pml)
>   			spin_unlock(ptl);
> +unlock_pmd:
>   		spin_unlock(pml);
>   
>   		mmu_notifier_invalidate_range_end(&range);
>   
> -		if (!skipped_uffd) {
> +		if (retracted) {
>   			mm_dec_nr_ptes(mm);
>   			page_table_check_pte_clear_range(mm, addr, pgt_pmd);
>   			pte_free_defer(mm, pmd_pgtable(pgt_pmd));
> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> index a78a4adf711ac..13a7705df3f87 100644
> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -313,7 +313,7 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd,
>   
>   	pte = __pte_offset_map(pmd, addr, &pmdval);
>   	if (likely(pte))
> -		*ptlp = pte_lockptr(mm, &pmdval);
> +		*ptlp = pte_lockptr(mm, pte);
>   	return pte;
>   }
>   
> @@ -371,7 +371,7 @@ pte_t *__pte_offset_map_lock(struct mm_struct *mm, pmd_t *pmd,
>   	pte = __pte_offset_map(pmd, addr, &pmdval);
>   	if (unlikely(!pte))
>   		return pte;
> -	ptl = pte_lockptr(mm, &pmdval);
> +	ptl = pte_lockptr(mm, pte);
>   	spin_lock(ptl);
>   	if (likely(pmd_same(pmdval, pmdp_get_lockless(pmd)))) {
>   		*ptlp = ptl;

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[David Hildenbrand]

On 29.07.24 09:48, Qi Zheng wrote:
> 
> 
> On 2024/7/26 02:39, David Hildenbrand wrote:
>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>> pointer.
>>
>> Let's change that. The two callers in pgtable-generic.c are easily
>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>> pte_offset_map_nolock() to obtain the lock, even though we won't actually
>> be traversing the page table.
>>
>> This makes the code more similar to the other variants and avoids other
>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>> reside now only in  pgtable-generic.c.
>>
>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>> the PTE table could have been removed in the meantime? At least it sounds
>> more future proof if we ever have other means of page table reclaim.
> 
> Agree, this helps us recheck the pmd entry.
> 
>>
>> It's not quite clear if holding the PTE table lock is really required:
>> what if someone else obtains the lock just after we unlock it? But we'll
>> leave that as is for now, maybe there are good reasons.
>>
>> This is a preparation for adapting hugetlb page table locking logic to
>> take the same locks as core-mm page table walkers would.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>    include/linux/mm.h   |  7 ++++---
>>    mm/khugepaged.c      | 21 +++++++++++++++------
>>    mm/pgtable-generic.c |  4 ++--
>>    3 files changed, 21 insertions(+), 11 deletions(-)
> 
> Since pte_lockptr() no longer has a pmd parameter, it is best to modify
> the comments above __pte_offset_map_lock() as well:
> 
> ```
> This helps the caller to avoid a later pte_lockptr(mm, *pmd), which
> might by that time act on a changed *pmd ...
> ```

Right, thanks a lot for the review!

The following on top;


 From a46b16aa9bfa02ffb425d364d7f00129a8e803ad Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Mon, 29 Jul 2024 10:43:34 +0200
Subject: [PATCH] fixup: mm: let pte_lockptr() consume a pte_t pointer

Let's adjust the comment, passing a pte to pte_lockptr() and dropping
a detail about changed *pmd, which no longer applies.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
  mm/pgtable-generic.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
index 13a7705df3f87..f17465b43d344 100644
--- a/mm/pgtable-generic.c
+++ b/mm/pgtable-generic.c
@@ -350,11 +350,11 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd,
   * pte_offset_map_nolock(mm, pmd, addr, ptlp), above, is like pte_offset_map();
   * but when successful, it also outputs a pointer to the spinlock in ptlp - as
   * pte_offset_map_lock() does, but in this case without locking it.  This helps
- * the caller to avoid a later pte_lockptr(mm, *pmd), which might by that time
- * act on a changed *pmd: pte_offset_map_nolock() provides the correct spinlock
- * pointer for the page table that it returns.  In principle, the caller should
- * recheck *pmd once the lock is taken; in practice, no callsite needs that -
- * either the mmap_lock for write, or pte_same() check on contents, is enough.
+ * the caller to avoid a later pte_lockptr(mm, pte): pte_offset_map_nolock()
+ * provides the correct spinlock pointer for the page table that it returns.
+ * In principle, the caller should recheck *pmd once the lock is taken; in
+ * practice, no callsite needs that - either the mmap_lock for write, or
+ * pte_same() check on contents, is enough.
   *
   * Note that free_pgtables(), used after unmapping detached vmas, or when
   * exiting the whole mm, does not take page table lock before freeing a page
-- 
2.45.2


-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Qi Zheng]

On 2024/7/29 16:46, David Hildenbrand wrote:
> On 29.07.24 09:48, Qi Zheng wrote:
>>
>>
>> On 2024/7/26 02:39, David Hildenbrand wrote:
>>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>>> pointer.
>>>
>>> Let's change that. The two callers in pgtable-generic.c are easily
>>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>>> pte_offset_map_nolock() to obtain the lock, even though we won't 
>>> actually
>>> be traversing the page table.
>>>
>>> This makes the code more similar to the other variants and avoids other
>>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>>> reside now only in  pgtable-generic.c.
>>>
>>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>>> the PTE table could have been removed in the meantime? At least it 
>>> sounds
>>> more future proof if we ever have other means of page table reclaim.
>>
>> Agree, this helps us recheck the pmd entry.
>>
>>>
>>> It's not quite clear if holding the PTE table lock is really required:
>>> what if someone else obtains the lock just after we unlock it? But we'll
>>> leave that as is for now, maybe there are good reasons.
>>>
>>> This is a preparation for adapting hugetlb page table locking logic to
>>> take the same locks as core-mm page table walkers would.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    include/linux/mm.h   |  7 ++++---
>>>    mm/khugepaged.c      | 21 +++++++++++++++------
>>>    mm/pgtable-generic.c |  4 ++--
>>>    3 files changed, 21 insertions(+), 11 deletions(-)
>>
>> Since pte_lockptr() no longer has a pmd parameter, it is best to modify
>> the comments above __pte_offset_map_lock() as well:
>>
>> ```
>> This helps the caller to avoid a later pte_lockptr(mm, *pmd), which
>> might by that time act on a changed *pmd ...
>> ```
> 
> Right, thanks a lot for the review!
> 
> The following on top;
> 
> 
>  From a46b16aa9bfa02ffb425d364d7f00129a8e803ad Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Mon, 29 Jul 2024 10:43:34 +0200
> Subject: [PATCH] fixup: mm: let pte_lockptr() consume a pte_t pointer
> 
> Let's adjust the comment, passing a pte to pte_lockptr() and dropping
> a detail about changed *pmd, which no longer applies.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   mm/pgtable-generic.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> index 13a7705df3f87..f17465b43d344 100644
> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -350,11 +350,11 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, 
> pmd_t *pmd,
>    * pte_offset_map_nolock(mm, pmd, addr, ptlp), above, is like 
> pte_offset_map();
>    * but when successful, it also outputs a pointer to the spinlock in 
> ptlp - as
>    * pte_offset_map_lock() does, but in this case without locking it.  
> This helps
> - * the caller to avoid a later pte_lockptr(mm, *pmd), which might by 
> that time
> - * act on a changed *pmd: pte_offset_map_nolock() provides the correct 
> spinlock
> - * pointer for the page table that it returns.  In principle, the 
> caller should
> - * recheck *pmd once the lock is taken; in practice, no callsite needs 
> that -
> - * either the mmap_lock for write, or pte_same() check on contents, is 
> enough.
> + * the caller to avoid a later pte_lockptr(mm, pte): 
> pte_offset_map_nolock()
> + * provides the correct spinlock pointer for the page table that it 
> returns.
> + * In principle, the caller should recheck *pmd once the lock is taken; in
> + * practice, no callsite needs that - either the mmap_lock for write, or
> + * pte_same() check on contents, is enough.
>    *
>    * Note that free_pgtables(), used after unmapping detached vmas, or when
>    * exiting the whole mm, does not take page table lock before freeing 
> a page

LGTM. Thanks!

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Marek Szyprowski]

On 25.07.2024 20:39, David Hildenbrand wrote:
> pte_lockptr() is the only *_lockptr() function that doesn't consume
> what would be expected: it consumes a pmd_t pointer instead of a pte_t
> pointer.
>
> Let's change that. The two callers in pgtable-generic.c are easily
> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
> pte_offset_map_nolock() to obtain the lock, even though we won't actually
> be traversing the page table.
>
> This makes the code more similar to the other variants and avoids other
> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
> reside now only in  pgtable-generic.c.
>
> Maybe, using pte_offset_map_nolock() is the right thing to do because
> the PTE table could have been removed in the meantime? At least it sounds
> more future proof if we ever have other means of page table reclaim.
>
> It's not quite clear if holding the PTE table lock is really required:
> what if someone else obtains the lock just after we unlock it? But we'll
> leave that as is for now, maybe there are good reasons.
>
> This is a preparation for adapting hugetlb page table locking logic to
> take the same locks as core-mm page table walkers would.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

This patch landed in today's linux-next as commit e98970a1d2d4 ("mm: let 
pte_lockptr() consume a pte_t pointer"). Unfortunately it causes the 
following issue on most of my ARM 32bit based test boards:

Run /sbin/init as init process
8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 
00000010 when read
[00000010] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 
6.10.0-12930-ge98970a1d2d4 #15137
Hardware name: Samsung Exynos (Flattened Device Tree)
PC is at __lock_acquire+0x20/0x1604
LR is at lock_acquire+0x21c/0x394
pc : [<c01990e4>]    lr : [<c019b2e8>]    psr: 20000093
sp : f082dc80  ip : 00000001  fp : c1d20000
r10: 00000000  r9 : c13096c8  r8 : 00000001
r7 : 00000000  r6 : c1d20000  r5 : c1278594  r4 : c1278594
r3 : 00000000  r2 : 00000000  r1 : 00000000  r0 : 00000010
Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 4000404a  DAC: 00000051
...
Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
Stack: (0xf082dc80 to 0xf082e000)
...
Call trace:
  __lock_acquire from lock_acquire+0x21c/0x394
  lock_acquire from _raw_spin_lock+0x3c/0x4c
  _raw_spin_lock from __pte_offset_map_lock+0x64/0x118
  __pte_offset_map_lock from handle_mm_fault+0xee0/0x13c8
  handle_mm_fault from __get_user_pages+0x18c/0x848
  __get_user_pages from get_user_pages_remote+0xdc/0x474
  get_user_pages_remote from get_arg_page+0x90/0x2a4
  get_arg_page from copy_string_kernel+0x14c/0x184
  copy_string_kernel from kernel_execve+0x94/0x174
  kernel_execve from try_to_run_init_process+0xc/0x3c
  try_to_run_init_process from kernel_init+0xcc/0x12c
  kernel_init from ret_from_fork+0x14/0x28
Exception stack(0xf082dfb0 to 0xf082dff8)
...
---[ end trace 0000000000000000 ]---
note: swapper/0[1] exited with irqs disabled
note: swapper/0[1] exited with preempt_count 1
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
---[ end Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x0000000b ]---

Reverting $subject on top of next-20240730 fixes/hides this issue.


> ---
>   include/linux/mm.h   |  7 ++++---
>   mm/khugepaged.c      | 21 +++++++++++++++------
>   mm/pgtable-generic.c |  4 ++--
>   3 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 2c6ccf088c7be..0472a5090b180 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2873,9 +2873,10 @@ static inline spinlock_t *ptlock_ptr(struct ptdesc *ptdesc)
>   }
>   #endif /* ALLOC_SPLIT_PTLOCKS */
>   
> -static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pmd_t *pmd)
> +static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pte_t *pte)
>   {
> -	return ptlock_ptr(page_ptdesc(pmd_page(*pmd)));
> +	/* PTE page tables don't currently exceed a single page. */
> +	return ptlock_ptr(virt_to_ptdesc(pte));
>   }
>   
>   static inline bool ptlock_init(struct ptdesc *ptdesc)
> @@ -2898,7 +2899,7 @@ static inline bool ptlock_init(struct ptdesc *ptdesc)
>   /*
>    * We use mm->page_table_lock to guard all pagetable pages of the mm.
>    */
> -static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pmd_t *pmd)
> +static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pte_t *pte)
>   {
>   	return &mm->page_table_lock;
>   }
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index cdd1d8655a76b..f3b3db1046155 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1697,12 +1697,13 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>   	i_mmap_lock_read(mapping);
>   	vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) {
>   		struct mmu_notifier_range range;
> +		bool retracted = false;
>   		struct mm_struct *mm;
>   		unsigned long addr;
>   		pmd_t *pmd, pgt_pmd;
>   		spinlock_t *pml;
>   		spinlock_t *ptl;
> -		bool skipped_uffd = false;
> +		pte_t *pte;
>   
>   		/*
>   		 * Check vma->anon_vma to exclude MAP_PRIVATE mappings that
> @@ -1739,9 +1740,17 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>   		mmu_notifier_invalidate_range_start(&range);
>   
>   		pml = pmd_lock(mm, pmd);
> -		ptl = pte_lockptr(mm, pmd);
> +
> +		/*
> +		 * No need to check the PTE table content, but we'll grab the
> +		 * PTE table lock while we zap it.
> +		 */
> +		pte = pte_offset_map_nolock(mm, pmd, addr, &ptl);
> +		if (!pte)
> +			goto unlock_pmd;
>   		if (ptl != pml)
>   			spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
> +		pte_unmap(pte);
>   
>   		/*
>   		 * Huge page lock is still held, so normally the page table
> @@ -1752,20 +1761,20 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>   		 * repeating the anon_vma check protects from one category,
>   		 * and repeating the userfaultfd_wp() check from another.
>   		 */
> -		if (unlikely(vma->anon_vma || userfaultfd_wp(vma))) {
> -			skipped_uffd = true;
> -		} else {
> +		if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) {
>   			pgt_pmd = pmdp_collapse_flush(vma, addr, pmd);
>   			pmdp_get_lockless_sync();
> +			retracted = true;
>   		}
>   
>   		if (ptl != pml)
>   			spin_unlock(ptl);
> +unlock_pmd:
>   		spin_unlock(pml);
>   
>   		mmu_notifier_invalidate_range_end(&range);
>   
> -		if (!skipped_uffd) {
> +		if (retracted) {
>   			mm_dec_nr_ptes(mm);
>   			page_table_check_pte_clear_range(mm, addr, pgt_pmd);
>   			pte_free_defer(mm, pmd_pgtable(pgt_pmd));
> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> index a78a4adf711ac..13a7705df3f87 100644
> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -313,7 +313,7 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd,
>   
>   	pte = __pte_offset_map(pmd, addr, &pmdval);
>   	if (likely(pte))
> -		*ptlp = pte_lockptr(mm, &pmdval);
> +		*ptlp = pte_lockptr(mm, pte);
>   	return pte;
>   }
>   
> @@ -371,7 +371,7 @@ pte_t *__pte_offset_map_lock(struct mm_struct *mm, pmd_t *pmd,
>   	pte = __pte_offset_map(pmd, addr, &pmdval);
>   	if (unlikely(!pte))
>   		return pte;
> -	ptl = pte_lockptr(mm, &pmdval);
> +	ptl = pte_lockptr(mm, pte);
>   	spin_lock(ptl);
>   	if (likely(pmd_same(pmdval, pmdp_get_lockless(pmd)))) {
>   		*ptlp = ptl;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[David Hildenbrand]

On 30.07.24 17:30, Marek Szyprowski wrote:
> On 25.07.2024 20:39, David Hildenbrand wrote:
>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>> pointer.
>>
>> Let's change that. The two callers in pgtable-generic.c are easily
>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>> pte_offset_map_nolock() to obtain the lock, even though we won't actually
>> be traversing the page table.
>>
>> This makes the code more similar to the other variants and avoids other
>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>> reside now only in  pgtable-generic.c.
>>
>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>> the PTE table could have been removed in the meantime? At least it sounds
>> more future proof if we ever have other means of page table reclaim.
>>
>> It's not quite clear if holding the PTE table lock is really required:
>> what if someone else obtains the lock just after we unlock it? But we'll
>> leave that as is for now, maybe there are good reasons.
>>
>> This is a preparation for adapting hugetlb page table locking logic to
>> take the same locks as core-mm page table walkers would.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> This patch landed in today's linux-next as commit e98970a1d2d4 ("mm: let
> pte_lockptr() consume a pte_t pointer"). Unfortunately it causes the
> following issue on most of my ARM 32bit based test boards:
> 

That is ... rather surprising.

The issue below seems to point at __pte_offset_map_lock(), where we 
essentially convert from

ptlock_ptr(page_ptdesc(pmd_page(*pmd)));

to

ptlock_ptr(virt_to_ptdesc(pte));


So we would get a NULL ptr from the ptdesc. Either the lock would 
actually not be allocated or virt_to_ptdesc() does something unexpected.

Leaf page tables on arm are also a single page, so we cannot possibly be 
staring at the wrong page-table-subpage.

Do we maybe have to special-case init-mm? But I don't see how that 
special-casing would have happened for now :/

What's your kernel config value of SPLIT_PTLOCK_CPUS?


-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[David Hildenbrand]

On 30.07.24 17:45, David Hildenbrand wrote:
> On 30.07.24 17:30, Marek Szyprowski wrote:
>> On 25.07.2024 20:39, David Hildenbrand wrote:
>>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>>> pointer.
>>>
>>> Let's change that. The two callers in pgtable-generic.c are easily
>>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>>> pte_offset_map_nolock() to obtain the lock, even though we won't actually
>>> be traversing the page table.
>>>
>>> This makes the code more similar to the other variants and avoids other
>>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>>> reside now only in  pgtable-generic.c.
>>>
>>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>>> the PTE table could have been removed in the meantime? At least it sounds
>>> more future proof if we ever have other means of page table reclaim.
>>>
>>> It's not quite clear if holding the PTE table lock is really required:
>>> what if someone else obtains the lock just after we unlock it? But we'll
>>> leave that as is for now, maybe there are good reasons.
>>>
>>> This is a preparation for adapting hugetlb page table locking logic to
>>> take the same locks as core-mm page table walkers would.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>
>> This patch landed in today's linux-next as commit e98970a1d2d4 ("mm: let
>> pte_lockptr() consume a pte_t pointer"). Unfortunately it causes the
>> following issue on most of my ARM 32bit based test boards:
>>
> 
> That is ... rather surprising.
> 
> The issue below seems to point at __pte_offset_map_lock(), where we
> essentially convert from
> 
> ptlock_ptr(page_ptdesc(pmd_page(*pmd)));
> 
> to
> 
> ptlock_ptr(virt_to_ptdesc(pte));

I'm wondering, is highmem involved here such that the PTE would be 
kmap'ed and virt_to_page() would not do what we would expect it to do?

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Marek Szyprowski]

On 30.07.2024 17:49, David Hildenbrand wrote:
> On 30.07.24 17:45, David Hildenbrand wrote:
>> On 30.07.24 17:30, Marek Szyprowski wrote:
>>> On 25.07.2024 20:39, David Hildenbrand wrote:
>>>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>>>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>>>> pointer.
>>>>
>>>> Let's change that. The two callers in pgtable-generic.c are easily
>>>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>>>> pte_offset_map_nolock() to obtain the lock, even though we won't 
>>>> actually
>>>> be traversing the page table.
>>>>
>>>> This makes the code more similar to the other variants and avoids 
>>>> other
>>>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>>>> reside now only in  pgtable-generic.c.
>>>>
>>>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>>>> the PTE table could have been removed in the meantime? At least it 
>>>> sounds
>>>> more future proof if we ever have other means of page table reclaim.
>>>>
>>>> It's not quite clear if holding the PTE table lock is really required:
>>>> what if someone else obtains the lock just after we unlock it? But 
>>>> we'll
>>>> leave that as is for now, maybe there are good reasons.
>>>>
>>>> This is a preparation for adapting hugetlb page table locking logic to
>>>> take the same locks as core-mm page table walkers would.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>
>>> This patch landed in today's linux-next as commit e98970a1d2d4 ("mm: 
>>> let
>>> pte_lockptr() consume a pte_t pointer"). Unfortunately it causes the
>>> following issue on most of my ARM 32bit based test boards:
>>>
>>
>> That is ... rather surprising.
>>
>> The issue below seems to point at __pte_offset_map_lock(), where we
>> essentially convert from
>>
>> ptlock_ptr(page_ptdesc(pmd_page(*pmd)));
>>
>> to
>>
>> ptlock_ptr(virt_to_ptdesc(pte));
>
> I'm wondering, is highmem involved here such that the PTE would be 
> kmap'ed and virt_to_page() would not do what we would expect it to do?

Yes, highmem is enabled on those boards and all of them have 1GB+ of 
RAM. For other kernel configuration options see 
arch/arm/configs/exynos_defconfig.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[David Hildenbrand]

On 30.07.24 18:08, Marek Szyprowski wrote:
> On 30.07.2024 17:49, David Hildenbrand wrote:
>> On 30.07.24 17:45, David Hildenbrand wrote:
>>> On 30.07.24 17:30, Marek Szyprowski wrote:
>>>> On 25.07.2024 20:39, David Hildenbrand wrote:
>>>>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>>>>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>>>>> pointer.
>>>>>
>>>>> Let's change that. The two callers in pgtable-generic.c are easily
>>>>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>>>>> pte_offset_map_nolock() to obtain the lock, even though we won't
>>>>> actually
>>>>> be traversing the page table.
>>>>>
>>>>> This makes the code more similar to the other variants and avoids
>>>>> other
>>>>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>>>>> reside now only in  pgtable-generic.c.
>>>>>
>>>>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>>>>> the PTE table could have been removed in the meantime? At least it
>>>>> sounds
>>>>> more future proof if we ever have other means of page table reclaim.
>>>>>
>>>>> It's not quite clear if holding the PTE table lock is really required:
>>>>> what if someone else obtains the lock just after we unlock it? But
>>>>> we'll
>>>>> leave that as is for now, maybe there are good reasons.
>>>>>
>>>>> This is a preparation for adapting hugetlb page table locking logic to
>>>>> take the same locks as core-mm page table walkers would.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>
>>>> This patch landed in today's linux-next as commit e98970a1d2d4 ("mm:
>>>> let
>>>> pte_lockptr() consume a pte_t pointer"). Unfortunately it causes the
>>>> following issue on most of my ARM 32bit based test boards:
>>>>
>>>
>>> That is ... rather surprising.
>>>
>>> The issue below seems to point at __pte_offset_map_lock(), where we
>>> essentially convert from
>>>
>>> ptlock_ptr(page_ptdesc(pmd_page(*pmd)));
>>>
>>> to
>>>
>>> ptlock_ptr(virt_to_ptdesc(pte));
>>
>> I'm wondering, is highmem involved here such that the PTE would be
>> kmap'ed and virt_to_page() would not do what we would expect it to do?
> 
> Yes, highmem is enabled on those boards and all of them have 1GB+ of
> RAM. For other kernel configuration options see
> arch/arm/configs/exynos_defconfig.

Yes, pretty sure that's it. virt_to_page() won't work on kmaped pages.
So looks like we cannot easily do the conversion in this patch. :(

We'll have to get hacky in patch #2 instead.

@Andrew, can you drop both patches for now? I'll send a v2 that
essentially does in v2 on top something like:
  

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index da800e56fe590..c2e330b1eee21 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -963,7 +963,13 @@ static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
          * will give us the same result: the per-MM PT lock.
          */
         if (huge_page_size(h) < PMD_SIZE)
-               return pte_lockptr(mm, pte);
+               /*
+                * pte_lockptr() needs the PMD, which we don't have. Because of
+                * highmem we cannot convert pte_lockptr() to consume a pte.
+                * But as we never have highmem page tables in hugetlb, we can
+                * safely use virt_to_ptdesc() here.
+                */
+               return ptlock_ptr(virt_to_ptdesc(pte));
         else if (huge_page_size(h) < PUD_SIZE)
                 return pmd_lockptr(mm, (pmd_t *) pte);
         return pud_lockptr(mm, (pud_t *) pte);


-- 
Cheers,

David / dhildenb

[PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[David Hildenbrand]

We recently made GUP's common page table walking code to also walk
hugetlb VMAs without most hugetlb special-casing, preparing for the
future of having less hugetlb-specific page table walking code in the
codebase. Turns out that we missed one page table locking detail: page
table locking for hugetlb folios that are not mapped using a single
PMD/PUD.

Assume we have hugetlb folio that spans multiple PTEs (e.g., 64 KiB
hugetlb folios on arm64 with 4 KiB base page size). GUP, as it walks the
page tables, will perform a pte_offset_map_lock() to grab the PTE table
lock.

However, hugetlb that concurrently modifies these page tables would
actually grab the mm->page_table_lock: with USE_SPLIT_PTE_PTLOCKS, the
locks would differ. Something similar can happen right now with hugetlb
folios that span multiple PMDs when USE_SPLIT_PMD_PTLOCKS.

Let's make huge_pte_lockptr() effectively uses the same PT locks as any
core-mm page table walker would.

There is one ugly case: powerpc 8xx, whereby we have an 8 MiB hugetlb
folio being mapped using two PTE page tables. While hugetlb wants to take
the PMD table lock, core-mm would grab the PTE table lock of one of both
PTE page tables. In such corner cases, we have to make sure that both
locks match, which is (fortunately!) currently guaranteed for 8xx as it
does not support SMP.

Fixes: 9cb28da54643 ("mm/gup: handle hugetlb in the generic follow_page_mask code")
Cc: <stable@vger.kernel.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/linux/hugetlb.h | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index c9bf68c239a01..da800e56fe590 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -944,10 +944,29 @@ static inline bool htlb_allow_alloc_fallback(int reason)
 static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
 					   struct mm_struct *mm, pte_t *pte)
 {
-	if (huge_page_size(h) == PMD_SIZE)
+	VM_WARN_ON(huge_page_size(h) == PAGE_SIZE);
+	VM_WARN_ON(huge_page_size(h) >= P4D_SIZE);
+
+	/*
+	 * hugetlb must use the exact same PT locks as core-mm page table
+	 * walkers would. When modifying a PTE table, hugetlb must take the
+	 * PTE PT lock, when modifying a PMD table, hugetlb must take the PMD
+	 * PT lock etc.
+	 *
+	 * The expectation is that any hugetlb folio smaller than a PMD is
+	 * always mapped into a single PTE table and that any hugetlb folio
+	 * smaller than a PUD (but at least as big as a PMD) is always mapped
+	 * into a single PMD table.
+	 *
+	 * If that does not hold for an architecture, then that architecture
+	 * must disable split PT locks such that all *_lockptr() functions
+	 * will give us the same result: the per-MM PT lock.
+	 */
+	if (huge_page_size(h) < PMD_SIZE)
+		return pte_lockptr(mm, pte);
+	else if (huge_page_size(h) < PUD_SIZE)
 		return pmd_lockptr(mm, (pmd_t *) pte);
-	VM_BUG_ON(huge_page_size(h) == PAGE_SIZE);
-	return &mm->page_table_lock;
+	return pud_lockptr(mm, (pud_t *) pte);
 }
 
 #ifndef hugepages_supported
-- 
2.45.2

Re: [PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[Baolin Wang]

On 2024/7/26 02:39, David Hildenbrand wrote:
> We recently made GUP's common page table walking code to also walk
> hugetlb VMAs without most hugetlb special-casing, preparing for the
> future of having less hugetlb-specific page table walking code in the
> codebase. Turns out that we missed one page table locking detail: page
> table locking for hugetlb folios that are not mapped using a single
> PMD/PUD.
> 
> Assume we have hugetlb folio that spans multiple PTEs (e.g., 64 KiB
> hugetlb folios on arm64 with 4 KiB base page size). GUP, as it walks the
> page tables, will perform a pte_offset_map_lock() to grab the PTE table
> lock.
> 
> However, hugetlb that concurrently modifies these page tables would
> actually grab the mm->page_table_lock: with USE_SPLIT_PTE_PTLOCKS, the
> locks would differ. Something similar can happen right now with hugetlb
> folios that span multiple PMDs when USE_SPLIT_PMD_PTLOCKS.
> 
> Let's make huge_pte_lockptr() effectively uses the same PT locks as any
> core-mm page table walker would.

Thanks for raising the issue again. I remember fixing this issue 2 years 
ago in commit fac35ba763ed ("mm/hugetlb: fix races when looking up a 
CONT-PTE/PMD size hugetlb page"), but it seems to be broken again.

> There is one ugly case: powerpc 8xx, whereby we have an 8 MiB hugetlb
> folio being mapped using two PTE page tables. While hugetlb wants to take
> the PMD table lock, core-mm would grab the PTE table lock of one of both
> PTE page tables. In such corner cases, we have to make sure that both
> locks match, which is (fortunately!) currently guaranteed for 8xx as it
> does not support SMP.
> 
> Fixes: 9cb28da54643 ("mm/gup: handle hugetlb in the generic follow_page_mask code")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   include/linux/hugetlb.h | 25 ++++++++++++++++++++++---
>   1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index c9bf68c239a01..da800e56fe590 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -944,10 +944,29 @@ static inline bool htlb_allow_alloc_fallback(int reason)
>   static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
>   					   struct mm_struct *mm, pte_t *pte)
>   {
> -	if (huge_page_size(h) == PMD_SIZE)
> +	VM_WARN_ON(huge_page_size(h) == PAGE_SIZE);
> +	VM_WARN_ON(huge_page_size(h) >= P4D_SIZE);
> +
> +	/*
> +	 * hugetlb must use the exact same PT locks as core-mm page table
> +	 * walkers would. When modifying a PTE table, hugetlb must take the
> +	 * PTE PT lock, when modifying a PMD table, hugetlb must take the PMD
> +	 * PT lock etc.
> +	 *
> +	 * The expectation is that any hugetlb folio smaller than a PMD is
> +	 * always mapped into a single PTE table and that any hugetlb folio
> +	 * smaller than a PUD (but at least as big as a PMD) is always mapped
> +	 * into a single PMD table.

ARM64 also supports cont-PMD size hugetlb, which is 32MiB size with a 4 
KiB base page size. This means the PT locks for 32MiB hugetlb may race 
again, as we currently only hold one PMD lock for several PMD entries of 
a cont-PMD size hugetlb.

> +	 *
> +	 * If that does not hold for an architecture, then that architecture
> +	 * must disable split PT locks such that all *_lockptr() functions
> +	 * will give us the same result: the per-MM PT lock.
> +	 */
> +	if (huge_page_size(h) < PMD_SIZE)
> +		return pte_lockptr(mm, pte);
> +	else if (huge_page_size(h) < PUD_SIZE)
>   		return pmd_lockptr(mm, (pmd_t *) pte);

IIUC, as I said above, this change doesn't fix the inconsistent lock for 
cont-PMD size hugetlb for GUP, and it will also break the lock rule for 
unmapping/migrating a cont-PMD size hugetlb (use mm->page_table_lock 
before for cont-PMD size hugetlb before).

> -	VM_BUG_ON(huge_page_size(h) == PAGE_SIZE);
> -	return &mm->page_table_lock;
> +	return pud_lockptr(mm, (pud_t *) pte);
>   }
>   
>   #ifndef hugepages_supported

Re: [PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[Baolin Wang]

On 2024/7/26 10:33, Baolin Wang wrote:
> 
> 
> On 2024/7/26 02:39, David Hildenbrand wrote:
>> We recently made GUP's common page table walking code to also walk
>> hugetlb VMAs without most hugetlb special-casing, preparing for the
>> future of having less hugetlb-specific page table walking code in the
>> codebase. Turns out that we missed one page table locking detail: page
>> table locking for hugetlb folios that are not mapped using a single
>> PMD/PUD.
>>
>> Assume we have hugetlb folio that spans multiple PTEs (e.g., 64 KiB
>> hugetlb folios on arm64 with 4 KiB base page size). GUP, as it walks the
>> page tables, will perform a pte_offset_map_lock() to grab the PTE table
>> lock.
>>
>> However, hugetlb that concurrently modifies these page tables would
>> actually grab the mm->page_table_lock: with USE_SPLIT_PTE_PTLOCKS, the
>> locks would differ. Something similar can happen right now with hugetlb
>> folios that span multiple PMDs when USE_SPLIT_PMD_PTLOCKS.
>>
>> Let's make huge_pte_lockptr() effectively uses the same PT locks as any
>> core-mm page table walker would.
> 
> Thanks for raising the issue again. I remember fixing this issue 2 years 
> ago in commit fac35ba763ed ("mm/hugetlb: fix races when looking up a 
> CONT-PTE/PMD size hugetlb page"), but it seems to be broken again.
> 
>> There is one ugly case: powerpc 8xx, whereby we have an 8 MiB hugetlb
>> folio being mapped using two PTE page tables. While hugetlb wants to take
>> the PMD table lock, core-mm would grab the PTE table lock of one of both
>> PTE page tables. In such corner cases, we have to make sure that both
>> locks match, which is (fortunately!) currently guaranteed for 8xx as it
>> does not support SMP.
>>
>> Fixes: 9cb28da54643 ("mm/gup: handle hugetlb in the generic 
>> follow_page_mask code")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   include/linux/hugetlb.h | 25 ++++++++++++++++++++++---
>>   1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> index c9bf68c239a01..da800e56fe590 100644
>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -944,10 +944,29 @@ static inline bool htlb_allow_alloc_fallback(int 
>> reason)
>>   static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
>>                          struct mm_struct *mm, pte_t *pte)
>>   {
>> -    if (huge_page_size(h) == PMD_SIZE)
>> +    VM_WARN_ON(huge_page_size(h) == PAGE_SIZE);
>> +    VM_WARN_ON(huge_page_size(h) >= P4D_SIZE);
>> +
>> +    /*
>> +     * hugetlb must use the exact same PT locks as core-mm page table
>> +     * walkers would. When modifying a PTE table, hugetlb must take the
>> +     * PTE PT lock, when modifying a PMD table, hugetlb must take the 
>> PMD
>> +     * PT lock etc.
>> +     *
>> +     * The expectation is that any hugetlb folio smaller than a PMD is
>> +     * always mapped into a single PTE table and that any hugetlb folio
>> +     * smaller than a PUD (but at least as big as a PMD) is always 
>> mapped
>> +     * into a single PMD table.
> 
> ARM64 also supports cont-PMD size hugetlb, which is 32MiB size with a 4 
> KiB base page size. This means the PT locks for 32MiB hugetlb may race 
> again, as we currently only hold one PMD lock for several PMD entries of 
> a cont-PMD size hugetlb.
> 
>> +     *
>> +     * If that does not hold for an architecture, then that architecture
>> +     * must disable split PT locks such that all *_lockptr() functions
>> +     * will give us the same result: the per-MM PT lock.
>> +     */
>> +    if (huge_page_size(h) < PMD_SIZE)
>> +        return pte_lockptr(mm, pte);
>> +    else if (huge_page_size(h) < PUD_SIZE)
>>           return pmd_lockptr(mm, (pmd_t *) pte);
> 
> IIUC, as I said above, this change doesn't fix the inconsistent lock for 
> cont-PMD size hugetlb for GUP, and it will also break the lock rule for 
> unmapping/migrating a cont-PMD size hugetlb (use mm->page_table_lock 
> before for cont-PMD size hugetlb before).

After more thinking, I realized I confused the PMD table with the PMD 
entry. Therefore, using the PMD table's lock is safe for cont-PMD size 
hugetlb. This change looks good to me. Sorry for noise.

Please feel free to add:
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> 
>> -    VM_BUG_ON(huge_page_size(h) == PAGE_SIZE);
>> -    return &mm->page_table_lock;
>> +    return pud_lockptr(mm, (pud_t *) pte);
>>   }
>>   #ifndef hugepages_supported

Re: [PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[David Hildenbrand]

>>
>>> +     *
>>> +     * If that does not hold for an architecture, then that architecture
>>> +     * must disable split PT locks such that all *_lockptr() functions
>>> +     * will give us the same result: the per-MM PT lock.
>>> +     */
>>> +    if (huge_page_size(h) < PMD_SIZE)
>>> +        return pte_lockptr(mm, pte);
>>> +    else if (huge_page_size(h) < PUD_SIZE)
>>>            return pmd_lockptr(mm, (pmd_t *) pte);
>>
>> IIUC, as I said above, this change doesn't fix the inconsistent lock for
>> cont-PMD size hugetlb for GUP, and it will also break the lock rule for
>> unmapping/migrating a cont-PMD size hugetlb (use mm->page_table_lock
>> before for cont-PMD size hugetlb before).
> 
> After more thinking, I realized I confused the PMD table with the PMD
> entry. Therefore, using the PMD table's lock is safe for cont-PMD size
> hugetlb. This change looks good to me. Sorry for noise.
> 

Thanks for the review, highly appreciated!

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[David Hildenbrand]

On 26.07.24 04:33, Baolin Wang wrote:
> 
> 
> On 2024/7/26 02:39, David Hildenbrand wrote:
>> We recently made GUP's common page table walking code to also walk
>> hugetlb VMAs without most hugetlb special-casing, preparing for the
>> future of having less hugetlb-specific page table walking code in the
>> codebase. Turns out that we missed one page table locking detail: page
>> table locking for hugetlb folios that are not mapped using a single
>> PMD/PUD.
>>
>> Assume we have hugetlb folio that spans multiple PTEs (e.g., 64 KiB
>> hugetlb folios on arm64 with 4 KiB base page size). GUP, as it walks the
>> page tables, will perform a pte_offset_map_lock() to grab the PTE table
>> lock.
>>
>> However, hugetlb that concurrently modifies these page tables would
>> actually grab the mm->page_table_lock: with USE_SPLIT_PTE_PTLOCKS, the
>> locks would differ. Something similar can happen right now with hugetlb
>> folios that span multiple PMDs when USE_SPLIT_PMD_PTLOCKS.
>>
>> Let's make huge_pte_lockptr() effectively uses the same PT locks as any
>> core-mm page table walker would.
> 
> Thanks for raising the issue again. I remember fixing this issue 2 years
> ago in commit fac35ba763ed ("mm/hugetlb: fix races when looking up a
> CONT-PTE/PMD size hugetlb page"), but it seems to be broken again.
> 

Ah, right! We fixed it by rerouting to hugetlb code that we then removed :D

Did we have a reproducer back then that would make my live easier?

>> There is one ugly case: powerpc 8xx, whereby we have an 8 MiB hugetlb
>> folio being mapped using two PTE page tables. While hugetlb wants to take
>> the PMD table lock, core-mm would grab the PTE table lock of one of both
>> PTE page tables. In such corner cases, we have to make sure that both
>> locks match, which is (fortunately!) currently guaranteed for 8xx as it
>> does not support SMP.
>>
>> Fixes: 9cb28da54643 ("mm/gup: handle hugetlb in the generic follow_page_mask code")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>    include/linux/hugetlb.h | 25 ++++++++++++++++++++++---
>>    1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> index c9bf68c239a01..da800e56fe590 100644
>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -944,10 +944,29 @@ static inline bool htlb_allow_alloc_fallback(int reason)
>>    static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
>>    					   struct mm_struct *mm, pte_t *pte)
>>    {
>> -	if (huge_page_size(h) == PMD_SIZE)
>> +	VM_WARN_ON(huge_page_size(h) == PAGE_SIZE);
>> +	VM_WARN_ON(huge_page_size(h) >= P4D_SIZE);
>> +
>> +	/*
>> +	 * hugetlb must use the exact same PT locks as core-mm page table
>> +	 * walkers would. When modifying a PTE table, hugetlb must take the
>> +	 * PTE PT lock, when modifying a PMD table, hugetlb must take the PMD
>> +	 * PT lock etc.
>> +	 *
>> +	 * The expectation is that any hugetlb folio smaller than a PMD is
>> +	 * always mapped into a single PTE table and that any hugetlb folio
>> +	 * smaller than a PUD (but at least as big as a PMD) is always mapped
>> +	 * into a single PMD table.
> 
> ARM64 also supports cont-PMD size hugetlb, which is 32MiB size with a 4
> KiB base page size. This means the PT locks for 32MiB hugetlb may race
> again, as we currently only hold one PMD lock for several PMD entries of
> a cont-PMD size hugetlb.

Exactly, that's the case where all cont-PMD entries fall into the same 
page table.

That's also what I will try reproducing with (migration racing with 
GUP). But the race window is small and a lot of other stuff is protected 
by the VMA lock.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[Baolin Wang]

On 2024/7/26 16:04, David Hildenbrand wrote:
> On 26.07.24 04:33, Baolin Wang wrote:
>>
>>
>> On 2024/7/26 02:39, David Hildenbrand wrote:
>>> We recently made GUP's common page table walking code to also walk
>>> hugetlb VMAs without most hugetlb special-casing, preparing for the
>>> future of having less hugetlb-specific page table walking code in the
>>> codebase. Turns out that we missed one page table locking detail: page
>>> table locking for hugetlb folios that are not mapped using a single
>>> PMD/PUD.
>>>
>>> Assume we have hugetlb folio that spans multiple PTEs (e.g., 64 KiB
>>> hugetlb folios on arm64 with 4 KiB base page size). GUP, as it walks the
>>> page tables, will perform a pte_offset_map_lock() to grab the PTE table
>>> lock.
>>>
>>> However, hugetlb that concurrently modifies these page tables would
>>> actually grab the mm->page_table_lock: with USE_SPLIT_PTE_PTLOCKS, the
>>> locks would differ. Something similar can happen right now with hugetlb
>>> folios that span multiple PMDs when USE_SPLIT_PMD_PTLOCKS.
>>>
>>> Let's make huge_pte_lockptr() effectively uses the same PT locks as any
>>> core-mm page table walker would.
>>
>> Thanks for raising the issue again. I remember fixing this issue 2 years
>> ago in commit fac35ba763ed ("mm/hugetlb: fix races when looking up a
>> CONT-PTE/PMD size hugetlb page"), but it seems to be broken again.
>>
> 
> Ah, right! We fixed it by rerouting to hugetlb code that we then removed :D
> 
> Did we have a reproducer back then that would make my live easier?

I don't have any reproducers right now. I remember I added some ugly 
hack code (adding delay() etc.) in kernel to analyze this issue, and not 
easy to reproduce. :(

Re: [PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[David Hildenbrand]

On 26.07.24 11:38, Baolin Wang wrote:
> 
> 
> On 2024/7/26 16:04, David Hildenbrand wrote:
>> On 26.07.24 04:33, Baolin Wang wrote:
>>>
>>>
>>> On 2024/7/26 02:39, David Hildenbrand wrote:
>>>> We recently made GUP's common page table walking code to also walk
>>>> hugetlb VMAs without most hugetlb special-casing, preparing for the
>>>> future of having less hugetlb-specific page table walking code in the
>>>> codebase. Turns out that we missed one page table locking detail: page
>>>> table locking for hugetlb folios that are not mapped using a single
>>>> PMD/PUD.
>>>>
>>>> Assume we have hugetlb folio that spans multiple PTEs (e.g., 64 KiB
>>>> hugetlb folios on arm64 with 4 KiB base page size). GUP, as it walks the
>>>> page tables, will perform a pte_offset_map_lock() to grab the PTE table
>>>> lock.
>>>>
>>>> However, hugetlb that concurrently modifies these page tables would
>>>> actually grab the mm->page_table_lock: with USE_SPLIT_PTE_PTLOCKS, the
>>>> locks would differ. Something similar can happen right now with hugetlb
>>>> folios that span multiple PMDs when USE_SPLIT_PMD_PTLOCKS.
>>>>
>>>> Let's make huge_pte_lockptr() effectively uses the same PT locks as any
>>>> core-mm page table walker would.
>>>
>>> Thanks for raising the issue again. I remember fixing this issue 2 years
>>> ago in commit fac35ba763ed ("mm/hugetlb: fix races when looking up a
>>> CONT-PTE/PMD size hugetlb page"), but it seems to be broken again.
>>>
>>
>> Ah, right! We fixed it by rerouting to hugetlb code that we then removed :D
>>
>> Did we have a reproducer back then that would make my live easier?
> 
> I don't have any reproducers right now. I remember I added some ugly
> hack code (adding delay() etc.) in kernel to analyze this issue, and not
> easy to reproduce. :(

Hah!

I tried with 32MB without luck -- migration simply takes too long. 64KB
did the trick within seconds!


On a VM with 2 vNUMA nodes, after reserving a bunch of 64KiB hugetlb pages:

# numactl -H
available: 2 nodes (0-1)
node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
node 0 size: 64439 MB
node 0 free: 64097 MB
node 1 cpus: 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
node 1 size: 64205 MB
node 1 free: 63809 MB
node distances:
node   0   1
   0:  10  20
   1:  20  10
# echo 100  > /sys/kernel/mm/hugepages/hugepages-64kB/nr_hugepagespages-64kB/nr_hugepagesepages-64kB/nr_hugepages
# gcc reproducer.c -o reproducer -O3 -lnuma -lpthread
# ./reproducer
[ 3105.936100] ------------[ cut here ]------------
[ 3105.939323] WARNING: CPU: 31 PID: 2732 at mm/gup.c:142 try_grab_folio+0x11c/0x188
[ 3105.944634] Modules linked in: nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables qrtr sunrpc vfat fat ext4 mbcache jbd2 virtio_net net_failover failover virtio_balloon dimlib loop dm_multipath nfnetlink zram xfs crct10dif_ce ghash_ce sha2_ce sha256_arm64 sha1_ce virtio_blk virtio_console virtio_mmio dm_mirror dm_region_hash dm_log dm_mod fuse
[ 3105.974841] CPU: 31 PID: 2732 Comm: reproducer Not tainted 6.10.0-64.eln141.aarch64 #1
[ 3105.980406] Hardware name: QEMU KVM Virtual Machine, BIOS edk2-20240524-4.fc40 05/24/2024
[ 3105.986185] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 3105.991108] pc : try_grab_folio+0x11c/0x188
[ 3105.994013] lr : follow_page_pte+0xd8/0x430
[ 3105.996986] sp : ffff80008eafb8f0
[ 3105.999346] x29: ffff80008eafb900 x28: ffffffe8d481f380 x27: 00f80001207cff43
[ 3106.004414] x26: 0000000000000001 x25: 0000000000000000 x24: ffff80008eafba48
[ 3106.009520] x23: 0000ffff9372f000 x22: ffff7a54459e2000 x21: ffff7a546c1aa978
[ 3106.014529] x20: ffffffe8d481f3c0 x19: 0000000000610041 x18: 0000000000000001
[ 3106.019506] x17: 0000000000000001 x16: ffffffffffffffff x15: 0000000000000000
[ 3106.024494] x14: ffffb85477fdfe08 x13: 0000ffff9372ffff x12: 0000000000000000
[ 3106.029469] x11: 1fffef4a88a96be1 x10: ffff7a54454b5f0c x9 : ffffb854771b12f0
[ 3106.034324] x8 : 0008000000000000 x7 : ffff7a546c1aa980 x6 : 0008000000000080
[ 3106.038902] x5 : 00000000001207cf x4 : 0000ffff9372f000 x3 : ffffffe8d481f000
[ 3106.043420] x2 : 0000000000610041 x1 : 0000000000000001 x0 : 0000000000000000
[ 3106.047957] Call trace:
[ 3106.049522]  try_grab_folio+0x11c/0x188
[ 3106.051996]  follow_pmd_mask.constprop.0.isra.0+0x150/0x2e0
[ 3106.055527]  follow_page_mask+0x1a0/0x2b8
[ 3106.058118]  __get_user_pages+0xf0/0x348
[ 3106.060647]  faultin_page_range+0xb0/0x360
[ 3106.063651]  do_madvise+0x340/0x598
...



# cat reproducer.c
#include <stdio.h>
#include <sys/mman.h>
#include <linux/mman.h>
#include <errno.h>
#include <string.h>
#include <numaif.h>
#include <pthread.h>

#define SIZE_64KB (64 * 1024ul)

static void *thread_fn(void *arg)
{
         char *mem = arg;

         /* Let GUP go crazy on the page without grabbing a reference. */
         while (1) {
                 if (madvise(mem, SIZE_64KB, MADV_POPULATE_WRITE)) {
                         fprintf(stderr, "madvise() failed: %d\n", errno);
                 }
         }
}

int main(void)
{
         pthread_t thread;
         unsigned int i;
         char *mem;

         mem = mmap(0, SIZE_64KB, PROT_READ|PROT_WRITE,
                    MAP_ANON|MAP_PRIVATE|MAP_HUGETLB|MAP_HUGE_64KB, -1, 0);
         if (mem == MAP_FAILED) {
                 fprintf(stderr, "mmap() failed: %d\n", errno);
                 return -1;
         }

         memset(mem, 0, SIZE_64KB);

         pthread_create(&thread, NULL, thread_fn, mem);

         /* Migrate it back and forth between two nodes. */
         for (i = 0; ; i++) {
                 void *pages[1] = { mem, };
                 int nodes[1] = { i % 2, };
                 int status[1] = { 0 };

                 if (move_pages(0, 1, pages, nodes, status, MPOL_MF_MOVE_ALL))
                         fprintf(stderr, "move_pages failed: %d\n", errno);
                 if (status[0] != nodes[0])
                         printf("migration failed\n");
         }
         return 0;
}


-- 
Cheers,

David / dhildenb

Re: [PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[Baolin Wang]

On 2024/7/26 19:40, David Hildenbrand wrote:
> On 26.07.24 11:38, Baolin Wang wrote:
>>
>>
>> On 2024/7/26 16:04, David Hildenbrand wrote:
>>> On 26.07.24 04:33, Baolin Wang wrote:
>>>>
>>>>
>>>> On 2024/7/26 02:39, David Hildenbrand wrote:
>>>>> We recently made GUP's common page table walking code to also walk
>>>>> hugetlb VMAs without most hugetlb special-casing, preparing for the
>>>>> future of having less hugetlb-specific page table walking code in the
>>>>> codebase. Turns out that we missed one page table locking detail: page
>>>>> table locking for hugetlb folios that are not mapped using a single
>>>>> PMD/PUD.
>>>>>
>>>>> Assume we have hugetlb folio that spans multiple PTEs (e.g., 64 KiB
>>>>> hugetlb folios on arm64 with 4 KiB base page size). GUP, as it 
>>>>> walks the
>>>>> page tables, will perform a pte_offset_map_lock() to grab the PTE 
>>>>> table
>>>>> lock.
>>>>>
>>>>> However, hugetlb that concurrently modifies these page tables would
>>>>> actually grab the mm->page_table_lock: with USE_SPLIT_PTE_PTLOCKS, the
>>>>> locks would differ. Something similar can happen right now with 
>>>>> hugetlb
>>>>> folios that span multiple PMDs when USE_SPLIT_PMD_PTLOCKS.
>>>>>
>>>>> Let's make huge_pte_lockptr() effectively uses the same PT locks as 
>>>>> any
>>>>> core-mm page table walker would.
>>>>
>>>> Thanks for raising the issue again. I remember fixing this issue 2 
>>>> years
>>>> ago in commit fac35ba763ed ("mm/hugetlb: fix races when looking up a
>>>> CONT-PTE/PMD size hugetlb page"), but it seems to be broken again.
>>>>
>>>
>>> Ah, right! We fixed it by rerouting to hugetlb code that we then 
>>> removed :D
>>>
>>> Did we have a reproducer back then that would make my live easier?
>>
>> I don't have any reproducers right now. I remember I added some ugly
>> hack code (adding delay() etc.) in kernel to analyze this issue, and not
>> easy to reproduce. :(
> 
> Hah!
> 
> I tried with 32MB without luck -- migration simply takes too long. 64KB
> did the trick within seconds!
> 
> 
> On a VM with 2 vNUMA nodes, after reserving a bunch of 64KiB hugetlb pages:
> 
> # numactl -H
> available: 2 nodes (0-1)
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 0 size: 64439 MB
> node 0 free: 64097 MB
> node 1 cpus: 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> node 1 size: 64205 MB
> node 1 free: 63809 MB
> node distances:
> node   0   1
>    0:  10  20
>    1:  20  10
> # echo 100  > 
> /sys/kernel/mm/hugepages/hugepages-64kB/nr_hugepagespages-64kB/nr_hugepagesepages-64kB/nr_hugepages
> # gcc reproducer.c -o reproducer -O3 -lnuma -lpthread
> # ./reproducer
> [ 3105.936100] ------------[ cut here ]------------
> [ 3105.939323] WARNING: CPU: 31 PID: 2732 at mm/gup.c:142 
> try_grab_folio+0x11c/0x188
> [ 3105.944634] Modules linked in: nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 
> nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct 
> nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill 
> ip_set nf_tables qrtr sunrpc vfat fat ext4 mbcache jbd2 virtio_net 
> net_failover failover virtio_balloon dimlib loop dm_multipath nfnetlink 
> zram xfs crct10dif_ce ghash_ce sha2_ce sha256_arm64 sha1_ce virtio_blk 
> virtio_console virtio_mmio dm_mirror dm_region_hash dm_log dm_mod fuse
> [ 3105.974841] CPU: 31 PID: 2732 Comm: reproducer Not tainted 
> 6.10.0-64.eln141.aarch64 #1
> [ 3105.980406] Hardware name: QEMU KVM Virtual Machine, BIOS 
> edk2-20240524-4.fc40 05/24/2024
> [ 3105.986185] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS 
> BTYPE=--)
> [ 3105.991108] pc : try_grab_folio+0x11c/0x188
> [ 3105.994013] lr : follow_page_pte+0xd8/0x430
> [ 3105.996986] sp : ffff80008eafb8f0
> [ 3105.999346] x29: ffff80008eafb900 x28: ffffffe8d481f380 x27: 
> 00f80001207cff43
> [ 3106.004414] x26: 0000000000000001 x25: 0000000000000000 x24: 
> ffff80008eafba48
> [ 3106.009520] x23: 0000ffff9372f000 x22: ffff7a54459e2000 x21: 
> ffff7a546c1aa978
> [ 3106.014529] x20: ffffffe8d481f3c0 x19: 0000000000610041 x18: 
> 0000000000000001
> [ 3106.019506] x17: 0000000000000001 x16: ffffffffffffffff x15: 
> 0000000000000000
> [ 3106.024494] x14: ffffb85477fdfe08 x13: 0000ffff9372ffff x12: 
> 0000000000000000
> [ 3106.029469] x11: 1fffef4a88a96be1 x10: ffff7a54454b5f0c x9 : 
> ffffb854771b12f0
> [ 3106.034324] x8 : 0008000000000000 x7 : ffff7a546c1aa980 x6 : 
> 0008000000000080
> [ 3106.038902] x5 : 00000000001207cf x4 : 0000ffff9372f000 x3 : 
> ffffffe8d481f000
> [ 3106.043420] x2 : 0000000000610041 x1 : 0000000000000001 x0 : 
> 0000000000000000
> [ 3106.047957] Call trace:
> [ 3106.049522]  try_grab_folio+0x11c/0x188
> [ 3106.051996]  follow_pmd_mask.constprop.0.isra.0+0x150/0x2e0
> [ 3106.055527]  follow_page_mask+0x1a0/0x2b8
> [ 3106.058118]  __get_user_pages+0xf0/0x348
> [ 3106.060647]  faultin_page_range+0xb0/0x360
> [ 3106.063651]  do_madvise+0x340/0x598
> ...
> 
> 
> 
> # cat reproducer.c
> #include <stdio.h>
> #include <sys/mman.h>
> #include <linux/mman.h>
> #include <errno.h>
> #include <string.h>
> #include <numaif.h>
> #include <pthread.h>
> 
> #define SIZE_64KB (64 * 1024ul)
> 
> static void *thread_fn(void *arg)
> {
>          char *mem = arg;
> 
>          /* Let GUP go crazy on the page without grabbing a reference. */
>          while (1) {
>                  if (madvise(mem, SIZE_64KB, MADV_POPULATE_WRITE)) {
>                          fprintf(stderr, "madvise() failed: %d\n", errno);
>                  }
>          }
> }
> 
> int main(void)
> {
>          pthread_t thread;
>          unsigned int i;
>          char *mem;
> 
>          mem = mmap(0, SIZE_64KB, PROT_READ|PROT_WRITE,
>                     MAP_ANON|MAP_PRIVATE|MAP_HUGETLB|MAP_HUGE_64KB, -1, 0);
>          if (mem == MAP_FAILED) {
>                  fprintf(stderr, "mmap() failed: %d\n", errno);
>                  return -1;
>          }
> 
>          memset(mem, 0, SIZE_64KB);
> 
>          pthread_create(&thread, NULL, thread_fn, mem);
> 
>          /* Migrate it back and forth between two nodes. */
>          for (i = 0; ; i++) {
>                  void *pages[1] = { mem, };
>                  int nodes[1] = { i % 2, };
>                  int status[1] = { 0 };
> 
>                  if (move_pages(0, 1, pages, nodes, status, 
> MPOL_MF_MOVE_ALL))
>                          fprintf(stderr, "move_pages failed: %d\n", errno);
>                  if (status[0] != nodes[0])
>                          printf("migration failed\n");
>          }
>          return 0;
> }

Great! Thanks for creating the reproducer (I will also try it if I find 
some time).

Re: [PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[Muchun Song]

> On Jul 26, 2024, at 02:39, David Hildenbrand <david@redhat.com> wrote:
> 
> We recently made GUP's common page table walking code to also walk
> hugetlb VMAs without most hugetlb special-casing, preparing for the
> future of having less hugetlb-specific page table walking code in the
> codebase. Turns out that we missed one page table locking detail: page
> table locking for hugetlb folios that are not mapped using a single
> PMD/PUD.
> 
> Assume we have hugetlb folio that spans multiple PTEs (e.g., 64 KiB
> hugetlb folios on arm64 with 4 KiB base page size). GUP, as it walks the
> page tables, will perform a pte_offset_map_lock() to grab the PTE table
> lock.
> 
> However, hugetlb that concurrently modifies these page tables would
> actually grab the mm->page_table_lock: with USE_SPLIT_PTE_PTLOCKS, the
> locks would differ. Something similar can happen right now with hugetlb
> folios that span multiple PMDs when USE_SPLIT_PMD_PTLOCKS.
> 
> Let's make huge_pte_lockptr() effectively uses the same PT locks as any
> core-mm page table walker would.
> 
> There is one ugly case: powerpc 8xx, whereby we have an 8 MiB hugetlb
> folio being mapped using two PTE page tables. While hugetlb wants to take
> the PMD table lock, core-mm would grab the PTE table lock of one of both
> PTE page tables. In such corner cases, we have to make sure that both
> locks match, which is (fortunately!) currently guaranteed for 8xx as it
> does not support SMP.
> 
> Fixes: 9cb28da54643 ("mm/gup: handle hugetlb in the generic follow_page_mask code")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: Muchun Song <muchun.song@linux.dev>

Thanks.

Re: [PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[Peter Xu]

On Thu, Jul 25, 2024 at 08:39:55PM +0200, David Hildenbrand wrote:
> We recently made GUP's common page table walking code to also walk
> hugetlb VMAs without most hugetlb special-casing, preparing for the
> future of having less hugetlb-specific page table walking code in the
> codebase. Turns out that we missed one page table locking detail: page
> table locking for hugetlb folios that are not mapped using a single
> PMD/PUD.
> 
> Assume we have hugetlb folio that spans multiple PTEs (e.g., 64 KiB
> hugetlb folios on arm64 with 4 KiB base page size). GUP, as it walks the
> page tables, will perform a pte_offset_map_lock() to grab the PTE table
> lock.
> 
> However, hugetlb that concurrently modifies these page tables would
> actually grab the mm->page_table_lock: with USE_SPLIT_PTE_PTLOCKS, the
> locks would differ. Something similar can happen right now with hugetlb
> folios that span multiple PMDs when USE_SPLIT_PMD_PTLOCKS.
> 
> Let's make huge_pte_lockptr() effectively uses the same PT locks as any
> core-mm page table walker would.
> 
> There is one ugly case: powerpc 8xx, whereby we have an 8 MiB hugetlb
> folio being mapped using two PTE page tables. While hugetlb wants to take
> the PMD table lock, core-mm would grab the PTE table lock of one of both
> PTE page tables. In such corner cases, we have to make sure that both
> locks match, which is (fortunately!) currently guaranteed for 8xx as it
> does not support SMP.

Do you mean "does not support SPLIT_PMD_PTLOCK" here instead of SMP?

> 
> Fixes: 9cb28da54643 ("mm/gup: handle hugetlb in the generic follow_page_mask code")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Patch looks all right to me:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks!

-- 
Peter Xu

Re: [PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[David Hildenbrand]

On 26.07.24 17:26, Peter Xu wrote:
> On Thu, Jul 25, 2024 at 08:39:55PM +0200, David Hildenbrand wrote:
>> We recently made GUP's common page table walking code to also walk
>> hugetlb VMAs without most hugetlb special-casing, preparing for the
>> future of having less hugetlb-specific page table walking code in the
>> codebase. Turns out that we missed one page table locking detail: page
>> table locking for hugetlb folios that are not mapped using a single
>> PMD/PUD.
>>
>> Assume we have hugetlb folio that spans multiple PTEs (e.g., 64 KiB
>> hugetlb folios on arm64 with 4 KiB base page size). GUP, as it walks the
>> page tables, will perform a pte_offset_map_lock() to grab the PTE table
>> lock.
>>
>> However, hugetlb that concurrently modifies these page tables would
>> actually grab the mm->page_table_lock: with USE_SPLIT_PTE_PTLOCKS, the
>> locks would differ. Something similar can happen right now with hugetlb
>> folios that span multiple PMDs when USE_SPLIT_PMD_PTLOCKS.
>>
>> Let's make huge_pte_lockptr() effectively uses the same PT locks as any
>> core-mm page table walker would.
>>
>> There is one ugly case: powerpc 8xx, whereby we have an 8 MiB hugetlb
>> folio being mapped using two PTE page tables. While hugetlb wants to take
>> the PMD table lock, core-mm would grab the PTE table lock of one of both
>> PTE page tables. In such corner cases, we have to make sure that both
>> locks match, which is (fortunately!) currently guaranteed for 8xx as it
>> does not support SMP.
> 
> Do you mean "does not support SPLIT_PMD_PTLOCK" here instead of SMP?

Split PT locks are only enabled once we have NR_CPUS >= 4, so without 
SMP (NR_CPUS == 1), no split PT locks are used. I document that in the 
other series that I just sent out in a better way.

> 
>>
>> Fixes: 9cb28da54643 ("mm/gup: handle hugetlb in the generic follow_page_mask code")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Patch looks all right to me:
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> Thanks!

Thanks Peter!

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 2/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[Oscar Salvador]

On Thu, Jul 25, 2024 at 08:39:55PM +0200, David Hildenbrand wrote:
> We recently made GUP's common page table walking code to also walk
> hugetlb VMAs without most hugetlb special-casing, preparing for the
> future of having less hugetlb-specific page table walking code in the
> codebase. Turns out that we missed one page table locking detail: page
> table locking for hugetlb folios that are not mapped using a single
> PMD/PUD.
> 
> Assume we have hugetlb folio that spans multiple PTEs (e.g., 64 KiB
> hugetlb folios on arm64 with 4 KiB base page size). GUP, as it walks the
> page tables, will perform a pte_offset_map_lock() to grab the PTE table
> lock.
> 
> However, hugetlb that concurrently modifies these page tables would
> actually grab the mm->page_table_lock: with USE_SPLIT_PTE_PTLOCKS, the
> locks would differ. Something similar can happen right now with hugetlb
> folios that span multiple PMDs when USE_SPLIT_PMD_PTLOCKS.
> 
> Let's make huge_pte_lockptr() effectively uses the same PT locks as any
> core-mm page table walker would.
> 
> There is one ugly case: powerpc 8xx, whereby we have an 8 MiB hugetlb
> folio being mapped using two PTE page tables. While hugetlb wants to take
> the PMD table lock, core-mm would grab the PTE table lock of one of both
> PTE page tables. In such corner cases, we have to make sure that both
> locks match, which is (fortunately!) currently guaranteed for 8xx as it
> does not support SMP.
> 
> Fixes: 9cb28da54643 ("mm/gup: handle hugetlb in the generic follow_page_mask code")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Oscar Salvador <osalvador@suse.de>


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v1 0/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[Andrew Morton]

On Thu, 25 Jul 2024 20:39:53 +0200 David Hildenbrand <david@redhat.com> wrote:

> Working on another generic page table walker that tries to avoid
> special-casing hugetlb, I found a page table locking issue with hugetlb
> folios that are not mapped using a single PMD/PUD.
> 
> For some hugetlb folio sizes, GUP will take different page table locks
> when walking the page tables than hugetlb when modifying the page tables.
> 
> I did not actually try reproducing an issue, but looking at
> follow_pmd_mask() where we might be rereading a PMD value multiple times
> it's rather clear that concurrent modifications are rather unpleasant.
> 
> In follow_page_pte() we might be better in that regard -- ptep_get() does
> a READ_ONCE() -- but who knows what else could happen concurrently in
> some weird corner cases (e.g., hugetlb folio getting unmapped and freed).
> 
> Did some basic sanity testing with various hugetlb sizes on x86-64 and
> arm64. Maybe I'll find some time to actually write a simple reproducer in
> the common weeks, so this wouldn't have to be all-theoretical for now.

When can we be confident that this change is merge-worthy?

> Only v6.10 is affected, so the #1 can be simply backported as a prereq
> patch along with the real fix.

I'll add the same Fixes: to [1/2], and cc:stable.

Re: [PATCH v1 0/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[David Hildenbrand]

On 25.07.24 22:41, Andrew Morton wrote:
> On Thu, 25 Jul 2024 20:39:53 +0200 David Hildenbrand <david@redhat.com> wrote:
> 
>> Working on another generic page table walker that tries to avoid
>> special-casing hugetlb, I found a page table locking issue with hugetlb
>> folios that are not mapped using a single PMD/PUD.
>>
>> For some hugetlb folio sizes, GUP will take different page table locks
>> when walking the page tables than hugetlb when modifying the page tables.
>>
>> I did not actually try reproducing an issue, but looking at
>> follow_pmd_mask() where we might be rereading a PMD value multiple times
>> it's rather clear that concurrent modifications are rather unpleasant.
>>
>> In follow_page_pte() we might be better in that regard -- ptep_get() does
>> a READ_ONCE() -- but who knows what else could happen concurrently in
>> some weird corner cases (e.g., hugetlb folio getting unmapped and freed).
>>
>> Did some basic sanity testing with various hugetlb sizes on x86-64 and
>> arm64. Maybe I'll find some time to actually write a simple reproducer in
>> the common weeks, so this wouldn't have to be all-theoretical for now.
> 
> When can we be confident that this change is merge-worthy?

I'm convinced that it is the right thing to do, but I don't think we 
have to rush this.

As Baolin notes, we fixed the same issue in the past, unfortunately also 
without a reproducer IIUC, so I'll try to reproduce the race, but I'm 
not 100% sure if I'll manage to do so..

So it's certainly merge-worthy after it had a bit of exposure to -next, 
but no need to rush this upstream.

Thanks!

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 0/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[David Hildenbrand]

On 26.07.24 11:19, David Hildenbrand wrote:
> On 25.07.24 22:41, Andrew Morton wrote:
>> On Thu, 25 Jul 2024 20:39:53 +0200 David Hildenbrand <david@redhat.com> wrote:
>>
>>> Working on another generic page table walker that tries to avoid
>>> special-casing hugetlb, I found a page table locking issue with hugetlb
>>> folios that are not mapped using a single PMD/PUD.
>>>
>>> For some hugetlb folio sizes, GUP will take different page table locks
>>> when walking the page tables than hugetlb when modifying the page tables.
>>>
>>> I did not actually try reproducing an issue, but looking at
>>> follow_pmd_mask() where we might be rereading a PMD value multiple times
>>> it's rather clear that concurrent modifications are rather unpleasant.
>>>
>>> In follow_page_pte() we might be better in that regard -- ptep_get() does
>>> a READ_ONCE() -- but who knows what else could happen concurrently in
>>> some weird corner cases (e.g., hugetlb folio getting unmapped and freed).
>>>
>>> Did some basic sanity testing with various hugetlb sizes on x86-64 and
>>> arm64. Maybe I'll find some time to actually write a simple reproducer in
>>> the common weeks, so this wouldn't have to be all-theoretical for now.
>>
>> When can we be confident that this change is merge-worthy?
> 
> I'm convinced that it is the right thing to do, but I don't think we
> have to rush this.
> 
> As Baolin notes, we fixed the same issue in the past, unfortunately also
> without a reproducer IIUC, so I'll try to reproduce the race, but I'm
> not 100% sure if I'll manage to do so..

Okay, so running this series against the reproducer I pulled out of my 
magic hat, this series seems to properly fix it.

-- 
Cheers,

David / dhildenb