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;
Subject: Re: [V2][PATCH] cgroup: fix memory leak caused by missing
 cgroup_bpf_offline
To:     Ming Lei <ming.lei@redhat.com>
Cc:     Tejun Heo <tj@kernel.org>, Zefan Li <lizefan.x@bytedance.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>, Roman Gushchin <guro@fb.com>,
        mkoutny@suse.com, cgroups@vger.kernel.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        bpf@vger.kernel.org
References: <20211018075623.26884-1-quanyang.wang@windriver.com>
 <YW04Gqqm3lDisRTc@T590>
From:   Quanyang Wang <quanyang.wang@windriver.com>
Message-ID: <8fdcaded-474e-139b-a9bc-5ab6f91fbd4f@windriver.com>
Date:   Mon, 18 Oct 2021 18:06:28 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
In-Reply-To: <YW04Gqqm3lDisRTc@T590>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SGZ5SndHZTZWa1FPWUt3NEVjb3ZEL0J1T2ErSkNZYk1MUXliYmhHcmt3T0Q5?=
 =?utf-8?B?eWNKNytiei9hV2hIaGVPa3A0eVA5VEM4UWlNUERpdnBqUGV4eFp4ajFMVEUw?=
 =?utf-8?B?N1hIdlVOekpBWXBnT1BCcFFGcW5SN3luNzFyNkJ2ZXFkV09lQVpmNFNURVBL?=
 =?utf-8?B?VVVHaUliWHl2TFFodTdpeVhKdlBoc1YvR3dGV044dlVxWlVkV1NLMHh3My9J?=
 =?utf-8?B?MVJyVEgxbEcycGY2Ukdjc3ZSa2x1bEp4eTE2Sm1TKzdKTlFKRXg4c25ZMThN?=
 =?utf-8?B?N2E1Mmw3VlBVYjBhVXpmNktVUDZZL3NLbHhRWWFFVU96dGdqSEIzM3RjTko2?=
 =?utf-8?B?dUZDVmxLTlVtU2hadFM3cnVpRzhWNEZpMmd4cHgycC8xTGFCM05XeEhKOGhB?=
 =?utf-8?B?b0FYQmdoY1JYNDJ6ek4yV0Y1WS93UzZJdFNvbTlQMnhjOGZEc04xUEptM1Vu?=
 =?utf-8?B?dk9aNitPMzFyNUtaV0xqQm96dWJTWjFzU0lON0lYdjhDOVRqOU9RbDN5V3NQ?=
 =?utf-8?B?Zk5SaTNxa1E2WTlhYVk1d0FIRVVEMHc4ZjMzN2VBbkdZV09VVVpaUmNvU0Yy?=
 =?utf-8?B?dWIwNG5wQ0Yyc1poclJZWjV4ZHhuUXNOYWZUaWVxend3akhYZ3ZXWmtmQXJB?=
 =?utf-8?B?aUdLOHl6ZzY5Y0M0QXRuUkhKR3p2ak56dHRtSzdpMzhxSXFZS0dBdGtLZXhS?=
 =?utf-8?B?clpjeVNxd0NmenE2OWxFNlZuaytoMXVndVpXazZjaGgrOFUzRUlNeUQ0MWJv?=
 =?utf-8?B?SUQ0MGFiS2hnbUZpTURldHhHYnhnalRyRkIvUS9KbW9yV2VwamlOd3lWL25o?=
 =?utf-8?B?eDdKSklsRmtHSTlsWDkzV1U2M01LckJ3YmQydHlaNExaRjhkSFNIK1YvcElX?=
 =?utf-8?B?MjlLazl2cEF6NU5qeTlkU0V2alY4RVFEdW9Hb1FmR29BOUY0aElzcjBtQXRh?=
 =?utf-8?B?WHBkaWNKNGRaRHNIZEFmczRWMWJMRDVROVN6NTI5c29TeG0vSmFoVWR1VHFq?=
 =?utf-8?B?SzRSS0JOS2NQOS9RQy9TMXVPT2lmc21MaHFkdDNuK0lZUDMzQ1J1NytMc2pB?=
 =?utf-8?B?bEFFUjJ1U0dxY2k5Y3g5cXpGSzRGd2xRZWdYVnMxSkFxblBybDFQaStucUli?=
 =?utf-8?B?VS9neTlMZXB6ZW9IVDYzd1FpQTJsdk81citDekMxRW9nU3VsN0prWkR2aDNV?=
 =?utf-8?B?ZFlCYUl5SDlBVW1IL3RrRk1odUR4VHpvdjdYQ0M3bEE2U3VjeGpUYjdGUnkr?=
 =?utf-8?B?WnRXOVJoa1NZM0tvb2xZVXdXQWlmN1hIV2JkSDN1aDRsWnEvUmtHaTRVWll0?=
 =?utf-8?B?a2Y2U0VNK0tvVDliSTR6c1JVMnkrcnNId3hiSDZjZ3NIU0NkUzVvVTBCMEh6?=
 =?utf-8?B?bWI1TEFCemxOSEdjQ3kvL3VYWXVpYVQzKzM0QldXQm1sM01nTUxOQXpUTjh3?=
 =?utf-8?B?dkFSTWR5NUpxTlNCbzR1cWVDcjh0RjRwdFlDZ2ZqTXBONGQ0cks3Vk4yeCtL?=
 =?utf-8?B?cGpoVzB4dUw1eHg0K0Y2cnNzNzI4ak1USGROb3JGRG1WNDZnTU83RUd0eVhn?=
 =?utf-8?B?NTBrVUprQ0ptUzB3Ty90NUV2TFV1MnU0bkVNZ1Z3RFN2cjNLQlErR2VXZDJP?=
 =?utf-8?B?Q0hzc1Q3RWYvbVRRdmFGazN1aEQzMkc0cFRlWHdsaHRiVjlzSVUrMTc5UHpi?=
 =?utf-8?B?dzZMTnZnQ3lRanZLc2V3dkFhNDhGbnd2QldjdlVqUGN3TEdJVENOM0IwejVy?=
 =?utf-8?Q?QZ4ii2sgFTz6ZctC71oGXWo3Thqf87loV1vzSMi?=
