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 80F20C77B73 for ; Tue, 2 May 2023 20:22:08 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D99146B0071; Tue, 2 May 2023 16:22:07 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D49AB6B0072; Tue, 2 May 2023 16:22:07 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C396A6B0074; Tue, 2 May 2023 16:22:07 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from mail-ed1-f41.google.com (mail-ed1-f41.google.com [209.85.208.41]) by kanga.kvack.org (Postfix) with ESMTP id 745606B0071 for ; Tue, 2 May 2023 16:22:07 -0400 (EDT) Received: by mail-ed1-f41.google.com with SMTP id 4fb4d7f45d1cf-50bd87539c2so1082472a12.0 for ; Tue, 02 May 2023 13:22:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1683058927; x=1685650927; 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=/q9Cv2MQT9eLyrPuhttAiOoNP+QHKlxs2pHWJJ60P9E=; b=PWA1P6JirIfHR9m+KmOxnNtkoYYR/kRT6PuZd6baEG63ngeiMk9acCtR1afWJEBCgc WAZttNKQ9cfB7mClkSe3gzkuHU2pYtLTZGk47QHa0dW0b2Q0Ag2PIf8BVAvhD9MOnSD0 el+3Erqa2j++W0vb6mnWDJITa0deywZjc95Mf7J9Q4BVU3d+IWE0VGF+akgvZPXpmaVS wRZFkeQ+SdqLZRIrfYy4YJpAywYogcMmrn2YzxcF9RLcK2HJSt0ZpsnF6WfNJLOhg7Li PWAvEcDqZZx2mLjrgdd0zS8/e7ilPVc5fhM/EgmeGPHYl6K5jTHE7VIg+ZcRgxS4TaZz FB3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683058927; x=1685650927; 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=/q9Cv2MQT9eLyrPuhttAiOoNP+QHKlxs2pHWJJ60P9E=; b=EEd0Ae9c3X9FIFHZg2tjiB7a0b3NA3OJDLt3M0Hdz4oiDDrnI4mQV3Ik8zNfrjtNUf jigc1rNMcPqloBS1LrIUvGJSus30pY0X3fTnxmV/nGZeJe2niD6EAhyENBAzJLrcRLX8 Po2wxHqTZlLOhCkEo+AHj+e+ZcDvPIbQnHkON4wmfIHHxpIocDjpqogzRfeVYuF8fwgS vsV01NPv80ooGE9sKz1PvTl1aLTrIWuCxj25uCGzYVUVsb3vp2UyVpgTY0AYGX4igrq8 cQxYSOsgqbe+DkuzAzvc/78BgPrsqgyJghLOk2G8qCwm+otPSBcnNx2RvFa/MmxS4qN8 0a9w== X-Gm-Message-State: AC+VfDxVJhNXYHNTdkqhuAJLphwRaNA2wcli7U6mF44lDGR83WzTBs4I jfusbvXaE6PLPPecsIj2FtresWR15bqyUeSzFrtG/A== X-Google-Smtp-Source: ACHHUZ6qmBMJ6SnM761gz0lfoCSbCZ8NVMbggUmrQAgm1Bi1iRN+BZTT3lvz+7XZkKaxm2uSMlTuEfWp9+YLV2XzEKE= X-Received: by 2002:a50:fe94:0:b0:506:af22:1271 with SMTP id d20-20020a50fe94000000b00506af221271mr9313716edt.0.1683058926595; Tue, 02 May 2023 13:22:06 -0700 (PDT) MIME-Version: 1.0 References: <20230502160839.361544-1-roman.gushchin@linux.dev> <20230502160839.361544-2-roman.gushchin@linux.dev> In-Reply-To: <20230502160839.361544-2-roman.gushchin@linux.dev> From: Yosry Ahmed Date: Tue, 2 May 2023 13:21:30 -0700 Message-ID: Subject: Re: [PATCH v2 2/2] mm: memcg: use READ_ONCE()/WRITE_ONCE() to access stock->cached To: Roman Gushchin Cc: linux-mm@kvack.org, Andrew Morton , Johannes Weiner , Michal Hocko , Shakeel Butt , Muchun Song , linux-kernel@vger.kernel.org, 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: > > A memcg pointer in the percpu stock can be accessed by drain_all_stock() > from another cpu in a lockless way. > In theory it might lead to an issue, similar to the one which has been > discovered with stock->cached_objcg, where the pointer was zeroed > between the check for being NULL and dereferencing. > In this case the issue is unlikely a real problem, but to make it > bulletproof and similar to stock->cached_objcg, let's annotate all > accesses to stock->cached with READ_ONCE()/WTRITE_ONCE(). Is it time to rename that to cached_memcg? :) Anyway, same comment as patch 1 about annotating all reads with READ_ONCE() vs. singling out the racy read. > > Signed-off-by: Roman Gushchin > Cc: Dmitry Vyukov > Cc: Yosry Ahmed > Cc: Shakeel Butt > --- > mm/memcontrol.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index c823c35c2ed4..1e364ad495a3 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2275,7 +2275,7 @@ static bool consume_stock(struct mem_cgroup *memcg,= unsigned int nr_pages) > local_lock_irqsave(&memcg_stock.stock_lock, flags); > > stock =3D this_cpu_ptr(&memcg_stock); > - if (memcg =3D=3D stock->cached && stock->nr_pages >=3D nr_pages) = { > + if (memcg =3D=3D READ_ONCE(stock->cached) && stock->nr_pages >=3D= nr_pages) { > stock->nr_pages -=3D nr_pages; > ret =3D true; > } > @@ -2290,7 +2290,7 @@ static bool consume_stock(struct mem_cgroup *memcg,= unsigned int nr_pages) > */ > static void drain_stock(struct memcg_stock_pcp *stock) > { > - struct mem_cgroup *old =3D stock->cached; > + struct mem_cgroup *old =3D READ_ONCE(stock->cached); > > if (!old) > return; > @@ -2303,7 +2303,7 @@ static void drain_stock(struct memcg_stock_pcp *sto= ck) > } > > css_put(&old->css); > - stock->cached =3D NULL; > + WRITE_ONCE(stock->cached, NULL); Is it me or can we call drain_stock() from memcg_hotplug_cpu_dead() without holding the lock, unlike all other callers. Is this a problem? > } > > static void drain_local_stock(struct work_struct *dummy) > @@ -2338,10 +2338,10 @@ static void __refill_stock(struct mem_cgroup *mem= cg, unsigned int nr_pages) > struct memcg_stock_pcp *stock; > > stock =3D this_cpu_ptr(&memcg_stock); > - if (stock->cached !=3D memcg) { /* reset if necessary */ > + if (READ_ONCE(stock->cached) !=3D memcg) { /* reset if necessary = */ > drain_stock(stock); > css_get(&memcg->css); > - stock->cached =3D memcg; > + WRITE_ONCE(stock->cached, memcg); > } > stock->nr_pages +=3D nr_pages; > > @@ -2383,7 +2383,7 @@ static void drain_all_stock(struct mem_cgroup *root= _memcg) > bool flush =3D false; > > rcu_read_lock(); > - memcg =3D stock->cached; > + memcg =3D READ_ONCE(stock->cached); > if (memcg && stock->nr_pages && > mem_cgroup_is_descendant(memcg, root_memcg)) > flush =3D true; > -- > 2.40.1 >