Re: KASAN: use-after-free Read in unregister_shrinker

[Kirill Tkhai]

On 06.06.2019 16:13, J. Bruce Fields wrote:
> On Thu, Jun 06, 2019 at 10:47:43AM +0300, Kirill Tkhai wrote:
>> This may be connected with that shrinker unregistering is forgotten on error path.
> 
> I was wondering about that too.  Seems like it would be hard to hit
> reproduceably though: one of the later allocations would have to fail,
> then later you'd have to create another namespace and this time have a
> later module's init fail.

Yes, it's had to bump into this in real life.

AFAIU, syzbot triggers such the problem by using fault-injections
on allocation places should_failslab()->should_fail(). It's possible
to configure a specific slab, so the allocations will fail with
requested probability.
 
> This is the patch I have, which also fixes a (probably less important)
> failure to free the slab cache.
> 
> --b.
> 
> commit 17c869b35dc9
> Author: J. Bruce Fields <bfields@xxxxxxxxxx>
> Date:   Wed Jun 5 18:03:52 2019 -0400
> 
>     nfsd: fix cleanup of nfsd_reply_cache_init on failure
>     
>     Make sure everything is cleaned up on failure.
>     
>     Especially important for the shrinker, which will otherwise eventually
>     be freed while still referred to by global data structures.
>     
>     Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
> 
> diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
> index ea39497205f0..3dcac164e010 100644
> --- a/fs/nfsd/nfscache.c
> +++ b/fs/nfsd/nfscache.c
> @@ -157,12 +157,12 @@ int nfsd_reply_cache_init(struct nfsd_net *nn)
>  	nn->nfsd_reply_cache_shrinker.seeks = 1;
>  	status = register_shrinker(&nn->nfsd_reply_cache_shrinker);
>  	if (status)
> -		return status;
> +		goto out_nomem;
>  
>  	nn->drc_slab = kmem_cache_create("nfsd_drc",
>  				sizeof(struct svc_cacherep), 0, 0, NULL);
>  	if (!nn->drc_slab)
> -		goto out_nomem;
> +		goto out_shrinker;
>  
>  	nn->drc_hashtbl = kcalloc(hashsize,
>  				sizeof(*nn->drc_hashtbl), GFP_KERNEL);
> @@ -170,7 +170,7 @@ int nfsd_reply_cache_init(struct nfsd_net *nn)
>  		nn->drc_hashtbl = vzalloc(array_size(hashsize,
>  						 sizeof(*nn->drc_hashtbl)));
>  		if (!nn->drc_hashtbl)
> -			goto out_nomem;
> +			goto out_slab;
>  	}
>  
>  	for (i = 0; i < hashsize; i++) {
> @@ -180,6 +180,10 @@ int nfsd_reply_cache_init(struct nfsd_net *nn)
>  	nn->drc_hashsize = hashsize;
>  
>  	return 0;
> +out_slab:
> +	kmem_cache_destroy(nn->drc_slab);
> +out_shrinker:
> +	unregister_shrinker(&nn->nfsd_reply_cache_shrinker);
>  out_nomem:
>  	printk(KERN_ERR "nfsd: failed to allocate reply cache\n");
>  	return -ENOMEM;

Looks OK for me. Feel free to add my reviewed-by if you want.

Reviewed-by: Kirill Tkhai <ktkhai@xxxxxxxxxxxxx>