[PATCH 0/2] userfaultfd: fix races around pmd_trans_huge() check

[PATCH 0/2] userfaultfd: fix races around pmd_trans_huge() check

[Jann Horn]

The pmd_trans_huge() code in mfill_atomic() is wrong in two different
ways depending on kernel version:

1. The pmd_trans_huge() check is racy and can lead to a BUG_ON() (if you hit
   the right two race windows) - I've tested this in a kernel build with
   some extra mdelay() calls. See the commit message for a description
   of the race scenario.
   On older kernels (before 6.5), I think the same bug can even
   theoretically lead to accessing transhuge page contents as a page table
   if you hit the right 5 narrow race windows (I haven't tested this case).
2. On newer kernels (>=6.5), for shmem mappings, khugepaged is allowed
   to yank page tables out from under us (though I haven't tested that),
   so I think the BUG_ON() checks in mfill_atomic() are just wrong.

I decided to write two separate fixes for these, so that the first fix
can be backported to kernels affected by the first bug.

Signed-off-by: Jann Horn <jannh@google.com>
---
Jann Horn (2):
      userfaultfd: Fix pmd_trans_huge() recheck race
      userfaultfd: Don't BUG_ON() if khugepaged yanks our page table

 mm/userfaultfd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
---
base-commit: d4560686726f7a357922f300fc81f5964be8df04
change-id: 20240812-uffd-thp-flip-fix-20f91f1151b9
-- 
Jann Horn <jannh@google.com>

[PATCH 1/2] userfaultfd: Fix pmd_trans_huge() recheck race

[Jann Horn]

The following race can occur:

  mfill_atomic                other thread
  ============                ============
                              <zap PMD>
  pmdp_get_lockless() [reads none pmd]
  <bail if trans_huge>
  <if none:>
                              <pagefault creates transhuge zeropage>
    __pte_alloc [no-op]
                              <zap PMD>
  <bail if pmd_trans_huge(*dst_pmd)>
  BUG_ON(pmd_none(*dst_pmd))

I have experimentally verified this in a kernel with extra mdelay() calls;
the BUG_ON(pmd_none(*dst_pmd)) triggers.

On kernels newer than commit 0d940a9b270b ("mm/pgtable: allow
pte_offset_map[_lock]() to fail"), this can't lead to anything worse than
a BUG_ON(), since the page table access helpers are actually designed to
deal with page tables concurrently disappearing; but on older kernels
(<=6.4), I think we could probably theoretically race past the two BUG_ON()
checks and end up treating a hugepage as a page table.

Cc: stable@vger.kernel.org
Fixes: c1a4de99fada ("userfaultfd: mcopy_atomic|mfill_zeropage: UFFDIO_COPY|UFFDIO_ZEROPAGE preparation")
Signed-off-by: Jann Horn <jannh@google.com>
---
 mm/userfaultfd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index e54e5c8907fa..ec3750467aa5 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -801,7 +801,8 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
 			break;
 		}
 		/* If an huge pmd materialized from under us fail */
-		if (unlikely(pmd_trans_huge(*dst_pmd))) {
+		dst_pmdval = pmdp_get_lockless(dst_pmd);
+		if (unlikely(pmd_none(dst_pmdval) || pmd_trans_huge(dst_pmdval))) {
 			err = -EFAULT;
 			break;
 		}

-- 
2.46.0.76.ge559c4bf1a-goog

[PATCH 2/2] userfaultfd: Don't BUG_ON() if khugepaged yanks our page table

[Jann Horn]

Since khugepaged was changed to allow retracting page tables in file
mappings without holding the mmap lock, these BUG_ON()s are wrong - get rid
of them.

We could also remove the preceding "if (unlikely(...))" block, but then
we could reach pte_offset_map_lock() with transhuge pages not just for file
mappings but also for anonymous mappings - which would probably be fine but
I think is not necessarily expected.

Cc: stable@vger.kernel.org
Fixes: 1d65b771bc08 ("mm/khugepaged: retract_page_tables() without mmap or vma lock")
Signed-off-by: Jann Horn <jannh@google.com>
---
 mm/userfaultfd.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index ec3750467aa5..0dfa97db6feb 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -806,9 +806,10 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
 			err = -EFAULT;
 			break;
 		}
-
-		BUG_ON(pmd_none(*dst_pmd));
-		BUG_ON(pmd_trans_huge(*dst_pmd));
+		/*
+		 * For shmem mappings, khugepaged is allowed to remove page
+		 * tables under us; pte_offset_map_lock() will deal with that.
+		 */
 
 		err = mfill_atomic_pte(dst_pmd, dst_vma, dst_addr,
 				       src_addr, flags, &folio);

-- 
2.46.0.76.ge559c4bf1a-goog

Re: [PATCH 1/2] userfaultfd: Fix pmd_trans_huge() recheck race

[Qi Zheng]

Hi Jann,

On 2024/8/13 00:42, Jann Horn wrote:
> The following race can occur:
> 
>    mfill_atomic                other thread
>    ============                ============
>                                <zap PMD>
>    pmdp_get_lockless() [reads none pmd]
>    <bail if trans_huge>
>    <if none:>
>                                <pagefault creates transhuge zeropage>
>      __pte_alloc [no-op]
>                                <zap PMD>
>    <bail if pmd_trans_huge(*dst_pmd)>
>    BUG_ON(pmd_none(*dst_pmd))
> 
> I have experimentally verified this in a kernel with extra mdelay() calls;
> the BUG_ON(pmd_none(*dst_pmd)) triggers.
> 
> On kernels newer than commit 0d940a9b270b ("mm/pgtable: allow
> pte_offset_map[_lock]() to fail"), this can't lead to anything worse than
> a BUG_ON(), since the page table access helpers are actually designed to
> deal with page tables concurrently disappearing; but on older kernels
> (<=6.4), I think we could probably theoretically race past the two BUG_ON()
> checks and end up treating a hugepage as a page table.
> 
> Cc: stable@vger.kernel.org
> Fixes: c1a4de99fada ("userfaultfd: mcopy_atomic|mfill_zeropage: UFFDIO_COPY|UFFDIO_ZEROPAGE preparation")
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>   mm/userfaultfd.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index e54e5c8907fa..ec3750467aa5 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -801,7 +801,8 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
>   			break;
>   		}
>   		/* If an huge pmd materialized from under us fail */
> -		if (unlikely(pmd_trans_huge(*dst_pmd))) {
> +		dst_pmdval = pmdp_get_lockless(dst_pmd);
> +		if (unlikely(pmd_none(dst_pmdval) || pmd_trans_huge(dst_pmdval))) {

Before commit 0d940a9b270b, should we also check for
is_pmd_migration_entry(), pmd_devmap() and pmd_bad() here?

Thanks,
Qi

>   			err = -EFAULT;
>   			break;
>   		}
> 

Re: [PATCH 2/2] userfaultfd: Don't BUG_ON() if khugepaged yanks our page table

[Qi Zheng]

On 2024/8/13 00:42, Jann Horn wrote:
> Since khugepaged was changed to allow retracting page tables in file
> mappings without holding the mmap lock, these BUG_ON()s are wrong - get rid
> of them.
> 
> We could also remove the preceding "if (unlikely(...))" block, but then
> we could reach pte_offset_map_lock() with transhuge pages not just for file
> mappings but also for anonymous mappings - which would probably be fine but
> I think is not necessarily expected.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1d65b771bc08 ("mm/khugepaged: retract_page_tables() without mmap or vma lock")
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>   mm/userfaultfd.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

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

> 
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index ec3750467aa5..0dfa97db6feb 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -806,9 +806,10 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
>   			err = -EFAULT;
>   			break;
>   		}
> -
> -		BUG_ON(pmd_none(*dst_pmd));
> -		BUG_ON(pmd_trans_huge(*dst_pmd));
> +		/*
> +		 * For shmem mappings, khugepaged is allowed to remove page
> +		 * tables under us; pte_offset_map_lock() will deal with that.
> +		 */
>   
>   		err = mfill_atomic_pte(dst_pmd, dst_vma, dst_addr,
>   				       src_addr, flags, &folio);
> 

Re: [PATCH 2/2] userfaultfd: Don't BUG_ON() if khugepaged yanks our page table

[David Hildenbrand]

On 12.08.24 18:42, Jann Horn wrote:
> Since khugepaged was changed to allow retracting page tables in file
> mappings without holding the mmap lock, these BUG_ON()s are wrong - get rid
> of them.
> 
> We could also remove the preceding "if (unlikely(...))" block, but then
> we could reach pte_offset_map_lock() with transhuge pages not just for file
> mappings but also for anonymous mappings - which would probably be fine but
> I think is not necessarily expected.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1d65b771bc08 ("mm/khugepaged: retract_page_tables() without mmap or vma lock")
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>   mm/userfaultfd.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index ec3750467aa5..0dfa97db6feb 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -806,9 +806,10 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
>   			err = -EFAULT;
>   			break;
>   		}
> -
> -		BUG_ON(pmd_none(*dst_pmd));
> -		BUG_ON(pmd_trans_huge(*dst_pmd));
> +		/*
> +		 * For shmem mappings, khugepaged is allowed to remove page
> +		 * tables under us; pte_offset_map_lock() will deal with that.
> +		 */
>   
>   		err = mfill_atomic_pte(dst_pmd, dst_vma, dst_addr,
>   				       src_addr, flags, &folio);
> 

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] userfaultfd: Fix pmd_trans_huge() recheck race

