| Message ID | 20171031093342.25216-3-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: > 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 >
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?
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.
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...
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
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);
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(+)