From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:34741 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753557AbdJaPp6 (ORCPT ); Tue, 31 Oct 2017 11:45:58 -0400 Date: Tue, 31 Oct 2017 16:45:57 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , linux-fsdevel Subject: Re: [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in fcntl_dirnotify() Message-ID: <20171031154557.GC26128@quack2.suse.cz> References: <20171031093342.25216-1-jack@suse.cz> <20171031093342.25216-2-jack@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue 31-10-17 14:11:49, Amir Goldstein wrote: > On Tue, Oct 31, 2017 at 11:33 AM, Jan Kara wrote: > > fsnotify_add_mark_locked() can fail (e.g. because of ENOMEM since commit > > 9dd813c15b2c "fsnotify: Move mark list head from object into dedicated > > structure"). > > Since we have the Fixes: tag, I rather have the regression commit only in Fixes: > tag (with the subject) and this description can start with: > "fsnotify_add_mark_locked() can fail, but we do not check its return value." Good suggestion, I'll update the changelog. > > Handle this error properly in fcntl_dirnotify() as > > otherwise we just hit BUG_ON(dn_mark->dn) in dnotify_free_mark(). > > > > Reported-by: syzkaller > > Fixes: 9dd813c15b2c101168808d4f5941a29985758973 > > I think for some backport maintainers the "cc: stable # v4.12" tags > matters. Yeah, but frankly I don't think you can trigger this unless you are using fault injection since GFP_KERNEL slab allocations never fail currently (the kernel will rather retry allocation indefinitely). That's why I didn't CC stable as I don't think real users can ever see this and for stable inclusion the bug needs to be a real treat to users... > > > Signed-off-by: Jan Kara > > --- > > fs/notify/dnotify/dnotify.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c > > index cba328315929..a50183bd0ab9 100644 > > --- a/fs/notify/dnotify/dnotify.c > > +++ b/fs/notify/dnotify/dnotify.c > > @@ -319,7 +319,9 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) > > dn_mark = container_of(fsn_mark, struct dnotify_mark, fsn_mark); > > spin_lock(&fsn_mark->lock); > > } else { > > - fsnotify_add_mark_locked(new_fsn_mark, inode, NULL, 0); > > + error = fsnotify_add_mark_locked(new_fsn_mark, inode, NULL, 0); > > + if (error) > > + goto out_err; > > out_err is not unlocking dnotify_group->mark_mutex, and probably need to > put fsn_mark as well? Argh, good point about mark_mutex (I wonder how come this didn't deadlock when I've tested it). You don't need to put fsn_mark - that is guaranteed to be NULL here. Honza -- Jan Kara SUSE Labs, CR