Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Sun, Dec 10, 2017 at 11:05:15AM +0100, Michal Hocko wrote: > > My unregister_shrinker() fortification patch > > ( > > http://lkml.kernel.org/r/1511523385-6433-1-git-send-email-penguin-ker...@i-love.sakura.ne.jp > > ) > > is not yet in the mmotm tree due to disagreement between Michal and I, but > > you can throw your sget_userns() patch into vfs.git#for-linus anyway. > > We will eventually apply unregister_shrinker() fortification patch. > > I've acked the patch > http://lkml.kernel.org/r/20171124122148.qevmiogh3pzr4...@dhcp22.suse.cz > I disagreed with your must_check patch which has nothing to do with the > patch disussed here. I've also suggested some changelog clarifications. > Please repost and I am pretty sure Andew will pick it up. Hell, I will pick it up, add sget one on top of that and put both into #for-linus.
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Sun, Dec 10, 2017 at 11:05:15AM +0100, Michal Hocko wrote: > > My unregister_shrinker() fortification patch > > ( > > http://lkml.kernel.org/r/1511523385-6433-1-git-send-email-penguin-ker...@i-love.sakura.ne.jp > > ) > > is not yet in the mmotm tree due to disagreement between Michal and I, but > > you can throw your sget_userns() patch into vfs.git#for-linus anyway. > > We will eventually apply unregister_shrinker() fortification patch. > > I've acked the patch > http://lkml.kernel.org/r/20171124122148.qevmiogh3pzr4...@dhcp22.suse.cz > I disagreed with your must_check patch which has nothing to do with the > patch disussed here. I've also suggested some changelog clarifications. > Please repost and I am pretty sure Andew will pick it up. Hell, I will pick it up, add sget one on top of that and put both into #for-linus.
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Sun 10-12-17 11:33:18, Tetsuo Handa wrote: > Al Viro wrote: > > On Sat, Dec 09, 2017 at 08:59:22PM +, Al Viro wrote: > > > On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote: > > > > On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > > > > > deactivate_locked_super(s); > > > > > s = ERR_PTR(-ENOMEM); > > > > > } > > > > > return s; > > > > > and be done with that. All there is to it... > > > > > > > > Al, do you plan to push this fix? Tetsuo's unregister_shrinker > > > > fortification is already in the mmotm tree. > > > > > > Is it in any git branch I could pull from? Or I could just throw it > > > into vfs.git#for-linus before the fix above - up to you, folks... > > > > Actually, looking at mmotm... I don't see it there. Which patch > > is it in? > > > > My unregister_shrinker() fortification patch > ( > http://lkml.kernel.org/r/1511523385-6433-1-git-send-email-penguin-ker...@i-love.sakura.ne.jp > ) > is not yet in the mmotm tree due to disagreement between Michal and I, but > you can throw your sget_userns() patch into vfs.git#for-linus anyway. > We will eventually apply unregister_shrinker() fortification patch. I've acked the patch http://lkml.kernel.org/r/20171124122148.qevmiogh3pzr4...@dhcp22.suse.cz I disagreed with your must_check patch which has nothing to do with the patch disussed here. I've also suggested some changelog clarifications. Please repost and I am pretty sure Andew will pick it up. -- Michal Hocko SUSE Labs
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Sun 10-12-17 11:33:18, Tetsuo Handa wrote: > Al Viro wrote: > > On Sat, Dec 09, 2017 at 08:59:22PM +, Al Viro wrote: > > > On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote: > > > > On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > > > > > deactivate_locked_super(s); > > > > > s = ERR_PTR(-ENOMEM); > > > > > } > > > > > return s; > > > > > and be done with that. All there is to it... > > > > > > > > Al, do you plan to push this fix? Tetsuo's unregister_shrinker > > > > fortification is already in the mmotm tree. > > > > > > Is it in any git branch I could pull from? Or I could just throw it > > > into vfs.git#for-linus before the fix above - up to you, folks... > > > > Actually, looking at mmotm... I don't see it there. Which patch > > is it in? > > > > My unregister_shrinker() fortification patch > ( > http://lkml.kernel.org/r/1511523385-6433-1-git-send-email-penguin-ker...@i-love.sakura.ne.jp > ) > is not yet in the mmotm tree due to disagreement between Michal and I, but > you can throw your sget_userns() patch into vfs.git#for-linus anyway. > We will eventually apply unregister_shrinker() fortification patch. I've acked the patch http://lkml.kernel.org/r/20171124122148.qevmiogh3pzr4...@dhcp22.suse.cz I disagreed with your must_check patch which has nothing to do with the patch disussed here. I've also suggested some changelog clarifications. Please repost and I am pretty sure Andew will pick it up. -- Michal Hocko SUSE Labs
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
Al Viro wrote: > On Sat, Dec 09, 2017 at 08:59:22PM +, Al Viro wrote: > > On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote: > > > On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > > > > deactivate_locked_super(s); > > > > s = ERR_PTR(-ENOMEM); > > > > } > > > > return s; > > > > and be done with that. All there is to it... > > > > > > Al, do you plan to push this fix? Tetsuo's unregister_shrinker > > > fortification is already in the mmotm tree. > > > > Is it in any git branch I could pull from? Or I could just throw it > > into vfs.git#for-linus before the fix above - up to you, folks... > > Actually, looking at mmotm... I don't see it there. Which patch > is it in? > My unregister_shrinker() fortification patch ( http://lkml.kernel.org/r/1511523385-6433-1-git-send-email-penguin-ker...@i-love.sakura.ne.jp ) is not yet in the mmotm tree due to disagreement between Michal and I, but you can throw your sget_userns() patch into vfs.git#for-linus anyway. We will eventually apply unregister_shrinker() fortification patch. I'm observing whether people notice my __must_check annotation patch ( http://lkml.kernel.org/r/1511523385-6433-2-git-send-email-penguin-ker...@i-love.sakura.ne.jp ) in linux-next.git and fix register_shrinker() callers. If people do not fix the callers, both patches need to be sent to linux.git.
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
Al Viro wrote: > On Sat, Dec 09, 2017 at 08:59:22PM +, Al Viro wrote: > > On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote: > > > On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > > > > deactivate_locked_super(s); > > > > s = ERR_PTR(-ENOMEM); > > > > } > > > > return s; > > > > and be done with that. All there is to it... > > > > > > Al, do you plan to push this fix? Tetsuo's unregister_shrinker > > > fortification is already in the mmotm tree. > > > > Is it in any git branch I could pull from? Or I could just throw it > > into vfs.git#for-linus before the fix above - up to you, folks... > > Actually, looking at mmotm... I don't see it there. Which patch > is it in? > My unregister_shrinker() fortification patch ( http://lkml.kernel.org/r/1511523385-6433-1-git-send-email-penguin-ker...@i-love.sakura.ne.jp ) is not yet in the mmotm tree due to disagreement between Michal and I, but you can throw your sget_userns() patch into vfs.git#for-linus anyway. We will eventually apply unregister_shrinker() fortification patch. I'm observing whether people notice my __must_check annotation patch ( http://lkml.kernel.org/r/1511523385-6433-2-git-send-email-penguin-ker...@i-love.sakura.ne.jp ) in linux-next.git and fix register_shrinker() callers. If people do not fix the callers, both patches need to be sent to linux.git.
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Sat, Dec 09, 2017 at 08:59:22PM +, Al Viro wrote: > On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote: > > On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > > > deactivate_locked_super(s); > > > s = ERR_PTR(-ENOMEM); > > > } > > > return s; > > > and be done with that. All there is to it... > > > > Al, do you plan to push this fix? Tetsuo's unregister_shrinker > > fortification is already in the mmotm tree. > > Is it in any git branch I could pull from? Or I could just throw it > into vfs.git#for-linus before the fix above - up to you, folks... Actually, looking at mmotm... I don't see it there. Which patch is it in?
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Sat, Dec 09, 2017 at 08:59:22PM +, Al Viro wrote: > On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote: > > On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > > > deactivate_locked_super(s); > > > s = ERR_PTR(-ENOMEM); > > > } > > > return s; > > > and be done with that. All there is to it... > > > > Al, do you plan to push this fix? Tetsuo's unregister_shrinker > > fortification is already in the mmotm tree. > > Is it in any git branch I could pull from? Or I could just throw it > into vfs.git#for-linus before the fix above - up to you, folks... Actually, looking at mmotm... I don't see it there. Which patch is it in?
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote: > On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > > deactivate_locked_super(s); > > s = ERR_PTR(-ENOMEM); > > } > > return s; > > and be done with that. All there is to it... > > Al, do you plan to push this fix? Tetsuo's unregister_shrinker > fortification is already in the mmotm tree. Is it in any git branch I could pull from? Or I could just throw it into vfs.git#for-linus before the fix above - up to you, folks...
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Wed, Nov 29, 2017 at 12:55:15PM +0100, Michal Hocko wrote: > On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > > deactivate_locked_super(s); > > s = ERR_PTR(-ENOMEM); > > } > > return s; > > and be done with that. All there is to it... > > Al, do you plan to push this fix? Tetsuo's unregister_shrinker > fortification is already in the mmotm tree. Is it in any git branch I could pull from? Or I could just throw it into vfs.git#for-linus before the fix above - up to you, folks...
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > deactivate_locked_super(s); > s = ERR_PTR(-ENOMEM); > } > return s; > and be done with that. All there is to it... Al, do you plan to push this fix? Tetsuo's unregister_shrinker fortification is already in the mmotm tree. -- Michal Hocko SUSE Labs
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > deactivate_locked_super(s); > s = ERR_PTR(-ENOMEM); > } > return s; > and be done with that. All there is to it... Al, do you plan to push this fix? Tetsuo's unregister_shrinker fortification is already in the mmotm tree. -- Michal Hocko SUSE Labs
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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_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(>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-ker...@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
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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_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(>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-ker...@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
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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_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(>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-ker...@i-love.sakura.ne.jp with patch description updated to include Syzbot report?
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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_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(>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-ker...@i-love.sakura.ne.jp with patch description updated to include Syzbot report?
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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_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(>list); return -ENOMEM; }
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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_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(>list); return -ENOMEM; }
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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_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.
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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_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.
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Thu 23-11-17 16:02:11, Michal Hocko wrote: > On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > > deactivate_locked_super(s); > > s = ERR_PTR(-ENOMEM); > > } > > return s; > > and be done with that. All there is to it... > > Who is going to unregister that shrinker on other failure paths? Scratch that. I've mixed destroy_unused_super with deactivate_locked_super. Go with whatever works... -- Michal Hocko SUSE Labs
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Thu 23-11-17 16:02:11, Michal Hocko wrote: > On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > > deactivate_locked_super(s); > > s = ERR_PTR(-ENOMEM); > > } > > return s; > > and be done with that. All there is to it... > > Who is going to unregister that shrinker on other failure paths? Scratch that. I've mixed destroy_unused_super with deactivate_locked_super. Go with whatever works... -- Michal Hocko SUSE Labs
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > deactivate_locked_super(s); > s = ERR_PTR(-ENOMEM); > } > return s; > and be done with that. All there is to it... Who is going to unregister that shrinker on other failure paths? -- Michal Hocko SUSE Labs
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
On Thu 23-11-17 14:55:40, 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_shrink) != 0)) { > deactivate_locked_super(s); > s = ERR_PTR(-ENOMEM); > } > return s; > and be done with that. All there is to it... Who is going to unregister that shrinker on other failure paths? -- Michal Hocko SUSE Labs
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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_shrink) != 0)) { deactivate_locked_super(s); s = ERR_PTR(-ENOMEM); } return s; and be done with that. All there is to it...
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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_shrink) != 0)) { deactivate_locked_super(s); s = ERR_PTR(-ENOMEM); } return s; and be done with that. All there is to it...
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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. --- >From f2a86a9dc45853149d4f29a5ecff77ec4c827b9f Mon Sep 17 00:00:00 2001 From: Michal HockoDate: Thu, 23 Nov 2017 12:28:35 +0100 Subject: [PATCH] fs: handle shrinker registration failure in sget_userns Syzbot has reported NULL ptr dereference during mntput because of sb shrinker being NULL CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 task: 8801d1dbe5c0 task.stack: 8801c9e38000 RIP: 0010:__list_del_entry_valid+0x7e/0x150 lib/list_debug.c:51 RSP: 0018:8801c9e3f108 EFLAGS: 00010246 RAX: dc00 RBX: RCX: RDX: RSI: 8801c53c6f98 RDI: 8801c53c6fa0 RBP: 8801c9e3f120 R08: 1100393c7d55 R09: 0004 R10: 8801c9e3ef70 R11: R12: R13: dc00 R14: 1100393c7e45 R15: 8801c53c6f98 FS: () GS:8801db30() knlGS: CS: 0010 DS: 002b ES: 002b CR0: 80050033 CR2: dbc23000 CR3: 0001c7269000 CR4: 001406e0 DR0: 2000 DR1: 2000 DR2: DR3: DR6: fffe0ff0 DR7: 0600 Call Trace: __list_del_entry include/linux/list.h:117 [inline] list_del include/linux/list.h:125 [inline] unregister_shrinker+0x79/0x300 mm/vmscan.c:301 deactivate_locked_super+0x64/0xd0 fs/super.c:308 deactivate_super+0x141/0x1b0 fs/super.c:340 cleanup_mnt+0xb2/0x150 fs/namespace.c:1173 mntput_no_expire+0x6e0/0xa90 fs/namespace.c:1237 mntput fs/namespace.c:1247 [inline] kern_unmount+0x9c/0xd0 fs/namespace.c:2999 mq_put_mnt+0x37/0x50 ipc/mqueue.c:1609 put_ipc_ns+0x4d/0x150 ipc/namespace.c:163 free_nsproxy+0xc0/0x1f0 kernel/nsproxy.c:180 switch_task_namespaces+0x9d/0xc0 kernel/nsproxy.c:229 exit_task_namespaces+0x17/0x20 kernel/nsproxy.c:234 do_exit+0x9b0/0x1ad0 kernel/exit.c:864 do_group_exit+0x149/0x400 kernel/exit.c:968 Tetsuo has properly pointed out that the real reason is that fault injection has caused register_shrinker to fail and the error path is not handled in sget_userns. Fix the issue by moving the shrinker registration up when the superblock is allocated and fail early even before we try to register the superblock. This should be safe wrt. parallel shrinker invocation as we are holding s_umount lock which blocks shrinker invocation. The issue is very unlikely to trigger in the production because small allocations do not fail usually. Debugged-by: Tetsuo Handa Signed-off-by: Michal Hocko --- fs/super.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/fs/super.c b/fs/super.c index d4e33e8f1e6f..29edf3d1875b 100644 --- a/fs/super.c +++ b/fs/super.c @@ -155,11 +155,19 @@ static void destroy_super_rcu(struct rcu_head *head) schedule_work(>destroy_work); } -/* Free a superblock that has never been seen by anyone */ +/* + * Free a superblock that has never been seen by anyone. Note that shrinkers + * could have been invoked already but we rely on s_umount to not actually + * touch it. + */ static void destroy_unused_super(struct super_block *s) { if (!s) return; + + if (!list_empty(>s_shrink.list)) + unregister_shrinker(>s_shrink); + up_write(>s_umount); list_lru_destroy(>s_dentry_lru); list_lru_destroy(>s_inode_lru); @@ -190,6 +198,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags, return NULL; INIT_LIST_HEAD(>s_mounts); + INIT_LIST_HEAD(>s_shrink.list); s->s_user_ns = get_user_ns(user_ns); if (security_sb_alloc(s)) @@ -252,6 +261,8 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags, s->s_shrink.count_objects = super_cache_count; s->s_shrink.batch = 1024; s->s_shrink.flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE; + if (register_shrinker(>s_shrink)) + goto fail; return s; fail: @@ -518,7 +529,6 @@ struct super_block *sget_userns(struct file_system_type *type, hlist_add_head(>s_instances, >fs_supers); spin_unlock(_lock); get_filesystem(type); - register_shrinker(>s_shrink); return s; } -- 2.15.0 -- Michal Hocko SUSE Labs
Re: [PATCH v2] fs: handle shrinker registration failure in sget_userns
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. --- >From f2a86a9dc45853149d4f29a5ecff77ec4c827b9f Mon Sep 17 00:00:00 2001 From: Michal Hocko Date: Thu, 23 Nov 2017 12:28:35 +0100 Subject: [PATCH] fs: handle shrinker registration failure in sget_userns Syzbot has reported NULL ptr dereference during mntput because of sb shrinker being NULL CPU: 1 PID: 13231 Comm: syz-executor1 Not tainted 4.14.0-rc8+ #82 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 task: 8801d1dbe5c0 task.stack: 8801c9e38000 RIP: 0010:__list_del_entry_valid+0x7e/0x150 lib/list_debug.c:51 RSP: 0018:8801c9e3f108 EFLAGS: 00010246 RAX: dc00 RBX: RCX: RDX: RSI: 8801c53c6f98 RDI: 8801c53c6fa0 RBP: 8801c9e3f120 R08: 1100393c7d55 R09: 0004 R10: 8801c9e3ef70 R11: R12: R13: dc00 R14: 1100393c7e45 R15: 8801c53c6f98 FS: () GS:8801db30() knlGS: CS: 0010 DS: 002b ES: 002b CR0: 80050033 CR2: dbc23000 CR3: 0001c7269000 CR4: 001406e0 DR0: 2000 DR1: 2000 DR2: DR3: DR6: fffe0ff0 DR7: 0600 Call Trace: __list_del_entry include/linux/list.h:117 [inline] list_del include/linux/list.h:125 [inline] unregister_shrinker+0x79/0x300 mm/vmscan.c:301 deactivate_locked_super+0x64/0xd0 fs/super.c:308 deactivate_super+0x141/0x1b0 fs/super.c:340 cleanup_mnt+0xb2/0x150 fs/namespace.c:1173 mntput_no_expire+0x6e0/0xa90 fs/namespace.c:1237 mntput fs/namespace.c:1247 [inline] kern_unmount+0x9c/0xd0 fs/namespace.c:2999 mq_put_mnt+0x37/0x50 ipc/mqueue.c:1609 put_ipc_ns+0x4d/0x150 ipc/namespace.c:163 free_nsproxy+0xc0/0x1f0 kernel/nsproxy.c:180 switch_task_namespaces+0x9d/0xc0 kernel/nsproxy.c:229 exit_task_namespaces+0x17/0x20 kernel/nsproxy.c:234 do_exit+0x9b0/0x1ad0 kernel/exit.c:864 do_group_exit+0x149/0x400 kernel/exit.c:968 Tetsuo has properly pointed out that the real reason is that fault injection has caused register_shrinker to fail and the error path is not handled in sget_userns. Fix the issue by moving the shrinker registration up when the superblock is allocated and fail early even before we try to register the superblock. This should be safe wrt. parallel shrinker invocation as we are holding s_umount lock which blocks shrinker invocation. The issue is very unlikely to trigger in the production because small allocations do not fail usually. Debugged-by: Tetsuo Handa Signed-off-by: Michal Hocko --- fs/super.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/fs/super.c b/fs/super.c index d4e33e8f1e6f..29edf3d1875b 100644 --- a/fs/super.c +++ b/fs/super.c @@ -155,11 +155,19 @@ static void destroy_super_rcu(struct rcu_head *head) schedule_work(>destroy_work); } -/* Free a superblock that has never been seen by anyone */ +/* + * Free a superblock that has never been seen by anyone. Note that shrinkers + * could have been invoked already but we rely on s_umount to not actually + * touch it. + */ static void destroy_unused_super(struct super_block *s) { if (!s) return; + + if (!list_empty(>s_shrink.list)) + unregister_shrinker(>s_shrink); + up_write(>s_umount); list_lru_destroy(>s_dentry_lru); list_lru_destroy(>s_inode_lru); @@ -190,6 +198,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags, return NULL; INIT_LIST_HEAD(>s_mounts); + INIT_LIST_HEAD(>s_shrink.list); s->s_user_ns = get_user_ns(user_ns); if (security_sb_alloc(s)) @@ -252,6 +261,8 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags, s->s_shrink.count_objects = super_cache_count; s->s_shrink.batch = 1024; s->s_shrink.flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE; + if (register_shrinker(>s_shrink)) + goto fail; return s; fail: @@ -518,7 +529,6 @@ struct super_block *sget_userns(struct file_system_type *type, hlist_add_head(>s_instances, >fs_supers); spin_unlock(_lock); get_filesystem(type); - register_shrinker(>s_shrink); return s; } -- 2.15.0 -- Michal Hocko SUSE Labs