Re: [BUG] Re: [PATCH v3 10/13] mm/khugepaged: collapse_pte_mapped_thp() with mmap_read_lock()
From: Hugh Dickins <hughd@google.com>
Date: 2023-08-21 19:48:47
Also in:
linux-mm, linux-s390, lkml, sparclinux
On Mon, 14 Aug 2023, Jann Horn wrote:
On Wed, Jul 12, 2023 at 6:42 AM Hugh Dickins [off-list ref] wrote:quoted
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@mail.gmail.com/ (local)
quoted hunk ↗ jump to hunk
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