From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id C31F7C433F5 for ; Mon, 29 Nov 2021 06:40:12 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4176B6B0072; Mon, 29 Nov 2021 01:40:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 3C0276B0073; Mon, 29 Nov 2021 01:40:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2887C6B0074; Mon, 29 Nov 2021 01:40:02 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0250.hostedemail.com [216.40.44.250]) by kanga.kvack.org (Postfix) with ESMTP id 19B756B0072 for ; Mon, 29 Nov 2021 01:40:02 -0500 (EST) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id CD8D91823882C for ; Mon, 29 Nov 2021 06:39:51 +0000 (UTC) X-FDA: 78861017424.29.46B30FA Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by imf11.hostedemail.com (Postfix) with ESMTP id B7244F000204 for ; Mon, 29 Nov 2021 06:39:50 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10182"; a="299302393" X-IronPort-AV: E=Sophos;i="5.87,272,1631602800"; d="scan'208";a="299302393" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Nov 2021 22:39:49 -0800 X-IronPort-AV: E=Sophos;i="5.87,272,1631602800"; d="scan'208";a="511556524" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.239.159.50]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Nov 2021 22:39:46 -0800 From: "Huang, Ying" To: Nadav Amit Cc: Andrew Morton , Linux-MM , LKML , syzbot , Mel Gorman , Andrea Arcangeli , "Andy Lutomirski" , Dave Hansen , Will Deacon , Yu Zhao , Marco Elver Subject: Re: [PATCH -V2] mm/rmap: fix potential batched TLB flush race User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) Date: Mon, 29 Nov 2021 14:39:40 +0800 Message-ID: <87ilwbv51v.fsf@yhuang6-desk2.ccr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: B7244F000204 X-Stat-Signature: tznznd85tn773xxt4xqbafp3wdnnc17z Authentication-Results: imf11.hostedemail.com; dkim=none; spf=none (imf11.hostedemail.com: domain of ying.huang@intel.com has no SPF policy when checking 134.134.136.100) smtp.mailfrom=ying.huang@intel.com; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=intel.com (policy=none) X-HE-Tag: 1638167990-923283 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Nadav Amit writes: >> On Nov 24, 2021, at 10:50 PM, Huang Ying wrote: >>=20 >> In theory, the following race is possible for batched TLB flushing. >>=20 >> CPU0 CPU1 >> ---- ---- >> shrink_page_list() >> unmap >> zap_pte_range() >> flush_tlb_batched_pending() >> flush_tlb_mm() >> try_to_unmap() >> set_tlb_ubc_flush_pending() >> mm->tlb_flush_batched =3D true >> mm->tlb_flush_batched =3D false >>=20 >> After the TLB is flushed on CPU1 via flush_tlb_mm() and before >> mm->tlb_flush_batched is set to false, some PTE is unmapped on CPU0 >> and the TLB flushing is pended. Then the pended TLB flushing will be >> lost. Although both set_tlb_ubc_flush_pending() and >> flush_tlb_batched_pending() are called with PTL locked, different PTL >> instances may be used. >>=20 >> Because the race window is really small, and the lost TLB flushing >> will cause problem only if a TLB entry is inserted before the >> unmapping in the race window, the race is only theoretical. But the >> fix is simple and cheap too. >>=20 >> Syzbot has reported this too as follows, >>=20 >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> BUG: KCSAN: data-race in flush_tlb_batched_pending / try_to_unmap_one >>=20 >> write to 0xffff8881072cfbbc of 1 bytes by task 17406 on cpu 1: >> flush_tlb_batched_pending+0x5f/0x80 mm/rmap.c:691 >> madvise_free_pte_range+0xee/0x7d0 mm/madvise.c:594 >> walk_pmd_range mm/pagewalk.c:128 [inline] >> walk_pud_range mm/pagewalk.c:205 [inline] >> walk_p4d_range mm/pagewalk.c:240 [inline] >> walk_pgd_range mm/pagewalk.c:277 [inline] >> __walk_page_range+0x981/0x1160 mm/pagewalk.c:379 >> walk_page_range+0x131/0x300 mm/pagewalk.c:475 >> madvise_free_single_vma mm/madvise.c:734 [inline] >> madvise_dontneed_free mm/madvise.c:822 [inline] >> madvise_vma mm/madvise.c:996 [inline] >> do_madvise+0xe4a/0x1140 mm/madvise.c:1202 >> __do_sys_madvise mm/madvise.c:1228 [inline] >> __se_sys_madvise mm/madvise.c:1226 [inline] >> __x64_sys_madvise+0x5d/0x70 mm/madvise.c:1226 >> do_syscall_x64 arch/x86/entry/common.c:50 [inline] >> do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80 >> entry_SYSCALL_64_after_hwframe+0x44/0xae >>=20 >> write to 0xffff8881072cfbbc of 1 bytes by task 71 on cpu 0: >> set_tlb_ubc_flush_pending mm/rmap.c:636 [inline] >> try_to_unmap_one+0x60e/0x1220 mm/rmap.c:1515 >> rmap_walk_anon+0x2fb/0x470 mm/rmap.c:2301 >> try_to_unmap+0xec/0x110 >> shrink_page_list+0xe91/0x2620 mm/vmscan.c:1719 >> shrink_inactive_list+0x3fb/0x730 mm/vmscan.c:2394 >> shrink_list mm/vmscan.c:2621 [inline] >> shrink_lruvec+0x3c9/0x710 mm/vmscan.c:2940 >> shrink_node_memcgs+0x23e/0x410 mm/vmscan.c:3129 >> shrink_node+0x8f6/0x1190 mm/vmscan.c:3252 >> kswapd_shrink_node mm/vmscan.c:4022 [inline] >> balance_pgdat+0x702/0xd30 mm/vmscan.c:4213 >> kswapd+0x200/0x340 mm/vmscan.c:4473 >> kthread+0x2c7/0x2e0 kernel/kthread.c:327 >> ret_from_fork+0x1f/0x30 >>=20 >> value changed: 0x01 -> 0x00 >>=20 >> Reported by Kernel Concurrency Sanitizer on: >> CPU: 0 PID: 71 Comm: kswapd0 Not tainted 5.16.0-rc1-syzkaller #0 >> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS = Google 01/01/2011 >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>=20 >> Signed-off-by: "Huang, Ying" >> Reported-by: syzbot+aa5bebed695edaccf0df@syzkaller.appspotmail.com >> Cc: Nadav Amit >> Cc: Mel Gorman >> Cc: Andrea Arcangeli >> Cc: Andy Lutomirski >> Cc: Dave Hansen >> Cc: Will Deacon >> Cc: Yu Zhao >> Cc: Marco Elver >> --- >> include/linux/mm_types.h | 2 +- >> mm/rmap.c | 30 +++++++++++++++++++++++------- >> 2 files changed, 24 insertions(+), 8 deletions(-) >>=20 >> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h >> index c3a6e6209600..789778067db9 100644 >> --- a/include/linux/mm_types.h >> +++ b/include/linux/mm_types.h >> @@ -632,7 +632,7 @@ struct mm_struct { >> atomic_t tlb_flush_pending; >> #ifdef CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH >> /* See flush_tlb_batched_pending() */ >> - bool tlb_flush_batched; >> + atomic_t tlb_flush_batched; >> #endif >> struct uprobes_state uprobes_state; >> #ifdef CONFIG_PREEMPT_RT >> diff --git a/mm/rmap.c b/mm/rmap.c >> index 163ac4e6bcee..2e6b19be5a18 100644 >> --- a/mm/rmap.c >> +++ b/mm/rmap.c >> @@ -621,6 +621,18 @@ void try_to_unmap_flush_dirty(void) >> try_to_unmap_flush(); >> } >>=20 >> +/* The upper 15 bit of mm->tlb_flush_batched records pending flushes */ > > Why 15? I think it will be easier to swallow if it was 32-bit (which > correspond to number of PIDs?) >=20 > What would happen on an overflow? If you regarded each pneding/flushed > counter as a separate atomic, that would have been easier. But anyhow, > a comment is necessary IMHO. I want to pack the "pending" and "flush" generations into one atomic_t, which is 32 bit. Previously I thought 15-bit is large enough to make overflow impossible in practice, but after more thought, I found that it isn't large enough. So I come up with some solution for overflow and I will send a new version of patch to show my idea. Why not 16 bit? Just want to make it easier to read the code via avoiding to deal with signed/unsigned. > >> +#define TLB_FLUSH_BATCH_PENDING_SHIFT 16 >> +#define TLB_FLUSH_BATCH_COUNT_MASK 0x7f > > 0x7f is not 15 bits the last time I checked. Oops! My fault! I will correct this in the new version. >> +#define TLB_FLUSH_BATCH_PENDING_ONE (1 << TLB_FLUSH_BATCH_PENDING_SHIFT) >> + >> +#define TLB_FLUSH_BATCH_PENDING(cnt) \ >> + (((cnt) >> TLB_FLUSH_BATCH_PENDING_SHIFT) & TLB_FLUSH_BATCH_COUNT_MASK) >> +#define TLB_FLUSH_BATCH_FLUSHED(cnt) \ >> + ((cnt) & TLB_FLUSH_BATCH_COUNT_MASK) >> +#define TLB_FLUSH_BATCH_PACK(pending, flushed) \ >> + (((pending) << TLB_FLUSH_BATCH_PENDING_SHIFT) | (flushed)) > > I would have preferred, when possible to avoid such macros. It just makes > reading the code harder. Yes. They are not elegant. But it's not good to hard code raw numbers too. >> + >> static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writabl= e) >> { >> struct tlbflush_unmap_batch *tlb_ubc =3D ¤t->tlb_ubc; >> @@ -633,7 +645,7 @@ static void set_tlb_ubc_flush_pending(struct mm_stru= ct *mm, bool writable) >> * before the PTE is cleared. >> */ >> barrier(); >> - mm->tlb_flush_batched =3D true; >> + atomic_add(TLB_FLUSH_BATCH_PENDING_ONE, &mm->tlb_flush_batched); > > Any reason to put the pending in the top bits instead of the low ones? > It can at least simplify the code a bit. In this patch, it is to avoid to make the overflowed bit of "pending" goes into "flushed". But I will change the algorithm in the new version, so this isn't necessary any more. > As for the barrier, I would > change it for smp_mb__before_atomic() or smp_wmb(). You want the > PTE changes and the counter updates to be ordered. You suggest to convert barrier() to something stricter? I think it deserves a separate patch. >>=20 >> /* >> * If the PTE was dirty then it's best to assume it's writable. The >> @@ -680,15 +692,19 @@ static bool should_defer_flush(struct mm_struct *m= m, enum ttu_flags flags) >> */ >> void flush_tlb_batched_pending(struct mm_struct *mm) >> { >> - if (data_race(mm->tlb_flush_batched)) { >> - flush_tlb_mm(mm); > > The previous smp_mb__before_atomic() or smp_wmb() should be matched > with an smp_mb__before_atomic() or smp_rmb() here, I think. atomic_cmpxchg() is fully ordered, so we don't need any more ordering call here. >> + int batched =3D atomic_read(&mm->tlb_flush_batched); >> + int pending =3D TLB_FLUSH_BATCH_PENDING(batched); >> + int flushed =3D TLB_FLUSH_BATCH_FLUSHED(batched); > > I would prefer them being unsigned. atomic_t is signed. So I think that it may be better to use signed when possible to avoid possible confusion. >>=20 >> + if (pending !=3D flushed) { >> + flush_tlb_mm(mm); >> /* >> - * Do not allow the compiler to re-order the clearing of >> - * tlb_flush_batched before the tlb is flushed. >> + * If the new TLB flushing is pended during flushing, >> + * leave mm->tlb_flush_batched as is, to avoid to lose >> + * flushing. >> */ >> - barrier(); >> - mm->tlb_flush_batched =3D false; >> + atomic_cmpxchg(&mm->tlb_flush_batched, batched, >> + TLB_FLUSH_BATCH_PACK(pending, pending)); >> } > > Overall, I am not overly excited about the fact the the mm generation > and this batching mechanism remain separated. This makes reasoning > about TLB flushes harder and can lead to unnecessary flushes. > > I understand the complexity in changing the code to get there, > and perhaps your approach is reasonable for now, if you at least > manage to deal with overflows. > > Thanks for doing all of that, sorry for being negative. I guess I=20 > suffer from NIH ("not invented here=E2=80=9D). Thanks for your review! Best Regards, Huang, Ying