[David Hildenbrand]

On 12.08.24 18:42, Jann Horn wrote:
> The following race can occur:
> 
>    mfill_atomic                other thread
>    ============                ============
>                                <zap PMD>
>    pmdp_get_lockless() [reads none pmd]
>    <bail if trans_huge>
>    <if none:>
>                                <pagefault creates transhuge zeropage>
>      __pte_alloc [no-op]
>                                <zap PMD>
>    <bail if pmd_trans_huge(*dst_pmd)>
>    BUG_ON(pmd_none(*dst_pmd))
> 
> I have experimentally verified this in a kernel with extra mdelay() calls;
> the BUG_ON(pmd_none(*dst_pmd)) triggers.
> 
> On kernels newer than commit 0d940a9b270b ("mm/pgtable: allow
> pte_offset_map[_lock]() to fail"), this can't lead to anything worse than
> a BUG_ON(), since the page table access helpers are actually designed to
> deal with page tables concurrently disappearing; but on older kernels
> (<=6.4), I think we could probably theoretically race past the two BUG_ON()
> checks and end up treating a hugepage as a page table.
> 
> Cc: stable@vger.kernel.org
> Fixes: c1a4de99fada ("userfaultfd: mcopy_atomic|mfill_zeropage: UFFDIO_COPY|UFFDIO_ZEROPAGE preparation")
> Signed-off-by: Jann Horn <jannh@google.com>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] userfaultfd: Fix pmd_trans_huge() recheck race

