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

[Jan Kara <jack@suse.cz>]

On Mon 10-04-23 21:08:26, Baokun Li wrote:
> KASAN report null-ptr-deref:
> ==================================================================
> BUG: KASAN: null-ptr-deref in bdi_split_work_to_wbs+0x5c5/0x7b0
> Write of size 8 at addr 0000000000000000 by task sync/943
> CPU: 5 PID: 943 Comm: sync Tainted: 6.3.0-rc5-next-20230406-dirty #461
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x7f/0xc0
>  print_report+0x2ba/0x340
>  kasan_report+0xc4/0x120
>  kasan_check_range+0x1b7/0x2e0
>  __kasan_check_write+0x24/0x40
>  bdi_split_work_to_wbs+0x5c5/0x7b0
>  sync_inodes_sb+0x195/0x630
>  sync_inodes_one_sb+0x3a/0x50
>  iterate_supers+0x106/0x1b0
>  ksys_sync+0x98/0x160
> [...]
> ==================================================================
> 
> The race that causes the above issue is as follows:
> 
>            cpu1                     cpu2
> -------------------------|-------------------------
> inode_switch_wbs
>  INIT_WORK(&isw->work, inode_switch_wbs_work_fn)
>  queue_rcu_work(isw_wq, &isw->work)
>  // queue_work async
>   inode_switch_wbs_work_fn
>    wb_put_many(old_wb, nr_switched)
>     percpu_ref_put_many
>      ref->data->release(ref)
>      cgwb_release
>       queue_work(cgwb_release_wq, &wb->release_work)
>       // queue_work async
>        &wb->release_work
>        cgwb_release_workfn
>                             ksys_sync
>                              iterate_supers
>                               sync_inodes_one_sb
>                                sync_inodes_sb
>                                 bdi_split_work_to_wbs
>                                  kmalloc(sizeof(*work), GFP_ATOMIC)
>                                  // alloc memory failed
>         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)
>                                      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.
> 
> This issue was introduced in v4.3-rc7 (see fix tag1). Both sync_inodes_sb()
> and __writeback_inodes_sb_nr() calls to bdi_split_work_to_wbs() can trigger
> this issue. For scenarios called via sync_inodes_sb(), originally commit
> 7fc5854f8c6e ("writeback: synchronize sync(2) against cgroup writeback
> membership switches") reduced the possibility of the issue by adding
> wb_switch_rwsem, but in v5.14-rc1 (see fix tag2) removed the
> "inode_io_list_del_locked(inode, old_wb)" from inode_switch_wbs_work_fn()
> so that wb->state contains WB_has_dirty_io, thus old_wb is not skipped
> when traversing wbs in bdi_split_work_to_wbs(), and the issue becomes
> easily reproducible again.
> 
> To solve this problem, percpu_ref_exit() is called under RCU protection
> to avoid race between cgwb_release_workfn() and bdi_split_work_to_wbs().
> Moreover, replace wb_get() with wb_tryget() in bdi_split_work_to_wbs(),
> and skip the current wb if wb_tryget() fails because the wb has already
> been shutdown.
> 
> Fixes: b817525a4a80 ("writeback: bdi_writeback iteration must not skip dying ones")
> Fixes: f3b6a6df38aa ("writeback, cgroup: keep list of inodes attached to bdi_writeback")
> Cc: stable@vger.kernel.org
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> V1->V2:
> 	Use RCU instead of wb_switch_rwsem to avoid race.

The cgwb shutdown code is really messy. But your change looks good to me
and I don't see an easier way around this race. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
>  fs/fs-writeback.c | 17 ++++++++++-------
>  mm/backing-dev.c  | 12 ++++++++++--
>  2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 195dc23e0d83..1db3e3c24b43 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -978,6 +978,16 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
>  			continue;
>  		}
>  
> +		/*
> +		 * If wb_tryget fails, the wb has been shutdown, skip it.
> +		 *
> +		 * Pin @wb so that it stays on @bdi->wb_list.  This allows
> +		 * continuing iteration from @wb after dropping and
> +		 * regrabbing rcu read lock.
> +		 */
> +		if (!wb_tryget(wb))
> +			continue;
> +
>  		/* alloc failed, execute synchronously using on-stack fallback */
>  		work = &fallback_work;
>  		*work = *base_work;
> @@ -986,13 +996,6 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
>  		work->done = &fallback_work_done;
>  
>  		wb_queue_work(wb, work);
> -
> -		/*
> -		 * Pin @wb so that it stays on @bdi->wb_list.  This allows
> -		 * continuing iteration from @wb after dropping and
> -		 * regrabbing rcu read lock.
> -		 */
> -		wb_get(wb);
>  		last_wb = wb;
>  
>  		rcu_read_unlock();
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index ad011308cebe..43b48750b491 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -507,6 +507,15 @@ static LIST_HEAD(offline_cgwbs);
>  static void cleanup_offline_cgwbs_workfn(struct work_struct *work);
>  static DECLARE_WORK(cleanup_offline_cgwbs_work, cleanup_offline_cgwbs_workfn);
>  
> +static void cgwb_free_rcu(struct rcu_head *rcu_head)
> +{
> +	struct bdi_writeback *wb = container_of(rcu_head,
> +			struct bdi_writeback, rcu);
> +
> +	percpu_ref_exit(&wb->refcnt);
> +	kfree(wb);
> +}
> +
>  static void cgwb_release_workfn(struct work_struct *work)
>  {
>  	struct bdi_writeback *wb = container_of(work, struct bdi_writeback,
> @@ -529,11 +538,10 @@ static void cgwb_release_workfn(struct work_struct *work)
>  	list_del(&wb->offline_node);
>  	spin_unlock_irq(&cgwb_lock);
>  
> -	percpu_ref_exit(&wb->refcnt);
>  	wb_exit(wb);
>  	bdi_put(bdi);
>  	WARN_ON_ONCE(!list_empty(&wb->b_attached));
> -	kfree_rcu(wb, rcu);
> +	call_rcu(&wb->rcu, cgwb_free_rcu);
>  }
>  
>  static void cgwb_release(struct percpu_ref *refcnt)
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR