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 1DB5FC00140 for ; Fri, 12 Aug 2022 11:26:27 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 759556B0073; Fri, 12 Aug 2022 07:26:27 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 709216B0075; Fri, 12 Aug 2022 07:26:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5D08B8E0001; Fri, 12 Aug 2022 07:26:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 4B28A6B0073 for ; Fri, 12 Aug 2022 07:26:27 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 1DAE8C1BB6 for ; Fri, 12 Aug 2022 11:26:27 +0000 (UTC) X-FDA: 79790712414.10.F131203 Received: from mail-vs1-f53.google.com (mail-vs1-f53.google.com [209.85.217.53]) by imf30.hostedemail.com (Postfix) with ESMTP id 901FD80186 for ; Fri, 12 Aug 2022 11:26:26 +0000 (UTC) Received: by mail-vs1-f53.google.com with SMTP id q15so522035vsr.0 for ; Fri, 12 Aug 2022 04:26:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc; bh=l6ANe7OLSs2lP132priVK+trTtZprCMMHdnLexTkcYs=; b=cbulXJ2X1GM7VeFpFsNH4B5cF+6IIkVya1vFuJ1ZP88Rpim+pn+2xDldVmms9y4fnX PWr8qVMeW00lQq74DTp179OZ52B2HCIVjFWWVg3wQNqQstxvw4HOUIcdtqTsulyCxjtI VY14oyLtqSuMSLjdQ9F8RFYEkYdBuUEKArObuz6kt2IF1j3CF8LxppqcQJqvtzZ3fWYD kKpkc1enVd3Um1F3YBCVq5l9aSuXuAwC1xvOrrVbaA6nooM6VT8oCCOT0TUfa0J1UsAh Es3fM1Z1ZESpa2rrGQn5oz/yfkgJNUWRT7VeJXA2iefTYdshDjvzhMaS89aVCqHJdLKq zXXg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc; bh=l6ANe7OLSs2lP132priVK+trTtZprCMMHdnLexTkcYs=; b=TsU8rfAz8NO2VH0xjeo3tKj7fZKdPThQ2doFXPRgz402HEn0fwxPmx6ynz8HrU6zNb izW7cNRg4MZGluhAf6auJpCfPYLerhujohd4B66zL1IcyW75BOv757ZfUgTc0TW0HQiF +qErqpdeWYE1lGIMUx/K1QQbxMG5zJ4I29IyQAZJES2BKAu6qwu4a7vJtpP0X/TZZUXw 6M8xfeX1//urBBzBox49+kinCKI7mhc3ila44Q8D0JcNUqD1FY/E+5ncY8cRfpYc2yY+ 4gYNL/XDShoCIcTfeT/bq4Lh68fE8h3l/I/YAR5Y9mTo9gprZ1VMO61HwK2t4t7z9l72 gfLA== X-Gm-Message-State: ACgBeo3DNI1fRDYPXQFR1IvzggNXik0yP0Mdd+XlEqHbBN0ZsBECx6IS DFSpU1f4odiOWiw5V05RNySRjND5/tVjCxRSfvw= X-Google-Smtp-Source: AA6agR7eRhzJ5qbC7QMZSEHPwBxL98qPwKICiNP035okRK1tdFf5nl4wb5ho1LLXA7DjnWx2GN3yTrW4DRJHQzCGmO4= X-Received: by 2002:a05:6102:750:b0:381:feef:d966 with SMTP id v16-20020a056102075000b00381feefd966mr1528054vsg.35.1660303584334; Fri, 12 Aug 2022 04:26:24 -0700 (PDT) MIME-Version: 1.0 References: <20220810151840.16394-1-laoar.shao@gmail.com> <20220810151840.16394-6-laoar.shao@gmail.com> <20220810170706.ikyrsuzupjwt65h7@google.com> <20220811154731.3smhom6v4qy2u6rd@google.com> <870C70CA-C760-40A5-8A04-F0962EFDF507@linux.dev> In-Reply-To: <870C70CA-C760-40A5-8A04-F0962EFDF507@linux.dev> From: Yafang Shao Date: Fri, 12 Aug 2022 19:25:46 +0800 Message-ID: Subject: Re: [PATCH bpf-next 05/15] bpf: Fix incorrect mem_cgroup_put To: Muchun Song 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-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1660303586; a=rsa-sha256; cv=none; b=cgWNgfCYu7XxzwyJh5C1cJC9eRiQJPd7CYouOe4ksGWW2ytMv1bVJ+gm5SmH1qFSimjJto iqaNzaPIHYHL0Iv5fSlHbrOkXMYQbmbSfPFKiUN+BNNWlAG4t9GsbPLnXSEAEyH+Us1fI4 FCY6G4YXACL8HI9ki+gkszra2buqBKw= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=cbulXJ2X; spf=pass (imf30.hostedemail.com: domain of laoar.shao@gmail.com designates 209.85.217.53 as permitted sender) smtp.mailfrom=laoar.shao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1660303586; 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=l6ANe7OLSs2lP132priVK+trTtZprCMMHdnLexTkcYs=; b=Q5Qd1KpDOVqt5v20laCOTxiPocDetni/dzN5bcNDrAIRkszt2rqoLLaedmmqQX8MHTkTUc YowXR3tB7smCimLyW3a/lmkn5RjhFQjU0AUr2hPtYv2CaadOUqWjDAvrxCjk2pgw87otO4 jKD5gGlHwot7EeuTOJpnPpZ4PDkcG8U= X-Rspamd-Queue-Id: 901FD80186 Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=cbulXJ2X; spf=pass (imf30.hostedemail.com: domain of laoar.shao@gmail.com designates 209.85.217.53 as permitted sender) smtp.mailfrom=laoar.shao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Rspam-User: X-Rspamd-Server: rspam12 X-Stat-Signature: tngqfgrgrn67zuwidh3azs5ppfpwh8ig X-HE-Tag: 1660303586-727034 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 Fri, Aug 12, 2022 at 1:34 PM Muchun Song wrote: > > > > > On Aug 12, 2022, at 08:27, Yafang Shao wrote: > > > > On Thu, Aug 11, 2022 at 11:47 PM Shakeel Butt wro= te: > >> > >> On Thu, Aug 11, 2022 at 10:49:13AM +0800, Yafang Shao wrote: > >>> On Thu, Aug 11, 2022 at 1:07 AM Shakeel Butt wr= ote: > >>>> > >>>> 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 pu= t it. > >>>> > >>>> No, it is ok to put root_mem_cgroup. css_put already handles the roo= t > >>>> cgroups. > >>>> > >>> > >>> Ah, this commit log doesn't describe the issue clearly. I should impr= ove 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 i= n > >>> bpf_map_put_memcg(). > >> > >> 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 tha= t > >> is NULL the function is returning root memcg and putting root memcg is > >> totally fine. > > > > 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 - > > > > static void bpf_map_save_memcg(struct bpf_map *map) > > { > > /* Currently if a map is created by a process belonging to the r= oot > > * 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(); > > } > > > > So for the root_mem_cgroup case, bpf_map_get_memcg() will return > > root_mem_cgroup directly without getting its css, right ? See below, > > > > static struct mem_cgroup *bpf_map_get_memcg(const struct bpf_map *map) > > { > > > > if (map->objcg) > > return get_mem_cgroup_from_objcg(map->objcg); > > > > return root_mem_cgroup; // without css_get(&memcg->css); > > } > > > > But it will put the css unconditionally. See below, > > > > memcg =3D bpf_map_get_memcg(map); > > ... > > mem_cgroup_put(memcg); > > > > 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); > } Indeed. I missed that. The call stack is so deep that I didn't look into it :( Thanks for the information. --=20 Regards Yafang