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 9D9C4C7EE26 for ; Tue, 2 May 2023 20:15:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 02E846B0071; Tue, 2 May 2023 16:15:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id EF9B46B0072; Tue, 2 May 2023 16:15:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D9B076B0074; Tue, 2 May 2023 16:15:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) by kanga.kvack.org (Postfix) with ESMTP id 810B56B0071 for ; Tue, 2 May 2023 16:15:39 -0400 (EDT) Received: by mail-ed1-f54.google.com with SMTP id 4fb4d7f45d1cf-50bc4b88998so5507689a12.3 for ; Tue, 02 May 2023 13:15:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1683058539; x=1685650539; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=F9QHtsPGpUiOO8P09+xIa3RCAMRlLX2P/2MPTz6R0SU=; b=W4MFXuLt4RZ5jIx3f+5AedVSYTsRoE4ZfFWepQfQaUFPEyKcIArk9quArn+eaQZK78 qScaPeEci5bNT7Iz0qpFcFce81vbEouaxh9uHUu6cdLUikeemOoc21OfNQsS3KIfw2te gAGQkspHf85vLxZghagW1gLkyCC/iJZSzMJEGlb6HnzOwijlPADsZck8XZ5cxw+BPINm 2GLpTXfWFoe35EkzWdFgWPAfGNhPgr259nrAvnw+nHBbQzF2d6NfTYqbEqSc0YahB0G2 kDH6zXEtZEAlYgyMT6qhebHD7QeY+NwVaUy3Uh48j2bQkEsQKXj/sScB03JP2MnFX4Ib I3Xg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683058539; x=1685650539; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=F9QHtsPGpUiOO8P09+xIa3RCAMRlLX2P/2MPTz6R0SU=; b=mHlrAFkxpaV2xjSt+b35IPSvrNlWCY90H3o5R/yNIIRbVEY472Rz1Y7aXCd4HKAY7b oGocTe2CtonBRFbOOdlecfX6AXxIpKv7/nCcagXAysLlBGKWk3rtod6MxVegz42iMKmC 7OsO3C7BVJOJMUvi5o4GUapaDOAb7Biw86dWW6ynBXzmg5QAV44QeUNGAsfNoF2oNf1I P3oGCQmLnEaZxl9R7Uu/jJzzlB3q3G7spBhI1IpVEds53CTjKKgZXOYFgZLewPMBOEnK c+CUh8lnFHDJIpccC/t/8aoH7BYUQRItUMWiuWmDSCxhK50ricpkkCgmFjdYo4Tl5ouQ 4I6A== X-Gm-Message-State: AC+VfDzQRQL4SFfrCAQJbeZRww//cVgYYDM6Cby5RXS6YVNxfF57rYmj Mgrf4zbrsVQJ502nVKs6ETGPd7XVXhx26e6HL3H+Nw== X-Google-Smtp-Source: ACHHUZ5SfrXf9BlUW1xsyqZ7gbDNKA9OjIbTRJxbPVHgNZQd3U5wwyx/cvb95Ct49JxzqUlMiiizBkfAxcpIkwglavk= X-Received: by 2002:a50:e604:0:b0:4ac:b687:f57e with SMTP id y4-20020a50e604000000b004acb687f57emr9358595edm.1.1683058538647; Tue, 02 May 2023 13:15:38 -0700 (PDT) MIME-Version: 1.0 References: <20230502160839.361544-1-roman.gushchin@linux.dev> In-Reply-To: <20230502160839.361544-1-roman.gushchin@linux.dev> From: Yosry Ahmed Date: Tue, 2 May 2023 13:15:02 -0700 Message-ID: Subject: Re: [PATCH v2 1/2] mm: kmem: fix a NULL pointer dereference in obj_stock_flush_required() To: Roman Gushchin Cc: linux-mm@kvack.org, Andrew Morton , Johannes Weiner , Michal Hocko , Shakeel Butt , Muchun Song , linux-kernel@vger.kernel.org, syzbot+774c29891415ab0fd29d@syzkaller.appspotmail.com, Dmitry Vyukov Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Tue, May 2, 2023 at 9:09=E2=80=AFAM Roman Gushchin wrote: > > KCSAN found an issue in obj_stock_flush_required(): > stock->cached_objcg can be reset between the check and dereference: > > =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 drain_all_stock / drain_obj_stock > > write to 0xffff888237c2a2f8 of 8 bytes by task 19625 on cpu 0: > drain_obj_stock+0x408/0x4e0 mm/memcontrol.c:3306 > refill_obj_stock+0x9c/0x1e0 mm/memcontrol.c:3340 > obj_cgroup_uncharge+0xe/0x10 mm/memcontrol.c:3408 > memcg_slab_free_hook mm/slab.h:587 [inline] > __cache_free mm/slab.c:3373 [inline] > __do_kmem_cache_free mm/slab.c:3577 [inline] > kmem_cache_free+0x105/0x280 mm/slab.c:3602 > __d_free fs/dcache.c:298 [inline] > dentry_free fs/dcache.c:375 [inline] > __dentry_kill+0x422/0x4a0 fs/dcache.c:621 > dentry_kill+0x8d/0x1e0 > dput+0x118/0x1f0 fs/dcache.c:913 > __fput+0x3bf/0x570 fs/file_table.c:329 > ____fput+0x15/0x20 fs/file_table.c:349 > task_work_run+0x123/0x160 kernel/task_work.c:179 > resume_user_mode_work include/linux/resume_user_mode.h:49 [inline] > exit_to_user_mode_loop+0xcf/0xe0 kernel/entry/common.c:171 > exit_to_user_mode_prepare+0x6a/0xa0 kernel/entry/common.c:203 > __syscall_exit_to_user_mode_work kernel/entry/common.c:285 [inline] > syscall_exit_to_user_mode+0x26/0x140 kernel/entry/common.c:296 > do_syscall_64+0x4d/0xc0 arch/x86/entry/common.c:86 > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > read to 0xffff888237c2a2f8 of 8 bytes by task 19632 on cpu 1: > obj_stock_flush_required mm/memcontrol.c:3319 [inline] > drain_all_stock+0x174/0x2a0 mm/memcontrol.c:2361 > try_charge_memcg+0x6d0/0xd10 mm/memcontrol.c:2703 > try_charge mm/memcontrol.c:2837 [inline] > mem_cgroup_charge_skmem+0x51/0x140 mm/memcontrol.c:7290 > sock_reserve_memory+0xb1/0x390 net/core/sock.c:1025 > sk_setsockopt+0x800/0x1e70 net/core/sock.c:1525 > udp_lib_setsockopt+0x99/0x6c0 net/ipv4/udp.c:2692 > udp_setsockopt+0x73/0xa0 net/ipv4/udp.c:2817 > sock_common_setsockopt+0x61/0x70 net/core/sock.c:3668 > __sys_setsockopt+0x1c3/0x230 net/socket.c:2271 > __do_sys_setsockopt net/socket.c:2282 [inline] > __se_sys_setsockopt net/socket.c:2279 [inline] > __x64_sys_setsockopt+0x66/0x80 net/socket.c:2279 > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > value changed: 0xffff8881382d52c0 -> 0xffff888138893740 > > Reported by Kernel Concurrency Sanitizer on: > CPU: 1 PID: 19632 Comm: syz-executor.0 Not tainted 6.3.0-rc2-syzkaller-00= 387-g534293368afa #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS G= oogle 03/02/2023 > > Fix it by using READ_ONCE()/WRITE_ONCE() for all accesses to > stock->cached_objcg. I believe all read accesses other than obj_stock_flush_required() are done under the lock, so READ_ONCE() wouldn't be needed AFAICT. Having READ_ONCE() only around the racy read can be useful to document the racy read and differentiate it from others. With that said, it's also inconvenient to keep track moving forward of which reading sites are racy, and it may be simpler to just annotate all readers with READ_ONCE(). I am not sure which approach is better, just thinking out loud. > > Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") > Reported-by: syzbot+774c29891415ab0fd29d@syzkaller.appspotmail.com > Reported-by: Dmitry Vyukov > Link: > https://lore.kernel.org/linux-mm/CACT4Y+ZfucZhM60YPphWiCLJr6+SGFhT+jjm8k1= P-a_8Kkxsjg@mail.gmail.com/T/#t > Signed-off-by: Roman Gushchin With the above said, I don't feel strongly either way, the patch looks good AFAICT: Reviewed-by: Yosry Ahmed > Cc: Yosry Ahmed > Cc: Shakeel Butt > --- > mm/memcontrol.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 4b27e245a055..c823c35c2ed4 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -3208,12 +3208,12 @@ void mod_objcg_state(struct obj_cgroup *objcg, st= ruct pglist_data *pgdat, > * accumulating over a page of vmstat data or when pgdat or idx > * changes. > */ > - if (stock->cached_objcg !=3D objcg) { > + if (READ_ONCE(stock->cached_objcg) !=3D objcg) { > old =3D drain_obj_stock(stock); > obj_cgroup_get(objcg); > stock->nr_bytes =3D atomic_read(&objcg->nr_charged_bytes) > ? atomic_xchg(&objcg->nr_charged_bytes, 0= ) : 0; > - stock->cached_objcg =3D objcg; > + WRITE_ONCE(stock->cached_objcg, objcg); > stock->cached_pgdat =3D pgdat; > } else if (stock->cached_pgdat !=3D pgdat) { > /* Flush the existing cached vmstat data */ > @@ -3267,7 +3267,7 @@ static bool consume_obj_stock(struct obj_cgroup *ob= jcg, unsigned int nr_bytes) > local_lock_irqsave(&memcg_stock.stock_lock, flags); > > stock =3D this_cpu_ptr(&memcg_stock); > - if (objcg =3D=3D stock->cached_objcg && stock->nr_bytes >=3D nr_b= ytes) { > + if (objcg =3D=3D READ_ONCE(stock->cached_objcg) && stock->nr_byte= s >=3D nr_bytes) { > stock->nr_bytes -=3D nr_bytes; > ret =3D true; > } > @@ -3279,7 +3279,7 @@ static bool consume_obj_stock(struct obj_cgroup *ob= jcg, unsigned int nr_bytes) > > static struct obj_cgroup *drain_obj_stock(struct memcg_stock_pcp *stock) > { > - struct obj_cgroup *old =3D stock->cached_objcg; > + struct obj_cgroup *old =3D READ_ONCE(stock->cached_objcg); > > if (!old) > return NULL; > @@ -3332,7 +3332,7 @@ static struct obj_cgroup *drain_obj_stock(struct me= mcg_stock_pcp *stock) > stock->cached_pgdat =3D NULL; > } > > - stock->cached_objcg =3D NULL; > + WRITE_ONCE(stock->cached_objcg, NULL); > /* > * The `old' objects needs to be released by the caller via > * obj_cgroup_put() outside of memcg_stock_pcp::stock_lock. > @@ -3343,10 +3343,11 @@ static struct obj_cgroup *drain_obj_stock(struct = memcg_stock_pcp *stock) > static bool obj_stock_flush_required(struct memcg_stock_pcp *stock, > struct mem_cgroup *root_memcg) > { > + struct obj_cgroup *objcg =3D READ_ONCE(stock->cached_objcg); > struct mem_cgroup *memcg; > > - if (stock->cached_objcg) { > - memcg =3D obj_cgroup_memcg(stock->cached_objcg); > + if (objcg) { > + memcg =3D obj_cgroup_memcg(objcg); > if (memcg && mem_cgroup_is_descendant(memcg, root_memcg)) > return true; > } > @@ -3365,10 +3366,10 @@ static void refill_obj_stock(struct obj_cgroup *o= bjcg, unsigned int nr_bytes, > local_lock_irqsave(&memcg_stock.stock_lock, flags); > > stock =3D this_cpu_ptr(&memcg_stock); > - if (stock->cached_objcg !=3D objcg) { /* reset if necessary */ > + if (READ_ONCE(stock->cached_objcg) !=3D objcg) { /* reset if nece= ssary */ > old =3D drain_obj_stock(stock); > obj_cgroup_get(objcg); > - stock->cached_objcg =3D objcg; > + WRITE_ONCE(stock->cached_objcg, objcg); > stock->nr_bytes =3D atomic_read(&objcg->nr_charged_bytes) > ? atomic_xchg(&objcg->nr_charged_bytes, 0= ) : 0; > allow_uncharge =3D true; /* Allow uncharge when objcg ch= anges */ > -- > 2.40.1 >