[PATCH 0/2] dnotify: Fix ENOMEM handling

[PATCH 0/2] dnotify: Fix ENOMEM handling

[Jan Kara]

Hello,

in response to syzkaller reported I plan to push following two fixes to
my tree. Review is welcome.

								Honza

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

[Jan Kara]

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(-)

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;
 	}
 
-- 
2.12.3

[PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly

[Jan Kara]

When fsnotify_add_mark_locked() fails it cleans up the mark it was
adding. Since the mark is already visible in group's list, we should
protect update of mark->flags with mark->lock. I'm not aware of any real
issues this could cause (since we also hold group->mark_mutex) but
better be safe and obey locking rules properly.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/mark.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 9991f8826734..47a827975b58 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -599,9 +599,11 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, struct inode *inode,
 
 	return ret;
 err:
+	spin_lock(&mark->lock);
 	mark->flags &= ~(FSNOTIFY_MARK_FLAG_ALIVE |
 			 FSNOTIFY_MARK_FLAG_ATTACHED);
 	list_del_init(&mark->g_list);
+	spin_unlock(&mark->lock);
 	atomic_dec(&group->num_marks);
 
 	fsnotify_put_mark(mark);
-- 
2.12.3

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

[Amir Goldstein]

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
>

Re: [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly

[Amir Goldstein]

On Tue, Oct 31, 2017 at 11:33 AM, Jan Kara <jack@suse.cz> wrote:
> When fsnotify_add_mark_locked() fails it cleans up the mark it was
> adding. Since the mark is already visible in group's list, we should
> protect update of mark->flags with mark->lock. I'm not aware of any real
> issues this could cause (since we also hold group->mark_mutex) but
> better be safe and obey locking rules properly.
>
> Signed-off-by: Jan Kara <jack@suse.cz>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

IMO, even though this does not fix a concrete bug, if it's worth
fixing in upstream, it's worth fixing in stable.
A future stable fix may either make this into a concrete bug
or just be harder to apply.

So I suggest to add the Fixes: and Cc: stable tags.

Greg,

Do you agree with this reasoning?


> ---
>  fs/notify/mark.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 9991f8826734..47a827975b58 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -599,9 +599,11 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, struct inode *inode,
>
>         return ret;
>  err:
> +       spin_lock(&mark->lock);
>         mark->flags &= ~(FSNOTIFY_MARK_FLAG_ALIVE |
>                          FSNOTIFY_MARK_FLAG_ATTACHED);
>         list_del_init(&mark->g_list);
> +       spin_unlock(&mark->lock);
>         atomic_dec(&group->num_marks);
>
>         fsnotify_put_mark(mark);
> --
> 2.12.3
>

Re: [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly

[Greg KH]

On Tue, Oct 31, 2017 at 02:26:35PM +0200, Amir Goldstein wrote:
> On Tue, Oct 31, 2017 at 11:33 AM, Jan Kara <jack@suse.cz> wrote:
> > When fsnotify_add_mark_locked() fails it cleans up the mark it was
> > adding. Since the mark is already visible in group's list, we should
> > protect update of mark->flags with mark->lock. I'm not aware of any real
> > issues this could cause (since we also hold group->mark_mutex) but
> > better be safe and obey locking rules properly.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> IMO, even though this does not fix a concrete bug, if it's worth
> fixing in upstream, it's worth fixing in stable.
> A future stable fix may either make this into a concrete bug
> or just be harder to apply.
> 
> So I suggest to add the Fixes: and Cc: stable tags.
> 
> Greg,
> 
> Do you agree with this reasoning?

If it doesn't fix an actual bug, how does that fit with the stable
kernel rules?

Re: [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly

[Amir Goldstein]

On Tue, Oct 31, 2017 at 2:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 31, 2017 at 02:26:35PM +0200, Amir Goldstein wrote:
>> On Tue, Oct 31, 2017 at 11:33 AM, Jan Kara <jack@suse.cz> wrote:
>> > When fsnotify_add_mark_locked() fails it cleans up the mark it was
>> > adding. Since the mark is already visible in group's list, we should
>> > protect update of mark->flags with mark->lock. I'm not aware of any real
>> > issues this could cause (since we also hold group->mark_mutex) but
>> > better be safe and obey locking rules properly.
>> >
>> > Signed-off-by: Jan Kara <jack@suse.cz>
>>
>> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>>
>> IMO, even though this does not fix a concrete bug, if it's worth
>> fixing in upstream, it's worth fixing in stable.
>> A future stable fix may either make this into a concrete bug
>> or just be harder to apply.
>>
>> So I suggest to add the Fixes: and Cc: stable tags.
>>
>> Greg,
>>
>> Do you agree with this reasoning?
>
> If it doesn't fix an actual bug, how does that fit with the stable
> kernel rules?
>

So this is the case of incorrect code w.r.t locking rules
that either does not hit a bug because of an indirect protection
(as Jan wrote in commit) or we did not find how to hit a bug.

Not sure how you want to call this, but if you think it doesn't belong
for stable we won't send it. That's why I called for your opinion.

Thanks,
Amir.

Re: [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly

[Greg KH]

On Tue, Oct 31, 2017 at 02:57:49PM +0200, Amir Goldstein wrote:
> On Tue, Oct 31, 2017 at 2:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Tue, Oct 31, 2017 at 02:26:35PM +0200, Amir Goldstein wrote:
> >> On Tue, Oct 31, 2017 at 11:33 AM, Jan Kara <jack@suse.cz> wrote:
> >> > When fsnotify_add_mark_locked() fails it cleans up the mark it was
> >> > adding. Since the mark is already visible in group's list, we should
> >> > protect update of mark->flags with mark->lock. I'm not aware of any real
> >> > issues this could cause (since we also hold group->mark_mutex) but
> >> > better be safe and obey locking rules properly.
> >> >
> >> > Signed-off-by: Jan Kara <jack@suse.cz>
> >>
> >> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> >>
> >> IMO, even though this does not fix a concrete bug, if it's worth
> >> fixing in upstream, it's worth fixing in stable.
> >> A future stable fix may either make this into a concrete bug
> >> or just be harder to apply.
> >>
> >> So I suggest to add the Fixes: and Cc: stable tags.
> >>
> >> Greg,
> >>
> >> Do you agree with this reasoning?
> >
> > If it doesn't fix an actual bug, how does that fit with the stable
> > kernel rules?
> >
> 
> So this is the case of incorrect code w.r.t locking rules
> that either does not hit a bug because of an indirect protection
> (as Jan wrote in commit) or we did not find how to hit a bug.
> 
> Not sure how you want to call this, but if you think it doesn't belong
> for stable we won't send it. That's why I called for your opinion.

How about we get the opinion of the developer and maintainer of the
subsystem, I will defer to them...

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

[Jan Kara]

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

Re: [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly

[Jan Kara]

On Tue 31-10-17 14:26:35, Amir Goldstein wrote:
> On Tue, Oct 31, 2017 at 11:33 AM, Jan Kara <jack@suse.cz> wrote:
> > When fsnotify_add_mark_locked() fails it cleans up the mark it was
> > adding. Since the mark is already visible in group's list, we should
> > protect update of mark->flags with mark->lock. I'm not aware of any real
> > issues this could cause (since we also hold group->mark_mutex) but
> > better be safe and obey locking rules properly.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> IMO, even though this does not fix a concrete bug, if it's worth
> fixing in upstream, it's worth fixing in stable.
> A future stable fix may either make this into a concrete bug
> or just be harder to apply.
> 
> So I suggest to add the Fixes: and Cc: stable tags.
> 
> Greg,
> 
> Do you agree with this reasoning?

Similarly as with patch 1, I don't think real users can hit this (and even
if they could, I doubt it would have any negative impact). So bothering
stable with this is just a waste of resources... It falls under the
"theoretical race condition" cathegory which is explicitely forbidden from
stable.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On Tue 31-10-17 16:45:57, Jan Kara wrote:
> 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:
> > > 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.

Attached is a new version of the patch.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-dnotify-Handle-errors-from-fsnotify_add_mark_locked-.patch --]
[-- Type: text/x-patch, Size: 1713 bytes --]

>From 54abcb76cd57877794e2007ac944d8437baf49c1 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 31 Oct 2017 09:53:28 +0100
Subject: [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in
 fcntl_dirnotify()

fsnotify_add_mark_locked() can fail but we do not check its return
value. This didn't matter before commit 9dd813c15b2c "fsnotify: Move
mark list head from object into dedicated structure" as none of possible
failures could happen for dnotify but after that commit -ENOMEM can be
returned. 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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index cba328315929..63a1ca4b9dee 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -319,7 +319,11 @@ 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) {
+			mutex_unlock(&dnotify_group->mark_mutex);
+			goto out_err;
+		}
 		spin_lock(&new_fsn_mark->lock);
 		fsn_mark = new_fsn_mark;
 		dn_mark = new_dn_mark;
@@ -345,6 +349,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

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

[Amir Goldstein]

On Tue, Oct 31, 2017 at 6:15 PM, Jan Kara <jack@suse.cz> wrote:
> On Tue 31-10-17 16:45:57, Jan Kara wrote:
>> 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:
>> > > 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.
>
> Attached is a new version of the patch.


Reviewed-by: Amir Goldstein <amir73il@gmail.com>