* [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
@ 2025-07-31 15:44 Suren Baghdasaryan
2025-07-31 17:30 ` Lokesh Gidra
2025-08-01 7:21 ` David Hildenbrand
0 siblings, 2 replies; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-07-31 15:44 UTC (permalink / raw)
To: akpm
Cc: peterx, david, aarcange, lokeshgidra, surenb, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
encounters a non-present THP, it fails to properly recognize an unmapped
hole and tries to access a non-existent folio, resulting in
a crash. Add a check to skip non-present THPs.
Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Cc: stable@vger.kernel.org
---
Changes since v1 [1]
- Fixed step size calculation, per Lokesh Gidra
- Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
[1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
1 file changed, 29 insertions(+), 16 deletions(-)
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index cbed91b09640..b5af31c22731 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
ptl = pmd_trans_huge_lock(src_pmd, src_vma);
if (ptl) {
- /* Check if we can move the pmd without splitting it. */
- if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
- !pmd_none(dst_pmdval)) {
- struct folio *folio = pmd_folio(*src_pmd);
+ if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
+ /* Check if we can move the pmd without splitting it. */
+ if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
+ !pmd_none(dst_pmdval)) {
+ if (pmd_present(*src_pmd)) {
+ struct folio *folio = pmd_folio(*src_pmd);
+
+ if (!folio || (!is_huge_zero_folio(folio) &&
+ !PageAnonExclusive(&folio->page))) {
+ spin_unlock(ptl);
+ err = -EBUSY;
+ break;
+ }
+ }
- if (!folio || (!is_huge_zero_folio(folio) &&
- !PageAnonExclusive(&folio->page))) {
spin_unlock(ptl);
- err = -EBUSY;
- break;
+ split_huge_pmd(src_vma, src_pmd, src_addr);
+ /* The folio will be split by move_pages_pte() */
+ continue;
}
+ err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
+ dst_pmdval, dst_vma, src_vma,
+ dst_addr, src_addr);
+ step_size = HPAGE_PMD_SIZE;
+ } else {
spin_unlock(ptl);
- split_huge_pmd(src_vma, src_pmd, src_addr);
- /* The folio will be split by move_pages_pte() */
- continue;
+ if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
+ err = -ENOENT;
+ break;
+ }
+ /* nothing to do to move a hole */
+ err = 0;
+ step_size = min(HPAGE_PMD_SIZE, src_start + len - src_addr);
}
-
- err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
- dst_pmdval, dst_vma, src_vma,
- dst_addr, src_addr);
- step_size = HPAGE_PMD_SIZE;
} else {
if (pmd_none(*src_pmd)) {
if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
base-commit: 01da54f10fddf3b01c5a3b80f6b16bbad390c302
--
2.50.1.552.g942d659e1b-goog
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-07-31 15:44 [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole Suren Baghdasaryan
@ 2025-07-31 17:30 ` Lokesh Gidra
2025-08-01 7:21 ` David Hildenbrand
1 sibling, 0 replies; 21+ messages in thread
From: Lokesh Gidra @ 2025-07-31 17:30 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, peterx, david, aarcange, linux-mm, linux-kernel,
syzbot+b446dbe27035ef6bd6c2, stable
On Thu, Jul 31, 2025 at 8:44 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> encounters a non-present THP, it fails to properly recognize an unmapped
> hole and tries to access a non-existent folio, resulting in
> a crash. Add a check to skip non-present THPs.
>
> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Reviewed-by: Lokesh Gidra <lokeshgidra@google.com>
> Cc: stable@vger.kernel.org
> ---
> Changes since v1 [1]
> - Fixed step size calculation, per Lokesh Gidra
> - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
>
> [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
>
> mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> 1 file changed, 29 insertions(+), 16 deletions(-)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index cbed91b09640..b5af31c22731 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
>
> ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> if (ptl) {
> - /* Check if we can move the pmd without splitting it. */
> - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> - !pmd_none(dst_pmdval)) {
> - struct folio *folio = pmd_folio(*src_pmd);
> + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> + /* Check if we can move the pmd without splitting it. */
> + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> + !pmd_none(dst_pmdval)) {
> + if (pmd_present(*src_pmd)) {
> + struct folio *folio = pmd_folio(*src_pmd);
> +
> + if (!folio || (!is_huge_zero_folio(folio) &&
> + !PageAnonExclusive(&folio->page))) {
> + spin_unlock(ptl);
> + err = -EBUSY;
> + break;
> + }
> + }
>
> - if (!folio || (!is_huge_zero_folio(folio) &&
> - !PageAnonExclusive(&folio->page))) {
> spin_unlock(ptl);
> - err = -EBUSY;
> - break;
> + split_huge_pmd(src_vma, src_pmd, src_addr);
> + /* The folio will be split by move_pages_pte() */
> + continue;
> }
>
> + err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
> + dst_pmdval, dst_vma, src_vma,
> + dst_addr, src_addr);
> + step_size = HPAGE_PMD_SIZE;
> + } else {
> spin_unlock(ptl);
> - split_huge_pmd(src_vma, src_pmd, src_addr);
> - /* The folio will be split by move_pages_pte() */
> - continue;
> + if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> + err = -ENOENT;
> + break;
> + }
> + /* nothing to do to move a hole */
> + err = 0;
> + step_size = min(HPAGE_PMD_SIZE, src_start + len - src_addr);
> }
> -
> - err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
> - dst_pmdval, dst_vma, src_vma,
> - dst_addr, src_addr);
> - step_size = HPAGE_PMD_SIZE;
> } else {
> if (pmd_none(*src_pmd)) {
> if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
>
> base-commit: 01da54f10fddf3b01c5a3b80f6b16bbad390c302
> --
> 2.50.1.552.g942d659e1b-goog
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-07-31 15:44 [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole Suren Baghdasaryan
2025-07-31 17:30 ` Lokesh Gidra
@ 2025-08-01 7:21 ` David Hildenbrand
2025-08-01 14:15 ` Peter Xu
2025-08-01 15:11 ` Suren Baghdasaryan
1 sibling, 2 replies; 21+ messages in thread
From: David Hildenbrand @ 2025-08-01 7:21 UTC (permalink / raw)
To: Suren Baghdasaryan, akpm
Cc: peterx, aarcange, lokeshgidra, linux-mm, linux-kernel,
syzbot+b446dbe27035ef6bd6c2, stable
On 31.07.25 17:44, Suren Baghdasaryan wrote:
Hi!
Did you mean in you patch description:
"userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
Talking about THP holes is very very confusing.
> When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> encounters a non-present THP, it fails to properly recognize an unmapped
You mean a "non-present PMD that is not a migration entry".
> hole and tries to access a non-existent folio, resulting in
> a crash. Add a check to skip non-present THPs.
That makes sense. The code we have after this patch is rather
complicated and hard to read.
>
> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> Cc: stable@vger.kernel.org
> ---
> Changes since v1 [1]
> - Fixed step size calculation, per Lokesh Gidra
> - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
>
> [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
>
> mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> 1 file changed, 29 insertions(+), 16 deletions(-)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index cbed91b09640..b5af31c22731 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
>
> ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> if (ptl) {
> - /* Check if we can move the pmd without splitting it. */
> - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> - !pmd_none(dst_pmdval)) {
> - struct folio *folio = pmd_folio(*src_pmd);
> + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> + /* Check if we can move the pmd without splitting it. */
> + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> + !pmd_none(dst_pmdval)) {
> + if (pmd_present(*src_pmd)) {
> + struct folio *folio = pmd_folio(*src_pmd);
> +
> + if (!folio || (!is_huge_zero_folio(folio) &&
> + !PageAnonExclusive(&folio->page))) {
> + spin_unlock(ptl);
> + err = -EBUSY;
> + break;
> + }
> + }
... in particular that. Is there some way to make this code simpler /
easier to read? Like moving that whole last folio-check thingy into a
helper?
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-01 7:21 ` David Hildenbrand
@ 2025-08-01 14:15 ` Peter Xu
2025-08-01 15:28 ` Suren Baghdasaryan
2025-08-01 15:11 ` Suren Baghdasaryan
1 sibling, 1 reply; 21+ messages in thread
From: Peter Xu @ 2025-08-01 14:15 UTC (permalink / raw)
To: David Hildenbrand
Cc: Suren Baghdasaryan, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> On 31.07.25 17:44, Suren Baghdasaryan wrote:
>
> Hi!
>
> Did you mean in you patch description:
>
> "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
>
> Talking about THP holes is very very confusing.
>
> > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > encounters a non-present THP, it fails to properly recognize an unmapped
>
> You mean a "non-present PMD that is not a migration entry".
>
> > hole and tries to access a non-existent folio, resulting in
> > a crash. Add a check to skip non-present THPs.
>
> That makes sense. The code we have after this patch is rather complicated
> and hard to read.
>
> >
> > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > Cc: stable@vger.kernel.org
> > ---
> > Changes since v1 [1]
> > - Fixed step size calculation, per Lokesh Gidra
> > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> >
> > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> >
> > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > 1 file changed, 29 insertions(+), 16 deletions(-)
> >
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index cbed91b09640..b5af31c22731 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > if (ptl) {
> > - /* Check if we can move the pmd without splitting it. */
> > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > - !pmd_none(dst_pmdval)) {
> > - struct folio *folio = pmd_folio(*src_pmd);
> > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
[1]
> > + /* Check if we can move the pmd without splitting it. */
> > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > + !pmd_none(dst_pmdval)) {
> > + if (pmd_present(*src_pmd)) {
> > + struct folio *folio = pmd_folio(*src_pmd);
> > +
> > + if (!folio || (!is_huge_zero_folio(folio) &&
> > + !PageAnonExclusive(&folio->page))) {
> > + spin_unlock(ptl);
> > + err = -EBUSY;
> > + break;
> > + }
> > + }
>
> ... in particular that. Is there some way to make this code simpler / easier
> to read? Like moving that whole last folio-check thingy into a helper?
One question might be relevant is, whether the check above [1] can be
dropped.
The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
before returning the ptl. I didn't follow closely on the recent changes on
mm side on possible new pmd swap entries, if migration is the only possible
one then it looks like [1] can be avoided.
And it also looks applicable to also drop the "else" later, because in "if
(ptl)" it cannot hit pmd_none().
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-01 7:21 ` David Hildenbrand
2025-08-01 14:15 ` Peter Xu
@ 2025-08-01 15:11 ` Suren Baghdasaryan
1 sibling, 0 replies; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-08-01 15:11 UTC (permalink / raw)
To: David Hildenbrand
Cc: akpm, peterx, aarcange, lokeshgidra, linux-mm, linux-kernel,
syzbot+b446dbe27035ef6bd6c2, stable
On Fri, Aug 1, 2025 at 7:21 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 31.07.25 17:44, Suren Baghdasaryan wrote:
>
> Hi!
>
> Did you mean in you patch description:
>
> "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
>
> Talking about THP holes is very very confusing.
Hi David,
Yes, "hole" is not a technical term, so I'll change as you suggested.
>
> > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > encounters a non-present THP, it fails to properly recognize an unmapped
>
> You mean a "non-present PMD that is not a migration entry".
Yes, will fix.
>
> > hole and tries to access a non-existent folio, resulting in
> > a crash. Add a check to skip non-present THPs.
>
> That makes sense. The code we have after this patch is rather
> complicated and hard to read.
>
> >
> > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > Cc: stable@vger.kernel.org
> > ---
> > Changes since v1 [1]
> > - Fixed step size calculation, per Lokesh Gidra
> > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> >
> > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> >
> > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > 1 file changed, 29 insertions(+), 16 deletions(-)
> >
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index cbed91b09640..b5af31c22731 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> >
> > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > if (ptl) {
> > - /* Check if we can move the pmd without splitting it. */
> > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > - !pmd_none(dst_pmdval)) {
> > - struct folio *folio = pmd_folio(*src_pmd);
> > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > + /* Check if we can move the pmd without splitting it. */
> > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > + !pmd_none(dst_pmdval)) {
> > + if (pmd_present(*src_pmd)) {
> > + struct folio *folio = pmd_folio(*src_pmd);
> > +
> > + if (!folio || (!is_huge_zero_folio(folio) &&
> > + !PageAnonExclusive(&folio->page))) {
> > + spin_unlock(ptl);
> > + err = -EBUSY;
> > + break;
> > + }
> > + }
>
> ... in particular that. Is there some way to make this code simpler /
> easier to read? Like moving that whole last folio-check thingy into a
> helper?
Do you mean refactor the section after "if (ptf)" into a separate function?
I was trying to minimize the code changes to simplify backporting but
since additional indentation changes this whole block, I think it does
not make much difference. Please let me know if I understood you
correctly and I'll move the code into a separate function.
Thanks,
Suren.
>
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-01 14:15 ` Peter Xu
@ 2025-08-01 15:28 ` Suren Baghdasaryan
2025-08-01 16:23 ` Peter Xu
0 siblings, 1 reply; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-08-01 15:28 UTC (permalink / raw)
To: Peter Xu
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> >
> > Hi!
> >
> > Did you mean in you patch description:
> >
> > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> >
> > Talking about THP holes is very very confusing.
> >
> > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > encounters a non-present THP, it fails to properly recognize an unmapped
> >
> > You mean a "non-present PMD that is not a migration entry".
> >
> > > hole and tries to access a non-existent folio, resulting in
> > > a crash. Add a check to skip non-present THPs.
> >
> > That makes sense. The code we have after this patch is rather complicated
> > and hard to read.
> >
> > >
> > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > Changes since v1 [1]
> > > - Fixed step size calculation, per Lokesh Gidra
> > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > >
> > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > >
> > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > index cbed91b09640..b5af31c22731 100644
> > > --- a/mm/userfaultfd.c
> > > +++ b/mm/userfaultfd.c
> > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > if (ptl) {
> > > - /* Check if we can move the pmd without splitting it. */
> > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > - !pmd_none(dst_pmdval)) {
> > > - struct folio *folio = pmd_folio(*src_pmd);
> > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
>
> [1]
>
> > > + /* Check if we can move the pmd without splitting it. */
> > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > + !pmd_none(dst_pmdval)) {
> > > + if (pmd_present(*src_pmd)) {
> > > + struct folio *folio = pmd_folio(*src_pmd);
> > > +
> > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > + !PageAnonExclusive(&folio->page))) {
> > > + spin_unlock(ptl);
> > > + err = -EBUSY;
> > > + break;
> > > + }
> > > + }
> >
> > ... in particular that. Is there some way to make this code simpler / easier
> > to read? Like moving that whole last folio-check thingy into a helper?
>
> One question might be relevant is, whether the check above [1] can be
> dropped.
>
> The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> before returning the ptl. I didn't follow closely on the recent changes on
> mm side on possible new pmd swap entries, if migration is the only possible
> one then it looks like [1] can be avoided.
Hi Peter,
is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
&& !pmd_present()) PMD to pass and that's when this crash is hit.
If we drop the check at [1] then the path that takes us to
split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
indication that split did not happen. Afterwards we will retry
thinking that PMD got split and leaving further remapping to
move_pages_pte() (see the comment before "continue"). I think in this
case it will end up in the same path again instead (infinite loop). I
didn't test this but from the code I think that's what would happen.
Does that make sense?
>
> And it also looks applicable to also drop the "else" later, because in "if
> (ptl)" it cannot hit pmd_none().
>
> Thanks,
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-01 15:28 ` Suren Baghdasaryan
@ 2025-08-01 16:23 ` Peter Xu
2025-08-01 16:41 ` Suren Baghdasaryan
0 siblings, 1 reply; 21+ messages in thread
From: Peter Xu @ 2025-08-01 16:23 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > >
> > > Hi!
> > >
> > > Did you mean in you patch description:
> > >
> > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > >
> > > Talking about THP holes is very very confusing.
> > >
> > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > >
> > > You mean a "non-present PMD that is not a migration entry".
> > >
> > > > hole and tries to access a non-existent folio, resulting in
> > > > a crash. Add a check to skip non-present THPs.
> > >
> > > That makes sense. The code we have after this patch is rather complicated
> > > and hard to read.
> > >
> > > >
> > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > Cc: stable@vger.kernel.org
> > > > ---
> > > > Changes since v1 [1]
> > > > - Fixed step size calculation, per Lokesh Gidra
> > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > >
> > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > >
> > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > index cbed91b09640..b5af31c22731 100644
> > > > --- a/mm/userfaultfd.c
> > > > +++ b/mm/userfaultfd.c
> > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > if (ptl) {
> > > > - /* Check if we can move the pmd without splitting it. */
> > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > - !pmd_none(dst_pmdval)) {
> > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> >
> > [1]
> >
> > > > + /* Check if we can move the pmd without splitting it. */
> > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > + !pmd_none(dst_pmdval)) {
> > > > + if (pmd_present(*src_pmd)) {
[2]
> > > > + struct folio *folio = pmd_folio(*src_pmd);
[3]
> > > > +
> > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > + !PageAnonExclusive(&folio->page))) {
> > > > + spin_unlock(ptl);
> > > > + err = -EBUSY;
> > > > + break;
> > > > + }
> > > > + }
> > >
> > > ... in particular that. Is there some way to make this code simpler / easier
> > > to read? Like moving that whole last folio-check thingy into a helper?
> >
> > One question might be relevant is, whether the check above [1] can be
> > dropped.
> >
> > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > before returning the ptl. I didn't follow closely on the recent changes on
> > mm side on possible new pmd swap entries, if migration is the only possible
> > one then it looks like [1] can be avoided.
>
> Hi Peter,
> is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> && !pmd_present()) PMD to pass and that's when this crash is hit.
First for all, thanks for looking into the issue with Lokesh; I am still
catching up with emails after taking weeks off.
I didn't yet read into the syzbot report, but I thought the bug was about
referencing the folio on top of a swap entry after reading your current
patch, which has:
if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
!pmd_none(dst_pmdval)) {
struct folio *folio = pmd_folio(*src_pmd); <----
Here looks like *src_pmd can be a migration entry. Is my understanding
correct?
> If we drop the check at [1] then the path that takes us to
If my above understanding is correct, IMHO it should be [2] above that
makes sure the reference won't happen on a swap entry, not necessarily [1]?
> split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> indication that split did not happen. Afterwards we will retry
So we're talking about the case where it's a swap pmd entry, right? Could
you elaborate why the split would fail? AFAIU, split_huge_pmd_locked()
should still work for a migration pmd entry.
Thanks,
> thinking that PMD got split and leaving further remapping to
> move_pages_pte() (see the comment before "continue"). I think in this
> case it will end up in the same path again instead (infinite loop). I
> didn't test this but from the code I think that's what would happen.
> Does that make sense?
>
> >
> > And it also looks applicable to also drop the "else" later, because in "if
> > (ptl)" it cannot hit pmd_none().
> >
> > Thanks,
> >
> > --
> > Peter Xu
> >
>
--
Peter Xu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-01 16:23 ` Peter Xu
@ 2025-08-01 16:41 ` Suren Baghdasaryan
2025-08-01 17:13 ` Peter Xu
0 siblings, 1 reply; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-08-01 16:41 UTC (permalink / raw)
To: Peter Xu
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Fri, Aug 1, 2025 at 9:23 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> > On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > > >
> > > > Hi!
> > > >
> > > > Did you mean in you patch description:
> > > >
> > > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > > >
> > > > Talking about THP holes is very very confusing.
> > > >
> > > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > > >
> > > > You mean a "non-present PMD that is not a migration entry".
> > > >
> > > > > hole and tries to access a non-existent folio, resulting in
> > > > > a crash. Add a check to skip non-present THPs.
> > > >
> > > > That makes sense. The code we have after this patch is rather complicated
> > > > and hard to read.
> > > >
> > > > >
> > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > Cc: stable@vger.kernel.org
> > > > > ---
> > > > > Changes since v1 [1]
> > > > > - Fixed step size calculation, per Lokesh Gidra
> > > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > > >
> > > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > > >
> > > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > index cbed91b09640..b5af31c22731 100644
> > > > > --- a/mm/userfaultfd.c
> > > > > +++ b/mm/userfaultfd.c
> > > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > if (ptl) {
> > > > > - /* Check if we can move the pmd without splitting it. */
> > > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > - !pmd_none(dst_pmdval)) {
> > > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > >
> > > [1]
> > >
> > > > > + /* Check if we can move the pmd without splitting it. */
> > > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > + !pmd_none(dst_pmdval)) {
> > > > > + if (pmd_present(*src_pmd)) {
>
> [2]
>
> > > > > + struct folio *folio = pmd_folio(*src_pmd);
>
> [3]
>
> > > > > +
> > > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > + !PageAnonExclusive(&folio->page))) {
> > > > > + spin_unlock(ptl);
> > > > > + err = -EBUSY;
> > > > > + break;
> > > > > + }
> > > > > + }
> > > >
> > > > ... in particular that. Is there some way to make this code simpler / easier
> > > > to read? Like moving that whole last folio-check thingy into a helper?
> > >
> > > One question might be relevant is, whether the check above [1] can be
> > > dropped.
> > >
> > > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > > before returning the ptl. I didn't follow closely on the recent changes on
> > > mm side on possible new pmd swap entries, if migration is the only possible
> > > one then it looks like [1] can be avoided.
> >
> > Hi Peter,
> > is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> > && !pmd_present()) PMD to pass and that's when this crash is hit.
>
> First for all, thanks for looking into the issue with Lokesh; I am still
> catching up with emails after taking weeks off.
>
> I didn't yet read into the syzbot report, but I thought the bug was about
> referencing the folio on top of a swap entry after reading your current
> patch, which has:
>
> if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> !pmd_none(dst_pmdval)) {
> struct folio *folio = pmd_folio(*src_pmd); <----
>
> Here looks like *src_pmd can be a migration entry. Is my understanding
> correct?
Correct.
>
> > If we drop the check at [1] then the path that takes us to
>
> If my above understanding is correct, IMHO it should be [2] above that
> makes sure the reference won't happen on a swap entry, not necessarily [1]?
Yes, in case of migration entry this is what protects us.
>
> > split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> > indication that split did not happen. Afterwards we will retry
>
> So we're talking about the case where it's a swap pmd entry, right?
Hmm, my understanding is that it's being treated as a swap entry but
in reality is not. I thought THPs are always split before they get
swapped, no?
> Could you elaborate why the split would fail?
Just looking at the code, split_huge_pmd_locked() checks for
(pmd_trans_huge(*pmd) || is_pmd_migration_entry(*pmd)).
pmd_trans_huge() is false if !pmd_present() and it's not a migration
entry, so __split_huge_pmd_locked() will be skipped.
> AFAIU, split_huge_pmd_locked()
> should still work for a migration pmd entry.
That is correct because the above mentioned is_pmd_migration_entry()
check will pass.
>
> Thanks,
>
> > thinking that PMD got split and leaving further remapping to
> > move_pages_pte() (see the comment before "continue"). I think in this
> > case it will end up in the same path again instead (infinite loop). I
> > didn't test this but from the code I think that's what would happen.
> > Does that make sense?
> >
> > >
> > > And it also looks applicable to also drop the "else" later, because in "if
> > > (ptl)" it cannot hit pmd_none().
> > >
> > > Thanks,
> > >
> > > --
> > > Peter Xu
> > >
> >
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-01 16:41 ` Suren Baghdasaryan
@ 2025-08-01 17:13 ` Peter Xu
2025-08-01 17:45 ` Suren Baghdasaryan
0 siblings, 1 reply; 21+ messages in thread
From: Peter Xu @ 2025-08-01 17:13 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Fri, Aug 01, 2025 at 09:41:31AM -0700, Suren Baghdasaryan wrote:
> On Fri, Aug 1, 2025 at 9:23 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> > > On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> > > >
> > > > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > > > >
> > > > > Hi!
> > > > >
> > > > > Did you mean in you patch description:
> > > > >
> > > > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > > > >
> > > > > Talking about THP holes is very very confusing.
> > > > >
> > > > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > > > >
> > > > > You mean a "non-present PMD that is not a migration entry".
> > > > >
> > > > > > hole and tries to access a non-existent folio, resulting in
> > > > > > a crash. Add a check to skip non-present THPs.
> > > > >
> > > > > That makes sense. The code we have after this patch is rather complicated
> > > > > and hard to read.
> > > > >
> > > > > >
> > > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > > Cc: stable@vger.kernel.org
> > > > > > ---
> > > > > > Changes since v1 [1]
> > > > > > - Fixed step size calculation, per Lokesh Gidra
> > > > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > > > >
> > > > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > > > >
> > > > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > > > >
> > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > > index cbed91b09640..b5af31c22731 100644
> > > > > > --- a/mm/userfaultfd.c
> > > > > > +++ b/mm/userfaultfd.c
> > > > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > > if (ptl) {
> > > > > > - /* Check if we can move the pmd without splitting it. */
> > > > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > - !pmd_none(dst_pmdval)) {
> > > > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > > >
> > > > [1]
> > > >
> > > > > > + /* Check if we can move the pmd without splitting it. */
> > > > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > + !pmd_none(dst_pmdval)) {
> > > > > > + if (pmd_present(*src_pmd)) {
> >
> > [2]
> >
> > > > > > + struct folio *folio = pmd_folio(*src_pmd);
> >
> > [3]
> >
> > > > > > +
> > > > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > > + !PageAnonExclusive(&folio->page))) {
> > > > > > + spin_unlock(ptl);
> > > > > > + err = -EBUSY;
> > > > > > + break;
> > > > > > + }
> > > > > > + }
> > > > >
> > > > > ... in particular that. Is there some way to make this code simpler / easier
> > > > > to read? Like moving that whole last folio-check thingy into a helper?
> > > >
> > > > One question might be relevant is, whether the check above [1] can be
> > > > dropped.
> > > >
> > > > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > > > before returning the ptl. I didn't follow closely on the recent changes on
> > > > mm side on possible new pmd swap entries, if migration is the only possible
> > > > one then it looks like [1] can be avoided.
> > >
> > > Hi Peter,
> > > is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> > > && !pmd_present()) PMD to pass and that's when this crash is hit.
> >
> > First for all, thanks for looking into the issue with Lokesh; I am still
> > catching up with emails after taking weeks off.
> >
> > I didn't yet read into the syzbot report, but I thought the bug was about
> > referencing the folio on top of a swap entry after reading your current
> > patch, which has:
> >
> > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > !pmd_none(dst_pmdval)) {
> > struct folio *folio = pmd_folio(*src_pmd); <----
> >
> > Here looks like *src_pmd can be a migration entry. Is my understanding
> > correct?
>
> Correct.
>
> >
> > > If we drop the check at [1] then the path that takes us to
> >
> > If my above understanding is correct, IMHO it should be [2] above that
> > makes sure the reference won't happen on a swap entry, not necessarily [1]?
>
> Yes, in case of migration entry this is what protects us.
>
> >
> > > split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> > > indication that split did not happen. Afterwards we will retry
> >
> > So we're talking about the case where it's a swap pmd entry, right?
>
> Hmm, my understanding is that it's being treated as a swap entry but
> in reality is not. I thought THPs are always split before they get
> swapped, no?
Yes they should be split, afaiu.
>
> > Could you elaborate why the split would fail?
>
> Just looking at the code, split_huge_pmd_locked() checks for
> (pmd_trans_huge(*pmd) || is_pmd_migration_entry(*pmd)).
> pmd_trans_huge() is false if !pmd_present() and it's not a migration
> entry, so __split_huge_pmd_locked() will be skipped.
Here might be the major part of where confusion came from: I thought it
must be a migration pmd entry to hit the issue, so it's not?
I checked the code just now:
__handle_mm_fault:
if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
VM_BUG_ON(thp_migration_supported() &&
!is_pmd_migration_entry(vmf.orig_pmd));
So IIUC pmd migration entry is still the only possible way to have a swap
entry. It doesn't look like we have "real" swap entries for PMD (which can
further points to some swapfiles)?
--
Peter Xu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-01 17:13 ` Peter Xu
@ 2025-08-01 17:45 ` Suren Baghdasaryan
2025-08-01 18:20 ` Peter Xu
0 siblings, 1 reply; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-08-01 17:45 UTC (permalink / raw)
To: Peter Xu
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Fri, Aug 1, 2025 at 5:13 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Aug 01, 2025 at 09:41:31AM -0700, Suren Baghdasaryan wrote:
> > On Fri, Aug 1, 2025 at 9:23 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> > > > On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> > > > >
> > > > > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > > > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > > > > >
> > > > > > Hi!
> > > > > >
> > > > > > Did you mean in you patch description:
> > > > > >
> > > > > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > > > > >
> > > > > > Talking about THP holes is very very confusing.
> > > > > >
> > > > > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > > > > >
> > > > > > You mean a "non-present PMD that is not a migration entry".
> > > > > >
> > > > > > > hole and tries to access a non-existent folio, resulting in
> > > > > > > a crash. Add a check to skip non-present THPs.
> > > > > >
> > > > > > That makes sense. The code we have after this patch is rather complicated
> > > > > > and hard to read.
> > > > > >
> > > > > > >
> > > > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > ---
> > > > > > > Changes since v1 [1]
> > > > > > > - Fixed step size calculation, per Lokesh Gidra
> > > > > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > > > > >
> > > > > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > > > index cbed91b09640..b5af31c22731 100644
> > > > > > > --- a/mm/userfaultfd.c
> > > > > > > +++ b/mm/userfaultfd.c
> > > > > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > > > if (ptl) {
> > > > > > > - /* Check if we can move the pmd without splitting it. */
> > > > > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > - !pmd_none(dst_pmdval)) {
> > > > > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > > > >
> > > > > [1]
> > > > >
> > > > > > > + /* Check if we can move the pmd without splitting it. */
> > > > > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > + !pmd_none(dst_pmdval)) {
> > > > > > > + if (pmd_present(*src_pmd)) {
> > >
> > > [2]
> > >
> > > > > > > + struct folio *folio = pmd_folio(*src_pmd);
> > >
> > > [3]
> > >
> > > > > > > +
> > > > > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > > > + !PageAnonExclusive(&folio->page))) {
> > > > > > > + spin_unlock(ptl);
> > > > > > > + err = -EBUSY;
> > > > > > > + break;
> > > > > > > + }
> > > > > > > + }
> > > > > >
> > > > > > ... in particular that. Is there some way to make this code simpler / easier
> > > > > > to read? Like moving that whole last folio-check thingy into a helper?
> > > > >
> > > > > One question might be relevant is, whether the check above [1] can be
> > > > > dropped.
> > > > >
> > > > > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > > > > before returning the ptl. I didn't follow closely on the recent changes on
> > > > > mm side on possible new pmd swap entries, if migration is the only possible
> > > > > one then it looks like [1] can be avoided.
> > > >
> > > > Hi Peter,
> > > > is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> > > > && !pmd_present()) PMD to pass and that's when this crash is hit.
> > >
> > > First for all, thanks for looking into the issue with Lokesh; I am still
> > > catching up with emails after taking weeks off.
> > >
> > > I didn't yet read into the syzbot report, but I thought the bug was about
> > > referencing the folio on top of a swap entry after reading your current
> > > patch, which has:
> > >
> > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > !pmd_none(dst_pmdval)) {
> > > struct folio *folio = pmd_folio(*src_pmd); <----
> > >
> > > Here looks like *src_pmd can be a migration entry. Is my understanding
> > > correct?
> >
> > Correct.
> >
> > >
> > > > If we drop the check at [1] then the path that takes us to
> > >
> > > If my above understanding is correct, IMHO it should be [2] above that
> > > makes sure the reference won't happen on a swap entry, not necessarily [1]?
> >
> > Yes, in case of migration entry this is what protects us.
> >
> > >
> > > > split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> > > > indication that split did not happen. Afterwards we will retry
> > >
> > > So we're talking about the case where it's a swap pmd entry, right?
> >
> > Hmm, my understanding is that it's being treated as a swap entry but
> > in reality is not. I thought THPs are always split before they get
> > swapped, no?
>
> Yes they should be split, afaiu.
>
> >
> > > Could you elaborate why the split would fail?
> >
> > Just looking at the code, split_huge_pmd_locked() checks for
> > (pmd_trans_huge(*pmd) || is_pmd_migration_entry(*pmd)).
> > pmd_trans_huge() is false if !pmd_present() and it's not a migration
> > entry, so __split_huge_pmd_locked() will be skipped.
>
> Here might be the major part of where confusion came from: I thought it
> must be a migration pmd entry to hit the issue, so it's not?
>
> I checked the code just now:
>
> __handle_mm_fault:
> if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
> VM_BUG_ON(thp_migration_supported() &&
> !is_pmd_migration_entry(vmf.orig_pmd));
>
> So IIUC pmd migration entry is still the only possible way to have a swap
> entry. It doesn't look like we have "real" swap entries for PMD (which can
> further points to some swapfiles)?
Correct. AFAIU here we stumble on a pmd entry which was allocated but
never populated.
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-01 17:45 ` Suren Baghdasaryan
@ 2025-08-01 18:20 ` Peter Xu
2025-08-01 19:30 ` Suren Baghdasaryan
0 siblings, 1 reply; 21+ messages in thread
From: Peter Xu @ 2025-08-01 18:20 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Fri, Aug 01, 2025 at 05:45:10PM +0000, Suren Baghdasaryan wrote:
> On Fri, Aug 1, 2025 at 5:13 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Fri, Aug 01, 2025 at 09:41:31AM -0700, Suren Baghdasaryan wrote:
> > > On Fri, Aug 1, 2025 at 9:23 AM Peter Xu <peterx@redhat.com> wrote:
> > > >
> > > > On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> > > > > On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > > > > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > > > > > >
> > > > > > > Hi!
> > > > > > >
> > > > > > > Did you mean in you patch description:
> > > > > > >
> > > > > > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > > > > > >
> > > > > > > Talking about THP holes is very very confusing.
> > > > > > >
> > > > > > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > > > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > > > > > >
> > > > > > > You mean a "non-present PMD that is not a migration entry".
> > > > > > >
> > > > > > > > hole and tries to access a non-existent folio, resulting in
> > > > > > > > a crash. Add a check to skip non-present THPs.
> > > > > > >
> > > > > > > That makes sense. The code we have after this patch is rather complicated
> > > > > > > and hard to read.
> > > > > > >
> > > > > > > >
> > > > > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > > > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > > > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > ---
> > > > > > > > Changes since v1 [1]
> > > > > > > > - Fixed step size calculation, per Lokesh Gidra
> > > > > > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > > > > > >
> > > > > > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > > > > > >
> > > > > > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > > > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > > > > index cbed91b09640..b5af31c22731 100644
> > > > > > > > --- a/mm/userfaultfd.c
> > > > > > > > +++ b/mm/userfaultfd.c
> > > > > > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > > > > if (ptl) {
> > > > > > > > - /* Check if we can move the pmd without splitting it. */
> > > > > > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > - !pmd_none(dst_pmdval)) {
> > > > > > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > > > > >
> > > > > > [1]
> > > > > >
> > > > > > > > + /* Check if we can move the pmd without splitting it. */
> > > > > > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > + !pmd_none(dst_pmdval)) {
> > > > > > > > + if (pmd_present(*src_pmd)) {
> > > >
> > > > [2]
> > > >
> > > > > > > > + struct folio *folio = pmd_folio(*src_pmd);
> > > >
> > > > [3]
> > > >
> > > > > > > > +
> > > > > > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > > > > + !PageAnonExclusive(&folio->page))) {
> > > > > > > > + spin_unlock(ptl);
> > > > > > > > + err = -EBUSY;
> > > > > > > > + break;
> > > > > > > > + }
> > > > > > > > + }
> > > > > > >
> > > > > > > ... in particular that. Is there some way to make this code simpler / easier
> > > > > > > to read? Like moving that whole last folio-check thingy into a helper?
> > > > > >
> > > > > > One question might be relevant is, whether the check above [1] can be
> > > > > > dropped.
> > > > > >
> > > > > > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > > > > > before returning the ptl. I didn't follow closely on the recent changes on
> > > > > > mm side on possible new pmd swap entries, if migration is the only possible
> > > > > > one then it looks like [1] can be avoided.
> > > > >
> > > > > Hi Peter,
> > > > > is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> > > > > && !pmd_present()) PMD to pass and that's when this crash is hit.
> > > >
> > > > First for all, thanks for looking into the issue with Lokesh; I am still
> > > > catching up with emails after taking weeks off.
> > > >
> > > > I didn't yet read into the syzbot report, but I thought the bug was about
> > > > referencing the folio on top of a swap entry after reading your current
> > > > patch, which has:
> > > >
> > > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > !pmd_none(dst_pmdval)) {
> > > > struct folio *folio = pmd_folio(*src_pmd); <----
> > > >
> > > > Here looks like *src_pmd can be a migration entry. Is my understanding
> > > > correct?
> > >
> > > Correct.
> > >
> > > >
> > > > > If we drop the check at [1] then the path that takes us to
> > > >
> > > > If my above understanding is correct, IMHO it should be [2] above that
> > > > makes sure the reference won't happen on a swap entry, not necessarily [1]?
> > >
> > > Yes, in case of migration entry this is what protects us.
> > >
> > > >
> > > > > split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> > > > > indication that split did not happen. Afterwards we will retry
> > > >
> > > > So we're talking about the case where it's a swap pmd entry, right?
> > >
> > > Hmm, my understanding is that it's being treated as a swap entry but
> > > in reality is not. I thought THPs are always split before they get
> > > swapped, no?
> >
> > Yes they should be split, afaiu.
> >
> > >
> > > > Could you elaborate why the split would fail?
> > >
> > > Just looking at the code, split_huge_pmd_locked() checks for
> > > (pmd_trans_huge(*pmd) || is_pmd_migration_entry(*pmd)).
> > > pmd_trans_huge() is false if !pmd_present() and it's not a migration
> > > entry, so __split_huge_pmd_locked() will be skipped.
> >
> > Here might be the major part of where confusion came from: I thought it
> > must be a migration pmd entry to hit the issue, so it's not?
> >
> > I checked the code just now:
> >
> > __handle_mm_fault:
> > if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
> > VM_BUG_ON(thp_migration_supported() &&
> > !is_pmd_migration_entry(vmf.orig_pmd));
> >
> > So IIUC pmd migration entry is still the only possible way to have a swap
> > entry. It doesn't look like we have "real" swap entries for PMD (which can
> > further points to some swapfiles)?
>
> Correct. AFAIU here we stumble on a pmd entry which was allocated but
> never populated.
Do you mean a pmd_none()?
If so, that goes back to my original question, on why
__pmd_trans_huge_lock() returns non-NULL if it's a pmd_none()? IMHO it
really should have returned NULL for pmd_none().
IOW, I still don't understand why below won't already work:
===8<===
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 52d7d5f144b8e..33e78f52ee9f5 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -1880,13 +1880,15 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
/* Check if we can move the pmd without splitting it. */
if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
!pmd_none(dst_pmdval)) {
- struct folio *folio = pmd_folio(*src_pmd);
-
- if (!folio || (!is_huge_zero_folio(folio) &&
- !PageAnonExclusive(&folio->page))) {
- spin_unlock(ptl);
- err = -EBUSY;
- break;
+ if (pmd_present(*src_pmd)) {
+ struct folio *folio = pmd_folio(*src_pmd);
+
+ if (!folio || (!is_huge_zero_folio(folio) &&
+ !PageAnonExclusive(&folio->page))) {
+ spin_unlock(ptl);
+ err = -EBUSY;
+ break;
+ }
}
spin_unlock(ptl);
===8<===
Likely I missed something important.. I'll be afk for a while soon, I'll
also double check (maybe early next week) on the reproducer.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-01 18:20 ` Peter Xu
@ 2025-08-01 19:30 ` Suren Baghdasaryan
2025-08-02 0:32 ` Peter Xu
0 siblings, 1 reply; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-08-01 19:30 UTC (permalink / raw)
To: Peter Xu
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Fri, Aug 1, 2025 at 6:21 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Aug 01, 2025 at 05:45:10PM +0000, Suren Baghdasaryan wrote:
> > On Fri, Aug 1, 2025 at 5:13 PM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Fri, Aug 01, 2025 at 09:41:31AM -0700, Suren Baghdasaryan wrote:
> > > > On Fri, Aug 1, 2025 at 9:23 AM Peter Xu <peterx@redhat.com> wrote:
> > > > >
> > > > > On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> > > > > > On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > >
> > > > > > > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > > > > > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > > > > > > >
> > > > > > > > Hi!
> > > > > > > >
> > > > > > > > Did you mean in you patch description:
> > > > > > > >
> > > > > > > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > > > > > > >
> > > > > > > > Talking about THP holes is very very confusing.
> > > > > > > >
> > > > > > > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > > > > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > > > > > > >
> > > > > > > > You mean a "non-present PMD that is not a migration entry".
> > > > > > > >
> > > > > > > > > hole and tries to access a non-existent folio, resulting in
> > > > > > > > > a crash. Add a check to skip non-present THPs.
> > > > > > > >
> > > > > > > > That makes sense. The code we have after this patch is rather complicated
> > > > > > > > and hard to read.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > > > > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > > > > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > > ---
> > > > > > > > > Changes since v1 [1]
> > > > > > > > > - Fixed step size calculation, per Lokesh Gidra
> > > > > > > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > > > > > > >
> > > > > > > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > > > > > > >
> > > > > > > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > > > > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > > > > > index cbed91b09640..b5af31c22731 100644
> > > > > > > > > --- a/mm/userfaultfd.c
> > > > > > > > > +++ b/mm/userfaultfd.c
> > > > > > > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > > > > > if (ptl) {
> > > > > > > > > - /* Check if we can move the pmd without splitting it. */
> > > > > > > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > - !pmd_none(dst_pmdval)) {
> > > > > > > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > > > > > >
> > > > > > > [1]
> > > > > > >
> > > > > > > > > + /* Check if we can move the pmd without splitting it. */
> > > > > > > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > + !pmd_none(dst_pmdval)) {
> > > > > > > > > + if (pmd_present(*src_pmd)) {
> > > > >
> > > > > [2]
> > > > >
> > > > > > > > > + struct folio *folio = pmd_folio(*src_pmd);
> > > > >
> > > > > [3]
> > > > >
> > > > > > > > > +
> > > > > > > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > > > > > + !PageAnonExclusive(&folio->page))) {
> > > > > > > > > + spin_unlock(ptl);
> > > > > > > > > + err = -EBUSY;
> > > > > > > > > + break;
> > > > > > > > > + }
> > > > > > > > > + }
> > > > > > > >
> > > > > > > > ... in particular that. Is there some way to make this code simpler / easier
> > > > > > > > to read? Like moving that whole last folio-check thingy into a helper?
> > > > > > >
> > > > > > > One question might be relevant is, whether the check above [1] can be
> > > > > > > dropped.
> > > > > > >
> > > > > > > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > > > > > > before returning the ptl. I didn't follow closely on the recent changes on
> > > > > > > mm side on possible new pmd swap entries, if migration is the only possible
> > > > > > > one then it looks like [1] can be avoided.
> > > > > >
> > > > > > Hi Peter,
> > > > > > is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> > > > > > && !pmd_present()) PMD to pass and that's when this crash is hit.
> > > > >
> > > > > First for all, thanks for looking into the issue with Lokesh; I am still
> > > > > catching up with emails after taking weeks off.
> > > > >
> > > > > I didn't yet read into the syzbot report, but I thought the bug was about
> > > > > referencing the folio on top of a swap entry after reading your current
> > > > > patch, which has:
> > > > >
> > > > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > !pmd_none(dst_pmdval)) {
> > > > > struct folio *folio = pmd_folio(*src_pmd); <----
> > > > >
> > > > > Here looks like *src_pmd can be a migration entry. Is my understanding
> > > > > correct?
> > > >
> > > > Correct.
> > > >
> > > > >
> > > > > > If we drop the check at [1] then the path that takes us to
> > > > >
> > > > > If my above understanding is correct, IMHO it should be [2] above that
> > > > > makes sure the reference won't happen on a swap entry, not necessarily [1]?
> > > >
> > > > Yes, in case of migration entry this is what protects us.
> > > >
> > > > >
> > > > > > split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> > > > > > indication that split did not happen. Afterwards we will retry
> > > > >
> > > > > So we're talking about the case where it's a swap pmd entry, right?
> > > >
> > > > Hmm, my understanding is that it's being treated as a swap entry but
> > > > in reality is not. I thought THPs are always split before they get
> > > > swapped, no?
> > >
> > > Yes they should be split, afaiu.
> > >
> > > >
> > > > > Could you elaborate why the split would fail?
> > > >
> > > > Just looking at the code, split_huge_pmd_locked() checks for
> > > > (pmd_trans_huge(*pmd) || is_pmd_migration_entry(*pmd)).
> > > > pmd_trans_huge() is false if !pmd_present() and it's not a migration
> > > > entry, so __split_huge_pmd_locked() will be skipped.
> > >
> > > Here might be the major part of where confusion came from: I thought it
> > > must be a migration pmd entry to hit the issue, so it's not?
> > >
> > > I checked the code just now:
> > >
> > > __handle_mm_fault:
> > > if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
> > > VM_BUG_ON(thp_migration_supported() &&
> > > !is_pmd_migration_entry(vmf.orig_pmd));
> > >
> > > So IIUC pmd migration entry is still the only possible way to have a swap
> > > entry. It doesn't look like we have "real" swap entries for PMD (which can
> > > further points to some swapfiles)?
> >
> > Correct. AFAIU here we stumble on a pmd entry which was allocated but
> > never populated.
>
> Do you mean a pmd_none()?
Yes.
>
> If so, that goes back to my original question, on why
> __pmd_trans_huge_lock() returns non-NULL if it's a pmd_none()? IMHO it
> really should have returned NULL for pmd_none().
That was exactly the answer I gave Lokesh when he theorized about the
cause of this crash but after reproducing it I saw that
pmd_trans_huge_lock() happily returns the PTL as long as PMD is not
pmd_none(). And that's because it passes as is_swap_pmd(). But even if
we change that we still need to implement the code to skip the entire
PMD.
>
> IOW, I still don't understand why below won't already work:
>
> ===8<===
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 52d7d5f144b8e..33e78f52ee9f5 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1880,13 +1880,15 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> /* Check if we can move the pmd without splitting it. */
> if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> !pmd_none(dst_pmdval)) {
> - struct folio *folio = pmd_folio(*src_pmd);
> -
> - if (!folio || (!is_huge_zero_folio(folio) &&
> - !PageAnonExclusive(&folio->page))) {
> - spin_unlock(ptl);
> - err = -EBUSY;
> - break;
> + if (pmd_present(*src_pmd)) {
> + struct folio *folio = pmd_folio(*src_pmd);
> +
> + if (!folio || (!is_huge_zero_folio(folio) &&
> + !PageAnonExclusive(&folio->page))) {
> + spin_unlock(ptl);
> + err = -EBUSY;
> + break;
> + }
> }
>
> spin_unlock(ptl);
>
> ===8<===
>
> Likely I missed something important.. I'll be afk for a while soon, I'll
> also double check (maybe early next week) on the reproducer.
>
> Thanks,
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-01 19:30 ` Suren Baghdasaryan
@ 2025-08-02 0:32 ` Peter Xu
2025-08-04 14:55 ` Suren Baghdasaryan
0 siblings, 1 reply; 21+ messages in thread
From: Peter Xu @ 2025-08-02 0:32 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Fri, Aug 01, 2025 at 07:30:02PM +0000, Suren Baghdasaryan wrote:
> On Fri, Aug 1, 2025 at 6:21 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Fri, Aug 01, 2025 at 05:45:10PM +0000, Suren Baghdasaryan wrote:
> > > On Fri, Aug 1, 2025 at 5:13 PM Peter Xu <peterx@redhat.com> wrote:
> > > >
> > > > On Fri, Aug 01, 2025 at 09:41:31AM -0700, Suren Baghdasaryan wrote:
> > > > > On Fri, Aug 1, 2025 at 9:23 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> > > > > > > On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > > > > > > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > > > > > > > >
> > > > > > > > > Hi!
> > > > > > > > >
> > > > > > > > > Did you mean in you patch description:
> > > > > > > > >
> > > > > > > > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > > > > > > > >
> > > > > > > > > Talking about THP holes is very very confusing.
> > > > > > > > >
> > > > > > > > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > > > > > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > > > > > > > >
> > > > > > > > > You mean a "non-present PMD that is not a migration entry".
> > > > > > > > >
> > > > > > > > > > hole and tries to access a non-existent folio, resulting in
> > > > > > > > > > a crash. Add a check to skip non-present THPs.
> > > > > > > > >
> > > > > > > > > That makes sense. The code we have after this patch is rather complicated
> > > > > > > > > and hard to read.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > > > > > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > > > > > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > > > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > > > ---
> > > > > > > > > > Changes since v1 [1]
> > > > > > > > > > - Fixed step size calculation, per Lokesh Gidra
> > > > > > > > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > > > > > > > >
> > > > > > > > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > > > > > > > >
> > > > > > > > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > > > > > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > > > > > > index cbed91b09640..b5af31c22731 100644
> > > > > > > > > > --- a/mm/userfaultfd.c
> > > > > > > > > > +++ b/mm/userfaultfd.c
> > > > > > > > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > > > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > > > > > > if (ptl) {
> > > > > > > > > > - /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > - !pmd_none(dst_pmdval)) {
> > > > > > > > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > > > > > > >
> > > > > > > > [1]
> > > > > > > >
> > > > > > > > > > + /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > + !pmd_none(dst_pmdval)) {
> > > > > > > > > > + if (pmd_present(*src_pmd)) {
> > > > > >
> > > > > > [2]
> > > > > >
> > > > > > > > > > + struct folio *folio = pmd_folio(*src_pmd);
> > > > > >
> > > > > > [3]
> > > > > >
> > > > > > > > > > +
> > > > > > > > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > > > > > > + !PageAnonExclusive(&folio->page))) {
> > > > > > > > > > + spin_unlock(ptl);
> > > > > > > > > > + err = -EBUSY;
> > > > > > > > > > + break;
> > > > > > > > > > + }
> > > > > > > > > > + }
> > > > > > > > >
> > > > > > > > > ... in particular that. Is there some way to make this code simpler / easier
> > > > > > > > > to read? Like moving that whole last folio-check thingy into a helper?
> > > > > > > >
> > > > > > > > One question might be relevant is, whether the check above [1] can be
> > > > > > > > dropped.
> > > > > > > >
> > > > > > > > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > > > > > > > before returning the ptl. I didn't follow closely on the recent changes on
> > > > > > > > mm side on possible new pmd swap entries, if migration is the only possible
> > > > > > > > one then it looks like [1] can be avoided.
> > > > > > >
> > > > > > > Hi Peter,
> > > > > > > is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> > > > > > > && !pmd_present()) PMD to pass and that's when this crash is hit.
> > > > > >
> > > > > > First for all, thanks for looking into the issue with Lokesh; I am still
> > > > > > catching up with emails after taking weeks off.
> > > > > >
> > > > > > I didn't yet read into the syzbot report, but I thought the bug was about
> > > > > > referencing the folio on top of a swap entry after reading your current
> > > > > > patch, which has:
> > > > > >
> > > > > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > !pmd_none(dst_pmdval)) {
> > > > > > struct folio *folio = pmd_folio(*src_pmd); <----
> > > > > >
> > > > > > Here looks like *src_pmd can be a migration entry. Is my understanding
> > > > > > correct?
> > > > >
> > > > > Correct.
> > > > >
> > > > > >
> > > > > > > If we drop the check at [1] then the path that takes us to
> > > > > >
> > > > > > If my above understanding is correct, IMHO it should be [2] above that
> > > > > > makes sure the reference won't happen on a swap entry, not necessarily [1]?
> > > > >
> > > > > Yes, in case of migration entry this is what protects us.
> > > > >
> > > > > >
> > > > > > > split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> > > > > > > indication that split did not happen. Afterwards we will retry
> > > > > >
> > > > > > So we're talking about the case where it's a swap pmd entry, right?
> > > > >
> > > > > Hmm, my understanding is that it's being treated as a swap entry but
> > > > > in reality is not. I thought THPs are always split before they get
> > > > > swapped, no?
> > > >
> > > > Yes they should be split, afaiu.
> > > >
> > > > >
> > > > > > Could you elaborate why the split would fail?
> > > > >
> > > > > Just looking at the code, split_huge_pmd_locked() checks for
> > > > > (pmd_trans_huge(*pmd) || is_pmd_migration_entry(*pmd)).
> > > > > pmd_trans_huge() is false if !pmd_present() and it's not a migration
> > > > > entry, so __split_huge_pmd_locked() will be skipped.
> > > >
> > > > Here might be the major part of where confusion came from: I thought it
> > > > must be a migration pmd entry to hit the issue, so it's not?
> > > >
> > > > I checked the code just now:
> > > >
> > > > __handle_mm_fault:
> > > > if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
> > > > VM_BUG_ON(thp_migration_supported() &&
> > > > !is_pmd_migration_entry(vmf.orig_pmd));
> > > >
> > > > So IIUC pmd migration entry is still the only possible way to have a swap
> > > > entry. It doesn't look like we have "real" swap entries for PMD (which can
> > > > further points to some swapfiles)?
> > >
> > > Correct. AFAIU here we stumble on a pmd entry which was allocated but
> > > never populated.
> >
> > Do you mean a pmd_none()?
>
> Yes.
>
> >
> > If so, that goes back to my original question, on why
> > __pmd_trans_huge_lock() returns non-NULL if it's a pmd_none()? IMHO it
> > really should have returned NULL for pmd_none().
>
> That was exactly the answer I gave Lokesh when he theorized about the
> cause of this crash but after reproducing it I saw that
> pmd_trans_huge_lock() happily returns the PTL as long as PMD is not
> pmd_none(). And that's because it passes as is_swap_pmd(). But even if
> we change that we still need to implement the code to skip the entire
> PMD.
The thing is I thought if pmd_trans_huge_lock() can return non-NULL, it
must be either a migration entry or a present THP. So are you describing a
THP but with present bit cleared? Do you know what is that entry, and why
it has present bit cleared?
I think my attention got attracted to pmd migration entry too much, so I
didn't really notice such possibility, as I believe migration pmd is broken
already in this path.
The original code:
ptl = pmd_trans_huge_lock(src_pmd, src_vma);
if (ptl) {
/* Check if we can move the pmd without splitting it. */
if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
!pmd_none(dst_pmdval)) {
struct folio *folio = pmd_folio(*src_pmd);
if (!folio || (!is_huge_zero_folio(folio) &&
!PageAnonExclusive(&folio->page))) {
spin_unlock(ptl);
err = -EBUSY;
break;
}
spin_unlock(ptl);
split_huge_pmd(src_vma, src_pmd, src_addr);
/* The folio will be split by move_pages_pte() */
continue;
}
err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
dst_pmdval, dst_vma, src_vma,
dst_addr, src_addr);
step_size = HPAGE_PMD_SIZE;
} else {
It'll get ptl for a migration pmd, then pmd_folio is risky without checking
present bit. That's what my previous smaller patch wanted to fix.
But besides that, IIUC it's all fine at least for a pmd migration entry,
because when with the smaller patch applied, either we'll try to split the
pmd migration entry, or we'll do move_pages_huge_pmd(), which internally
handles the pmd migration entry too by waiting on it:
if (!pmd_trans_huge(src_pmdval)) {
spin_unlock(src_ptl);
if (is_pmd_migration_entry(src_pmdval)) {
pmd_migration_entry_wait(mm, &src_pmdval);
return -EAGAIN;
}
return -ENOENT;
}
Then logically after the migration entry got recovered, we'll either see a
real THP or pmd none next time.
Some explanation on the problematic non-present THP entry would be helpful.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-02 0:32 ` Peter Xu
@ 2025-08-04 14:55 ` Suren Baghdasaryan
2025-08-05 14:39 ` Peter Xu
0 siblings, 1 reply; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-08-04 14:55 UTC (permalink / raw)
To: Peter Xu
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Fri, Aug 1, 2025 at 5:32 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Aug 01, 2025 at 07:30:02PM +0000, Suren Baghdasaryan wrote:
> > On Fri, Aug 1, 2025 at 6:21 PM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Fri, Aug 01, 2025 at 05:45:10PM +0000, Suren Baghdasaryan wrote:
> > > > On Fri, Aug 1, 2025 at 5:13 PM Peter Xu <peterx@redhat.com> wrote:
> > > > >
> > > > > On Fri, Aug 01, 2025 at 09:41:31AM -0700, Suren Baghdasaryan wrote:
> > > > > > On Fri, Aug 1, 2025 at 9:23 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > >
> > > > > > > On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> > > > > > > > On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > > > > > > > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > > > > > > > > >
> > > > > > > > > > Hi!
> > > > > > > > > >
> > > > > > > > > > Did you mean in you patch description:
> > > > > > > > > >
> > > > > > > > > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > > > > > > > > >
> > > > > > > > > > Talking about THP holes is very very confusing.
> > > > > > > > > >
> > > > > > > > > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > > > > > > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > > > > > > > > >
> > > > > > > > > > You mean a "non-present PMD that is not a migration entry".
> > > > > > > > > >
> > > > > > > > > > > hole and tries to access a non-existent folio, resulting in
> > > > > > > > > > > a crash. Add a check to skip non-present THPs.
> > > > > > > > > >
> > > > > > > > > > That makes sense. The code we have after this patch is rather complicated
> > > > > > > > > > and hard to read.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > > > > > > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > > > > > > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > > > > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > > > > ---
> > > > > > > > > > > Changes since v1 [1]
> > > > > > > > > > > - Fixed step size calculation, per Lokesh Gidra
> > > > > > > > > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > > > > > > > > >
> > > > > > > > > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > > > > > > > > >
> > > > > > > > > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > > > > > > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > > > > > > > index cbed91b09640..b5af31c22731 100644
> > > > > > > > > > > --- a/mm/userfaultfd.c
> > > > > > > > > > > +++ b/mm/userfaultfd.c
> > > > > > > > > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > > > > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > > > > > > > if (ptl) {
> > > > > > > > > > > - /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > > - !pmd_none(dst_pmdval)) {
> > > > > > > > > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > > > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > > > > > > > >
> > > > > > > > > [1]
> > > > > > > > >
> > > > > > > > > > > + /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > > + !pmd_none(dst_pmdval)) {
> > > > > > > > > > > + if (pmd_present(*src_pmd)) {
> > > > > > >
> > > > > > > [2]
> > > > > > >
> > > > > > > > > > > + struct folio *folio = pmd_folio(*src_pmd);
> > > > > > >
> > > > > > > [3]
> > > > > > >
> > > > > > > > > > > +
> > > > > > > > > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > > > > > > > + !PageAnonExclusive(&folio->page))) {
> > > > > > > > > > > + spin_unlock(ptl);
> > > > > > > > > > > + err = -EBUSY;
> > > > > > > > > > > + break;
> > > > > > > > > > > + }
> > > > > > > > > > > + }
> > > > > > > > > >
> > > > > > > > > > ... in particular that. Is there some way to make this code simpler / easier
> > > > > > > > > > to read? Like moving that whole last folio-check thingy into a helper?
> > > > > > > > >
> > > > > > > > > One question might be relevant is, whether the check above [1] can be
> > > > > > > > > dropped.
> > > > > > > > >
> > > > > > > > > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > > > > > > > > before returning the ptl. I didn't follow closely on the recent changes on
> > > > > > > > > mm side on possible new pmd swap entries, if migration is the only possible
> > > > > > > > > one then it looks like [1] can be avoided.
> > > > > > > >
> > > > > > > > Hi Peter,
> > > > > > > > is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> > > > > > > > && !pmd_present()) PMD to pass and that's when this crash is hit.
> > > > > > >
> > > > > > > First for all, thanks for looking into the issue with Lokesh; I am still
> > > > > > > catching up with emails after taking weeks off.
> > > > > > >
> > > > > > > I didn't yet read into the syzbot report, but I thought the bug was about
> > > > > > > referencing the folio on top of a swap entry after reading your current
> > > > > > > patch, which has:
> > > > > > >
> > > > > > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > !pmd_none(dst_pmdval)) {
> > > > > > > struct folio *folio = pmd_folio(*src_pmd); <----
> > > > > > >
> > > > > > > Here looks like *src_pmd can be a migration entry. Is my understanding
> > > > > > > correct?
> > > > > >
> > > > > > Correct.
> > > > > >
> > > > > > >
> > > > > > > > If we drop the check at [1] then the path that takes us to
> > > > > > >
> > > > > > > If my above understanding is correct, IMHO it should be [2] above that
> > > > > > > makes sure the reference won't happen on a swap entry, not necessarily [1]?
> > > > > >
> > > > > > Yes, in case of migration entry this is what protects us.
> > > > > >
> > > > > > >
> > > > > > > > split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> > > > > > > > indication that split did not happen. Afterwards we will retry
> > > > > > >
> > > > > > > So we're talking about the case where it's a swap pmd entry, right?
> > > > > >
> > > > > > Hmm, my understanding is that it's being treated as a swap entry but
> > > > > > in reality is not. I thought THPs are always split before they get
> > > > > > swapped, no?
> > > > >
> > > > > Yes they should be split, afaiu.
> > > > >
> > > > > >
> > > > > > > Could you elaborate why the split would fail?
> > > > > >
> > > > > > Just looking at the code, split_huge_pmd_locked() checks for
> > > > > > (pmd_trans_huge(*pmd) || is_pmd_migration_entry(*pmd)).
> > > > > > pmd_trans_huge() is false if !pmd_present() and it's not a migration
> > > > > > entry, so __split_huge_pmd_locked() will be skipped.
> > > > >
> > > > > Here might be the major part of where confusion came from: I thought it
> > > > > must be a migration pmd entry to hit the issue, so it's not?
> > > > >
> > > > > I checked the code just now:
> > > > >
> > > > > __handle_mm_fault:
> > > > > if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
> > > > > VM_BUG_ON(thp_migration_supported() &&
> > > > > !is_pmd_migration_entry(vmf.orig_pmd));
> > > > >
> > > > > So IIUC pmd migration entry is still the only possible way to have a swap
> > > > > entry. It doesn't look like we have "real" swap entries for PMD (which can
> > > > > further points to some swapfiles)?
> > > >
> > > > Correct. AFAIU here we stumble on a pmd entry which was allocated but
> > > > never populated.
> > >
> > > Do you mean a pmd_none()?
> >
> > Yes.
> >
> > >
> > > If so, that goes back to my original question, on why
> > > __pmd_trans_huge_lock() returns non-NULL if it's a pmd_none()? IMHO it
> > > really should have returned NULL for pmd_none().
> >
> > That was exactly the answer I gave Lokesh when he theorized about the
> > cause of this crash but after reproducing it I saw that
> > pmd_trans_huge_lock() happily returns the PTL as long as PMD is not
> > pmd_none(). And that's because it passes as is_swap_pmd(). But even if
> > we change that we still need to implement the code to skip the entire
> > PMD.
>
> The thing is I thought if pmd_trans_huge_lock() can return non-NULL, it
> must be either a migration entry or a present THP. So are you describing a
> THP but with present bit cleared? Do you know what is that entry, and why
> it has present bit cleared?
In this case it's because earlier we allocated that PMD here:
https://elixir.bootlin.com/linux/v6.16/source/mm/userfaultfd.c#L1797
but wouldn't that be the same if the PMD was mapped and then got
unmapped later? My understanding is that we allocate the PMD at the
line I pointed to make UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES case the same
as this unmapped PMD case. If my assumption is incorrect then we could
skip the hole earlier instead of allocating the PMD for it.
>
> I think my attention got attracted to pmd migration entry too much, so I
> didn't really notice such possibility, as I believe migration pmd is broken
> already in this path.
>
> The original code:
>
> ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> if (ptl) {
> /* Check if we can move the pmd without splitting it. */
> if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> !pmd_none(dst_pmdval)) {
> struct folio *folio = pmd_folio(*src_pmd);
>
> if (!folio || (!is_huge_zero_folio(folio) &&
> !PageAnonExclusive(&folio->page))) {
> spin_unlock(ptl);
> err = -EBUSY;
> break;
> }
>
> spin_unlock(ptl);
> split_huge_pmd(src_vma, src_pmd, src_addr);
> /* The folio will be split by move_pages_pte() */
> continue;
> }
>
> err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
> dst_pmdval, dst_vma, src_vma,
> dst_addr, src_addr);
> step_size = HPAGE_PMD_SIZE;
> } else {
>
> It'll get ptl for a migration pmd, then pmd_folio is risky without checking
> present bit. That's what my previous smaller patch wanted to fix.
>
> But besides that, IIUC it's all fine at least for a pmd migration entry,
> because when with the smaller patch applied, either we'll try to split the
> pmd migration entry, or we'll do move_pages_huge_pmd(), which internally
> handles the pmd migration entry too by waiting on it:
>
> if (!pmd_trans_huge(src_pmdval)) {
> spin_unlock(src_ptl);
> if (is_pmd_migration_entry(src_pmdval)) {
> pmd_migration_entry_wait(mm, &src_pmdval);
> return -EAGAIN;
> }
> return -ENOENT;
> }
>
> Then logically after the migration entry got recovered, we'll either see a
> real THP or pmd none next time.
Yes, for migration entries adding the "if (pmd_present(*src_pmd))"
before getting the folio is enough. The problematic case is
(!pmd_none(*src_pmd) && !pmd_present(*src_pmd)) and not a migration
entry.
>
> Some explanation on the problematic non-present THP entry would be helpful.
>
> Thanks,
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-04 14:55 ` Suren Baghdasaryan
@ 2025-08-05 14:39 ` Peter Xu
2025-08-05 14:57 ` Suren Baghdasaryan
0 siblings, 1 reply; 21+ messages in thread
From: Peter Xu @ 2025-08-05 14:39 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Mon, Aug 04, 2025 at 07:55:42AM -0700, Suren Baghdasaryan wrote:
> On Fri, Aug 1, 2025 at 5:32 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Fri, Aug 01, 2025 at 07:30:02PM +0000, Suren Baghdasaryan wrote:
> > > On Fri, Aug 1, 2025 at 6:21 PM Peter Xu <peterx@redhat.com> wrote:
> > > >
> > > > On Fri, Aug 01, 2025 at 05:45:10PM +0000, Suren Baghdasaryan wrote:
> > > > > On Fri, Aug 1, 2025 at 5:13 PM Peter Xu <peterx@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, Aug 01, 2025 at 09:41:31AM -0700, Suren Baghdasaryan wrote:
> > > > > > > On Fri, Aug 1, 2025 at 9:23 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> > > > > > > > > On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > > > > > > > > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi!
> > > > > > > > > > >
> > > > > > > > > > > Did you mean in you patch description:
> > > > > > > > > > >
> > > > > > > > > > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > > > > > > > > > >
> > > > > > > > > > > Talking about THP holes is very very confusing.
> > > > > > > > > > >
> > > > > > > > > > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > > > > > > > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > > > > > > > > > >
> > > > > > > > > > > You mean a "non-present PMD that is not a migration entry".
> > > > > > > > > > >
> > > > > > > > > > > > hole and tries to access a non-existent folio, resulting in
> > > > > > > > > > > > a crash. Add a check to skip non-present THPs.
> > > > > > > > > > >
> > > > > > > > > > > That makes sense. The code we have after this patch is rather complicated
> > > > > > > > > > > and hard to read.
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > > > > > > > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > > > > > > > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > > > > > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > > > > > ---
> > > > > > > > > > > > Changes since v1 [1]
> > > > > > > > > > > > - Fixed step size calculation, per Lokesh Gidra
> > > > > > > > > > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > > > > > > > > > >
> > > > > > > > > > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > > > > > > > > > >
> > > > > > > > > > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > > > > > > > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > > > > > > > > index cbed91b09640..b5af31c22731 100644
> > > > > > > > > > > > --- a/mm/userfaultfd.c
> > > > > > > > > > > > +++ b/mm/userfaultfd.c
> > > > > > > > > > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > > > > > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > > > > > > > > if (ptl) {
> > > > > > > > > > > > - /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > > > - !pmd_none(dst_pmdval)) {
> > > > > > > > > > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > > > > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > > > > > > > > >
> > > > > > > > > > [1]
> > > > > > > > > >
> > > > > > > > > > > > + /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > > > + !pmd_none(dst_pmdval)) {
> > > > > > > > > > > > + if (pmd_present(*src_pmd)) {
> > > > > > > >
> > > > > > > > [2]
> > > > > > > >
> > > > > > > > > > > > + struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > >
> > > > > > > > [3]
> > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > > > > > > > > + !PageAnonExclusive(&folio->page))) {
> > > > > > > > > > > > + spin_unlock(ptl);
> > > > > > > > > > > > + err = -EBUSY;
> > > > > > > > > > > > + break;
> > > > > > > > > > > > + }
> > > > > > > > > > > > + }
> > > > > > > > > > >
> > > > > > > > > > > ... in particular that. Is there some way to make this code simpler / easier
> > > > > > > > > > > to read? Like moving that whole last folio-check thingy into a helper?
> > > > > > > > > >
> > > > > > > > > > One question might be relevant is, whether the check above [1] can be
> > > > > > > > > > dropped.
> > > > > > > > > >
> > > > > > > > > > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > > > > > > > > > before returning the ptl. I didn't follow closely on the recent changes on
> > > > > > > > > > mm side on possible new pmd swap entries, if migration is the only possible
> > > > > > > > > > one then it looks like [1] can be avoided.
> > > > > > > > >
> > > > > > > > > Hi Peter,
> > > > > > > > > is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> > > > > > > > > && !pmd_present()) PMD to pass and that's when this crash is hit.
> > > > > > > >
> > > > > > > > First for all, thanks for looking into the issue with Lokesh; I am still
> > > > > > > > catching up with emails after taking weeks off.
> > > > > > > >
> > > > > > > > I didn't yet read into the syzbot report, but I thought the bug was about
> > > > > > > > referencing the folio on top of a swap entry after reading your current
> > > > > > > > patch, which has:
> > > > > > > >
> > > > > > > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > !pmd_none(dst_pmdval)) {
> > > > > > > > struct folio *folio = pmd_folio(*src_pmd); <----
> > > > > > > >
> > > > > > > > Here looks like *src_pmd can be a migration entry. Is my understanding
> > > > > > > > correct?
> > > > > > >
> > > > > > > Correct.
> > > > > > >
> > > > > > > >
> > > > > > > > > If we drop the check at [1] then the path that takes us to
> > > > > > > >
> > > > > > > > If my above understanding is correct, IMHO it should be [2] above that
> > > > > > > > makes sure the reference won't happen on a swap entry, not necessarily [1]?
> > > > > > >
> > > > > > > Yes, in case of migration entry this is what protects us.
> > > > > > >
> > > > > > > >
> > > > > > > > > split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> > > > > > > > > indication that split did not happen. Afterwards we will retry
> > > > > > > >
> > > > > > > > So we're talking about the case where it's a swap pmd entry, right?
> > > > > > >
> > > > > > > Hmm, my understanding is that it's being treated as a swap entry but
> > > > > > > in reality is not. I thought THPs are always split before they get
> > > > > > > swapped, no?
> > > > > >
> > > > > > Yes they should be split, afaiu.
> > > > > >
> > > > > > >
> > > > > > > > Could you elaborate why the split would fail?
> > > > > > >
> > > > > > > Just looking at the code, split_huge_pmd_locked() checks for
> > > > > > > (pmd_trans_huge(*pmd) || is_pmd_migration_entry(*pmd)).
> > > > > > > pmd_trans_huge() is false if !pmd_present() and it's not a migration
> > > > > > > entry, so __split_huge_pmd_locked() will be skipped.
> > > > > >
> > > > > > Here might be the major part of where confusion came from: I thought it
> > > > > > must be a migration pmd entry to hit the issue, so it's not?
> > > > > >
> > > > > > I checked the code just now:
> > > > > >
> > > > > > __handle_mm_fault:
> > > > > > if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
> > > > > > VM_BUG_ON(thp_migration_supported() &&
> > > > > > !is_pmd_migration_entry(vmf.orig_pmd));
> > > > > >
> > > > > > So IIUC pmd migration entry is still the only possible way to have a swap
> > > > > > entry. It doesn't look like we have "real" swap entries for PMD (which can
> > > > > > further points to some swapfiles)?
> > > > >
> > > > > Correct. AFAIU here we stumble on a pmd entry which was allocated but
> > > > > never populated.
> > > >
> > > > Do you mean a pmd_none()?
> > >
> > > Yes.
> > >
> > > >
> > > > If so, that goes back to my original question, on why
> > > > __pmd_trans_huge_lock() returns non-NULL if it's a pmd_none()? IMHO it
> > > > really should have returned NULL for pmd_none().
> > >
> > > That was exactly the answer I gave Lokesh when he theorized about the
> > > cause of this crash but after reproducing it I saw that
> > > pmd_trans_huge_lock() happily returns the PTL as long as PMD is not
> > > pmd_none(). And that's because it passes as is_swap_pmd(). But even if
> > > we change that we still need to implement the code to skip the entire
> > > PMD.
> >
> > The thing is I thought if pmd_trans_huge_lock() can return non-NULL, it
> > must be either a migration entry or a present THP. So are you describing a
> > THP but with present bit cleared? Do you know what is that entry, and why
> > it has present bit cleared?
>
> In this case it's because earlier we allocated that PMD here:
> https://elixir.bootlin.com/linux/v6.16/source/mm/userfaultfd.c#L1797
AFAIU, this line is not about allocation of any pmd entry, but the pmd
pgtable page that _holds_ the PMDs:
static inline pmd_t *pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long address)
{
return (unlikely(pud_none(*pud)) && __pmd_alloc(mm, pud, address))?
NULL: pmd_offset(pud, address);
}
It makes sure the PUD entry, not the PMD entry, be populated.
> but wouldn't that be the same if the PMD was mapped and then got
> unmapped later? My understanding is that we allocate the PMD at the
> line I pointed to make UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES case the same
> as this unmapped PMD case. If my assumption is incorrect then we could
> skip the hole earlier instead of allocating the PMD for it.
>
> >
> > I think my attention got attracted to pmd migration entry too much, so I
> > didn't really notice such possibility, as I believe migration pmd is broken
> > already in this path.
> >
> > The original code:
> >
> > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > if (ptl) {
> > /* Check if we can move the pmd without splitting it. */
> > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > !pmd_none(dst_pmdval)) {
> > struct folio *folio = pmd_folio(*src_pmd);
> >
> > if (!folio || (!is_huge_zero_folio(folio) &&
> > !PageAnonExclusive(&folio->page))) {
> > spin_unlock(ptl);
> > err = -EBUSY;
> > break;
> > }
> >
> > spin_unlock(ptl);
> > split_huge_pmd(src_vma, src_pmd, src_addr);
> > /* The folio will be split by move_pages_pte() */
> > continue;
> > }
> >
> > err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
> > dst_pmdval, dst_vma, src_vma,
> > dst_addr, src_addr);
> > step_size = HPAGE_PMD_SIZE;
> > } else {
> >
> > It'll get ptl for a migration pmd, then pmd_folio is risky without checking
> > present bit. That's what my previous smaller patch wanted to fix.
> >
> > But besides that, IIUC it's all fine at least for a pmd migration entry,
> > because when with the smaller patch applied, either we'll try to split the
> > pmd migration entry, or we'll do move_pages_huge_pmd(), which internally
> > handles the pmd migration entry too by waiting on it:
> >
> > if (!pmd_trans_huge(src_pmdval)) {
> > spin_unlock(src_ptl);
> > if (is_pmd_migration_entry(src_pmdval)) {
> > pmd_migration_entry_wait(mm, &src_pmdval);
> > return -EAGAIN;
> > }
> > return -ENOENT;
> > }
> >
> > Then logically after the migration entry got recovered, we'll either see a
> > real THP or pmd none next time.
>
> Yes, for migration entries adding the "if (pmd_present(*src_pmd))"
> before getting the folio is enough. The problematic case is
> (!pmd_none(*src_pmd) && !pmd_present(*src_pmd)) and not a migration
> entry.
I thought we could have any of below here on the pmd entry:
(0) pmd_none, which should constantly have pmd_trans_huge_lock -> NULL
(1) pmd pgtable entry, which must have PRESENT && !TRANS, so
pmd_trans_huge_lock -> NULL,
(2) pmd migration, pmd_trans_huge_lock -> valid
(3) pmd thp, pmd_trans_huge_lock -> valid
I thought (2) was broken, which we seem to agree upon.. however if so the
smaller patch should fix it, per explanation in my previous reply. OTOH I
can't think of (4).
Said that, I just noticed (3) can be broken as well - could it be a
prot_none entry? The very confusing part of this patch is it seems to
think it's pmd_none() here as holes:
if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
...
} else {
spin_unlock(ptl);
if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
err = -ENOENT;
break;
}
/* nothing to do to move a hole */
err = 0;
step_size = min(HPAGE_PMD_SIZE, src_start + len - src_addr);
}
But is it really? Again, I don't think pmd_none() could happen with
pmd_trans_huge_lock() returning the ptl.
Could you double check this? E.g. with this line if that makes sense to
you:
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 8bf8ff0be990f..d2d4f2a0ae69f 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -1903,6 +1903,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
dst_addr, src_addr);
step_size = HPAGE_PMD_SIZE;
} else {
+ BUG_ON(!pmd_none(*src_pmd));
spin_unlock(ptl);
if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
err = -ENOENT;
I would expect it constantly BUG() here, if that explains my thoughts.
Now I doubt it's a prot_none THP.. aka, a THP that got numa hint to be
moved. If so, we may need to process it / move it / .. but we likely
should never skip it. We can double check the buggy pmd entry you hit
(besides migration entry) first.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-05 14:39 ` Peter Xu
@ 2025-08-05 14:57 ` Suren Baghdasaryan
2025-08-05 20:39 ` Suren Baghdasaryan
0 siblings, 1 reply; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-08-05 14:57 UTC (permalink / raw)
To: Peter Xu
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Tue, Aug 5, 2025 at 7:39 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Aug 04, 2025 at 07:55:42AM -0700, Suren Baghdasaryan wrote:
> > On Fri, Aug 1, 2025 at 5:32 PM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Fri, Aug 01, 2025 at 07:30:02PM +0000, Suren Baghdasaryan wrote:
> > > > On Fri, Aug 1, 2025 at 6:21 PM Peter Xu <peterx@redhat.com> wrote:
> > > > >
> > > > > On Fri, Aug 01, 2025 at 05:45:10PM +0000, Suren Baghdasaryan wrote:
> > > > > > On Fri, Aug 1, 2025 at 5:13 PM Peter Xu <peterx@redhat.com> wrote:
> > > > > > >
> > > > > > > On Fri, Aug 01, 2025 at 09:41:31AM -0700, Suren Baghdasaryan wrote:
> > > > > > > > On Fri, Aug 1, 2025 at 9:23 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> > > > > > > > > > On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > > > > > > > > > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi!
> > > > > > > > > > > >
> > > > > > > > > > > > Did you mean in you patch description:
> > > > > > > > > > > >
> > > > > > > > > > > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > > > > > > > > > > >
> > > > > > > > > > > > Talking about THP holes is very very confusing.
> > > > > > > > > > > >
> > > > > > > > > > > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > > > > > > > > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > > > > > > > > > > >
> > > > > > > > > > > > You mean a "non-present PMD that is not a migration entry".
> > > > > > > > > > > >
> > > > > > > > > > > > > hole and tries to access a non-existent folio, resulting in
> > > > > > > > > > > > > a crash. Add a check to skip non-present THPs.
> > > > > > > > > > > >
> > > > > > > > > > > > That makes sense. The code we have after this patch is rather complicated
> > > > > > > > > > > > and hard to read.
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > > > > > > > > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > > > > > > > > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > > > > > > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > Changes since v1 [1]
> > > > > > > > > > > > > - Fixed step size calculation, per Lokesh Gidra
> > > > > > > > > > > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > > > > > > > > > > >
> > > > > > > > > > > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > > > > > > > > > > >
> > > > > > > > > > > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > > > > > > > > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > > > > > > > > > index cbed91b09640..b5af31c22731 100644
> > > > > > > > > > > > > --- a/mm/userfaultfd.c
> > > > > > > > > > > > > +++ b/mm/userfaultfd.c
> > > > > > > > > > > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > > > > > > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > > > > > > > > > if (ptl) {
> > > > > > > > > > > > > - /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > > > > - !pmd_none(dst_pmdval)) {
> > > > > > > > > > > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > > > > > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > > > > > > > > > >
> > > > > > > > > > > [1]
> > > > > > > > > > >
> > > > > > > > > > > > > + /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > > > > + !pmd_none(dst_pmdval)) {
> > > > > > > > > > > > > + if (pmd_present(*src_pmd)) {
> > > > > > > > >
> > > > > > > > > [2]
> > > > > > > > >
> > > > > > > > > > > > > + struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > > >
> > > > > > > > > [3]
> > > > > > > > >
> > > > > > > > > > > > > +
> > > > > > > > > > > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > > > > > > > > > + !PageAnonExclusive(&folio->page))) {
> > > > > > > > > > > > > + spin_unlock(ptl);
> > > > > > > > > > > > > + err = -EBUSY;
> > > > > > > > > > > > > + break;
> > > > > > > > > > > > > + }
> > > > > > > > > > > > > + }
> > > > > > > > > > > >
> > > > > > > > > > > > ... in particular that. Is there some way to make this code simpler / easier
> > > > > > > > > > > > to read? Like moving that whole last folio-check thingy into a helper?
> > > > > > > > > > >
> > > > > > > > > > > One question might be relevant is, whether the check above [1] can be
> > > > > > > > > > > dropped.
> > > > > > > > > > >
> > > > > > > > > > > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > > > > > > > > > > before returning the ptl. I didn't follow closely on the recent changes on
> > > > > > > > > > > mm side on possible new pmd swap entries, if migration is the only possible
> > > > > > > > > > > one then it looks like [1] can be avoided.
> > > > > > > > > >
> > > > > > > > > > Hi Peter,
> > > > > > > > > > is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> > > > > > > > > > && !pmd_present()) PMD to pass and that's when this crash is hit.
> > > > > > > > >
> > > > > > > > > First for all, thanks for looking into the issue with Lokesh; I am still
> > > > > > > > > catching up with emails after taking weeks off.
> > > > > > > > >
> > > > > > > > > I didn't yet read into the syzbot report, but I thought the bug was about
> > > > > > > > > referencing the folio on top of a swap entry after reading your current
> > > > > > > > > patch, which has:
> > > > > > > > >
> > > > > > > > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > !pmd_none(dst_pmdval)) {
> > > > > > > > > struct folio *folio = pmd_folio(*src_pmd); <----
> > > > > > > > >
> > > > > > > > > Here looks like *src_pmd can be a migration entry. Is my understanding
> > > > > > > > > correct?
> > > > > > > >
> > > > > > > > Correct.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > If we drop the check at [1] then the path that takes us to
> > > > > > > > >
> > > > > > > > > If my above understanding is correct, IMHO it should be [2] above that
> > > > > > > > > makes sure the reference won't happen on a swap entry, not necessarily [1]?
> > > > > > > >
> > > > > > > > Yes, in case of migration entry this is what protects us.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> > > > > > > > > > indication that split did not happen. Afterwards we will retry
> > > > > > > > >
> > > > > > > > > So we're talking about the case where it's a swap pmd entry, right?
> > > > > > > >
> > > > > > > > Hmm, my understanding is that it's being treated as a swap entry but
> > > > > > > > in reality is not. I thought THPs are always split before they get
> > > > > > > > swapped, no?
> > > > > > >
> > > > > > > Yes they should be split, afaiu.
> > > > > > >
> > > > > > > >
> > > > > > > > > Could you elaborate why the split would fail?
> > > > > > > >
> > > > > > > > Just looking at the code, split_huge_pmd_locked() checks for
> > > > > > > > (pmd_trans_huge(*pmd) || is_pmd_migration_entry(*pmd)).
> > > > > > > > pmd_trans_huge() is false if !pmd_present() and it's not a migration
> > > > > > > > entry, so __split_huge_pmd_locked() will be skipped.
> > > > > > >
> > > > > > > Here might be the major part of where confusion came from: I thought it
> > > > > > > must be a migration pmd entry to hit the issue, so it's not?
> > > > > > >
> > > > > > > I checked the code just now:
> > > > > > >
> > > > > > > __handle_mm_fault:
> > > > > > > if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
> > > > > > > VM_BUG_ON(thp_migration_supported() &&
> > > > > > > !is_pmd_migration_entry(vmf.orig_pmd));
> > > > > > >
> > > > > > > So IIUC pmd migration entry is still the only possible way to have a swap
> > > > > > > entry. It doesn't look like we have "real" swap entries for PMD (which can
> > > > > > > further points to some swapfiles)?
> > > > > >
> > > > > > Correct. AFAIU here we stumble on a pmd entry which was allocated but
> > > > > > never populated.
> > > > >
> > > > > Do you mean a pmd_none()?
> > > >
> > > > Yes.
> > > >
> > > > >
> > > > > If so, that goes back to my original question, on why
> > > > > __pmd_trans_huge_lock() returns non-NULL if it's a pmd_none()? IMHO it
> > > > > really should have returned NULL for pmd_none().
> > > >
> > > > That was exactly the answer I gave Lokesh when he theorized about the
> > > > cause of this crash but after reproducing it I saw that
> > > > pmd_trans_huge_lock() happily returns the PTL as long as PMD is not
> > > > pmd_none(). And that's because it passes as is_swap_pmd(). But even if
> > > > we change that we still need to implement the code to skip the entire
> > > > PMD.
> > >
> > > The thing is I thought if pmd_trans_huge_lock() can return non-NULL, it
> > > must be either a migration entry or a present THP. So are you describing a
> > > THP but with present bit cleared? Do you know what is that entry, and why
> > > it has present bit cleared?
> >
> > In this case it's because earlier we allocated that PMD here:
> > https://elixir.bootlin.com/linux/v6.16/source/mm/userfaultfd.c#L1797
>
> AFAIU, this line is not about allocation of any pmd entry, but the pmd
> pgtable page that _holds_ the PMDs:
>
> static inline pmd_t *pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long address)
> {
> return (unlikely(pud_none(*pud)) && __pmd_alloc(mm, pud, address))?
> NULL: pmd_offset(pud, address);
> }
>
> It makes sure the PUD entry, not the PMD entry, be populated.
Hmm. Then I was reading this code completely wrong and need to rethink
what is happening here.
>
> > but wouldn't that be the same if the PMD was mapped and then got
> > unmapped later? My understanding is that we allocate the PMD at the
> > line I pointed to make UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES case the same
> > as this unmapped PMD case. If my assumption is incorrect then we could
> > skip the hole earlier instead of allocating the PMD for it.
> >
> > >
> > > I think my attention got attracted to pmd migration entry too much, so I
> > > didn't really notice such possibility, as I believe migration pmd is broken
> > > already in this path.
> > >
> > > The original code:
> > >
> > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > if (ptl) {
> > > /* Check if we can move the pmd without splitting it. */
> > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > !pmd_none(dst_pmdval)) {
> > > struct folio *folio = pmd_folio(*src_pmd);
> > >
> > > if (!folio || (!is_huge_zero_folio(folio) &&
> > > !PageAnonExclusive(&folio->page))) {
> > > spin_unlock(ptl);
> > > err = -EBUSY;
> > > break;
> > > }
> > >
> > > spin_unlock(ptl);
> > > split_huge_pmd(src_vma, src_pmd, src_addr);
> > > /* The folio will be split by move_pages_pte() */
> > > continue;
> > > }
> > >
> > > err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
> > > dst_pmdval, dst_vma, src_vma,
> > > dst_addr, src_addr);
> > > step_size = HPAGE_PMD_SIZE;
> > > } else {
> > >
> > > It'll get ptl for a migration pmd, then pmd_folio is risky without checking
> > > present bit. That's what my previous smaller patch wanted to fix.
> > >
> > > But besides that, IIUC it's all fine at least for a pmd migration entry,
> > > because when with the smaller patch applied, either we'll try to split the
> > > pmd migration entry, or we'll do move_pages_huge_pmd(), which internally
> > > handles the pmd migration entry too by waiting on it:
> > >
> > > if (!pmd_trans_huge(src_pmdval)) {
> > > spin_unlock(src_ptl);
> > > if (is_pmd_migration_entry(src_pmdval)) {
> > > pmd_migration_entry_wait(mm, &src_pmdval);
> > > return -EAGAIN;
> > > }
> > > return -ENOENT;
> > > }
> > >
> > > Then logically after the migration entry got recovered, we'll either see a
> > > real THP or pmd none next time.
> >
> > Yes, for migration entries adding the "if (pmd_present(*src_pmd))"
> > before getting the folio is enough. The problematic case is
> > (!pmd_none(*src_pmd) && !pmd_present(*src_pmd)) and not a migration
> > entry.
>
> I thought we could have any of below here on the pmd entry:
>
> (0) pmd_none, which should constantly have pmd_trans_huge_lock -> NULL
>
> (1) pmd pgtable entry, which must have PRESENT && !TRANS, so
> pmd_trans_huge_lock -> NULL,
>
> (2) pmd migration, pmd_trans_huge_lock -> valid
>
> (3) pmd thp, pmd_trans_huge_lock -> valid
>
> I thought (2) was broken, which we seem to agree upon.. however if so the
> smaller patch should fix it, per explanation in my previous reply. OTOH I
> can't think of (4).
The case I was hitting is (!pmd_none && !pmd_present &&
!is_pmd_migration_entry). My original thinking was that this entry was
newly allocated at the line I pointed earlier but now I'm not so sure
anymore.
>
> Said that, I just noticed (3) can be broken as well - could it be a
> prot_none entry? The very confusing part of this patch is it seems to
> think it's pmd_none() here as holes:
>
> if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> ...
> } else {
> spin_unlock(ptl);
> if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> err = -ENOENT;
> break;
> }
> /* nothing to do to move a hole */
> err = 0;
> step_size = min(HPAGE_PMD_SIZE, src_start + len - src_addr);
> }
>
> But is it really? Again, I don't think pmd_none() could happen with
> pmd_trans_huge_lock() returning the ptl.
That is true, in the pmd_none() case pmd_trans_huge_lock() returns NULL.
>
> Could you double check this? E.g. with this line if that makes sense to
> you:
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 8bf8ff0be990f..d2d4f2a0ae69f 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1903,6 +1903,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> dst_addr, src_addr);
> step_size = HPAGE_PMD_SIZE;
> } else {
> + BUG_ON(!pmd_none(*src_pmd));
> spin_unlock(ptl);
> if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> err = -ENOENT;
>
> I would expect it constantly BUG() here, if that explains my thoughts.
I'll add this and check.
>
> Now I doubt it's a prot_none THP.. aka, a THP that got numa hint to be
> moved. If so, we may need to process it / move it / .. but we likely
> should never skip it. We can double check the buggy pmd entry you hit
> (besides migration entry) first.
Let me log the flags of the entry when this issue happens. That should
provide more insights.
Thanks,
Suren.
>
> Thanks,
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-05 14:57 ` Suren Baghdasaryan
@ 2025-08-05 20:39 ` Suren Baghdasaryan
2025-08-05 23:41 ` Suren Baghdasaryan
0 siblings, 1 reply; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-08-05 20:39 UTC (permalink / raw)
To: Peter Xu
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Tue, Aug 5, 2025 at 7:57 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Tue, Aug 5, 2025 at 7:39 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Mon, Aug 04, 2025 at 07:55:42AM -0700, Suren Baghdasaryan wrote:
> > > On Fri, Aug 1, 2025 at 5:32 PM Peter Xu <peterx@redhat.com> wrote:
> > > >
> > > > On Fri, Aug 01, 2025 at 07:30:02PM +0000, Suren Baghdasaryan wrote:
> > > > > On Fri, Aug 1, 2025 at 6:21 PM Peter Xu <peterx@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, Aug 01, 2025 at 05:45:10PM +0000, Suren Baghdasaryan wrote:
> > > > > > > On Fri, Aug 1, 2025 at 5:13 PM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, Aug 01, 2025 at 09:41:31AM -0700, Suren Baghdasaryan wrote:
> > > > > > > > > On Fri, Aug 1, 2025 at 9:23 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> > > > > > > > > > > On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > > > > > > > > > > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi!
> > > > > > > > > > > > >
> > > > > > > > > > > > > Did you mean in you patch description:
> > > > > > > > > > > > >
> > > > > > > > > > > > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > > > > > > > > > > > >
> > > > > > > > > > > > > Talking about THP holes is very very confusing.
> > > > > > > > > > > > >
> > > > > > > > > > > > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > > > > > > > > > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > > > > > > > > > > > >
> > > > > > > > > > > > > You mean a "non-present PMD that is not a migration entry".
> > > > > > > > > > > > >
> > > > > > > > > > > > > > hole and tries to access a non-existent folio, resulting in
> > > > > > > > > > > > > > a crash. Add a check to skip non-present THPs.
> > > > > > > > > > > > >
> > > > > > > > > > > > > That makes sense. The code we have after this patch is rather complicated
> > > > > > > > > > > > > and hard to read.
> > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > > > > > > > > > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > > > > > > > > > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > > > > > > > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > > > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > Changes since v1 [1]
> > > > > > > > > > > > > > - Fixed step size calculation, per Lokesh Gidra
> > > > > > > > > > > > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > > > > > > > > > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > > > > > > > > > > index cbed91b09640..b5af31c22731 100644
> > > > > > > > > > > > > > --- a/mm/userfaultfd.c
> > > > > > > > > > > > > > +++ b/mm/userfaultfd.c
> > > > > > > > > > > > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > > > > > > > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > > > > > > > > > > if (ptl) {
> > > > > > > > > > > > > > - /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > > > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > > > > > - !pmd_none(dst_pmdval)) {
> > > > > > > > > > > > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > > > > > > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > > > > > > > > > > >
> > > > > > > > > > > > [1]
> > > > > > > > > > > >
> > > > > > > > > > > > > > + /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > > > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > > > > > + !pmd_none(dst_pmdval)) {
> > > > > > > > > > > > > > + if (pmd_present(*src_pmd)) {
> > > > > > > > > >
> > > > > > > > > > [2]
> > > > > > > > > >
> > > > > > > > > > > > > > + struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > > > >
> > > > > > > > > > [3]
> > > > > > > > > >
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > > > > > > > > > > + !PageAnonExclusive(&folio->page))) {
> > > > > > > > > > > > > > + spin_unlock(ptl);
> > > > > > > > > > > > > > + err = -EBUSY;
> > > > > > > > > > > > > > + break;
> > > > > > > > > > > > > > + }
> > > > > > > > > > > > > > + }
> > > > > > > > > > > > >
> > > > > > > > > > > > > ... in particular that. Is there some way to make this code simpler / easier
> > > > > > > > > > > > > to read? Like moving that whole last folio-check thingy into a helper?
> > > > > > > > > > > >
> > > > > > > > > > > > One question might be relevant is, whether the check above [1] can be
> > > > > > > > > > > > dropped.
> > > > > > > > > > > >
> > > > > > > > > > > > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > > > > > > > > > > > before returning the ptl. I didn't follow closely on the recent changes on
> > > > > > > > > > > > mm side on possible new pmd swap entries, if migration is the only possible
> > > > > > > > > > > > one then it looks like [1] can be avoided.
> > > > > > > > > > >
> > > > > > > > > > > Hi Peter,
> > > > > > > > > > > is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> > > > > > > > > > > && !pmd_present()) PMD to pass and that's when this crash is hit.
> > > > > > > > > >
> > > > > > > > > > First for all, thanks for looking into the issue with Lokesh; I am still
> > > > > > > > > > catching up with emails after taking weeks off.
> > > > > > > > > >
> > > > > > > > > > I didn't yet read into the syzbot report, but I thought the bug was about
> > > > > > > > > > referencing the folio on top of a swap entry after reading your current
> > > > > > > > > > patch, which has:
> > > > > > > > > >
> > > > > > > > > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > !pmd_none(dst_pmdval)) {
> > > > > > > > > > struct folio *folio = pmd_folio(*src_pmd); <----
> > > > > > > > > >
> > > > > > > > > > Here looks like *src_pmd can be a migration entry. Is my understanding
> > > > > > > > > > correct?
> > > > > > > > >
> > > > > > > > > Correct.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > If we drop the check at [1] then the path that takes us to
> > > > > > > > > >
> > > > > > > > > > If my above understanding is correct, IMHO it should be [2] above that
> > > > > > > > > > makes sure the reference won't happen on a swap entry, not necessarily [1]?
> > > > > > > > >
> > > > > > > > > Yes, in case of migration entry this is what protects us.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> > > > > > > > > > > indication that split did not happen. Afterwards we will retry
> > > > > > > > > >
> > > > > > > > > > So we're talking about the case where it's a swap pmd entry, right?
> > > > > > > > >
> > > > > > > > > Hmm, my understanding is that it's being treated as a swap entry but
> > > > > > > > > in reality is not. I thought THPs are always split before they get
> > > > > > > > > swapped, no?
> > > > > > > >
> > > > > > > > Yes they should be split, afaiu.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > Could you elaborate why the split would fail?
> > > > > > > > >
> > > > > > > > > Just looking at the code, split_huge_pmd_locked() checks for
> > > > > > > > > (pmd_trans_huge(*pmd) || is_pmd_migration_entry(*pmd)).
> > > > > > > > > pmd_trans_huge() is false if !pmd_present() and it's not a migration
> > > > > > > > > entry, so __split_huge_pmd_locked() will be skipped.
> > > > > > > >
> > > > > > > > Here might be the major part of where confusion came from: I thought it
> > > > > > > > must be a migration pmd entry to hit the issue, so it's not?
> > > > > > > >
> > > > > > > > I checked the code just now:
> > > > > > > >
> > > > > > > > __handle_mm_fault:
> > > > > > > > if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
> > > > > > > > VM_BUG_ON(thp_migration_supported() &&
> > > > > > > > !is_pmd_migration_entry(vmf.orig_pmd));
> > > > > > > >
> > > > > > > > So IIUC pmd migration entry is still the only possible way to have a swap
> > > > > > > > entry. It doesn't look like we have "real" swap entries for PMD (which can
> > > > > > > > further points to some swapfiles)?
> > > > > > >
> > > > > > > Correct. AFAIU here we stumble on a pmd entry which was allocated but
> > > > > > > never populated.
> > > > > >
> > > > > > Do you mean a pmd_none()?
> > > > >
> > > > > Yes.
> > > > >
> > > > > >
> > > > > > If so, that goes back to my original question, on why
> > > > > > __pmd_trans_huge_lock() returns non-NULL if it's a pmd_none()? IMHO it
> > > > > > really should have returned NULL for pmd_none().
> > > > >
> > > > > That was exactly the answer I gave Lokesh when he theorized about the
> > > > > cause of this crash but after reproducing it I saw that
> > > > > pmd_trans_huge_lock() happily returns the PTL as long as PMD is not
> > > > > pmd_none(). And that's because it passes as is_swap_pmd(). But even if
> > > > > we change that we still need to implement the code to skip the entire
> > > > > PMD.
> > > >
> > > > The thing is I thought if pmd_trans_huge_lock() can return non-NULL, it
> > > > must be either a migration entry or a present THP. So are you describing a
> > > > THP but with present bit cleared? Do you know what is that entry, and why
> > > > it has present bit cleared?
> > >
> > > In this case it's because earlier we allocated that PMD here:
> > > https://elixir.bootlin.com/linux/v6.16/source/mm/userfaultfd.c#L1797
> >
> > AFAIU, this line is not about allocation of any pmd entry, but the pmd
> > pgtable page that _holds_ the PMDs:
> >
> > static inline pmd_t *pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long address)
> > {
> > return (unlikely(pud_none(*pud)) && __pmd_alloc(mm, pud, address))?
> > NULL: pmd_offset(pud, address);
> > }
> >
> > It makes sure the PUD entry, not the PMD entry, be populated.
>
> Hmm. Then I was reading this code completely wrong and need to rethink
> what is happening here.
>
> >
> > > but wouldn't that be the same if the PMD was mapped and then got
> > > unmapped later? My understanding is that we allocate the PMD at the
> > > line I pointed to make UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES case the same
> > > as this unmapped PMD case. If my assumption is incorrect then we could
> > > skip the hole earlier instead of allocating the PMD for it.
> > >
> > > >
> > > > I think my attention got attracted to pmd migration entry too much, so I
> > > > didn't really notice such possibility, as I believe migration pmd is broken
> > > > already in this path.
> > > >
> > > > The original code:
> > > >
> > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > if (ptl) {
> > > > /* Check if we can move the pmd without splitting it. */
> > > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > !pmd_none(dst_pmdval)) {
> > > > struct folio *folio = pmd_folio(*src_pmd);
> > > >
> > > > if (!folio || (!is_huge_zero_folio(folio) &&
> > > > !PageAnonExclusive(&folio->page))) {
> > > > spin_unlock(ptl);
> > > > err = -EBUSY;
> > > > break;
> > > > }
> > > >
> > > > spin_unlock(ptl);
> > > > split_huge_pmd(src_vma, src_pmd, src_addr);
> > > > /* The folio will be split by move_pages_pte() */
> > > > continue;
> > > > }
> > > >
> > > > err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
> > > > dst_pmdval, dst_vma, src_vma,
> > > > dst_addr, src_addr);
> > > > step_size = HPAGE_PMD_SIZE;
> > > > } else {
> > > >
> > > > It'll get ptl for a migration pmd, then pmd_folio is risky without checking
> > > > present bit. That's what my previous smaller patch wanted to fix.
> > > >
> > > > But besides that, IIUC it's all fine at least for a pmd migration entry,
> > > > because when with the smaller patch applied, either we'll try to split the
> > > > pmd migration entry, or we'll do move_pages_huge_pmd(), which internally
> > > > handles the pmd migration entry too by waiting on it:
> > > >
> > > > if (!pmd_trans_huge(src_pmdval)) {
> > > > spin_unlock(src_ptl);
> > > > if (is_pmd_migration_entry(src_pmdval)) {
> > > > pmd_migration_entry_wait(mm, &src_pmdval);
> > > > return -EAGAIN;
> > > > }
> > > > return -ENOENT;
> > > > }
> > > >
> > > > Then logically after the migration entry got recovered, we'll either see a
> > > > real THP or pmd none next time.
> > >
> > > Yes, for migration entries adding the "if (pmd_present(*src_pmd))"
> > > before getting the folio is enough. The problematic case is
> > > (!pmd_none(*src_pmd) && !pmd_present(*src_pmd)) and not a migration
> > > entry.
> >
> > I thought we could have any of below here on the pmd entry:
> >
> > (0) pmd_none, which should constantly have pmd_trans_huge_lock -> NULL
> >
> > (1) pmd pgtable entry, which must have PRESENT && !TRANS, so
> > pmd_trans_huge_lock -> NULL,
> >
> > (2) pmd migration, pmd_trans_huge_lock -> valid
> >
> > (3) pmd thp, pmd_trans_huge_lock -> valid
> >
> > I thought (2) was broken, which we seem to agree upon.. however if so the
> > smaller patch should fix it, per explanation in my previous reply. OTOH I
> > can't think of (4).
>
> The case I was hitting is (!pmd_none && !pmd_present &&
> !is_pmd_migration_entry). My original thinking was that this entry was
> newly allocated at the line I pointed earlier but now I'm not so sure
> anymore.
Hmm, now I can't reproduce this case... I'm pretty sure I've seen that
case before but now I hit an occasional migration entry and that's
all. I must have done something wrong when testing it before.
>
> >
> > Said that, I just noticed (3) can be broken as well - could it be a
> > prot_none entry? The very confusing part of this patch is it seems to
> > think it's pmd_none() here as holes:
> >
> > if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > ...
> > } else {
> > spin_unlock(ptl);
> > if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> > err = -ENOENT;
> > break;
> > }
> > /* nothing to do to move a hole */
> > err = 0;
> > step_size = min(HPAGE_PMD_SIZE, src_start + len - src_addr);
> > }
> >
> > But is it really? Again, I don't think pmd_none() could happen with
> > pmd_trans_huge_lock() returning the ptl.
>
> That is true, in the pmd_none() case pmd_trans_huge_lock() returns NULL.
>
> >
> > Could you double check this? E.g. with this line if that makes sense to
> > you:
> >
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index 8bf8ff0be990f..d2d4f2a0ae69f 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -1903,6 +1903,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > dst_addr, src_addr);
> > step_size = HPAGE_PMD_SIZE;
> > } else {
> > + BUG_ON(!pmd_none(*src_pmd));
> > spin_unlock(ptl);
> > if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> > err = -ENOENT;
> >
> > I would expect it constantly BUG() here, if that explains my thoughts.
>
> I'll add this and check.
This check does trigger and I logged pmd_val=0x8b3714067 and I think
this is normal. _PAGE_BIT_PRESENT is set and _PAGE_BIT_PSE is not set,
so is_swap_pmd()==false and pmd_trans_huge()==false, therefore
pmd_trans_huge_lock() returns NULL.
>
> >
> > Now I doubt it's a prot_none THP.. aka, a THP that got numa hint to be
> > moved. If so, we may need to process it / move it / .. but we likely
> > should never skip it. We can double check the buggy pmd entry you hit
> > (besides migration entry) first.
>
> Let me log the flags of the entry when this issue happens. That should
> provide more insights.
> Thanks,
> Suren.
>
> >
> > Thanks,
> >
> > --
> > Peter Xu
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-05 20:39 ` Suren Baghdasaryan
@ 2025-08-05 23:41 ` Suren Baghdasaryan
2025-08-06 0:40 ` Peter Xu
0 siblings, 1 reply; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-08-05 23:41 UTC (permalink / raw)
To: Peter Xu
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Tue, Aug 5, 2025 at 1:39 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Tue, Aug 5, 2025 at 7:57 AM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > On Tue, Aug 5, 2025 at 7:39 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Mon, Aug 04, 2025 at 07:55:42AM -0700, Suren Baghdasaryan wrote:
> > > > On Fri, Aug 1, 2025 at 5:32 PM Peter Xu <peterx@redhat.com> wrote:
> > > > >
> > > > > On Fri, Aug 01, 2025 at 07:30:02PM +0000, Suren Baghdasaryan wrote:
> > > > > > On Fri, Aug 1, 2025 at 6:21 PM Peter Xu <peterx@redhat.com> wrote:
> > > > > > >
> > > > > > > On Fri, Aug 01, 2025 at 05:45:10PM +0000, Suren Baghdasaryan wrote:
> > > > > > > > On Fri, Aug 1, 2025 at 5:13 PM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Aug 01, 2025 at 09:41:31AM -0700, Suren Baghdasaryan wrote:
> > > > > > > > > > On Fri, Aug 1, 2025 at 9:23 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Aug 01, 2025 at 08:28:38AM -0700, Suren Baghdasaryan wrote:
> > > > > > > > > > > > On Fri, Aug 1, 2025 at 7:16 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Fri, Aug 01, 2025 at 09:21:30AM +0200, David Hildenbrand wrote:
> > > > > > > > > > > > > > On 31.07.25 17:44, Suren Baghdasaryan wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi!
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Did you mean in you patch description:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > "userfaultfd: fix a crash in UFFDIO_MOVE with some non-present PMDs"
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Talking about THP holes is very very confusing.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
> > > > > > > > > > > > > > > encounters a non-present THP, it fails to properly recognize an unmapped
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > You mean a "non-present PMD that is not a migration entry".
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > hole and tries to access a non-existent folio, resulting in
> > > > > > > > > > > > > > > a crash. Add a check to skip non-present THPs.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > That makes sense. The code we have after this patch is rather complicated
> > > > > > > > > > > > > > and hard to read.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > > > > > > > > > > > > > > Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com
> > > > > > > > > > > > > > > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > > > > > > > > > > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > > > > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > Changes since v1 [1]
> > > > > > > > > > > > > > > - Fixed step size calculation, per Lokesh Gidra
> > > > > > > > > > > > > > > - Added missing check for UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES, per Lokesh Gidra
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > [1] https://lore.kernel.org/all/20250730170733.3829267-1-surenb@google.com/
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > mm/userfaultfd.c | 45 +++++++++++++++++++++++++++++----------------
> > > > > > > > > > > > > > > 1 file changed, 29 insertions(+), 16 deletions(-)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > > > > > > > > > > > > index cbed91b09640..b5af31c22731 100644
> > > > > > > > > > > > > > > --- a/mm/userfaultfd.c
> > > > > > > > > > > > > > > +++ b/mm/userfaultfd.c
> > > > > > > > > > > > > > > @@ -1818,28 +1818,41 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > > > > > > > > > > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > > > > > > > > > > > if (ptl) {
> > > > > > > > > > > > > > > - /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > > > > > > - if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > > > > > > - !pmd_none(dst_pmdval)) {
> > > > > > > > > > > > > > > - struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > > > > > > > > > + if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > > > > > > > > > > > >
> > > > > > > > > > > > > [1]
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > + /* Check if we can move the pmd without splitting it. */
> > > > > > > > > > > > > > > + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > > > > > > + !pmd_none(dst_pmdval)) {
> > > > > > > > > > > > > > > + if (pmd_present(*src_pmd)) {
> > > > > > > > > > >
> > > > > > > > > > > [2]
> > > > > > > > > > >
> > > > > > > > > > > > > > > + struct folio *folio = pmd_folio(*src_pmd);
> > > > > > > > > > >
> > > > > > > > > > > [3]
> > > > > > > > > > >
> > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > + if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > > > > > > > > > > > + !PageAnonExclusive(&folio->page))) {
> > > > > > > > > > > > > > > + spin_unlock(ptl);
> > > > > > > > > > > > > > > + err = -EBUSY;
> > > > > > > > > > > > > > > + break;
> > > > > > > > > > > > > > > + }
> > > > > > > > > > > > > > > + }
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > ... in particular that. Is there some way to make this code simpler / easier
> > > > > > > > > > > > > > to read? Like moving that whole last folio-check thingy into a helper?
> > > > > > > > > > > > >
> > > > > > > > > > > > > One question might be relevant is, whether the check above [1] can be
> > > > > > > > > > > > > dropped.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The thing is __pmd_trans_huge_lock() does double check the pmd to be !none
> > > > > > > > > > > > > before returning the ptl. I didn't follow closely on the recent changes on
> > > > > > > > > > > > > mm side on possible new pmd swap entries, if migration is the only possible
> > > > > > > > > > > > > one then it looks like [1] can be avoided.
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Peter,
> > > > > > > > > > > > is_swap_pmd() check in __pmd_trans_huge_lock() allows for (!pmd_none()
> > > > > > > > > > > > && !pmd_present()) PMD to pass and that's when this crash is hit.
> > > > > > > > > > >
> > > > > > > > > > > First for all, thanks for looking into the issue with Lokesh; I am still
> > > > > > > > > > > catching up with emails after taking weeks off.
> > > > > > > > > > >
> > > > > > > > > > > I didn't yet read into the syzbot report, but I thought the bug was about
> > > > > > > > > > > referencing the folio on top of a swap entry after reading your current
> > > > > > > > > > > patch, which has:
> > > > > > > > > > >
> > > > > > > > > > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > > > > > > > !pmd_none(dst_pmdval)) {
> > > > > > > > > > > struct folio *folio = pmd_folio(*src_pmd); <----
> > > > > > > > > > >
> > > > > > > > > > > Here looks like *src_pmd can be a migration entry. Is my understanding
> > > > > > > > > > > correct?
> > > > > > > > > >
> > > > > > > > > > Correct.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > If we drop the check at [1] then the path that takes us to
> > > > > > > > > > >
> > > > > > > > > > > If my above understanding is correct, IMHO it should be [2] above that
> > > > > > > > > > > makes sure the reference won't happen on a swap entry, not necessarily [1]?
> > > > > > > > > >
> > > > > > > > > > Yes, in case of migration entry this is what protects us.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > split_huge_pmd() will bail out inside split_huge_pmd_locked() with no
> > > > > > > > > > > > indication that split did not happen. Afterwards we will retry
> > > > > > > > > > >
> > > > > > > > > > > So we're talking about the case where it's a swap pmd entry, right?
> > > > > > > > > >
> > > > > > > > > > Hmm, my understanding is that it's being treated as a swap entry but
> > > > > > > > > > in reality is not. I thought THPs are always split before they get
> > > > > > > > > > swapped, no?
> > > > > > > > >
> > > > > > > > > Yes they should be split, afaiu.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Could you elaborate why the split would fail?
> > > > > > > > > >
> > > > > > > > > > Just looking at the code, split_huge_pmd_locked() checks for
> > > > > > > > > > (pmd_trans_huge(*pmd) || is_pmd_migration_entry(*pmd)).
> > > > > > > > > > pmd_trans_huge() is false if !pmd_present() and it's not a migration
> > > > > > > > > > entry, so __split_huge_pmd_locked() will be skipped.
> > > > > > > > >
> > > > > > > > > Here might be the major part of where confusion came from: I thought it
> > > > > > > > > must be a migration pmd entry to hit the issue, so it's not?
> > > > > > > > >
> > > > > > > > > I checked the code just now:
> > > > > > > > >
> > > > > > > > > __handle_mm_fault:
> > > > > > > > > if (unlikely(is_swap_pmd(vmf.orig_pmd))) {
> > > > > > > > > VM_BUG_ON(thp_migration_supported() &&
> > > > > > > > > !is_pmd_migration_entry(vmf.orig_pmd));
> > > > > > > > >
> > > > > > > > > So IIUC pmd migration entry is still the only possible way to have a swap
> > > > > > > > > entry. It doesn't look like we have "real" swap entries for PMD (which can
> > > > > > > > > further points to some swapfiles)?
> > > > > > > >
> > > > > > > > Correct. AFAIU here we stumble on a pmd entry which was allocated but
> > > > > > > > never populated.
> > > > > > >
> > > > > > > Do you mean a pmd_none()?
> > > > > >
> > > > > > Yes.
> > > > > >
> > > > > > >
> > > > > > > If so, that goes back to my original question, on why
> > > > > > > __pmd_trans_huge_lock() returns non-NULL if it's a pmd_none()? IMHO it
> > > > > > > really should have returned NULL for pmd_none().
> > > > > >
> > > > > > That was exactly the answer I gave Lokesh when he theorized about the
> > > > > > cause of this crash but after reproducing it I saw that
> > > > > > pmd_trans_huge_lock() happily returns the PTL as long as PMD is not
> > > > > > pmd_none(). And that's because it passes as is_swap_pmd(). But even if
> > > > > > we change that we still need to implement the code to skip the entire
> > > > > > PMD.
> > > > >
> > > > > The thing is I thought if pmd_trans_huge_lock() can return non-NULL, it
> > > > > must be either a migration entry or a present THP. So are you describing a
> > > > > THP but with present bit cleared? Do you know what is that entry, and why
> > > > > it has present bit cleared?
> > > >
> > > > In this case it's because earlier we allocated that PMD here:
> > > > https://elixir.bootlin.com/linux/v6.16/source/mm/userfaultfd.c#L1797
> > >
> > > AFAIU, this line is not about allocation of any pmd entry, but the pmd
> > > pgtable page that _holds_ the PMDs:
> > >
> > > static inline pmd_t *pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long address)
> > > {
> > > return (unlikely(pud_none(*pud)) && __pmd_alloc(mm, pud, address))?
> > > NULL: pmd_offset(pud, address);
> > > }
> > >
> > > It makes sure the PUD entry, not the PMD entry, be populated.
> >
> > Hmm. Then I was reading this code completely wrong and need to rethink
> > what is happening here.
> >
> > >
> > > > but wouldn't that be the same if the PMD was mapped and then got
> > > > unmapped later? My understanding is that we allocate the PMD at the
> > > > line I pointed to make UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES case the same
> > > > as this unmapped PMD case. If my assumption is incorrect then we could
> > > > skip the hole earlier instead of allocating the PMD for it.
> > > >
> > > > >
> > > > > I think my attention got attracted to pmd migration entry too much, so I
> > > > > didn't really notice such possibility, as I believe migration pmd is broken
> > > > > already in this path.
> > > > >
> > > > > The original code:
> > > > >
> > > > > ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> > > > > if (ptl) {
> > > > > /* Check if we can move the pmd without splitting it. */
> > > > > if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> > > > > !pmd_none(dst_pmdval)) {
> > > > > struct folio *folio = pmd_folio(*src_pmd);
> > > > >
> > > > > if (!folio || (!is_huge_zero_folio(folio) &&
> > > > > !PageAnonExclusive(&folio->page))) {
> > > > > spin_unlock(ptl);
> > > > > err = -EBUSY;
> > > > > break;
> > > > > }
> > > > >
> > > > > spin_unlock(ptl);
> > > > > split_huge_pmd(src_vma, src_pmd, src_addr);
> > > > > /* The folio will be split by move_pages_pte() */
> > > > > continue;
> > > > > }
> > > > >
> > > > > err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
> > > > > dst_pmdval, dst_vma, src_vma,
> > > > > dst_addr, src_addr);
> > > > > step_size = HPAGE_PMD_SIZE;
> > > > > } else {
> > > > >
> > > > > It'll get ptl for a migration pmd, then pmd_folio is risky without checking
> > > > > present bit. That's what my previous smaller patch wanted to fix.
> > > > >
> > > > > But besides that, IIUC it's all fine at least for a pmd migration entry,
> > > > > because when with the smaller patch applied, either we'll try to split the
> > > > > pmd migration entry, or we'll do move_pages_huge_pmd(), which internally
> > > > > handles the pmd migration entry too by waiting on it:
> > > > >
> > > > > if (!pmd_trans_huge(src_pmdval)) {
> > > > > spin_unlock(src_ptl);
> > > > > if (is_pmd_migration_entry(src_pmdval)) {
> > > > > pmd_migration_entry_wait(mm, &src_pmdval);
> > > > > return -EAGAIN;
> > > > > }
> > > > > return -ENOENT;
> > > > > }
> > > > >
> > > > > Then logically after the migration entry got recovered, we'll either see a
> > > > > real THP or pmd none next time.
> > > >
> > > > Yes, for migration entries adding the "if (pmd_present(*src_pmd))"
> > > > before getting the folio is enough. The problematic case is
> > > > (!pmd_none(*src_pmd) && !pmd_present(*src_pmd)) and not a migration
> > > > entry.
> > >
> > > I thought we could have any of below here on the pmd entry:
> > >
> > > (0) pmd_none, which should constantly have pmd_trans_huge_lock -> NULL
> > >
> > > (1) pmd pgtable entry, which must have PRESENT && !TRANS, so
> > > pmd_trans_huge_lock -> NULL,
> > >
> > > (2) pmd migration, pmd_trans_huge_lock -> valid
> > >
> > > (3) pmd thp, pmd_trans_huge_lock -> valid
> > >
> > > I thought (2) was broken, which we seem to agree upon.. however if so the
> > > smaller patch should fix it, per explanation in my previous reply. OTOH I
> > > can't think of (4).
> >
> > The case I was hitting is (!pmd_none && !pmd_present &&
> > !is_pmd_migration_entry). My original thinking was that this entry was
> > newly allocated at the line I pointed earlier but now I'm not so sure
> > anymore.
>
> Hmm, now I can't reproduce this case... I'm pretty sure I've seen that
> case before but now I hit an occasional migration entry and that's
> all. I must have done something wrong when testing it before.
Ok, I let the reproducer run for half a day and it did not hit this
case, so I must have done something wrong during my initial
investigation. Sorry for the confusion. I could have sworn that I saw
this case but now it just does not happen.
With migration entry being the only case that leads to that
pmd_folio(), the only check we need to add is the "if
(pmd_present(*src_pmd))" before pmd_folio(). Would you like me to
check anything else or should I go ahead and post that fix?
>
> >
> > >
> > > Said that, I just noticed (3) can be broken as well - could it be a
> > > prot_none entry? The very confusing part of this patch is it seems to
> > > think it's pmd_none() here as holes:
> > >
> > > if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
> > > ...
> > > } else {
> > > spin_unlock(ptl);
> > > if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> > > err = -ENOENT;
> > > break;
> > > }
> > > /* nothing to do to move a hole */
> > > err = 0;
> > > step_size = min(HPAGE_PMD_SIZE, src_start + len - src_addr);
> > > }
> > >
> > > But is it really? Again, I don't think pmd_none() could happen with
> > > pmd_trans_huge_lock() returning the ptl.
> >
> > That is true, in the pmd_none() case pmd_trans_huge_lock() returns NULL.
> >
> > >
> > > Could you double check this? E.g. with this line if that makes sense to
> > > you:
> > >
> > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > index 8bf8ff0be990f..d2d4f2a0ae69f 100644
> > > --- a/mm/userfaultfd.c
> > > +++ b/mm/userfaultfd.c
> > > @@ -1903,6 +1903,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > dst_addr, src_addr);
> > > step_size = HPAGE_PMD_SIZE;
> > > } else {
> > > + BUG_ON(!pmd_none(*src_pmd));
> > > spin_unlock(ptl);
> > > if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> > > err = -ENOENT;
> > >
> > > I would expect it constantly BUG() here, if that explains my thoughts.
> >
> > I'll add this and check.
>
> This check does trigger and I logged pmd_val=0x8b3714067 and I think
> this is normal. _PAGE_BIT_PRESENT is set and _PAGE_BIT_PSE is not set,
> so is_swap_pmd()==false and pmd_trans_huge()==false, therefore
> pmd_trans_huge_lock() returns NULL.
>
>
> >
> > >
> > > Now I doubt it's a prot_none THP.. aka, a THP that got numa hint to be
> > > moved. If so, we may need to process it / move it / .. but we likely
> > > should never skip it. We can double check the buggy pmd entry you hit
> > > (besides migration entry) first.
> >
> > Let me log the flags of the entry when this issue happens. That should
> > provide more insights.
> > Thanks,
> > Suren.
> >
> > >
> > > Thanks,
> > >
> > > --
> > > Peter Xu
> > >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-05 23:41 ` Suren Baghdasaryan
@ 2025-08-06 0:40 ` Peter Xu
2025-08-06 15:06 ` Suren Baghdasaryan
0 siblings, 1 reply; 21+ messages in thread
From: Peter Xu @ 2025-08-06 0:40 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Tue, Aug 05, 2025 at 04:41:18PM -0700, Suren Baghdasaryan wrote:
> Ok, I let the reproducer run for half a day and it did not hit this
> case, so I must have done something wrong during my initial
> investigation. Sorry for the confusion. I could have sworn that I saw
> this case but now it just does not happen.
I'm wildly guessing you might have hit the numa balancing bug I mentioned,
that might explain what you mentioned previously on the testing results.
It might just be tricky to reproduce:
- We'll need a valid THP (pmd) first in the MOVE source region
- THP needs to be selected by numa balancing for a check (marking
prot_none)
- (before any further access..) UFFDIO_MOVE needs to happen on top trying
to move the whole THP being marked as prot_none.
AFAICT, task_numa_work() is the only place that can mark the THP, and when
it happens, should see change_huge_pmd(cp_flags=MM_CP_PROT_NUMA) and then
returns with HPAGE_PMD_NR.
[sorry I am still pretty occupied with other things. I can try to reproduce
together with you after I get more time back]
> With migration entry being the only case that leads to that
> pmd_folio(), the only check we need to add is the "if
> (pmd_present(*src_pmd))" before pmd_folio(). Would you like me to
> check anything else or should I go ahead and post that fix?
We could fix the migration entry first, then if any of us can reproduce the
above numa balancing issue then it can be a 2nd patch on top.
After all, so far we didn't yet prove it, either some unreproduceable test,
or pure code analysis. Meanwhile it might also be cleaner if we have one
patch fix one issue, rather than having one patch fix two bugs.
What do you think?
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-06 0:40 ` Peter Xu
@ 2025-08-06 15:06 ` Suren Baghdasaryan
2025-08-06 15:46 ` Suren Baghdasaryan
0 siblings, 1 reply; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-08-06 15:06 UTC (permalink / raw)
To: Peter Xu
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Tue, Aug 5, 2025 at 5:41 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Aug 05, 2025 at 04:41:18PM -0700, Suren Baghdasaryan wrote:
> > Ok, I let the reproducer run for half a day and it did not hit this
> > case, so I must have done something wrong during my initial
> > investigation. Sorry for the confusion. I could have sworn that I saw
> > this case but now it just does not happen.
>
> I'm wildly guessing you might have hit the numa balancing bug I mentioned,
> that might explain what you mentioned previously on the testing results.
> It might just be tricky to reproduce:
>
> - We'll need a valid THP (pmd) first in the MOVE source region
>
> - THP needs to be selected by numa balancing for a check (marking
> prot_none)
>
> - (before any further access..) UFFDIO_MOVE needs to happen on top trying
> to move the whole THP being marked as prot_none.
>
> AFAICT, task_numa_work() is the only place that can mark the THP, and when
> it happens, should see change_huge_pmd(cp_flags=MM_CP_PROT_NUMA) and then
> returns with HPAGE_PMD_NR.
>
> [sorry I am still pretty occupied with other things. I can try to reproduce
> together with you after I get more time back]
>
> > With migration entry being the only case that leads to that
> > pmd_folio(), the only check we need to add is the "if
> > (pmd_present(*src_pmd))" before pmd_folio(). Would you like me to
> > check anything else or should I go ahead and post that fix?
>
> We could fix the migration entry first, then if any of us can reproduce the
> above numa balancing issue then it can be a 2nd patch on top.
>
> After all, so far we didn't yet prove it, either some unreproduceable test,
> or pure code analysis. Meanwhile it might also be cleaner if we have one
> patch fix one issue, rather than having one patch fix two bugs.
>
> What do you think?
Agree, that seems reasonable. I'll post the new fix today.
Thanks,
Suren.
>
> Thanks,
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole
2025-08-06 15:06 ` Suren Baghdasaryan
@ 2025-08-06 15:46 ` Suren Baghdasaryan
0 siblings, 0 replies; 21+ messages in thread
From: Suren Baghdasaryan @ 2025-08-06 15:46 UTC (permalink / raw)
To: Peter Xu
Cc: David Hildenbrand, akpm, aarcange, lokeshgidra, linux-mm,
linux-kernel, syzbot+b446dbe27035ef6bd6c2, stable
On Wed, Aug 6, 2025 at 8:06 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Tue, Aug 5, 2025 at 5:41 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Tue, Aug 05, 2025 at 04:41:18PM -0700, Suren Baghdasaryan wrote:
> > > Ok, I let the reproducer run for half a day and it did not hit this
> > > case, so I must have done something wrong during my initial
> > > investigation. Sorry for the confusion. I could have sworn that I saw
> > > this case but now it just does not happen.
> >
> > I'm wildly guessing you might have hit the numa balancing bug I mentioned,
> > that might explain what you mentioned previously on the testing results.
> > It might just be tricky to reproduce:
> >
> > - We'll need a valid THP (pmd) first in the MOVE source region
> >
> > - THP needs to be selected by numa balancing for a check (marking
> > prot_none)
> >
> > - (before any further access..) UFFDIO_MOVE needs to happen on top trying
> > to move the whole THP being marked as prot_none.
> >
> > AFAICT, task_numa_work() is the only place that can mark the THP, and when
> > it happens, should see change_huge_pmd(cp_flags=MM_CP_PROT_NUMA) and then
> > returns with HPAGE_PMD_NR.
> >
> > [sorry I am still pretty occupied with other things. I can try to reproduce
> > together with you after I get more time back]
> >
> > > With migration entry being the only case that leads to that
> > > pmd_folio(), the only check we need to add is the "if
> > > (pmd_present(*src_pmd))" before pmd_folio(). Would you like me to
> > > check anything else or should I go ahead and post that fix?
> >
> > We could fix the migration entry first, then if any of us can reproduce the
> > above numa balancing issue then it can be a 2nd patch on top.
> >
> > After all, so far we didn't yet prove it, either some unreproduceable test,
> > or pure code analysis. Meanwhile it might also be cleaner if we have one
> > patch fix one issue, rather than having one patch fix two bugs.
> >
> > What do you think?
>
> Agree, that seems reasonable. I'll post the new fix today.
v3 is posted at
https://lore.kernel.org/all/20250806154015.769024-1-surenb@google.com/
> Thanks,
> Suren.
>
> >
> > Thanks,
> >
> > --
> > Peter Xu
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2025-08-06 15:47 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-07-31 15:44 [PATCH v2 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole Suren Baghdasaryan
2025-07-31 17:30 ` Lokesh Gidra
2025-08-01 7:21 ` David Hildenbrand
2025-08-01 14:15 ` Peter Xu
2025-08-01 15:28 ` Suren Baghdasaryan
2025-08-01 16:23 ` Peter Xu
2025-08-01 16:41 ` Suren Baghdasaryan
2025-08-01 17:13 ` Peter Xu
2025-08-01 17:45 ` Suren Baghdasaryan
2025-08-01 18:20 ` Peter Xu
2025-08-01 19:30 ` Suren Baghdasaryan
2025-08-02 0:32 ` Peter Xu
2025-08-04 14:55 ` Suren Baghdasaryan
2025-08-05 14:39 ` Peter Xu
2025-08-05 14:57 ` Suren Baghdasaryan
2025-08-05 20:39 ` Suren Baghdasaryan
2025-08-05 23:41 ` Suren Baghdasaryan
2025-08-06 0:40 ` Peter Xu
2025-08-06 15:06 ` Suren Baghdasaryan
2025-08-06 15:46 ` Suren Baghdasaryan
2025-08-01 15:11 ` Suren Baghdasaryan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox