From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752179AbdKXHsm (ORCPT ); Fri, 24 Nov 2017 02:48:42 -0500 Received: from mx2.suse.de ([195.135.220.15]:49901 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751631AbdKXHsk (ORCPT ); Fri, 24 Nov 2017 02:48:40 -0500 Date: Fri, 24 Nov 2017 08:48:37 +0100 From: Michal Hocko To: Tetsuo Handa Cc: viro@ZenIV.linux.org.uk, david@fromorbit.com, jack@suse.cz, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, akpm@linux-foundation.org Subject: Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns Message-ID: <20171124074837.hkxy7fgklkj72uko@dhcp22.suse.cz> References: <20171123145540.GB21978@ZenIV.linux.org.uk> <201711240004.GGE82352.MVFtFLHOOOFSQJ@I-love.SAKURA.ne.jp> <20171123152819.GC21978@ZenIV.linux.org.uk> <201711240035.AGF64509.HLOFFOQSMFOJtV@I-love.SAKURA.ne.jp> <20171123154404.GD21978@ZenIV.linux.org.uk> <201711240651.ADG43703.HOSJtFQFMFOOLV@I-love.SAKURA.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201711240651.ADG43703.HOSJtFQFMFOOLV@I-love.SAKURA.ne.jp> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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@I-love.SAKURA.ne.jp > 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