X-OriginatorOrg: windriver.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 88320268-e3f7-4a28-17e9-08d9921ef963
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5819.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Oct 2021 10:06:38.1555
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ddb2873-a1ad-4a18-ae4e-4644631433be
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: P3WrhpzfxCb+CqOiPT+Rd5idUnCTPMfaWDofLy0LlAg7f7SWJj+pgMBBZURldqbfeNexEjgATC52OM6uoGEg78DqRV4UMuGAD3eKOwJKhTc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4821
X-Proofpoint-ORIG-GUID: jfT_8gHnwXYnPhtQX_ppCC8_ymuL--96
X-Proofpoint-GUID: jfT_8gHnwXYnPhtQX_ppCC8_ymuL--96
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-18_03,2021-10-14_02,2020-04-07_01
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 lowpriorityscore=0
 clxscore=1015 phishscore=0 malwarescore=0 mlxlogscore=999 impostorscore=0
 spamscore=0 adultscore=0 bulkscore=0 suspectscore=0 priorityscore=1501
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109230001
 definitions=main-2110180063
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Ming,

On 10/18/21 5:02 PM, Ming Lei wrote:
> On Mon, Oct 18, 2021 at 03:56:23PM +0800, quanyang.wang@windriver.com wrote:
>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>
>> When enabling CONFIG_CGROUP_BPF, kmemleak can be observed by running
>> the command as below:
>>
>>      $mount -t cgroup -o none,name=foo cgroup cgroup/
>>      $umount cgroup/
>>
>> unreferenced object 0xc3585c40 (size 64):
>>    comm "mount", pid 425, jiffies 4294959825 (age 31.990s)
>>    hex dump (first 32 bytes):
>>      01 00 00 80 84 8c 28 c0 00 00 00 00 00 00 00 00  ......(.........
>>      00 00 00 00 00 00 00 00 6c 43 a0 c3 00 00 00 00  ........lC......
>>    backtrace:
>>      [<e95a2f9e>] cgroup_bpf_inherit+0x44/0x24c
>>      [<1f03679c>] cgroup_setup_root+0x174/0x37c
>>      [<ed4b0ac5>] cgroup1_get_tree+0x2c0/0x4a0
>>      [<f85b12fd>] vfs_get_tree+0x24/0x108
>>      [<f55aec5c>] path_mount+0x384/0x988
>>      [<e2d5e9cd>] do_mount+0x64/0x9c
>>      [<208c9cfe>] sys_mount+0xfc/0x1f4
>>      [<06dd06e0>] ret_fast_syscall+0x0/0x48
>>      [<a8308cb3>] 0xbeb4daa8
>>
>> This is because that since the commit 2b0d3d3e4fcf ("percpu_ref: reduce
>> memory footprint of percpu_ref in fast path") 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. Adding cgroup_bpf_offline which calls
>> percpu_ref_kill to cgroup_kill_sb can free root_cgrp->bpf.refcnt.data in
>> umount path.
>>
>> This patch also fixes the commit 4bfc0bb2c60e ("bpf: decouple the lifetime
>> of cgroup_bpf from cgroup itself"). A cgroup_bpf_offline is needed to do a
>> cleanup that frees the resources which are allocated by cgroup_bpf_inherit
>> in cgroup_setup_root.
>>
>> And inside cgroup_bpf_offline, cgroup_get() is at the beginning and
>> cgroup_put is at the end of cgroup_bpf_release which is called by
>> cgroup_bpf_offline. So cgroup_bpf_offline can keep the balance of
>> cgroup's refcount.
>>
>> Fixes: 2b0d3d3e4fcf ("percpu_ref: reduce memory footprint of percpu_ref in fast path")
> 
> If I understand correctly, cgroup_bpf_release() won't be called without
> your patch. So anything allocated in cgroup_bpf_inherit() will be
> leaked?
No, for now cgroup_bpf_release is called to release bpf.refcnt.data of 
the cgroup which is not root_cgroup. Only root_cgroup's bpf data is leaked.

For non-root cgroup:
cgroup_mkdir
-> cgroup_create
-->cgroup_bpf_inherit(cgrp_A)
cgroup_rmdir
->cgroup_destroy_locked()
-->cgroup_bpf_offline(cgrp_A)
So for non-root cgroup, there is no memory leak.


For root cgroup:
cgroup_setup_root
->cgroup_bpf_inherit(root_cgrp)
cgroup_kill_sb:
-> (Here should be call cgroup_bpf_offline, or else leak occurs)

Thanks,
Quanyang


> 
> If that is true, 'Fixes: 2b0d3d3e4fcf' looks misleading, cause people has to
> backport your patch if 4bfc0bb2c60e is applied. Meantime, this fix isn't
> needed if 4bfc0bb2c60e isn't merged.
> 
> 
> Thanks,
> Ming
>