Re: [PATCH] mm: list_lru: Fix NULL pointer dereference in list_lru_add()

[Qi Zheng <zhengqi.arch@bytedance.com>]

On 2024/7/17 10:25, Youling Tang wrote:
> 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 <tangyouling@kylinos.cn>
>>>>
>>>> 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}?

Nope, we should pre-allocated the list_lru_one before calling
list_lru_add().

> 
> To avoid hiding the actual BUG(previous changes), we might make the 
> following
> changes,

Even if you do this, you should still modify all callers
to handle this failure.

> 
> 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.