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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxx>

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@xxxxxxxxx/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@xxxxxxxxx/

Thanks,
Youling.




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux