Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 11 Oct 2021 09:29:22 -0700
From:   Roman Gushchin <guro@fb.com>
To:     Michal =?iso-8859-1?Q?Koutn=FD?= <mkoutny@suse.com>
CC:     <quanyang.wang@windriver.com>, Tejun Heo <tj@kernel.org>,
        Li Zefan <lizefan@huawei.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Jens Axboe <axboe@kernel.dk>, Ming Lei <ming.lei@redhat.com>,
        <cgroups@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <netdev@vger.kernel.org>, <bpf@vger.kernel.org>
Subject: Re: [PATCH] cgroup: fix memory leak caused by missing
 cgroup_bpf_offline
Message-ID: <YWRmYk4hHhPf602i@carbon.dhcp.thefacebook.com>
References: <20211007121603.1484881-1-quanyang.wang@windriver.com>
 <20211011162128.GC61605@blackbody.suse.cz>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20211011162128.GC61605@blackbody.suse.cz>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?mvd3SZBT185Ai38aYGdApm8aiPTeJ1jDTNi/3Bib97LLN9kwAIDrkrT1um9L?=
 =?us-ascii?Q?GU+o5kogdqq7NHaH7nI7sqnCgQHzGOUAlkRz3Zmx2wgjloKHSWZx06JZ3oXP?=
 =?us-ascii?Q?fznWdGszxG/brKKP9FTuFNoq+ebDt8wZEo1EliL2pe8ZdQ28GXY8vlgZ03/5?=
 =?us-ascii?Q?GtedsNFdcVeZiGM0RrRo5gDQUNVzaoJ2gzPoTtMKfbbPZxvHlPIlnssUGWT7?=
 =?us-ascii?Q?GRSWZGwnU2Ef2y9B+sFhIAre8mQaBUWLkiex6pPmqhi01IcdfYjFs2Pm3q4s?=
 =?us-ascii?Q?K+b0GW2TTgngdoAm4QihaAWPIzxuB+W0djCsQyFJ+oNdbtHGzr7mc5FdTXGl?=
 =?us-ascii?Q?56jz2dRMZdKJINyrXa91AdN1DL+KJKE7CZrQaOhIptXWNAdEYDKIxIxKdoiv?=
 =?us-ascii?Q?b4NaNAzgv4VpjORxhNRfFsuPHLPzQzrUPB7vL9aqkNOmAPh4XEvNcwGV4C22?=
 =?us-ascii?Q?7QpKJf5BUddMz+wSRHfMyPsVBg7JnVzKJHyHV53q9LmZZ5EgpNQQ4cG1qbs8?=
 =?us-ascii?Q?xws0h6n4a1KIa75kpnXs7hy5S26JwMgMk6BgO1k7gzdnjzc1xcTQt8CUwCZo?=
 =?us-ascii?Q?7hwwovkRnSHgNbw9GqntQtg4pU+eFXiOQHd1dMkEqYTMK75V2+W1kT4bHeg4?=
 =?us-ascii?Q?TPh5VL6ychJFYTlh0zVtofc8TWPoTSJneAoS+mpm+fj2OKDhEK2eA7jaNDoR?=
 =?us-ascii?Q?pXvbbGQmdz4CTCirUgiv7tnRwMCYifhXgLLDA/zw03gTiMUPrvxCYXzPgKur?=
 =?us-ascii?Q?UbHu4Z8CK1qq2XhQT6RkFZoOOS8TJSnJUdl3V/TqAQaWDqyBRhnEsWTnv2Nx?=
 =?us-ascii?Q?qKRLzkllcVPeRkxRZp0UfjKpktbs9SwM+JzcODYPYC4JaWpjl98jOoyJo0sZ?=
 =?us-ascii?Q?lBFL7fwwwJrBtBFFeP/pRROINw37B7T3rrGqAXZy8v8LMhIx4yAJVuMcDVOS?=
 =?us-ascii?Q?zU9dTqj9XcPC49axT/7lyKkYgbyaiWjuezVd7ke7xiJZ8vbBN89tIUl6TT8A?=
 =?us-ascii?Q?VOystm9su+H+4n/3U25KYWssIH17LKjOyj4WL65JhfIT/ERCJwDGDEEMhzTT?=
 =?us-ascii?Q?5VydaMmgoVoloUhq94AQpcqQyf+vgFlW7T9S+DEJgwKyP7tY1OhV6gZ+qSrb?=
 =?us-ascii?Q?POyLwlYdsou8pIWt+MBFHC3GBRXrz/pMh7PuaFTTBS+e4OHBJJjDXTgXonxK?=
 =?us-ascii?Q?OXM/5CthR1dcRt9aJYWf0qfBF+w8To/oBSyVKBMTwLeK/qKTD+k7rRp5tjaw?=
 =?us-ascii?Q?RLnO/nTE/7K5mxQt3Z9QwqudHRGHAsTvg82CeOWnzoJ2JwBNYGiqVu0ujm6D?=
 =?us-ascii?Q?vO9YS26G4wCFg3H1KplVQZBirAROTFZupZq48Ii4wnRZWQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 95830cc2-15c1-4296-df51-08d98cd44ada
X-MS-Exchange-CrossTenant-AuthSource: BYAPR15MB4136.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Oct 2021 16:29:26.8188
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: hPceonMP9ZaS5EAUbKOp6cyafPXedJcOWOVFSs1vuuDpa4fGel9bkgzrpPTOc4LI
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR15MB4648
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: tFoJpr9sMfw-1e2EpNa-X1akoTLX73X2
X-Proofpoint-ORIG-GUID: tFoJpr9sMfw-1e2EpNa-X1akoTLX73X2
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.182.1,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.0.607.475
 definitions=2021-10-11_05,2021-10-11_01,2020-04-07_01
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 impostorscore=0
 mlxscore=0 adultscore=0 lowpriorityscore=0 malwarescore=0 suspectscore=0
 mlxlogscore=999 bulkscore=0 clxscore=1011 spamscore=0 phishscore=0
 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109230001 definitions=main-2110110097
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Oct 11, 2021 at 06:21:28PM +0200, Michal Koutny wrote:
> Hello.
> 
> On Thu, Oct 07, 2021 at 08:16:03PM +0800, quanyang.wang@windriver.com wrote:
> > This is because that root_cgrp->bpf.refcnt.data is allocated by the
> > function percpu_ref_init in cgroup_bpf_inherit which is called by
> > cgroup_setup_root when mounting, but not freed along with root_cgrp
> > when umounting.
> 
> Good catch!

+1

> 
> > Adding cgroup_bpf_offline which calls percpu_ref_kill to
> > cgroup_kill_sb can free root_cgrp->bpf.refcnt.data in umount path.
> 
> That is sensible.
> 
> > Fixes: 2b0d3d3e4fcfb ("percpu_ref: reduce memory footprint of percpu_ref in fast path")
> 
> Why this Fixes:? Is the leak absent before the percpu_ref refactoring?

I agree, the "fixes" tag looks dubious to me.

> I guess the embedded data are free'd together with cgroup. Makes me
> wonder why struct cgroup_bpf has a separate percpu_ref counter from
> struct cgroup...

This is because a cgroup can stay a long time (sometimes effectively forever)
in a dying state, so we want to release bpf structures earlier.

Thanks!