From: Pasha Tatashin <pasha.tatashin@soleen.com>
To: Wei Xu <weixugc@google.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Linux MM <linux-mm@kvack.org>,
Andrew Morton <akpm@linux-foundation.org>,
David Rientjes <rientjes@google.com>,
Paul Turner <pjt@google.com>, Greg Thelen <gthelen@google.com>,
Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
Mike Rapoport <rppt@kernel.org>,
Dave Hansen <dave.hansen@linux.intel.com>,
"H. Peter Anvin" <hpa@zytor.com>,
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
Jiri Slaby <jirislaby@kernel.org>,
Muchun Song <songmuchun@bytedance.com>,
Fusion Future <qydwhotmail@gmail.com>,
Hugh Dickins <hughd@google.com>, Zi Yan <ziy@nvidia.com>,
Anshuman Khandual <anshuman.khandual@arm.com>
Subject: Re: [PATCH v4 4/4] mm/page_table_check: check entries at pmd levels
Date: Mon, 31 Jan 2022 15:16:15 -0500 [thread overview]
Message-ID: <CA+CK2bDYhzQ7wP0Ef3n6YGDo2=C0cdhYnEq9XkbOGzPKBp4SSQ@mail.gmail.com> (raw)
In-Reply-To: <CAAPL-u-yo4LuFdudYMEccUYTOMpvnqEjzMZRjOhUBkR4t=BSDw@mail.gmail.com>
On Fri, Jan 28, 2022 at 6:39 PM Wei Xu <weixugc@google.com> wrote:
>
> On Wed, Jan 26, 2022 at 10:36 AM Pasha Tatashin
> <pasha.tatashin@soleen.com> wrote:
> >
> > syzbot detected a case where the page table counters were not properly
> > updated.
> >
> > syzkaller login: ------------[ cut here ]------------
> > kernel BUG at mm/page_table_check.c:162!
> > invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> > CPU: 0 PID: 3099 Comm: pasha Not tainted 5.16.0+ #48
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIO4
> > RIP: 0010:__page_table_check_zero+0x159/0x1a0
> > Code: 7d 3a b2 ff 45 39 f5 74 2a e8 43 38 b2 ff 4d 85 e4 01
> > RSP: 0018:ffff888010667418 EFLAGS: 00010293
> > RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000
> > RDX: ffff88800cea8680 RSI: ffffffff81becaf9 RDI: 0000000003
> > RBP: ffff888010667450 R08: 0000000000000001 R09: 0000000000
> > R10: ffffffff81becaab R11: 0000000000000001 R12: ffff888008
> > R13: 0000000000000001 R14: 0000000000000200 R15: dffffc0000
> > FS: 0000000000000000(0000) GS:ffff888035e00000(0000) knlG0
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007ffd875cad00 CR3: 00000000094ce000 CR4: 0000000000
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000
> > Call Trace:
> > <TASK>
> > free_pcp_prepare+0x3be/0xaa0
> > free_unref_page+0x1c/0x650
> > ? trace_hardirqs_on+0x6a/0x1d0
> > free_compound_page+0xec/0x130
> > free_transhuge_page+0x1be/0x260
> > __put_compound_page+0x90/0xd0
> > release_pages+0x54c/0x1060
> > ? filemap_remove_folio+0x161/0x210
> > ? lock_downgrade+0x720/0x720
> > ? __put_page+0x150/0x150
> > ? filemap_free_folio+0x164/0x350
> > __pagevec_release+0x7c/0x110
> > shmem_undo_range+0x85e/0x1250
> > ...
> >
> > The repro involved having a huge page that is split due to uprobe event
> > temporarily replacing one of the pages in the huge page. Later the huge
> > page was combined again, but the counters were off, as the PTE level
> > was not properly updated.
> >
> > Make sure that when PMD is cleared and prior to freeing the level the
> > PTEs are updated.
> >
> > Fixes: df4e817b7108 ("mm: page table check")
> >
> > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> > ---
> > include/linux/page_table_check.h | 18 ++++++++++++++++++
> > mm/khugepaged.c | 3 +++
> > mm/page_table_check.c | 21 +++++++++++++++++++++
> > 3 files changed, 42 insertions(+)
> >
> > diff --git a/include/linux/page_table_check.h b/include/linux/page_table_check.h
> > index 38cace1da7b6..e88bbe37727b 100644
> > --- a/include/linux/page_table_check.h
> > +++ b/include/linux/page_table_check.h
> > @@ -26,6 +26,8 @@ void __page_table_check_pmd_set(struct mm_struct *mm, unsigned long addr,
> > pmd_t *pmdp, pmd_t pmd);
> > void __page_table_check_pud_set(struct mm_struct *mm, unsigned long addr,
> > pud_t *pudp, pud_t pud);
> > +void __page_table_check_pmd_clear_full(struct mm_struct *mm, unsigned long addr,
> > + pmd_t pmd);
> >
> > static inline void page_table_check_alloc(struct page *page, unsigned int order)
> > {
> > @@ -100,6 +102,16 @@ static inline void page_table_check_pud_set(struct mm_struct *mm,
> > __page_table_check_pud_set(mm, addr, pudp, pud);
> > }
> >
> > +static inline void page_table_check_pmd_clear_full(struct mm_struct *mm,
> > + unsigned long addr,
> > + pmd_t pmd)
> > +{
> > + if (static_branch_likely(&page_table_check_disabled))
> > + return;
> > +
> > + __page_table_check_pmd_clear_full(mm, addr, pmd);
> > +}
> > +
> > #else
> >
> > static inline void page_table_check_alloc(struct page *page, unsigned int order)
> > @@ -143,5 +155,11 @@ static inline void page_table_check_pud_set(struct mm_struct *mm,
> > {
> > }
> >
> > +static inline void page_table_check_pmd_clear_full(struct mm_struct *mm,
> > + unsigned long addr,
> > + pmd_t pmd)
> > +{
> > +}
> > +
> > #endif /* CONFIG_PAGE_TABLE_CHECK */
> > #endif /* __LINUX_PAGE_TABLE_CHECK_H */
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 30e59e4af272..d84977c6dc0d 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -16,6 +16,7 @@
> > #include <linux/hashtable.h>
> > #include <linux/userfaultfd_k.h>
> > #include <linux/page_idle.h>
> > +#include <linux/page_table_check.h>
> > #include <linux/swapops.h>
> > #include <linux/shmem_fs.h>
> >
> > @@ -1422,10 +1423,12 @@ static void collapse_and_free_pmd(struct mm_struct *mm, struct vm_area_struct *v
> > spinlock_t *ptl;
> > pmd_t pmd;
> >
> > + mmap_assert_write_locked(mm);
> > ptl = pmd_lock(vma->vm_mm, pmdp);
> > pmd = pmdp_collapse_flush(vma, addr, pmdp);
> > spin_unlock(ptl);
> > mm_dec_nr_ptes(mm);
> > + page_table_check_pmd_clear_full(mm, addr, pmd);
>
Hi Wei,
Thank you for your feedback,
> pmdp_collapse_flush() already calls page_table_check_pmd_clear() via
> pmdp_huge_get_and_clean(). Both pmdp_table_check_pmd_clear() and
> page_table_check_pmd_clear_full() can call
> __page_table_check_pmd_clear(). If that happens, then the page table
> check counters can be messed up. Certainly, there is no bug here
> because the pmd is not huge and __page_table_check_pmd_clear() should
> be skipped in both calls. But it would be better to avoid this
> unnecessary subtlety by renaming page_table_check_pmd_clear_full() to
> page_table_check_clear_pte_range() and not calling
> __page_table_check_pmd_clear() there. To make the code even more
Makes sense, I will rename page_table_check_pmd_clear_full() to
page_table_check_clear_pte_range()
and remove the call to __page_table_check_pmd_clear().
> clear, __page_table_check_pmd_clear() can also be renamed as
> __page_table_check_huge_pmd_clear() (similar for its callers).
Let's keep the current names for now as this does not affect the bug
fix. Perhaps, I can rename it later when working on ARM64 support.
Pasha
prev parent reply other threads:[~2022-01-31 20:16 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-26 18:36 [PATCH v4 0/4] page table check fixes and cleanups Pasha Tatashin
2022-01-26 18:36 ` [PATCH v4 1/4] mm/debug_vm_pgtable: remove pte entry from the page table Pasha Tatashin
2022-01-26 18:36 ` [PATCH v4 2/4] mm/page_table_check: use unsigned long for page counters and cleanup Pasha Tatashin
2022-01-26 18:36 ` [PATCH v4 3/4] mm/khugepaged: unify collapse pmd clear, flush and free Pasha Tatashin
2022-01-26 18:36 ` [PATCH v4 4/4] mm/page_table_check: check entries at pmd levels Pasha Tatashin
2022-01-26 19:26 ` David Rientjes
2022-01-28 23:39 ` Wei Xu
2022-01-31 20:16 ` Pasha Tatashin [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+CK2bDYhzQ7wP0Ef3n6YGDo2=C0cdhYnEq9XkbOGzPKBp4SSQ@mail.gmail.com' \
--to=pasha.tatashin@soleen.com \
--cc=akpm@linux-foundation.org \
--cc=aneesh.kumar@linux.ibm.com \
--cc=anshuman.khandual@arm.com \
--cc=dave.hansen@linux.intel.com \
--cc=gthelen@google.com \
--cc=hpa@zytor.com \
--cc=hughd@google.com \
--cc=jirislaby@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mingo@redhat.com \
--cc=pjt@google.com \
--cc=qydwhotmail@gmail.com \
--cc=rientjes@google.com \
--cc=rppt@kernel.org \
--cc=songmuchun@bytedance.com \
--cc=weixugc@google.com \
--cc=will@kernel.org \
--cc=ziy@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox