| Message ID | 20171031093342.25216-2-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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." > 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. > 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? > spin_lock(&new_fsn_mark->lock); > fsn_mark = new_fsn_mark; > dn_mark = new_dn_mark; > @@ -345,6 +347,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) > */ > if (dn_mark == new_dn_mark) > destroy = 1; > + error = 0; > goto out; > } > > -- > 2.12.3 >
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
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; spin_lock(&new_fsn_mark->lock); fsn_mark = new_fsn_mark; dn_mark = new_dn_mark; @@ -345,6 +347,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) */ if (dn_mark == new_dn_mark) destroy = 1; + error = 0; goto out; }
fsnotify_add_mark_locked() can fail (e.g. because of ENOMEM since commit 9dd813c15b2c "fsnotify: Move mark list head from object into dedicated structure"). 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 Signed-off-by: Jan Kara <jack@suse.cz> --- fs/notify/dnotify/dnotify.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)