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 X-Spam-Level: X-Spam-Status: No, score=-19.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C55B0C33CB2 for ; Wed, 29 Jan 2020 09:06:54 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 789ED20708 for ; Wed, 29 Jan 2020 09:06:54 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="na0yHElP" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 789ED20708 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 12A3B6B0007; Wed, 29 Jan 2020 04:06:54 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 0DBA46B0008; Wed, 29 Jan 2020 04:06:54 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F31476B000A; Wed, 29 Jan 2020 04:06:53 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0044.hostedemail.com [216.40.44.44]) by kanga.kvack.org (Postfix) with ESMTP id DE9EA6B0007 for ; Wed, 29 Jan 2020 04:06:53 -0500 (EST) Received: from smtpin27.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 96C4687C9 for ; Wed, 29 Jan 2020 09:06:53 +0000 (UTC) X-FDA: 76430091906.27.jump05_7652bf52eff37 X-HE-Tag: jump05_7652bf52eff37 X-Filterd-Recvd-Size: 7695 Received: from mail-oi1-f193.google.com (mail-oi1-f193.google.com [209.85.167.193]) by imf50.hostedemail.com (Postfix) with ESMTP for ; Wed, 29 Jan 2020 09:06:52 +0000 (UTC) Received: by mail-oi1-f193.google.com with SMTP id p125so13250558oif.10 for ; Wed, 29 Jan 2020 01:06:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=t2s6x20EEWG92600QNdFV4esykKiTJqORk9G9g0F7T8=; b=na0yHElPJgb8ZKTDyya2uNiDGIJ+JxF+qDGOrdj03iQI5D/HgjkyxO2oMTJR+GaHZV FN5f3LwUUXQj4q/8MRhZwRHC6Hq+FlnzM8IQ+/LxLiUF25E3PA8AwkXB6uFCE8uLbxUW i6JBkHhySsci7kLCUKlI4rmsWuMIlCqAvR7WMseWdw4lJShZ2f+xcV6f88OTGviPuV9y GgUXrUe13YEGDZliycAuhs6uw9mTjQq/aDen6gLTAr9WJfpxHycbodZElceAoQugypuP DnAB4j3JpB1gUtAlf/qBWLoSVsskkEec3DkNC2mdH3vMUWKcCPwxA/TgccaxtxEw/b+t DYag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=t2s6x20EEWG92600QNdFV4esykKiTJqORk9G9g0F7T8=; b=qjqqC5IIKiKKpbOnr0gS19rEv0yi0itoNHoYmerKf1gxUfwkEyI6AIJ05VP5pDtKAr pdTJ0KWRuuWVaunxxQN8jnjbqqlB9Shj65ku0m6dFNPg5Gn7tVk42da6mgXWDCFiCtEI OgeCBPZc1uP653UzrAXrgWXKN746M4usa9PcxhkMVnzfjcL7pD+GaO48tgjxD5vkjbGq PqqwkT7EftqxYD0Dkk/jrKbYlyo29icXXrtej6B/ZcKk/9BdiMPSWEpzhtlhHVoAksm6 ZPooOMqeW+n6OHmBBfs6M04LRCe48lil7n3pwOfb6bU7JdOAAzReq3uzB9N9C+IFQLFO oYhA== X-Gm-Message-State: APjAAAUr/xEjebPMw4PwsWo9i6/h9S+tIY4G2mVjQBxD93VhoyuOc+iX J0iJtXnCtICR+wbjxchapqHMfh5fYHosbjjC+/LlvQ== X-Google-Smtp-Source: APXvYqwpzNHXG7UZJVurZ6ZnBKo2tCEq58PM1Lk3Ya7fuzeuWmvDzy1wRuCPRWutEkSn9Cjd0ohlbPdxDhaTqokQ5O8= X-Received: by 2002:aca:d4c1:: with SMTP id l184mr5832340oig.172.1580288811877; Wed, 29 Jan 2020 01:06:51 -0800 (PST) MIME-Version: 1.0 References: <20200129042019.3632-1-cai@lca.pw> <20200129085124.GF24244@dhcp22.suse.cz> In-Reply-To: <20200129085124.GF24244@dhcp22.suse.cz> From: Marco Elver Date: Wed, 29 Jan 2020 10:06:40 +0100 Message-ID: Subject: Re: [PATCH -next] mm/page_counter: mark intentional data races To: Michal Hocko Cc: Qian Cai , Andrew Morton , Johannes Weiner , Linux Memory Management List , LKML Content-Type: text/plain; charset="UTF-8" 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: On Wed, 29 Jan 2020 at 09:51, Michal Hocko wrote: > > On Tue 28-01-20 23:20:19, Qian Cai wrote: > > The commit 3e32cb2e0a12 ("mm: memcontrol: lockless page counters") > > had memcg->memsw->failcnt and ->watermark could be accessed concurrently > > as reported by KCSAN, > > > > Reported by Kernel Concurrency Sanitizer on: > > BUG: KCSAN: data-race in page_counter_try_charge / page_counter_try_charge > > > > read to 0xffff8fb18c4cd190 of 8 bytes by task 1081 on cpu 59: > > page_counter_try_charge+0x4d/0x150 mm/page_counter.c:138 > > try_charge+0x131/0xd50 Why are the line numbers for the remaining symbols missing? Doesn't scripts/decode_stacktrace.sh give you all line numbers? [ As an aside: if you want to use what syzbot uses to put line numbers on symbols, which is a bit faster: https://github.com/google/syzkaller/tree/master/tools/syz-symbolize https://github.com/google/syzkaller/blob/master/docs/linux/setup.md then 'go build tools/syz-symbolize'. ] > > __memcg_kmem_charge_memcg+0x58/0x140 > > __memcg_kmem_charge+0xcc/0x280 > > __alloc_pages_nodemask+0x1e1/0x450 > > alloc_pages_current+0xa6/0x120 > > pte_alloc_one+0x17/0xd0 > > __pte_alloc+0x3a/0x1f0 > > copy_p4d_range+0xc36/0x1990 > > copy_page_range+0x21d/0x360 > > dup_mmap+0x5f5/0x7a0 > > dup_mm+0xa2/0x240 > > copy_process+0x1b3f/0x3460 > > _do_fork+0xaa/0xa20 > > __x64_sys_clone+0x13b/0x170 > > do_syscall_64+0x91/0xb47 > > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > > > write to 0xffff8fb18c4cd190 of 8 bytes by task 1153 on cpu 120: > > page_counter_try_charge+0x5b/0x150 mm/page_counter.c:139 > > try_charge+0x131/0xd50 > > mem_cgroup_try_charge+0x159/0x460 > > mem_cgroup_try_charge_delay+0x3d/0xa0 > > wp_page_copy+0x14d/0x930 > > do_wp_page+0x107/0x7b0 > > __handle_mm_fault+0xce6/0xd40 > > handle_mm_fault+0xfc/0x2f0 > > do_page_fault+0x263/0x6f9 > > page_fault+0x34/0x40 > > > > Since the failcnt and watermark are tolerant of some inaccuracy, a data > > race will not be harmful, thus mark them as intentional data races with > > the data_race() macro. > > I am not familiar with KCSAN and git grep for data_race on the current > linux-next doesn't really show any users of this macro. Is there a > general consensus that data_race is going to be used to silence all > KCSAN false positives? It was discussed here: https://lore.kernel.org/linux-fsdevel/CAHk-=wg5CkOEF8DTez1Qu0XTEFw_oHhxN98bDnFqbY7HL5AB2g@mail.gmail.com/ If they're intentional data races that should remain, data_race() is one option. There are 4 options (other than address the data race) to deal with 'false positives': https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/dev-tools/kcsan.rst#n101 That being said, every use of data_race() needs to be justified, and not just applied without understanding the issue. See below. > > Signed-off-by: Qian Cai > > --- > > mm/page_counter.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/mm/page_counter.c b/mm/page_counter.c > > index de31470655f6..13934636eafd 100644 > > --- a/mm/page_counter.c > > +++ b/mm/page_counter.c > > @@ -82,8 +82,8 @@ void page_counter_charge(struct page_counter *counter, unsigned long nr_pages) > > * This is indeed racy, but we can live with some > > * inaccuracy in the watermark. > > */ > > - if (new > c->watermark) > > - c->watermark = new; > > + if (data_race(new > c->watermark)) > > + data_race(c->watermark = new); These should be using 'READ_ONCE' and 'WRITE_ONCE' for c->watermark. Store or load tearing would change the logic here, since the comparison might see garbage. > > } > > } > > > > @@ -126,7 +126,7 @@ bool page_counter_try_charge(struct page_counter *counter, > > * This is racy, but we can live with some > > * inaccuracy in the failcnt. > > */ > > - c->failcnt++; > > + data_race(c->failcnt++); This is probably fine. > > *fail = c; > > goto failed; > > } > > @@ -135,8 +135,8 @@ bool page_counter_try_charge(struct page_counter *counter, > > * Just like with failcnt, we can live with some > > * inaccuracy in the watermark. > > */ > > - if (new > c->watermark) > > - c->watermark = new; > > + if (data_race(new > c->watermark)) > > + data_race(c->watermark = new); This should be READ_ONCE / WRITE_ONCE. > > } > > return true; > > > > -- > > 2.21.0 (Apple Git-122.2) > > -- > Michal Hocko > SUSE Labs