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 A98DCC3DA49 for ; Wed, 17 Jul 2024 02:25:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 216506B0098; Tue, 16 Jul 2024 22:25:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1C6626B0099; Tue, 16 Jul 2024 22:25:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0B5026B009B; Tue, 16 Jul 2024 22:25:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id E272B6B0098 for ; Tue, 16 Jul 2024 22:25:49 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 9525A160890 for ; Wed, 17 Jul 2024 02:25:49 +0000 (UTC) X-FDA: 82347654018.23.6DAF130 Received: from out-189.mta0.migadu.com (out-189.mta0.migadu.com [91.218.175.189]) by imf24.hostedemail.com (Postfix) with ESMTP id 5179D18001B for ; Wed, 17 Jul 2024 02:25:47 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=er4erhwE; spf=pass (imf24.hostedemail.com: domain of youling.tang@linux.dev designates 91.218.175.189 as permitted sender) smtp.mailfrom=youling.tang@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721183128; 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=G30BQG0YFrLuwyR4GyhsvLZYo3tePM7M64A2WhdMUMM=; b=ojkmCbn+6YDbz193eQxrjDSY/T4Sq3pMHdZRX//jDEcfZeaOBpuSwQxkxOSnSZBSM/R00h 6tl62kmMFiRfPksfzmp7rFpG6vpEWGUCgMc4OpcBk/xbs763dYmOIyDNKkhaVMmzEqYGfS VSfkCeZLyztslo8bAoxGLfrkR64Fcio= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=er4erhwE; spf=pass (imf24.hostedemail.com: domain of youling.tang@linux.dev designates 91.218.175.189 as permitted sender) smtp.mailfrom=youling.tang@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721183128; a=rsa-sha256; cv=none; b=FR4LXj7opEPioiwmd0MmtewZcI/fn3N7T4jWFbnRdjxVy9pePh5CglDAaHJih1jGLj76I/ ezpntGX1wjBnxm9LozzNhucm4N9v7lDDd/eNiuIEPthsVsbOJgFXOXTZP9gnAD/N0ZinNC dv0Mw4mzjJDLmPPQGfNe5/itjtdT8NE= X-Envelope-To: zhengqi.arch@bytedance.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1721183144; h=from:from: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; bh=G30BQG0YFrLuwyR4GyhsvLZYo3tePM7M64A2WhdMUMM=; b=er4erhwEyw0FD29E5+7gNWVG6CwtegTTz5txeEu/JmbaD5DMnhSNQsROOVDXesbgSl3TjC oyKowVzXFgwgO19PmP3LirCvqQ1vJoox+asytPlN9BTXzw7kWeMLonYcgTNbZsntDUCqJR zPt373jfZohVCqViRcrA+NcSjxZ6uOA= X-Envelope-To: kent.overstreet@linux.dev X-Envelope-To: akpm@linux-foundation.org X-Envelope-To: linux-mm@kvack.org X-Envelope-To: linux-kernel@vger.kernel.org X-Envelope-To: tangyouling@kylinos.cn Message-ID: <68e2ecad-e8ee-4dd5-a49e-8f8507d4e03c@linux.dev> Date: Wed, 17 Jul 2024 10:25:16 +0800 MIME-Version: 1.0 Subject: Re: [PATCH] mm: list_lru: Fix NULL pointer dereference in list_lru_add() To: Qi Zheng Cc: Kent Overstreet , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Youling Tang References: <20240712032554.444823-1-youling.tang@linux.dev> <56a81429-4e1e-46f9-8844-acb1afd66952@bytedance.com> Content-Language: en-US, en-AU X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Youling Tang In-Reply-To: <56a81429-4e1e-46f9-8844-acb1afd66952@bytedance.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Rspamd-Server: rspam03 X-Rspam-User: X-Rspamd-Queue-Id: 5179D18001B X-Stat-Signature: f7h3a7sxd6f6x9amk4si3w1u6pt9moc1 X-HE-Tag: 1721183147-600030 X-HE-Meta: U2FsdGVkX19hK7uC6ahw/C2KTxu57CnMp1xZiiTla0G2z3MsIVjw3UIRBT6UrDHLkVp6msK/IpGbNQRW9Qn28sqYKonaR5gEuSQ+NPFWTawNEcYaXjJIRhP7/K5ZN+PnU59wj6pkODqqMYqB8unLjvb0a+xDQxcQGGeXDwM8CD7kpn4U6892uOBoI3Kru5miC3/NVRHfGp7E6wWJor6x6gxB5GYnnRHmBpYJVWOvraRaDEKPfkYN44HIz23z5mIvup2b6RFong8HQxVVqCqAC7ADBUolgJJJ3B9tr0I1lOdesgFuQZC4jPpTqaXjhYiXpIzz/R2R86P3uUh/rWYiVb+nazLGvksidA1Jw4xObUP55k/Nb1V1jAyddx23/v0ME5c3cBEr9/bDK5nzB2iP+862NxdYXjbmS2Q3AXa0GTFXNZ+6OwVeFHErk+Ir2cTzR8D7Ad2UbWbUk/cznQ3z5dA3nArKi2Mt84/71aGHSFmlE4GA5uF0za3hyA5IPKPCq3AYmJHULgRKaI+DMiizfKwBDEEg6qPFUGPJv2+YigGWIRgxXzPjC4UwjoJKet3XESLUorNCb750t0HfeuYEBnghRM2oxrCWLpXHlIc2aj/lFtNKhWavwKCsMTu04JQNVgmrzWUW8FPk5TpwjInhsZY/tHXazlgxZzu6/MaguqOiKb5W1UIYTJV3iXUTprdpYlOU4jPZkUFvT573xIprk0rjry6cso1DyVcypyb4mnbBgNusFO83pEHRjOHW9ySk1gD3MvFgYIP+qVrxLAhNVCzV3GGChTdbBYFo7P+bOP50dUS3gv2Ztg4tFZOwKhUs1MHmKrqTD2aktVKP49h+Ts/qTHN1KooOWkBKqCqa2kizwUkB3CQaU9ahbzP+XPSXLojXToLukU0KX3w7WWKTrP4Mtn8XMcGYDn2um5G/gCAJJY2sanx4HXuwnZcuBSktBHeF7oD5NM9Jd6P84ij KunKpYfX XSHgV7i3ntmz6EHgxAtkeXz6To5ZWJrza8ALhSb8PyEw5iQUMV0BXILIqTlo4EdfHRD+wIDp0UNfBQYJcgtsTu+FLJ+ALM+O+teZX8lQDTRIgF2vq6L4kIRoMoLELBWK2U0P18mP6yc6s2MGuBAmef9ZJRf3w3mwO6jtBRL/0N4nwPBmUQelx2K4y47u7NmfM7NoMOAgTPpss70eNawzf2gvv8q5GNskGuYt3LlAozdzSe5TEYDfH2Dyb9XrS/oVrR8hZ 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 15/07/2024 11:27, Qi Zheng wrote: > > > On 2024/7/12 12:07, Kent Overstreet wrote: >> On Fri, Jul 12, 2024 at 11:25:54AM GMT, Youling Tang wrote: >>> From: Youling Tang >>> >>> Note that list_lru_from_memcg_idx() may return NULL, so it is necessary >>> to error handle the return value to avoid triggering NULL pointer >>> dereference BUG. >>> >>> The issue was triggered for discussion [1], >>> Link [1]: >>> https://lore.kernel.org/linux-bcachefs/84de6cb1-57bd-42f7-8029-4203820ef0b4@linux.dev/T/#m901bb26cdb1d9d4bacebf0d034f0a5a712cc93a6 >> >> I see no explanation for why this is the correct fix, and I doubt it is. >> What's the real reason for the NULL lru_list_one, and why doesn't this >> come up on other filesystems? > > Agree, IIRC, the list_lru_one will be pre-allocated in the allocation > path of inode/dentry etc. Yes, this issue does not fix why lru_list_one is NULL. lru_list_one is NULL because the inode is allocated using kmem_cache_alloc() instead of kmem_cache_alloc_lru(), and the lru argument passed to slab_alloc_node() is NULL. See [1] for the actual fix. However, the return value of list_lru_from_memcg_idx() may be NULL. When list_lru_one is NULL, the kernel will panic directly. Do we need to add error handling when list_lru_one is NULL in list_lru_{add, del}? To avoid hiding the actual BUG(previous changes), we might make the following changes, diff --git a/mm/list_lru.c b/mm/list_lru.c index 3fd64736bc45..fa86d3eff90b 100644 --- a/mm/list_lru.c +++ b/mm/list_lru.c @@ -94,6 +94,9 @@ bool list_lru_add(struct list_lru *lru, struct list_head *item, int nid,         spin_lock(&nlru->lock);         if (list_empty(item)) {                 l = list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(memcg)); +               if (WARN_ON_ONCE(!l)) +                       goto out; +                 list_add_tail(item, &l->list);                 /* Set shrinker bit if the first element was added */                 if (!l->nr_items++) @@ -102,6 +105,7 @@ bool list_lru_add(struct list_lru *lru, struct list_head *item, int nid,                 spin_unlock(&nlru->lock);                 return true;         } +out:         spin_unlock(&nlru->lock);         return false;  } @@ -126,12 +130,16 @@ bool list_lru_del(struct list_lru *lru, struct list_head *item, int nid,         spin_lock(&nlru->lock);         if (!list_empty(item)) {                 l = list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(memcg)); +               if (WARN_ON_ONCE(!l)) +                       goto out; +                 list_del_init(item);                 l->nr_items--;                 nlru->nr_items--;                 spin_unlock(&nlru->lock);                 return true;         } +out:         spin_unlock(&nlru->lock);         return false;  } Link: [1]: https://lore.kernel.org/all/20240716025816.52156-1-youling.tang@linux.dev/ Thanks, Youling.