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 554A6C00140 for ; Fri, 12 Aug 2022 05:34:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A318D8E0002; Fri, 12 Aug 2022 01:34:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9B8D28E0001; Fri, 12 Aug 2022 01:34:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8328F8E0002; Fri, 12 Aug 2022 01:34:53 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 6E7328E0001 for ; Fri, 12 Aug 2022 01:34:53 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 3AD811C74F8 for ; Fri, 12 Aug 2022 05:34:53 +0000 (UTC) X-FDA: 79789826466.27.0AC03E6 Received: from out2.migadu.com (out2.migadu.com [188.165.223.204]) by imf28.hostedemail.com (Postfix) with ESMTP id 749E3C0171 for ; Fri, 12 Aug 2022 05:34:52 +0000 (UTC) Content-Type: text/plain; charset=utf-8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1660282490; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eT9oB/Jxs0HX/eo2S3EGE6XjZBx5ygu2ZiQitesEUO4=; b=w2pIQGWCS1/EKSQZpsMmnNHdguMKuLCCLx81UoSZ1cJIns5aJlRW/klnxQxiXEXBqXDzj1 /I1800EVWMbwRGR+k99rKu6F5nZla3D0AmNRp7u7RrQnx4XQIv2OWT8dzbMuEXRDZOd6di B6IqiwAAkFlhCc3zqmARwFxJq42GCM0= MIME-Version: 1.0 Subject: Re: [PATCH bpf-next 05/15] bpf: Fix incorrect mem_cgroup_put X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Muchun Song In-Reply-To: Date: Fri, 12 Aug 2022 13:33:54 +0800 Cc: Shakeel Butt , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Martin Lau , Song Liu , Yonghong Song , john fastabend , KP Singh , Stanislav Fomichev , Hao Luo , jolsa@kernel.org, Johannes Weiner , Michal Hocko , Roman Gushchin , Muchun Song , Andrew Morton , netdev , bpf , Linux MM Content-Transfer-Encoding: quoted-printable Message-Id: <870C70CA-C760-40A5-8A04-F0962EFDF507@linux.dev> References: <20220810151840.16394-1-laoar.shao@gmail.com> <20220810151840.16394-6-laoar.shao@gmail.com> <20220810170706.ikyrsuzupjwt65h7@google.com> <20220811154731.3smhom6v4qy2u6rd@google.com> To: Yafang Shao X-Migadu-Flow: FLOW_OUT X-Migadu-Auth-User: linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1660282492; a=rsa-sha256; cv=none; b=N0y3zZuQHhrIjMh05Bje+XJh688IsigGhD4p3u6e+HqBLsSRXy94T/RESFnapb1aFKY4H2 B7iKSMqQ7hzlVE3XI2VvAw/FS2h0mcmAuSiCWxy4HoQS2E4s7lQWCB14WFfRIy14oVYmDg DvxjjByHReZrVPaJTqzabMK5LHqlcaA= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=w2pIQGWC; spf=pass (imf28.hostedemail.com: domain of muchun.song@linux.dev designates 188.165.223.204 as permitted sender) smtp.mailfrom=muchun.song@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1660282492; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=eT9oB/Jxs0HX/eo2S3EGE6XjZBx5ygu2ZiQitesEUO4=; b=JIko097FcO34XWjEcshcQ8YGbnjFq+3bScc9CR+Gy9KJZFmcyTNoDHbtITvDHm9Ayw9jOP 6iBsSoX1EbddHQGzveXXcqkrF+ZU06OoO0s9HSPm/XVvedAhYaknCBa6ZvTMn49mYiIEso pY0JSTnZUJvWsChVFKuo+i/K4Sxu33I= X-Stat-Signature: dsobzzqeuyk4ue49fcioawstiumifhks X-Rspamd-Queue-Id: 749E3C0171 X-Rspam-User: X-Rspamd-Server: rspam03 Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=w2pIQGWC; spf=pass (imf28.hostedemail.com: domain of muchun.song@linux.dev designates 188.165.223.204 as permitted sender) smtp.mailfrom=muchun.song@linux.dev; dmarc=pass (policy=none) header.from=linux.dev X-HE-Tag: 1660282492-677824 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 Aug 12, 2022, at 08:27, Yafang Shao wrote: >=20 > On Thu, Aug 11, 2022 at 11:47 PM Shakeel Butt = wrote: >>=20 >> On Thu, Aug 11, 2022 at 10:49:13AM +0800, Yafang Shao wrote: >>> On Thu, Aug 11, 2022 at 1:07 AM Shakeel Butt = wrote: >>>>=20 >>>> On Wed, Aug 10, 2022 at 03:18:30PM +0000, Yafang Shao wrote: >>>>> The memcg may be the root_mem_cgroup, in which case we shouldn't = put it. >>>>=20 >>>> No, it is ok to put root_mem_cgroup. css_put already handles the = root >>>> cgroups. >>>>=20 >>>=20 >>> Ah, this commit log doesn't describe the issue clearly. I should = improve it. >>> The issue is that in bpf_map_get_memcg() it doesn't get the objcg if >>> map->objcg is NULL (that can happen if the map belongs to the root >>> memcg), so we shouldn't put the objcg if map->objcg is NULL neither = in >>> bpf_map_put_memcg(). >>=20 >> Sorry I am still not understanding. We are not 'getting' objcg in >> bpf_map_get_memcg(). We are 'getting' memcg from map->objcg and if = that >> is NULL the function is returning root memcg and putting root memcg = is >> totally fine. >=20 > When the map belongs to root_mem_cgroup, the map->objcg is NULL, right = ? > See also bpf_map_save_memcg() and it describes clearly in the comment = - >=20 > static void bpf_map_save_memcg(struct bpf_map *map) > { > /* Currently if a map is created by a process belonging to the = root > * memory cgroup, get_obj_cgroup_from_current() will return = NULL. > * So we have to check map->objcg for being NULL each time it's > * being used. > */ > map->objcg =3D get_obj_cgroup_from_current(); > } >=20 > So for the root_mem_cgroup case, bpf_map_get_memcg() will return > root_mem_cgroup directly without getting its css, right ? See below, >=20 > static struct mem_cgroup *bpf_map_get_memcg(const struct bpf_map *map) > { >=20 > if (map->objcg) > return get_mem_cgroup_from_objcg(map->objcg); >=20 > return root_mem_cgroup; // without css_get(&memcg->css); > } >=20 > But it will put the css unconditionally. See below, >=20 > memcg =3D bpf_map_get_memcg(map); > ... > mem_cgroup_put(memcg); >=20 > So we should put it *conditionally* as well. Hi, No. We could put root_mem_cgroup unconditionally since the root css is treated as no reference css. See css_put(). static inline void css_put(struct cgroup_subsys_state *css) { // The root memcg=E2=80=99s css has been set with CSS_NO_REF. if (!(css->flags & CSS_NO_REF)) percpu_ref_put(&css->refcnt); } Muchun, Thanks. >=20 > memcg =3D bpf_map_get_memcg(map); > ... > + if (map->objcg) > mem_cgroup_put(memcg); >=20 > Is it clear to you ? >=20 >> Or are you saying that root_mem_cgroup is NULL? >>=20 >=20 > No >=20 > --=20 > Regards > Yafang