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 <[email protected]> > 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 <[email protected]> > > 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 <[email protected]>
