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 E0927D1AD48 for ; Wed, 16 Oct 2024 11:43:36 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 47DC66B007B; Wed, 16 Oct 2024 07:43:36 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 42D606B0082; Wed, 16 Oct 2024 07:43:36 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2A7236B0083; Wed, 16 Oct 2024 07:43:36 -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 0D1676B007B for ; Wed, 16 Oct 2024 07:43:36 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id F40E6AC94E for ; Wed, 16 Oct 2024 11:43:15 +0000 (UTC) X-FDA: 82679280012.07.95D8886 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.223.130]) by imf22.hostedemail.com (Postfix) with ESMTP id CA663C000A for ; Wed, 16 Oct 2024 11:43:22 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=RojoMyQi; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b="shRhXQK/"; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=RojoMyQi; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b="shRhXQK/"; dmarc=none; spf=pass (imf22.hostedemail.com: domain of vbabka@suse.cz designates 195.135.223.130 as permitted sender) smtp.mailfrom=vbabka@suse.cz ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729078907; a=rsa-sha256; cv=none; b=f/wZxnO5Bc0vqxTCka+B/Qx6xh9Fxz7DKniJKkLCRJKIU+FtwS5aHSnRNbx22rMOaMlZwO 4zgr/iz09BTXzpyv5Z0CyZLfBaOrz/Sv3lnTu9OKzsBe+v6Jr7RDKGtxpc1eYuKnMtguz5 LBpdY8ebI+U6U86HQjnPJxamSnj4mb0= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=RojoMyQi; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b="shRhXQK/"; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=RojoMyQi; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b="shRhXQK/"; dmarc=none; spf=pass (imf22.hostedemail.com: domain of vbabka@suse.cz designates 195.135.223.130 as permitted sender) smtp.mailfrom=vbabka@suse.cz ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729078907; 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=U3CfdPYkhCLzx0ji2WFxvD1fuKUNA+OHA1fpR9DcWvc=; b=g0tMwtSL8EZ/9+WE6+h74TNeMQPLbaXWb4f+fcGEzGRgXXF9vqrJJlVKVq9QaYKqkythR2 1TRgBTS/pSlj5W0LrDLbLBxFAfVDJP/R1InpYr9G0b0H62CR9FOtNWTdMUfBeZKT7J47YQ NR7OVWVOpwjIWpPRdOD665B+3hznMQg= Received: from imap1.dmz-prg2.suse.org (unknown [10.150.64.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 2EA5E21DCF; Wed, 16 Oct 2024 11:43:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1729079011; h=from:from:reply-to: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=U3CfdPYkhCLzx0ji2WFxvD1fuKUNA+OHA1fpR9DcWvc=; b=RojoMyQi3/fVOIxvM9D9uxs8noyj/p7XAM50bHjih9IYyYEz6CY5q6YVPqFFbdmpFjZ9R2 j5GJF4tFIIQq7Pq+1CJjW2QijIniSc7SCr5kSWC9G/21KOoSq7Jn0yfuHsRwtG/bfXq4h2 o2xQ1/Irx9ra6tU3yx1te+2/BIHL7HA= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1729079011; h=from:from:reply-to: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=U3CfdPYkhCLzx0ji2WFxvD1fuKUNA+OHA1fpR9DcWvc=; b=shRhXQK/VBtPXB3wggtARJWOt7kyLgnU9+wYN+yd7yFRW0x1oLCcAMoMJ3fRWadQLBMjm2 xDenq09ZE4j7y6Cw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1729079011; h=from:from:reply-to: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=U3CfdPYkhCLzx0ji2WFxvD1fuKUNA+OHA1fpR9DcWvc=; b=RojoMyQi3/fVOIxvM9D9uxs8noyj/p7XAM50bHjih9IYyYEz6CY5q6YVPqFFbdmpFjZ9R2 j5GJF4tFIIQq7Pq+1CJjW2QijIniSc7SCr5kSWC9G/21KOoSq7Jn0yfuHsRwtG/bfXq4h2 o2xQ1/Irx9ra6tU3yx1te+2/BIHL7HA= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1729079011; h=from:from:reply-to: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=U3CfdPYkhCLzx0ji2WFxvD1fuKUNA+OHA1fpR9DcWvc=; b=shRhXQK/VBtPXB3wggtARJWOt7kyLgnU9+wYN+yd7yFRW0x1oLCcAMoMJ3fRWadQLBMjm2 xDenq09ZE4j7y6Cw== Received: from imap1.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id 12F4F1376C; Wed, 16 Oct 2024 11:43:31 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id Cj1JBOOmD2d3ZQAAD6G6ig (envelope-from ); Wed, 16 Oct 2024 11:43:31 +0000 Message-ID: Date: Wed, 16 Oct 2024 13:43:30 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] mm: shrinker: avoid memleak in alloc_shrinker_info Content-Language: en-US To: Muchun Song , chenridong Cc: Anshuman Khandual , "Kirill A. Shutemov" , akpm@linux-foundation.org, david@fromorbit.com, zhengqi.arch@bytedance.com, roman.gushchin@linux.dev, linux-mm@kvack.org, linux-kernel@vger.kernel.org, wangweiyang2@huawei.com References: <20241014032336.482088-1-chenridong@huaweicloud.com> <4a18e997-3a94-4248-8923-c3764d12b0d6@huawei.com> From: Vlastimil Babka In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: CA663C000A X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: ze57wjoywwx7k8ubi5qzke4hufbs59pi X-HE-Tag: 1729079002-361873 X-HE-Meta: U2FsdGVkX1+u8VLLYcf5zRlAtgsamXfh9KgMC0IaEDnhCddLHWq14b0VfnI3mV7csoV6WLh5gaHlMBsabQ/gy6ghuhFx8rMuHou/d23xfKzO+/uV4PKv5h77iM9k03D1sdlbxH8gm87edZqw4ApTSKyj4MooAqA2gfOh48hoo+3WzC9KmBuapRRRsTTuYOOg96xWyqx1J52GLsNjx0yxHsEqsWWWKSfYoTvUP3c1vyAFlTEPr2o6NE3BFovcLwSnxQ37LraPQKHLUbR1jqJQ4Cn5xTPq6d5ikreOW/6CpU5RhW/qWWASPKPk4ysD0DDuBEyp20jLTAhnSuYk+pSQfVqfW9tKnienF69OvTL2ZuiyW61wPiGCfU9weANgl8gsQX+UdSfVO2RrerPtUmsk6P/IhfQqyr9IwYnfoCCmbQ0DycerIsLVVZxcZsTSkoF3FJrvykETUsD08oJQe8f9MkQsws+5fHmcgyEBP7oo0i3nJsKsamfPAsg2BFq6sx/kWmchaFwZ1M6MoaQSw2OCqP1jklX9ON8Vh/lL5Mze/NXbRIkAW1AcME2i8hK7B9K6Tx1hOkBno6at9tHxUSk2NSJPp7DFs46xGa+prkYPiazFmGyhkehzYOLNihiLruEuzXpYjVGLgDmLmdRRZXaiQPju1so1Aoso+9IMs6BVjzohr4ip87bzxWrsQ8UeqAlJML46UC8klM2u9u03ARgjbNGu9bSIbMIMvW9gIwxyDPIIEzcXm+qs6APcH7urDebIejRMyGcFmT+avgc6fWACrW9BQtQAlvJhZnbfOPjEGMYDND8ilOoc55ClX+cwl2OsYQHuM8NLdWCpP0lf9rJen8oQwv5i7ohCLUg+X60YnyUt6216e16XXlMvpmCdUDIFX2ud15wWnAuGs/1iK66EFcFezc9yKJrMmku/DK/d4up71NFajHFk5OseOuV/klaQa0ureZ6S08qC5xxOFgN 8WjVz6na mX8eKmQFGt7lI7OfX8t2J83uF+COoIHf1Em8ciuzTjt/gZx/CGTrbKZFGws67yFJBQAl4ZoIXvlB/9Her+hPkjJKPZcc/NHds95teDKC9odk9nO91x98pnXYwftBWDxTea4uzOSxQ3CmYHTi1avPFF+/3s/0kY932yYhQfOaFAzVKsjuo/lgvNVPeJVTXODDrgQ4lIU+AI9TzUpaK1O4ArZ5BlDDxtaRz4R+yRGUdgrCUJTvW3KwYJ4W4usvy0lKZ49G0JVM7EeLGbkU5csHtlayClttYAEZ80Tmxo4excm/nPiJa29tzpDImbKWr/pBbh//zxkk1ZmrQ6q0Cj3rK6eFvxEhesDYLFfCnMFPtgtzWdG8= 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: List-Subscribe: List-Unsubscribe: On 10/16/24 04:21, Muchun Song wrote: > > >> On Oct 16, 2024, at 09:25, chenridong wrote: >> >> >> >> On 2024/10/15 14:55, Anshuman Khandual wrote: >>> On 10/14/24 16:59, Kirill A. Shutemov wrote: >>>> On Mon, Oct 14, 2024 at 03:23:36AM +0000, Chen Ridong wrote: >>>>> From: Chen Ridong >>>>> >>>>> A memleak was found as bellow: >>>>> >>>>> unreferenced object 0xffff8881010d2a80 (size 32): >>>>> comm "mkdir", pid 1559, jiffies 4294932666 >>>>> hex dump (first 32 bytes): >>>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >>>>> 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 @............... >>>>> backtrace (crc 2e7ef6fa): >>>>> [] __kmalloc_node_noprof+0x394/0x470 >>>>> [] alloc_shrinker_info+0x7b/0x1a0 >>>>> [] mem_cgroup_css_online+0x11a/0x3b0 >>>>> [] online_css+0x29/0xa0 >>>>> [] cgroup_apply_control_enable+0x20d/0x360 >>>>> [] cgroup_mkdir+0x168/0x5f0 >>>>> [] kernfs_iop_mkdir+0x5e/0x90 >>>>> [] vfs_mkdir+0x144/0x220 >>>>> [] do_mkdirat+0x87/0x130 >>>>> [] __x64_sys_mkdir+0x49/0x70 >>>>> [] do_syscall_64+0x68/0x140 >>>>> [] entry_SYSCALL_64_after_hwframe+0x76/0x7e >>>>> >>>>> In the alloc_shrinker_info function, when shrinker_unit_alloc return >>>>> err, the info won't be freed. Just fix it. >>>>> >>>>> Fixes: 307bececcd12 ("mm: shrinker: add a secondary array for shrinker_info::{map, nr_deferred}") >>>>> Signed-off-by: Chen Ridong >>>>> --- >>>>> mm/shrinker.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/mm/shrinker.c b/mm/shrinker.c >>>>> index dc5d2a6fcfc4..92270413190d 100644 >>>>> --- a/mm/shrinker.c >>>>> +++ b/mm/shrinker.c >>>>> @@ -97,6 +97,7 @@ int alloc_shrinker_info(struct mem_cgroup *memcg) >>>>> err: >>>>> mutex_unlock(&shrinker_mutex); >>>>> + kvfree(info); >>>>> free_shrinker_info(memcg); >>>>> return -ENOMEM; >>>>> } >>>> >>>> NAK. If in the future there going to one more error case after >>>> rcu_assign_pointer() we will end up with double free. >>>> >>>> This should be safer: >>>> >>>> diff --git a/mm/shrinker.c b/mm/shrinker.c >>>> index dc5d2a6fcfc4..763fd556bc7d 100644 >>>> --- a/mm/shrinker.c >>>> +++ b/mm/shrinker.c >>>> @@ -87,8 +87,10 @@ int alloc_shrinker_info(struct mem_cgroup *memcg) >>>> if (!info) >>>> goto err; >>>> info->map_nr_max = shrinker_nr_max; >>>> - if (shrinker_unit_alloc(info, NULL, nid)) >>>> + if (shrinker_unit_alloc(info, NULL, nid)) { >>>> + kvfree(info); >>>> goto err; >>>> + } >>>> rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, info); >>>> } >>>> mutex_unlock(&shrinker_mutex); >>> Agreed, this is what I mentioned earlier as well. >>> ------------------------------------------------------------------ >>> I guess kvfree() should be called just after shrinker_unit_alloc() >>> fails but before calling into "goto err" >>> ------------------------------------------------------------------ >> >> After discussion, it seems that v1 is acceptable. >> Hi, Muchun, do you have any other opinions? > > I insist on my opinion, not mixing two different approaches > to do release resources. So instead we mix the cleanup of the whole function with the cleanup of what is effectively a per-iteration temporary variable? The fact there was already a confusion in this thread about whether it's safe and relies on kvfree(NULL) to be a no-op, should be a hint. So no, I a gree with Kirill and others. Ideally the fix would also move the declaration of info into the for loop to make its scope more obvious. > Thanks. > >> >> Best regards, >> Ridong > >