On Mon, 14 Aug 2023, Jann Horn wrote: > On Wed, Jul 12, 2023 at 6:42 AM Hugh Dickins <hughd@xxxxxxxxxx> wrote: > > Bring collapse_and_free_pmd() back into collapse_pte_mapped_thp(). > > It does need mmap_read_lock(), but it does not need mmap_write_lock(), > > nor vma_start_write() nor i_mmap lock nor anon_vma lock. All racing > > paths are relying on pte_offset_map_lock() and pmd_lock(), so use those. > > We can still have a racing userfaultfd operation at the "/* step 4: > remove page table */" point that installs a new PTE before the page > table is removed. And you've been very polite not to remind me that this is exactly what you warned me about, in connection with retract_page_tables(), nearly three months ago: https://lore.kernel.org/linux-mm/CAG48ez0aF1Rf1apSjn9YcnfyFQ4YqSd4GqB6f2wfhF7jMdi5Hg@xxxxxxxxxxxxxx/ > > To reproduce, patch a delay into the kernel like this: > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index 9a6e0d507759..27cc8dfbf3a7 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -20,6 +20,7 @@ > #include <linux/swapops.h> > #include <linux/shmem_fs.h> > #include <linux/ksm.h> > +#include <linux/delay.h> > > #include <asm/tlb.h> > #include <asm/pgalloc.h> > @@ -1617,6 +1618,11 @@ int collapse_pte_mapped_thp(struct mm_struct > *mm, unsigned long addr, > } > > /* step 4: remove page table */ > + if (strcmp(current->comm, "DELAYME") == 0) { > + pr_warn("%s: BEGIN DELAY INJECTION\n", __func__); > + mdelay(5000); > + pr_warn("%s: END DELAY INJECTION\n", __func__); > + } > > /* Huge page lock is still held, so page table must remain empty */ > pml = pmd_lock(mm, pmd); > > > And then run the attached reproducer against mm/mm-everything. You > should get this in dmesg: > > [ 206.578096] BUG: Bad rss-counter state mm:000000000942ebea > type:MM_ANONPAGES val:1 Very helpful, thank you Jann. I got a bit distracted when I then found mm's recent addition of UFFDIO_POISON: thought I needed to change both collapse_pte_mapped_thp() and retract_page_tables() now to cope with mfill_atomic_pte_poison() inserting into even a userfaultfd_armed shared VMA. But eventually, on second thoughts, realized that's only inserting a pte marker, invalid, so won't cause any actual trouble. A little untidy, to leave that behind in a supposedly empty page table about to be freed, but not worth refactoring these functions to avoid a non-bug. And though syzbot and JH may find some fun with it, I don't think any real application would be insertng a PTE_MARKER_POISONED where a huge page collapse is almost complete. So I scaled back to a more proportionate fix, following. Sorry, I've slightly messed up applying the "DELAY INJECTION" patch above: not intentional, honest! (mdelay while holding the locks is still good.) Hugh
