mm: list_lru: Fix NULL pointer dereference in list_lru_add()

From: Youling Tang <[email protected]>

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/[email protected]/T/#m901bb26cdb1d9d4bacebf0d034f0a5a712cc93a6

Quoted 6 lines · Youling Tang

From: Youling Tang <[email protected]>

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.

Quoted 2 lines

The issue was triggered for discussion [1],
Link [1]: https://lore.kernel.org/linux-bcachefs/[email protected]/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?

Hi, Kent

Quoted 7 lines · Kent Overstreet

Quoted 6 lines · Youling Tang

From: Youling Tang <[email protected]>

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.

Quoted 5 lines

Quoted 2 lines

The issue was triggered for discussion [1],
Link [1]: https://lore.kernel.org/linux-bcachefs/[email protected]/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?

We can break it down into two questions (independent of each other):
1) Error handling is necessary when l (lru_list_one) is NULL here.

Quoted 12 lines · Youling Tang

Hi, Kent

Quoted 9 lines · Kent Overstreet

Quoted 8 lines · Youling Tang

From: Youling Tang <[email protected]>

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/[email protected]/T/#m901bb26cdb1d9d4bacebf0d034f0a5a712cc93a6

Quoted 5 lines

Quoted 3 lines

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?

We can break it down into two questions (independent of each other):
1) Error handling is necessary when l (lru_list_one) is NULL here.

No, you're just hiding the actual bug - since I wasn't clear, I'm naking this patch.

Hi, Kent

Quoted 16 lines · Kent Overstreet

Quoted 15 lines · Youling Tang

Hi, Kent

Quoted 12 lines · Kent Overstreet

Quoted 8 lines · Youling Tang

From: Youling Tang <[email protected]>

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/[email protected]/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?

Quoted 4 lines

Quoted 2 lines

We can break it down into two questions (independent of each other):
1) Error handling is necessary when l (lru_list_one) is NULL here.

No, you're just hiding the actual bug - since I wasn't clear, I'm naking this patch.

We should use kmem_cache_alloc_lru() instead of kmem_cache_alloc(), similar to the [1] modification.

Apply the following patch to fix the problem:

fs/bcachefs/fs.c +1 -1

index f9c9a95d7d4c..79a580dfb5e1 100644
@@ -227,7 +227,8 @@ static struct inode *bch2_alloc_inode(struct super_block
 *sb)
 
  static struct bch_inode_info *__bch2_new_inode(struct bch_fs *c)
  {
-       struct bch_inode_info *inode = kmem_cache_alloc(bch2_inode_cache,
 GFP_NOFS);
+       struct bch_inode_info *inode = alloc_inode_sb(c->vfs_sb,
 bch2_inode_cache, GFP_NOFS);

        if (!inode)
                return NULL;

Link [1]: https://lwn.net/ml/linux-kernel/[email protected]/

Thanks,
Youling.

Quoted 6 lines · Youling Tang

From: Youling Tang <[email protected]>

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.

Quoted 2 lines

The issue was triggered for discussion [1],
Link [1]: https://lore.kernel.org/linux-bcachefs/[email protected]/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?

Quoted 10 lines · Kent Overstreet

Quoted 8 lines · Youling Tang

From: Youling Tang <[email protected]>

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/[email protected]/T/#m901bb26cdb1d9d4bacebf0d034f0a5a712cc93a6

Quoted 3 lines

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.

Quoted 13 lines · Qi Zheng

Quoted 10 lines · Kent Overstreet

Quoted 8 lines · Youling Tang

From: Youling Tang <[email protected]>

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/[email protected]/T/#m901bb26cdb1d9d4bacebf0d034f0a5a712cc93a6

Quoted 6 lines

Quoted 3 lines

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,

mm/list_lru.c +3 -0

index 3fd64736bc45..fa86d3eff90b 100644
@@ -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++)

mm/list_lru.c +1 -0

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

 }

mm/list_lru.c +4 -0

@@ -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/[email protected]/

Thanks,
Youling.