When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it encounters a non-present PMD (migration entry), it proceeds with folio access even though the folio is not present. Add the missing check and let split_huge_pmd() handle migration entries.
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 v2 [1] - Updated the title and changelog, per David Hildenbrand - Removed extra checks for non-present not-migration PMD entries, per Peter Xu
[1] https://lore.kernel.org/all/20250731154442.319568-1-surenb@google.com/
mm/userfaultfd.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index 5431c9dd7fd7..116481606be8 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -1826,13 +1826,16 @@ 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; + /* Can be a migration entry */ + 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);
base-commit: 8e7e0c6d09502e44aa7a8fce0821e042a6ec03d1
On Wed, Aug 06, 2025 at 08:40:15AM -0700, Suren Baghdasaryan wrote:
When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
The migration entry can appear with/without ALLOW_SRC_HOLES, right? Maybe drop this line?
If we need another repost, the subject can further be tailored to mention migration entry too rather than non-present. IMHO that's clearer on explaining the issue this patch is fixing (e.g. a valid transhuge THP can also have present bit cleared).
encounters a non-present PMD (migration entry), it proceeds with folio access even though the folio is not present. Add the missing check and
IMHO "... even though folio is not present" is pretty vague. Maybe "... even though it's a swap entry"? Fundamentally it's because of the different layouts of normal THP v.s. a swap entry, hence pmd_folio() should not be used on top of swap entries.
let split_huge_pmd() handle migration entries.
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 v2 [1]
- Updated the title and changelog, per David Hildenbrand
- Removed extra checks for non-present not-migration PMD entries,
per Peter Xu
[1] https://lore.kernel.org/all/20250731154442.319568-1-surenb@google.com/
mm/userfaultfd.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index 5431c9dd7fd7..116481606be8 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -1826,13 +1826,16 @@ 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;
/* Can be a migration entry */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;} }
The change itself looks all correct, thanks. If you agree with above commit message / subject updates, feel free to take this after some amendment of the commit message:
Reviewed-by: Peter Xu peterx@redhat.com
spin_unlock(ptl);
base-commit: 8e7e0c6d09502e44aa7a8fce0821e042a6ec03d1
2.50.1.565.gc32cd1483b-goog
On Wed, Aug 6, 2025 at 9:56 AM Peter Xu peterx@redhat.com wrote:
On Wed, Aug 06, 2025 at 08:40:15AM -0700, Suren Baghdasaryan wrote:
When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
The migration entry can appear with/without ALLOW_SRC_HOLES, right? Maybe drop this line?
Yes, you are right. I'll update.
If we need another repost, the subject can further be tailored to mention migration entry too rather than non-present. IMHO that's clearer on explaining the issue this patch is fixing (e.g. a valid transhuge THP can also have present bit cleared).
encounters a non-present PMD (migration entry), it proceeds with folio access even though the folio is not present. Add the missing check and
IMHO "... even though folio is not present" is pretty vague. Maybe "... even though it's a swap entry"? Fundamentally it's because of the different layouts of normal THP v.s. a swap entry, hence pmd_folio() should not be used on top of swap entries.
Well, technically a migration entry is a non_swap_entry(), so calling migration entries "swap entries" is confusing to me. Any better wording we can use or do you think that's ok?
let split_huge_pmd() handle migration entries.
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 v2 [1]
- Updated the title and changelog, per David Hildenbrand
- Removed extra checks for non-present not-migration PMD entries,
per Peter Xu
[1] https://lore.kernel.org/all/20250731154442.319568-1-surenb@google.com/
mm/userfaultfd.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index 5431c9dd7fd7..116481606be8 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -1826,13 +1826,16 @@ 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;
/* Can be a migration entry */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;} }The change itself looks all correct, thanks. If you agree with above commit message / subject updates, feel free to take this after some amendment of the commit message:
Reviewed-by: Peter Xu peterx@redhat.com
spin_unlock(ptl);base-commit: 8e7e0c6d09502e44aa7a8fce0821e042a6ec03d1
2.50.1.565.gc32cd1483b-goog
-- Peter Xu
On Wed, Aug 06, 2025 at 10:09:30AM -0700, Suren Baghdasaryan wrote:
On Wed, Aug 6, 2025 at 9:56 AM Peter Xu peterx@redhat.com wrote:
On Wed, Aug 06, 2025 at 08:40:15AM -0700, Suren Baghdasaryan wrote:
When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
The migration entry can appear with/without ALLOW_SRC_HOLES, right? Maybe drop this line?
Yes, you are right. I'll update.
If we need another repost, the subject can further be tailored to mention migration entry too rather than non-present. IMHO that's clearer on explaining the issue this patch is fixing (e.g. a valid transhuge THP can also have present bit cleared).
encounters a non-present PMD (migration entry), it proceeds with folio access even though the folio is not present. Add the missing check and
IMHO "... even though folio is not present" is pretty vague. Maybe "... even though it's a swap entry"? Fundamentally it's because of the different layouts of normal THP v.s. a swap entry, hence pmd_folio() should not be used on top of swap entries.
Well, technically a migration entry is a non_swap_entry(), so calling migration entries "swap entries" is confusing to me. Any better wording we can use or do you think that's ok?
The more general definition of "swap entry" should follow what swp_entry_t is defined, where, for example, is_migration_entry() itself takes swp_entry_t as input. So it should be fine, but I agree it's indeed confusing.
If we want to make it clearer, IMHO we could rename non_swap_entry() instead to is_swapfile_entry() / is_real_swap_entry() / ... but that can be discussed separately. Here, if we want to make it super accurate, we could also use "swp_entry_t" instead of "swap entry", that'll be 100% accurate.
Thanks,
On Wed, Aug 6, 2025 at 11:09 AM Peter Xu peterx@redhat.com wrote:
On Wed, Aug 06, 2025 at 10:09:30AM -0700, Suren Baghdasaryan wrote:
On Wed, Aug 6, 2025 at 9:56 AM Peter Xu peterx@redhat.com wrote:
On Wed, Aug 06, 2025 at 08:40:15AM -0700, Suren Baghdasaryan wrote:
When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
The migration entry can appear with/without ALLOW_SRC_HOLES, right? Maybe drop this line?
Yes, you are right. I'll update.
If we need another repost, the subject can further be tailored to mention migration entry too rather than non-present. IMHO that's clearer on explaining the issue this patch is fixing (e.g. a valid transhuge THP can also have present bit cleared).
encounters a non-present PMD (migration entry), it proceeds with folio access even though the folio is not present. Add the missing check and
IMHO "... even though folio is not present" is pretty vague. Maybe "... even though it's a swap entry"? Fundamentally it's because of the different layouts of normal THP v.s. a swap entry, hence pmd_folio() should not be used on top of swap entries.
Well, technically a migration entry is a non_swap_entry(), so calling migration entries "swap entries" is confusing to me. Any better wording we can use or do you think that's ok?
The more general definition of "swap entry" should follow what swp_entry_t is defined, where, for example, is_migration_entry() itself takes swp_entry_t as input. So it should be fine, but I agree it's indeed confusing.
If we want to make it clearer, IMHO we could rename non_swap_entry() instead to is_swapfile_entry() / is_real_swap_entry() / ... but that can be discussed separately. Here, if we want to make it super accurate, we could also use "swp_entry_t" instead of "swap entry", that'll be 100% accurate.
Ok, that I think is our best option. I'll post an update shortly. Thanks!
Thanks,
-- Peter Xu
On Wed, Aug 6, 2025 at 11:21 AM Suren Baghdasaryan surenb@google.com wrote:
On Wed, Aug 6, 2025 at 11:09 AM Peter Xu peterx@redhat.com wrote:
On Wed, Aug 06, 2025 at 10:09:30AM -0700, Suren Baghdasaryan wrote:
On Wed, Aug 6, 2025 at 9:56 AM Peter Xu peterx@redhat.com wrote:
On Wed, Aug 06, 2025 at 08:40:15AM -0700, Suren Baghdasaryan wrote:
When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it
The migration entry can appear with/without ALLOW_SRC_HOLES, right? Maybe drop this line?
Yes, you are right. I'll update.
If we need another repost, the subject can further be tailored to mention migration entry too rather than non-present. IMHO that's clearer on explaining the issue this patch is fixing (e.g. a valid transhuge THP can also have present bit cleared).
encounters a non-present PMD (migration entry), it proceeds with folio access even though the folio is not present. Add the missing check and
IMHO "... even though folio is not present" is pretty vague. Maybe "... even though it's a swap entry"? Fundamentally it's because of the different layouts of normal THP v.s. a swap entry, hence pmd_folio() should not be used on top of swap entries.
Well, technically a migration entry is a non_swap_entry(), so calling migration entries "swap entries" is confusing to me. Any better wording we can use or do you think that's ok?
The more general definition of "swap entry" should follow what swp_entry_t is defined, where, for example, is_migration_entry() itself takes swp_entry_t as input. So it should be fine, but I agree it's indeed confusing.
If we want to make it clearer, IMHO we could rename non_swap_entry() instead to is_swapfile_entry() / is_real_swap_entry() / ... but that can be discussed separately. Here, if we want to make it super accurate, we could also use "swp_entry_t" instead of "swap entry", that'll be 100% accurate.
Ok, that I think is our best option. I'll post an update shortly.
Posted at https://lore.kernel.org/all/20250806220022.926763-1-surenb@google.com/ Thanks!
Thanks!
Thanks,
-- Peter Xu
linux-stable-mirror@lists.linaro.org
-
Peter Xu -
Suren Baghdasaryan