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
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
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?
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,
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
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
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
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)?
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
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,
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
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,
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
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,
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
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
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
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,
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
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
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
linux-stable-mirror@lists.linaro.org
-
David Hildenbrand -
Lokesh Gidra -
Peter Xu -
Suren Baghdasaryan