Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns

[Michal Hocko]

On Fri 24-11-17 06:51:09, Tetsuo Handa wrote:
> Al Viro wrote:
> > On Fri, Nov 24, 2017 at 12:35:29AM +0900, Tetsuo Handa wrote:
> > > Al Viro wrote:
> > > > On Fri, Nov 24, 2017 at 12:04:23AM +0900, Tetsuo Handa wrote:
> > > > > Al Viro wrote:
> > > > > > On Thu, Nov 23, 2017 at 03:35:37PM +0100, Michal Hocko wrote:
> > > > > > > Hopefully less screwed version.  But as I've said I am not really
> > > > > > > familiar with the code and do not feel competent to change it so please
> > > > > > > be very careful here. I've moved the shrinker registration to
> > > > > > > alloc_super which turned out to be simpler.
> > > > > > 
> > > > > > I don't get it.  Why the hell do we need all that PITA in the first place?
> > > > > > Just make sget_userns() end with
> > > > > > 	if (unlikely(regsiter_shrinker(&s->s_shrink) != 0)) {
> > > > > > 		deactivate_locked_super(s);
> > > > > > 		s = ERR_PTR(-ENOMEM);
> > > > > > 	}
> > > > > > 	return s;
> > > > > > and be done with that.  All there is to it...
> > > > > > 
> > > > > 
> > > > > Doesn't deactivate_locked_super() call unregister_shrinker() ?
> > > > 
> > > > And?  unregister_shrinker() will do list_del() on empty list_head
> > > > and kfree(NULL); where's the problem with that?
> > > > 
> > > The problem is that calling unregister_shrinker() without successful
> > > register_shrinker() causes crash due to s_shrink.list.{prev,next} == NULL.
> > 
> > *shrug*
> >         shrinker->nr_deferred = kzalloc(size, GFP_KERNEL);
> >         if (!shrinker->nr_deferred) {
> > 		/* make sure it's in consistent state */
> > 		INIT_LIST_HEAD(&shrinker->list);
> >                 return -ENOMEM;
> > 	}
> > 
> > 
> 
> Yes, that will work.
> 
> Michal, like Al thinks, making unregister_shrinker() no-op if
> register_shrinker() failed simplifies things. Can we go with
> http://lkml.kernel.org/r/1511265853-15654-1-git-send-email-penguin-kernel@xxxxxxxxxxxxxxxxxxx
> with patch description updated to include Syzbot report?

I am not opposed to that patch. I just want it to make sure callers _do_
handle the error because a missing shrinker can cause memory pressure
realated issues. unregister_shrinker definitely shouldn't blow up but
I wanted it to warn. This would however trigger a false positive in this
path, right? It is true that the allocation failure would already
trigger warning so the clean up path could be more relaxed. It can be
still quite some time between those two events.

In any case. I do not have a strong preference. If relying on
deactivate_locked_super is really seem much better for the vfs code then 
let's go with your patch without warning.
-- 
Michal Hocko
SUSE Labs