* [PATCH 0/2] dnotify: Fix ENOMEM handling @ 2017-10-31 9:33 Jan Kara 2017-10-31 9:33 ` [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in fcntl_dirnotify() Jan Kara 2017-10-31 9:33 ` [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly Jan Kara 0 siblings, 2 replies; 12+ messages in thread From: Jan Kara @ 2017-10-31 9:33 UTC (permalink / raw) To: linux-fsdevel; +Cc: amir73il Hello, in response to syzkaller reported I plan to push following two fixes to my tree. Review is welcome. Honza ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in fcntl_dirnotify() 2017-10-31 9:33 [PATCH 0/2] dnotify: Fix ENOMEM handling Jan Kara @ 2017-10-31 9:33 ` Jan Kara 2017-10-31 12:11 ` Amir Goldstein 2017-10-31 9:33 ` [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly Jan Kara 1 sibling, 1 reply; 12+ messages in thread From: Jan Kara @ 2017-10-31 9:33 UTC (permalink / raw) To: linux-fsdevel; +Cc: amir73il, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in fcntl_dirnotify() 2017-10-31 9:33 ` [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in fcntl_dirnotify() Jan Kara @ 2017-10-31 12:11 ` Amir Goldstein 2017-10-31 15:45 ` Jan Kara 0 siblings, 1 reply; 12+ messages in thread From: Amir Goldstein @ 2017-10-31 12:11 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in fcntl_dirnotify() 2017-10-31 12:11 ` Amir Goldstein @ 2017-10-31 15:45 ` Jan Kara 2017-10-31 16:15 ` Jan Kara 0 siblings, 1 reply; 12+ messages in thread From: Jan Kara @ 2017-10-31 15:45 UTC (permalink / raw) To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in fcntl_dirnotify() 2017-10-31 15:45 ` Jan Kara @ 2017-10-31 16:15 ` Jan Kara 2017-10-31 16:28 ` Amir Goldstein 0 siblings, 1 reply; 12+ messages in thread From: Jan Kara @ 2017-10-31 16:15 UTC (permalink / raw) To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel [-- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in fcntl_dirnotify() 2017-10-31 16:15 ` Jan Kara @ 2017-10-31 16:28 ` Amir Goldstein 0 siblings, 0 replies; 12+ messages in thread From: Amir Goldstein @ 2017-10-31 16:28 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly 2017-10-31 9:33 [PATCH 0/2] dnotify: Fix ENOMEM handling Jan Kara 2017-10-31 9:33 ` [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in fcntl_dirnotify() Jan Kara @ 2017-10-31 9:33 ` Jan Kara 2017-10-31 12:26 ` Amir Goldstein 1 sibling, 1 reply; 12+ messages in thread From: Jan Kara @ 2017-10-31 9:33 UTC (permalink / raw) To: linux-fsdevel; +Cc: amir73il, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly 2017-10-31 9:33 ` [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly Jan Kara @ 2017-10-31 12:26 ` Amir Goldstein 2017-10-31 12:50 ` Greg KH 2017-10-31 16:10 ` Jan Kara 0 siblings, 2 replies; 12+ messages in thread From: Amir Goldstein @ 2017-10-31 12:26 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, Greg KH 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly 2017-10-31 12:26 ` Amir Goldstein @ 2017-10-31 12:50 ` Greg KH 2017-10-31 12:57 ` Amir Goldstein 2017-10-31 16:10 ` Jan Kara 1 sibling, 1 reply; 12+ messages in thread From: Greg KH @ 2017-10-31 12:50 UTC (permalink / raw) To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly 2017-10-31 12:50 ` Greg KH @ 2017-10-31 12:57 ` Amir Goldstein 2017-10-31 13:40 ` Greg KH 0 siblings, 1 reply; 12+ messages in thread From: Amir Goldstein @ 2017-10-31 12:57 UTC (permalink / raw) To: Greg KH; +Cc: Jan Kara, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly 2017-10-31 12:57 ` Amir Goldstein @ 2017-10-31 13:40 ` Greg KH 0 siblings, 0 replies; 12+ messages in thread From: Greg KH @ 2017-10-31 13:40 UTC (permalink / raw) To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel 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... ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly 2017-10-31 12:26 ` Amir Goldstein 2017-10-31 12:50 ` Greg KH @ 2017-10-31 16:10 ` Jan Kara 1 sibling, 0 replies; 12+ messages in thread From: Jan Kara @ 2017-10-31 16:10 UTC (permalink / raw) To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel, Greg KH 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-10-31 16:28 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-31 9:33 [PATCH 0/2] dnotify: Fix ENOMEM handling Jan Kara 2017-10-31 9:33 ` [PATCH 1/2] dnotify: Handle errors from fsnotify_add_mark_locked() in fcntl_dirnotify() Jan Kara 2017-10-31 12:11 ` Amir Goldstein 2017-10-31 15:45 ` Jan Kara 2017-10-31 16:15 ` Jan Kara 2017-10-31 16:28 ` Amir Goldstein 2017-10-31 9:33 ` [PATCH 2/2] fsnotify: Protect bail out path of fsnotify_add_mark_locked() properly Jan Kara 2017-10-31 12:26 ` Amir Goldstein 2017-10-31 12:50 ` Greg KH 2017-10-31 12:57 ` Amir Goldstein 2017-10-31 13:40 ` Greg KH 2017-10-31 16:10 ` Jan Kara
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.