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