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 3A95BD2A533 for ; Wed, 16 Oct 2024 17:02:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 919576B0083; Wed, 16 Oct 2024 13:02:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8C9716B0088; Wed, 16 Oct 2024 13:02:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 742A76B0089; Wed, 16 Oct 2024 13:02:28 -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 583E96B0083 for ; Wed, 16 Oct 2024 13:02:28 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 4F8B0A07F8 for ; Wed, 16 Oct 2024 17:02:09 +0000 (UTC) X-FDA: 82680083598.27.1BA24F1 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.223.130]) by imf29.hostedemail.com (Postfix) with ESMTP id 9C7B012002A for ; Wed, 16 Oct 2024 17:02:13 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=nUxGi6DD; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=MEF5krdu; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=nUxGi6DD; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=MEF5krdu; spf=pass (imf29.hostedemail.com: domain of vbabka@suse.cz designates 195.135.223.130 as permitted sender) smtp.mailfrom=vbabka@suse.cz; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729098002; 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=/q/Uv6qWezncSwuZ+eugisRocLDwMwtqjAd1EYzt6G4=; b=034pirqE+VHuRlXGBP6Dci19EQitY/L2hWepA+fuw8RkPBpxcMHKbuUxbndh6MjfZKhrse ZKWXDW4JOTD/UhZo8XS6FNlnmFKVQeSevC0H8LEDvGdCn1s64XlIy+X1P/+SDZb1KoyeGC eS5riAUgfFIbKIH00e9qg1ergDGSqYc= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729098002; a=rsa-sha256; cv=none; b=pKsA/CFzNayDrnNFAsnOnhMlnAxJ5gh4MhzsZA7vYwbXHlRRmU1T3cvdEwlwqEPYCZKS28 LdYdyVuLXgI9+H36FdqbPHy3RvZ5FXvzxT4lE1RPa+7zwa08qXeal/ThNwdIBJycyB6g3X TSp3NGmYz6NOg+E1odSVMy9naxr6jlU= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=nUxGi6DD; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=MEF5krdu; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=nUxGi6DD; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=MEF5krdu; spf=pass (imf29.hostedemail.com: domain of vbabka@suse.cz designates 195.135.223.130 as permitted sender) smtp.mailfrom=vbabka@suse.cz; dmarc=none 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 74AF921ED2; Wed, 16 Oct 2024 17:02:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1729098143; 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:autocrypt:autocrypt; bh=/q/Uv6qWezncSwuZ+eugisRocLDwMwtqjAd1EYzt6G4=; b=nUxGi6DDsTv+Ho30H1l7v5BD8icSPi52yfQH78Yj3XLDIN50tcMdRVU8NiwVFPE3nz0c52 f7XhQBUOEwG9TnW0TgSHlZ09D/hVlnk8bOfS9nobWqLQoWEO1r0pXvkjNaHCdQvn24JaoH FS5BovJFAGkjnuDktHgTACGuN6leHYM= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1729098143; 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:autocrypt:autocrypt; bh=/q/Uv6qWezncSwuZ+eugisRocLDwMwtqjAd1EYzt6G4=; b=MEF5krduLFOKIYgx/iZSJ/ax5VY0H5WtWw0wIo4XGcxom4xE3PDaHTmwveR1T0AQAZu5vy YHCs+DYVgmAea3Cg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1729098143; 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:autocrypt:autocrypt; bh=/q/Uv6qWezncSwuZ+eugisRocLDwMwtqjAd1EYzt6G4=; b=nUxGi6DDsTv+Ho30H1l7v5BD8icSPi52yfQH78Yj3XLDIN50tcMdRVU8NiwVFPE3nz0c52 f7XhQBUOEwG9TnW0TgSHlZ09D/hVlnk8bOfS9nobWqLQoWEO1r0pXvkjNaHCdQvn24JaoH FS5BovJFAGkjnuDktHgTACGuN6leHYM= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1729098143; 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:autocrypt:autocrypt; bh=/q/Uv6qWezncSwuZ+eugisRocLDwMwtqjAd1EYzt6G4=; b=MEF5krduLFOKIYgx/iZSJ/ax5VY0H5WtWw0wIo4XGcxom4xE3PDaHTmwveR1T0AQAZu5vy YHCs+DYVgmAea3Cg== 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 5A4CD13433; Wed, 16 Oct 2024 17:02:23 +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 nKSxFZ/xD2cjTQAAD6G6ig (envelope-from ); Wed, 16 Oct 2024 17:02:23 +0000 Message-ID: <0b883f9e-451f-41c2-805f-7f5bc7eebee2@suse.cz> Date: Wed, 16 Oct 2024 19:02:23 +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 Cc: chenridong , 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: <1BD74B20-879A-4159-B957-1223553217C1@linux.dev> From: Vlastimil Babka Autocrypt: addr=vbabka@suse.cz; keydata= xsFNBFZdmxYBEADsw/SiUSjB0dM+vSh95UkgcHjzEVBlby/Fg+g42O7LAEkCYXi/vvq31JTB KxRWDHX0R2tgpFDXHnzZcQywawu8eSq0LxzxFNYMvtB7sV1pxYwej2qx9B75qW2plBs+7+YB 87tMFA+u+L4Z5xAzIimfLD5EKC56kJ1CsXlM8S/LHcmdD9Ctkn3trYDNnat0eoAcfPIP2OZ+ 9oe9IF/R28zmh0ifLXyJQQz5ofdj4bPf8ecEW0rhcqHfTD8k4yK0xxt3xW+6Exqp9n9bydiy tcSAw/TahjW6yrA+6JhSBv1v2tIm+itQc073zjSX8OFL51qQVzRFr7H2UQG33lw2QrvHRXqD Ot7ViKam7v0Ho9wEWiQOOZlHItOOXFphWb2yq3nzrKe45oWoSgkxKb97MVsQ+q2SYjJRBBH4 8qKhphADYxkIP6yut/eaj9ImvRUZZRi0DTc8xfnvHGTjKbJzC2xpFcY0DQbZzuwsIZ8OPJCc LM4S7mT25NE5kUTG/TKQCk922vRdGVMoLA7dIQrgXnRXtyT61sg8PG4wcfOnuWf8577aXP1x 6mzw3/jh3F+oSBHb/GcLC7mvWreJifUL2gEdssGfXhGWBo6zLS3qhgtwjay0Jl+kza1lo+Cv BB2T79D4WGdDuVa4eOrQ02TxqGN7G0Biz5ZLRSFzQSQwLn8fbwARAQABzSBWbGFzdGltaWwg QmFia2EgPHZiYWJrYUBzdXNlLmN6PsLBlAQTAQoAPgIbAwULCQgHAwUVCgkICwUWAgMBAAIe AQIXgBYhBKlA1DSZLC6OmRA9UCJPp+fMgqZkBQJkBREIBQkRadznAAoJECJPp+fMgqZkNxIQ ALZRqwdUGzqL2aeSavbum/VF/+td+nZfuH0xeWiO2w8mG0+nPd5j9ujYeHcUP1edE7uQrjOC Gs9sm8+W1xYnbClMJTsXiAV88D2btFUdU1mCXURAL9wWZ8Jsmz5ZH2V6AUszvNezsS/VIT87 AmTtj31TLDGwdxaZTSYLwAOOOtyqafOEq+gJB30RxTRE3h3G1zpO7OM9K6ysLdAlwAGYWgJJ V4JqGsQ/lyEtxxFpUCjb5Pztp7cQxhlkil0oBYHkudiG8j1U3DG8iC6rnB4yJaLphKx57NuQ PIY0Bccg+r9gIQ4XeSK2PQhdXdy3UWBr913ZQ9AI2usid3s5vabo4iBvpJNFLgUmxFnr73SJ KsRh/2OBsg1XXF/wRQGBO9vRuJUAbnaIVcmGOUogdBVS9Sun/Sy4GNA++KtFZK95U7J417/J Hub2xV6Ehc7UGW6fIvIQmzJ3zaTEfuriU1P8ayfddrAgZb25JnOW7L1zdYL8rXiezOyYZ8Fm ZyXjzWdO0RpxcUEp6GsJr11Bc4F3aae9OZtwtLL/jxc7y6pUugB00PodgnQ6CMcfR/HjXlae h2VS3zl9+tQWHu6s1R58t5BuMS2FNA58wU/IazImc/ZQA+slDBfhRDGYlExjg19UXWe/gMcl De3P1kxYPgZdGE2eZpRLIbt+rYnqQKy8UxlszsBNBFsZNTUBCACfQfpSsWJZyi+SHoRdVyX5 J6rI7okc4+b571a7RXD5UhS9dlVRVVAtrU9ANSLqPTQKGVxHrqD39XSw8hxK61pw8p90pg4G /N3iuWEvyt+t0SxDDkClnGsDyRhlUyEWYFEoBrrCizbmahOUwqkJbNMfzj5Y7n7OIJOxNRkB IBOjPdF26dMP69BwePQao1M8Acrrex9sAHYjQGyVmReRjVEtv9iG4DoTsnIR3amKVk6si4Ea X/mrapJqSCcBUVYUFH8M7bsm4CSxier5ofy8jTEa/CfvkqpKThTMCQPNZKY7hke5qEq1CBk2 wxhX48ZrJEFf1v3NuV3OimgsF2odzieNABEBAAHCwXwEGAEKACYCGwwWIQSpQNQ0mSwujpkQ PVAiT6fnzIKmZAUCZAUSmwUJDK5EZgAKCRAiT6fnzIKmZOJGEACOKABgo9wJXsbWhGWYO7mD 8R8mUyJHqbvaz+yTLnvRwfe/VwafFfDMx5GYVYzMY9TWpA8psFTKTUIIQmx2scYsRBUwm5VI EurRWKqENcDRjyo+ol59j0FViYysjQQeobXBDDE31t5SBg++veI6tXfpco/UiKEsDswL1WAr tEAZaruo7254TyH+gydURl2wJuzo/aZ7Y7PpqaODbYv727Dvm5eX64HCyyAH0s6sOCyGF5/p eIhrOn24oBf67KtdAN3H9JoFNUVTYJc1VJU3R1JtVdgwEdr+NEciEfYl0O19VpLE/PZxP4wX PWnhf5WjdoNI1Xec+RcJ5p/pSel0jnvBX8L2cmniYnmI883NhtGZsEWj++wyKiS4NranDFlA HdDM3b4lUth1pTtABKQ1YuTvehj7EfoWD3bv9kuGZGPrAeFNiHPdOT7DaXKeHpW9homgtBxj 8aX/UkSvEGJKUEbFL9cVa5tzyialGkSiZJNkWgeHe+jEcfRT6pJZOJidSCdzvJpbdJmm+eED w9XOLH1IIWh7RURU7G1iOfEfmImFeC3cbbS73LQEFGe1urxvIH5K/7vX+FkNcr9ujwWuPE9b 1C2o4i/yZPLXIVy387EjA6GZMqvQUFuSTs/GeBcv0NjIQi8867H3uLjz+mQy63fAitsDwLmR EP+ylKVEKb0Q2A== In-Reply-To: <1BD74B20-879A-4159-B957-1223553217C1@linux.dev> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 9C7B012002A X-Stat-Signature: b91kdzt7um6ox8eubgbxyrj7ita6rbei X-Rspamd-Server: rspam09 X-Rspam-User: X-HE-Tag: 1729098133-923631 X-HE-Meta: U2FsdGVkX18WBHoGuOQfMSLuMse5T5i8uOJfDfwy/FkPf61kP/oxFe5+0Conxuk5Yt0Krp7TmWkIsjinEQXTW8/BUfQJ7tNnr26NgWHmykEoYkQtqXNl41cXG/UE2dIOezHENzObrde4AW5RXFe3oCs7yCucxGibTuqrvVwP4F46uPLIPO3EJZntOEbC7BXAurm0gqse1Wh72U6gwr+nk1UcmpeT89ATSAdYNPVY2D1O8byPudLJawrX1HtvHwE1hsfs2tr4W84JnI3I3v8aOEaMJpXQoCTU1Deze4R6uq27ZmbRXzFrDH+jeAvHfwGU0PHAmTvSoPRLbWIQPCCmRZ0NFmdV87stjtkXch0tCE5oUE9PxNI7KCxYNXg51+keOlTepeKJXkBygFYf++VcAy4SsVDaczQ/bbC1pm7YiXVxlMLNEwfrdk/DR1qO5XSSB1R71SGT5e0j5qee2ZP6+wFO4MCJchwXOMMwwd7X5XxKdtpa9r2eckQ6uNr1wY7P14nDV+DXSRM3OzEnU3RvfOCLZvzB5fYxHdsYEpiUpFUjyrUxXVWKr87U7QDDKeY0PVbLGYTZpcGQeUlPgHYx4Pj+Gu6bfRNNkp8bxhK4g1hKvVYHfvPYSMvnhaLSkDnjRWrcJmg3b6vwVp3XhdYRCk9Ci506gfRB8DY3cgId2GefWQn3rqxmE+x3A/HuRngSWZHhDiUKkdWWNFOMidWabsMOGGUDZa9ve4/rAPMFR/MLtMJwN1Oi6c+9IzjKCGVUW2bvjeKPCv4yq1GB7JYRAnFOZOldG3czhnP6mxbnA/ju5G0aT0wn5rn0Z5qpVo6tp1dg0qK3bENoMXQ8mP2yrqZkmgPCUBwE3y3GxZ/qpGYlAIUSwuB2ZNt714EGh5GCRAYIUryYNZZVtivazg+/fD5Xrta3K/Q/KPd+H77bNpiB8d2Zz14/pDUXc8MHkAS37htZGrjA8TeywC8hmN/ EdmxHSla 2PzwSdJKC120GRdk2GzR3nysSK+L3YF/ohUA+7nx3xRfYEkh9bCyQgZcd1Hp5EqbQ1MsuQI0fr4G44LYBNgAocVYLFBWNuDnKu1BKkv1po6FsfT9Jp8TGIgFLNkeiYXwlCILm2oWR2h1h177bnIUay9TLh2P5lAwES1TiiC44kdSl0zep61ybPI6+amG08U0VlsgRSfIlnt7TOR3vOo5gV5jiIzJx+UqmYBJdBZR5af2kxs7MPtUWtRNJysOqtvvRnH3du45OKNJif6AoHGVAeJkn1DFzgDg1imh6XJjmWMrk56mgbsFUuAGpao4TGCdhl1GGhMCwzdmdmR/mcm23VqlNxzrzlsU+EKQ/VqsLi3A2PdmHaOlJ59igrVBx3znR1eG+ 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 16:08, Muchun Song wrote: > > >> On Oct 16, 2024, at 19:43, Vlastimil Babka wrote: >> 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. > > Yes. I think someone is confused about my opinion. > I don’t care about whether we should apply this hit. > If we think the hint is tricky, we could add another > label to fix it like I suggested previously. Because > we already use goto-based approaches to > cleanup the resources, why not keeping > consistent? I think we're rather pragmatic than striving to be consistent for the sake of consistency. goto is not the nicest thing in the world, but we (unlike other projects) use it where it makes sense to avoid if/else nesting explosion. Here for the info it's not the most pragmatic option. > It will be easier for us to add a new > "if" statement and handle the failure case in the future. Let's not overengineer things for hypothetical future. > For example, if we use his v1 proposal, we should do > the cleanups again for info. But for goto-based > version, we just add another label to do the > cleanups and go to the new label for failure case. goto-based fix is what I insisted on. I copied my previous suggested fix here to clarify my opinion. Again, info is a loop-iteration-local variable, v1 fix making it truly local is the way to go. If there are further cleanups added in the loop itself in the future, they could hopefully keep being local to the loop as well. Cleanup of info outside the loop iteration is breaking its real scope. > > --- a/mm/shrinker.c > +++ b/mm/shrinker.c > @@ -88,13 +88,14 @@ int alloc_shrinker_info(struct mem_cgroup *memcg) > goto err; > info->map_nr_max = shrinker_nr_max; > if (shrinker_unit_alloc(info, NULL, nid)) > - goto err; > + goto free; > rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, info); > } > mutex_unlock(&shrinker_mutex); > > return ret; > - > +free: > + kvfree(info); > err: > mutex_unlock(&shrinker_mutex); > free_shrinker_info(memcg); > > Muchun, > Thanks. > >> >> 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