Re: [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in fcntl_dirnotify()

[Jan Kara <jack@suse.cz>]

On Tue 31-10-17 14:11:49, Amir Goldstein wrote:
> On Tue, Oct 31, 2017 at 11:33 AM, Jan Kara <jack@suse.cz> 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 <jack@suse.cz>
> > ---
> >  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 <jack@suse.com>
SUSE Labs, CR