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 F2109C433EF for ; Tue, 30 Nov 2021 01:38:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 217EF6B0071; Mon, 29 Nov 2021 20:38:44 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 1C6406B0072; Mon, 29 Nov 2021 20:38:44 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 08F416B0073; Mon, 29 Nov 2021 20:38:44 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0140.hostedemail.com [216.40.44.140]) by kanga.kvack.org (Postfix) with ESMTP id EFF766B0071 for ; Mon, 29 Nov 2021 20:38:43 -0500 (EST) Received: from smtpin02.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id A69E7181AF5EA for ; Tue, 30 Nov 2021 01:38:33 +0000 (UTC) X-FDA: 78863886906.02.3BC8501 Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by imf23.hostedemail.com (Postfix) with ESMTP id 498499000386 for ; Tue, 30 Nov 2021 01:38:24 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10183"; a="296921776" X-IronPort-AV: E=Sophos;i="5.87,273,1631602800"; d="scan'208";a="296921776" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Nov 2021 17:38:30 -0800 X-IronPort-AV: E=Sophos;i="5.87,273,1631602800"; d="scan'208";a="511952327" 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; 29 Nov 2021 17:38:27 -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 References: <87ilwbv51v.fsf@yhuang6-desk2.ccr.corp.intel.com> Date: Tue, 30 Nov 2021 09:38:25 +0800 In-Reply-To: (Nadav Amit's message of "Mon, 29 Nov 2021 16:51:04 +0000") Message-ID: <87a6hmv2we.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Stat-Signature: kua53sq77w9geaz9tzos7ka8xdapfcms X-Rspamd-Queue-Id: 498499000386 X-Rspamd-Server: rspam07 Authentication-Results: imf23.hostedemail.com; dkim=none; spf=none (imf23.hostedemail.com: domain of ying.huang@intel.com has no SPF policy when checking 134.134.136.31) smtp.mailfrom=ying.huang@intel.com; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=intel.com (policy=none) X-HE-Tag: 1638236304-514633 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 28, 2021, at 10:39 PM, Huang, Ying wrote: >>=20 >> Nadav Amit writes: >>=20 >>>> 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, BIO= S 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 = */ >>>=20 >>> 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. >>=20 >> 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. >>=20 >> Why not 16 bit? Just want to make it easier to read the code via >> avoiding to deal with signed/unsigned. > > I don=E2=80=99t think it helps. If it doesn't hurt too, why not? >>=20 >>>=20 >>>> +#define TLB_FLUSH_BATCH_PENDING_SHIFT 16 >>>> +#define TLB_FLUSH_BATCH_COUNT_MASK 0x7f >>>=20 >>> 0x7f is not 15 bits the last time I checked. >>=20 >> Oops! My fault! I will correct this in the new version. >>=20 >>>> +#define TLB_FLUSH_BATCH_PENDING_ONE (1 << TLB_FLUSH_BATCH_PENDING_SHI= FT) >>>> + >>>> +#define TLB_FLUSH_BATCH_PENDING(cnt) \ >>>> + (((cnt) >> TLB_FLUSH_BATCH_PENDING_SHIFT) & TLB_FLUSH_BATCH_COUNT_MA= SK) >>>> +#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)) >>>=20 >>> I would have preferred, when possible to avoid such macros. It just mak= es >>> reading the code harder. >>=20 >> Yes. They are not elegant. But it's not good to hard code raw numbers = too. > > Having macros for TLB_FLUSH_PENDING_SHIFT and TLB_FLUSH_PENDING_MASK is > reasonable. Here we have macros with arguments, and I am not even sure th= at > the upper-case names here are compliant with the kernel code styling. > I also think it is unnecessary, and open-coding will make the code more > readable. > > If you had bit-fields/struct with pending & flushed, and then did > type-casting, then although others might not be too happy with that, > I do think it would=E2=80=99ve made the code more readable. Otherwise, th= ese=20 > macros are just confusing IMHO. OK. I will only keep the constant definition. >>=20 >>>> + >>>> static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writa= ble) >>>> { >>>> struct tlbflush_unmap_batch *tlb_ubc =3D ¤t->tlb_ubc; >>>> @@ -633,7 +645,7 @@ static void set_tlb_ubc_flush_pending(struct mm_st= ruct *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); >>>=20 >>> Any reason to put the pending in the top bits instead of the low ones? >>> It can at least simplify the code a bit. >>=20 >> 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. >>=20 >>> 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. >>=20 >> You suggest to convert barrier() to something stricter? I think it >> deserves a separate patch. > > If you relaxed the memory barrier, yes - new patch, but I am not sure > that if you make it stricter you need a different patch. Stricter memory barrier increases overhead too. So we only add them when necessary. I think a separate patch is better because I think the necessity should be documented in that patch. Best Regards, Huang, Ying