Re: [RFC PATCH] writeback, cgroup: fix null-ptr-deref write in bdi_split_work_to_wbs

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

 



Sorry for the noise, my understanding of this issue is wrong.

Please ignore this patch.

I will send a v2 patch.


On 2023/4/6 22:02, Baokun Li wrote:
KASAN report null-ptr-deref:
==================================================================
BUG: KASAN: null-ptr-deref in bdi_split_work_to_wbs+0x6ce/0x6e0
Write of size 8 at addr 0000000000000000 by task syz-executor.3/3514

CPU: 3 PID: 3514 Comm: syz-executor.3 Not tainted 5.10.0-dirty #1
Call Trace:
  dump_stack+0xbe/0xfd
  __kasan_report.cold+0x34/0x84
  kasan_report+0x3a/0x50
  check_memory_region+0xfd/0x1f0
  bdi_split_work_to_wbs+0x6ce/0x6e0
  __writeback_inodes_sb_nr+0x184/0x1f0
  try_to_writeback_inodes_sb+0x7f/0xa0
  ext4_nonda_switch+0x125/0x130
  ext4_da_write_begin+0x126/0x6e0
  generic_perform_write+0x199/0x3a0
  ext4_buffered_write_iter+0x16d/0x2b0
  ext4_file_write_iter+0xea/0x140
  new_sync_write+0x2fa/0x430
  vfs_write+0x4a1/0x570
  ksys_write+0xf6/0x1f0
  do_syscall_64+0x30/0x40
  entry_SYSCALL_64_after_hwframe+0x61/0xc6
RIP: 0033:0x45513d
[...]
==================================================================

Above issue may happen as follows:

             cpu1                        cpu2
----------------------------|----------------------------
ext4_nonda_switch
  try_to_writeback_inodes_sb
   __writeback_inodes_sb_nr
    bdi_split_work_to_wbs
     kmalloc(sizeof(*work), GFP_ATOMIC)  ---> alloc mem failed
                                 inode_switch_wbs
                                  inode_switch_wbs_work_fn
                                   wb_put_many
                                    percpu_ref_put_many
                                     ref->data->release(ref)
                                      cgwb_release
                                       &wb->release_work
                                        cgwb_release_workfn
                                         percpu_ref_exit
                                          ref->data = NULL
                                          kfree(data)
     wb_get(wb)
      percpu_ref_get(&wb->refcnt)
       percpu_ref_get_many(ref, 1)
        atomic_long_add(nr, &ref->data->count) ---> ref->data = NULL
         atomic64_add(i, v) ---> trigger null-ptr-deref

bdi_split_work_to_wbs() traverses &bdi->wb_list to split work into all wbs.
If the allocation of new work fails, the on-stack fallback will be used and
the reference count of the current wb is increased afterwards. If cgroup
writeback membership switches occur before getting the reference count and
the current wb is released as old_wd, then calling wb_get() or wb_put()
will trigger the null pointer dereference above.

A similar problem is fixed in commit 7fc5854f8c6e ("writeback: synchronize
sync(2) against cgroup writeback membership switches"), but the patch only
adds locks to sync_inodes_sb() and not to the __writeback_inodes_sb_nr()
function that also calls bdi_split_work_to_wbs() function. So avoid the
above race by adding the same lock to __writeback_inodes_sb_nr() and
expanding the range of wb_switch_rwsem held in inode_switch_wbs_work_fn().

Fixes: b817525a4a80 ("writeback: bdi_writeback iteration must not skip dying ones")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
---
  fs/fs-writeback.c | 12 ++++++++++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 195dc23e0d83..52825aaf549b 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -506,13 +506,13 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
  	spin_unlock(&new_wb->list_lock);
  	spin_unlock(&old_wb->list_lock);
- up_read(&bdi->wb_switch_rwsem);
-
  	if (nr_switched) {
  		wb_wakeup(new_wb);
  		wb_put_many(old_wb, nr_switched);
  	}
+ up_read(&bdi->wb_switch_rwsem);
+
  	for (inodep = isw->inodes; *inodep; inodep++)
  		iput(*inodep);
  	wb_put(new_wb);
@@ -936,6 +936,11 @@ static long wb_split_bdi_pages(struct bdi_writeback *wb, long nr_pages)
   * have dirty inodes.  If @base_work->nr_page isn't %LONG_MAX, it's
   * distributed to the busy wbs according to each wb's proportion in the
   * total active write bandwidth of @bdi.
+ *
+ * Called under &bdi->wb_switch_rwsem, otherwise bdi_split_work_to_wbs()
+ * may race against cgwb (cgroup writeback) membership switches, which may
+ * cause some inodes to fail to write back, or even trigger a null pointer
+ * dereference using a freed wb.
   */
  static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
  				  struct wb_writeback_work *base_work,
@@ -2637,8 +2642,11 @@ static void __writeback_inodes_sb_nr(struct super_block *sb, unsigned long nr,
  		return;
  	WARN_ON(!rwsem_is_locked(&sb->s_umount));
+ /* protect against inode wb switch, see inode_switch_wbs_work_fn() */
+	bdi_down_write_wb_switch_rwsem(bdi);
  	bdi_split_work_to_wbs(sb->s_bdi, &work, skip_if_busy);
  	wb_wait_for_completion(&done);
+	bdi_up_write_wb_switch_rwsem(bdi);
  }
/**
--
With Best Regards,
Baokun Li
.



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux