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