[Jann Horn]

On Tue, Aug 13, 2024 at 8:19 AM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>
> Hi Jann,
>
> On 2024/8/13 00:42, Jann Horn wrote:
> > The following race can occur:
> >
> >    mfill_atomic                other thread
> >    ============                ============
> >                                <zap PMD>
> >    pmdp_get_lockless() [reads none pmd]
> >    <bail if trans_huge>
> >    <if none:>
> >                                <pagefault creates transhuge zeropage>
> >      __pte_alloc [no-op]
> >                                <zap PMD>
> >    <bail if pmd_trans_huge(*dst_pmd)>
> >    BUG_ON(pmd_none(*dst_pmd))
> >
> > I have experimentally verified this in a kernel with extra mdelay() calls;
> > the BUG_ON(pmd_none(*dst_pmd)) triggers.
> >
> > On kernels newer than commit 0d940a9b270b ("mm/pgtable: allow
> > pte_offset_map[_lock]() to fail"), this can't lead to anything worse than
> > a BUG_ON(), since the page table access helpers are actually designed to
> > deal with page tables concurrently disappearing; but on older kernels
> > (<=6.4), I think we could probably theoretically race past the two BUG_ON()
> > checks and end up treating a hugepage as a page table.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: c1a4de99fada ("userfaultfd: mcopy_atomic|mfill_zeropage: UFFDIO_COPY|UFFDIO_ZEROPAGE preparation")
> > Signed-off-by: Jann Horn <jannh@google.com>
> > ---
> >   mm/userfaultfd.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index e54e5c8907fa..ec3750467aa5 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -801,7 +801,8 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
> >                       break;
> >               }
> >               /* If an huge pmd materialized from under us fail */
> > -             if (unlikely(pmd_trans_huge(*dst_pmd))) {
> > +             dst_pmdval = pmdp_get_lockless(dst_pmd);
> > +             if (unlikely(pmd_none(dst_pmdval) || pmd_trans_huge(dst_pmdval))) {
>
> Before commit 0d940a9b270b, should we also check for
> is_pmd_migration_entry(), pmd_devmap() and pmd_bad() here?

Oooh. I think you're right that this check is insufficient, thanks for
spotting that.

I think I should probably change the check to something like this?

if (unlikely(!pmd_present(dst_pmdval) || pmd_trans_huge(dst_pmdval) ||
pmd_devmap(dst_pmdval) || pmd_bad(dst_pmdval))) {

!pmd_present() implies !is_pmd_migration_entry(). And the pmd_bad() at
the end shouldn't be necessary if everything is working right, I'm
just tacking it on to be safe.

I'll send a v2 with this change soon.

(Alternatively, pmd_leaf() might be useful here, but then we'd have to
figure an alternate way of doing this for the backport.)