On Thu, Jun 6, 2019 at 3:43 PM Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote:
>
> 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.
No fault injection was involved in triggering of this bug.
Fault injection is clearly visible in console log as "INJECTING
FAILURE at this stack track" splats and also for bugs with repros it
would be noted in the syzkaller repro as "fault_call": N. So somehow
this bug was triggered as is.
But overall syzkaller can do better then the old probabilistic
injection. The probabilistic injection tend to both under-test what we
want to test and also crash some system services. syzkaller uses the
new "systematic fault injection" that allows to test specifically each
failure site separately in each syscall separately.
All kernel testing systems should use it. Also in couple with KASAN,
KMEMLEAK, LOCKDEP. It's indispensable in finding kernel bugs.
> > 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>
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/275f77ad-1962-6a60-e60b-6b8845f12c34%40virtuozzo.com.
> For more options, visit https://groups.google.com/d/optout.
