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>
