[dm-devel] [PATCH] dm zoned: Fix reference counter initial value of chunk works

[Damien Le Moal]

On 2020/02/27 9:18, Shin'ichiro Kawasaki wrote:
> Dm-zoned initializes reference counters of new chunk works with zero
> value and refcount_inc() is called to increment the counter. However, the
> refcount_inc() function handles the addition to zero value as an error
> and triggers the warning as follows:
> 
> refcount_t: addition on 0; use-after-free.
> WARNING: CPU: 7 PID: 1506 at lib/refcount.c:25 refcount_warn_saturate+0x68/0xf0
> Modules linked in: dm_zoned bridge stp llc rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache nf_conntrack_netbios_ns nf_o
> CPU: 7 PID: 1506 Comm: systemd-udevd Not tainted 5.4.0+ #134
> ...
> Call Trace:
>  dmz_map+0x2d2/0x350 [dm_zoned]
>  __map_bio+0x42/0x1a0
>  __split_and_process_non_flush+0x14a/0x1b0
>  __split_and_process_bio+0x83/0x240
>  ? kmem_cache_alloc+0x165/0x220
>  dm_process_bio+0x90/0x230
>  ? generic_make_request_checks+0x2e7/0x680
>  dm_make_request+0x3e/0xb0
>  generic_make_request+0xcf/0x320
>  ? memcg_drain_all_list_lrus+0x1c0/0x1c0
>  submit_bio+0x3c/0x160
>  ? guard_bio_eod+0x2c/0x130
>  mpage_readpages+0x182/0x1d0
>  ? bdev_evict_inode+0xf0/0xf0
>  read_pages+0x6b/0x1b0
>  __do_page_cache_readahead+0x1ba/0x1d0
>  force_page_cache_readahead+0x93/0x100
>  generic_file_read_iter+0x83a/0xe40
>  ? __seccomp_filter+0x7b/0x670
>  new_sync_read+0x12a/0x1c0
>  vfs_read+0x9d/0x150
>  ksys_read+0x5f/0xe0
>  do_syscall_64+0x5b/0x180
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> ...
> 
> After this warning, following refcount API calls for the counter all fail
> to change the counter value.
> 
> Fix this by setting the initial reference counter value not zero but one
> for the new chunk works. Instead, do not call refcount_inc() via
> dmz_get_chunk_work() for the new chunks works.
> 
> The failure was observed with linux version 5.4 with CONFIG_REFCOUNT_FULL
> enabled. Refcount rework was merged to linux version 5.5 by the
> commit 168829ad09ca ("Merge branch 'locking-core-for-linus' of
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip"). After this
> commit, CONFIG_REFCOUNT_FULL was removed and the failure was observed
> regardless of kernel configuration.
> 
> Linux version 4.20 merged the commit 092b5648760a ("dm zoned: target: use
> refcount_t for dm zoned reference counters"). Before this commit, dm
> zoned used atomic_t APIs which does not check addition to zero, then this
> fix is not necessary.
> 
> Fixes: 092b5648760a ("dm zoned: target: use refcount_t for dm zoned reference counters")
> Cc: stable at vger.kernel.org # 5.5
> Cc: stable at vger.kernel.org # 5.4
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki at wdc.com>

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal at wdc.com>


> ---
>  drivers/md/dm-zoned-target.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
> index 70a1063161c0..b1e64cd31647 100644
> --- a/drivers/md/dm-zoned-target.c
> +++ b/drivers/md/dm-zoned-target.c
> @@ -533,8 +533,9 @@ static int dmz_queue_chunk_work(struct dmz_target *dmz, struct bio *bio)
>  
>  	/* Get the BIO chunk work. If one is not active yet, create one */
>  	cw = radix_tree_lookup(&dmz->chunk_rxtree, chunk);
> -	if (!cw) {
> -
> +	if (cw) {
> +		dmz_get_chunk_work(cw);
> +	} else {
>  		/* Create a new chunk work */
>  		cw = kmalloc(sizeof(struct dm_chunk_work), GFP_NOIO);
>  		if (unlikely(!cw)) {
> @@ -543,7 +544,7 @@ static int dmz_queue_chunk_work(struct dmz_target *dmz, struct bio *bio)
>  		}
>  
>  		INIT_WORK(&cw->work, dmz_chunk_work);
> -		refcount_set(&cw->refcount, 0);
> +		refcount_set(&cw->refcount, 1);
>  		cw->target = dmz;
>  		cw->chunk = chunk;
>  		bio_list_init(&cw->bio_list);
> @@ -556,7 +557,6 @@ static int dmz_queue_chunk_work(struct dmz_target *dmz, struct bio *bio)
>  	}
>  
>  	bio_list_add(&cw->bio_list, bio);
> -	dmz_get_chunk_work(cw);
>  
>  	dmz_reclaim_bio_acc(dmz->reclaim);
>  	if (queue_work(dmz->chunk_wq, &cw->work))
> 


-- 
Damien Le Moal
Western Digital Research