[(backport)] fanotify: fix handling of events on child sub-directory
diff mbox series

Message ID 20181127090907.GB5245@veci.piliscsaba.redhat.com
State New
Headers show
Series
  • [(backport)] fanotify: fix handling of events on child sub-directory
Related show

Commit Message

Miklos Szeredi Nov. 27, 2018, 9:09 a.m. UTC
Hi Amir,

Here's a backport of this patch to 4.18 and earlier.  Tested good with
ltp/fanotify09.

I didn't quite understand the relevance of masking against ALL_FSNOTIFY_EVENTS
in __fsnotify_parent() and the backport in fsnotify() is also not quite trivial.

So, can you please review?

Thanks,
Miklos

---
From: Amir Goldstein <amir73il@gmail.com>
Date: Tue, 30 Oct 2018 20:29:53 +0200
Subject: [PATCH] fanotify: fix handling of events on child sub-directory

When an event is reported on a sub-directory and the parent inode has
a mark mask with FS_EVENT_ON_CHILD|FS_ISDIR, the event will be sent to
fsnotify() even if the event type is not in the parent mark mask
(e.g. FS_OPEN).

Further more, if that event happened on a mount or a filesystem with
a mount/sb mark that does have that event type in their mask, the "on
child" event will be reported on the mount/sb mark.  That is not
desired, because user will get a duplicate event for the same action.

Note that the event reported on the victim inode is never merged with
the event reported on the parent inode, because of the check in
should_merge(): old_fsn->inode == new_fsn->inode.

Fix this by looking for a match of an actual event type (i.e. not just
FS_ISDIR) in parent's inode mark mask and by not reporting an "on child"
event to group if event type is only found on mount/sb marks.

[backport hint: The bug seems to have always been in fanotify, but this
                patch will only apply cleanly to v4.19.y]

Cc: <stable@vger.kernel.org> # v4.19
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
(cherry picked from commit b469e7e47c8a075cc08bcd1e85d4365134bdcdd5)
---
 fs/notify/fanotify/fanotify.c | 10 +++++-----
 fs/notify/fsnotify.c          |  8 ++++++--
 2 files changed, 11 insertions(+), 7 deletions(-)

Comments

Amir Goldstein Nov. 27, 2018, 11:24 a.m. UTC | #1
On Tue, Nov 27, 2018 at 11:09 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> Hi Amir,
>
> Here's a backport of this patch to 4.18 and earlier.  Tested good with
> ltp/fanotify09.

AFAICS this backport is identical to my v4.19 backport and yes, it looks fine.
I just missed the fact that my v4.19 does apply cleanly on v4.18 or I would
have asked Greg to apply it.

OTOH, your backport is good for v4.18 not earlier, nothing before:
837a39343847 fanotify: generalize fanotify_should_send_event()

>
> I didn't quite understand the relevance of masking against ALL_FSNOTIFY_EVENTS
> in __fsnotify_parent()

Right... that is because stable is missing:
007d1e8395ea fsnotify: generalize handling of extra event flags

The mask in __fsnotify_parent() (on master) the same as (stable) mask
in fsnotify(),
i.e. test_mask = (mask & ~FS_EVENT_ON_CHILD);
so its a very minor but low hanging optimization once ALL_FSNOTIFY_EVENTS
is defined. So in the backport the mask in __fsnotify_parent() does nothing
but its not critical either.

> and the backport in fsnotify() is also not quite trivial.

It means: if __fsnotify_parent() is reporting this event, then the
event is not intended for a mount mark.
__fsnotify_parent() is always called paired with fsnotify() (not for parent)
and the mount mark will be getting the event in that second call.

So this is a double layered fix for the bug:

The check in fsnotify() optimizes away events reported from __fsnotify_parent()
if the parent inode does NOT have event in commutative mask of ANY
group, but mount mark DOES have event in mask of SOME groups.

The check in fanotify_should_send_event() does the same for a
SPECIFIC group.

>
> So, can you please review?
>

Looks good,

Thanks,
Amir.

> Date: Tue, 30 Oct 2018 20:29:53 +0200
> Subject: [PATCH] fanotify: fix handling of events on child sub-directory
>
> When an event is reported on a sub-directory and the parent inode has
> a mark mask with FS_EVENT_ON_CHILD|FS_ISDIR, the event will be sent to
> fsnotify() even if the event type is not in the parent mark mask
> (e.g. FS_OPEN).
>
> Further more, if that event happened on a mount or a filesystem with
> a mount/sb mark that does have that event type in their mask, the "on
> child" event will be reported on the mount/sb mark.  That is not
> desired, because user will get a duplicate event for the same action.
>
> Note that the event reported on the victim inode is never merged with
> the event reported on the parent inode, because of the check in
> should_merge(): old_fsn->inode == new_fsn->inode.
>
> Fix this by looking for a match of an actual event type (i.e. not just
> FS_ISDIR) in parent's inode mark mask and by not reporting an "on child"
> event to group if event type is only found on mount/sb marks.
>
> [backport hint: The bug seems to have always been in fanotify, but this
>                 patch will only apply cleanly to v4.19.y]
>
> Cc: <stable@vger.kernel.org> # v4.19
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> (cherry picked from commit b469e7e47c8a075cc08bcd1e85d4365134bdcdd5)
> ---
>  fs/notify/fanotify/fanotify.c | 10 +++++-----
>  fs/notify/fsnotify.c          |  8 ++++++--
>  2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index f90842efea13..78126bd7c162 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -113,12 +113,12 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
>                         continue;
>                 mark = iter_info->marks[type];
>                 /*
> -                * if the event is for a child and this inode doesn't care about
> -                * events on the child, don't send it!
> +                * If the event is for a child and this mark doesn't care about
> +                * events on a child, don't send it!
>                  */
> -               if (type == FSNOTIFY_OBJ_TYPE_INODE &&
> -                   (event_mask & FS_EVENT_ON_CHILD) &&
> -                   !(mark->mask & FS_EVENT_ON_CHILD))
> +               if (event_mask & FS_EVENT_ON_CHILD &&
> +                   (type != FSNOTIFY_OBJ_TYPE_INODE ||
> +                    !(mark->mask & FS_EVENT_ON_CHILD)))
>                         continue;
>
>                 marks_mask |= mark->mask;
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index ababdbfab537..46d27b357226 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -158,9 +158,9 @@ int __fsnotify_parent(const struct path *path, struct dentry *dentry, __u32 mask
>         parent = dget_parent(dentry);
>         p_inode = parent->d_inode;
>
> -       if (unlikely(!fsnotify_inode_watches_children(p_inode)))
> +       if (unlikely(!fsnotify_inode_watches_children(p_inode))) {
>                 __fsnotify_update_child_dentry_flags(p_inode);
> -       else if (p_inode->i_fsnotify_mask & mask) {
> +       } else if (p_inode->i_fsnotify_mask & mask & ~FS_EVENT_ON_CHILD) {
>                 struct name_snapshot name;
>
>                 /* we are notifying a parent so come up with the new mask which
> @@ -329,6 +329,10 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
>         else
>                 mnt = NULL;
>
> +       /* An event "on child" is not intended for a mount mark */
> +       if (mask & FS_EVENT_ON_CHILD)
> +               mnt = NULL;
> +
>         /*
>          * Optimization: srcu_read_lock() has a memory barrier which can
>          * be expensive.  It protects walking the *_fsnotify_marks lists.
> --
> 2.14.5
>
Miklos Szeredi Nov. 27, 2018, 11:41 a.m. UTC | #2
On Tue, Nov 27, 2018 at 12:24 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Nov 27, 2018 at 11:09 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > Hi Amir,
> >
> > Here's a backport of this patch to 4.18 and earlier.  Tested good with
> > ltp/fanotify09.
>
> AFAICS this backport is identical to my v4.19 backport and yes, it looks fine.
> I just missed the fact that my v4.19 does apply cleanly on v4.18 or I would
> have asked Greg to apply it.

Where's your v4.19 backport?  I don't see it only any list.

> OTOH, your backport is good for v4.18 not earlier, nothing before:
> 837a39343847 fanotify: generalize fanotify_should_send_event()

Okay.

>
> >
> > I didn't quite understand the relevance of masking against ALL_FSNOTIFY_EVENTS
> > in __fsnotify_parent()
>
> Right... that is because stable is missing:
> 007d1e8395ea fsnotify: generalize handling of extra event flags
>
> The mask in __fsnotify_parent() (on master) the same as (stable) mask
> in fsnotify(),
> i.e. test_mask = (mask & ~FS_EVENT_ON_CHILD);
> so its a very minor but low hanging optimization once ALL_FSNOTIFY_EVENTS
> is defined. So in the backport the mask in __fsnotify_parent() does nothing
> but its not critical either.

Okay, I'll remove it to avoid confusion.   General comment about not
mixing optimization with fix applies here...

>
> > and the backport in fsnotify() is also not quite trivial.
>
> It means: if __fsnotify_parent() is reporting this event, then the
> event is not intended for a mount mark.
> __fsnotify_parent() is always called paired with fsnotify() (not for parent)
> and the mount mark will be getting the event in that second call.
>
> So this is a double layered fix for the bug:
>
> The check in fsnotify() optimizes away events reported from __fsnotify_parent()
> if the parent inode does NOT have event in commutative mask of ANY
> group, but mount mark DOES have event in mask of SOME groups.
>
> The check in fanotify_should_send_event() does the same for a
> SPECIFIC group.

Got it.  So the *real* fix is in fanotify_should_send_event(), the
rest are optimizations.

>
> >
> > So, can you please review?
> >
>
> Looks good,

Thanks for looking.

Miklos
Amir Goldstein Nov. 27, 2018, 2:25 p.m. UTC | #3
On Tue, Nov 27, 2018 at 1:42 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, Nov 27, 2018 at 12:24 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Nov 27, 2018 at 11:09 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > Hi Amir,
> > >
> > > Here's a backport of this patch to 4.18 and earlier.  Tested good with
> > > ltp/fanotify09.
> >
> > AFAICS this backport is identical to my v4.19 backport and yes, it looks fine.
> > I just missed the fact that my v4.19 does apply cleanly on v4.18 or I would
> > have asked Greg to apply it.
>
> Where's your v4.19 backport?  I don't see it only any list.

Hmm, I replied to this message with a patch, but my reply is not in the archive:
https://www.spinics.net/lists/stable-commits/msg103743.html

So I guess my backport patch is nowhere....
I did also CC Greg and Jan, so maybe Greg can say what went wrong?

Anyway, I confirm that your backport is correct for v4.19.y and v4.18.y

Thanks,
Amir.
Jan Kara Nov. 27, 2018, 4:23 p.m. UTC | #4
On Tue 27-11-18 16:25:41, Amir Goldstein wrote:
> On Tue, Nov 27, 2018 at 1:42 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, Nov 27, 2018 at 12:24 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Nov 27, 2018 at 11:09 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > Hi Amir,
> > > >
> > > > Here's a backport of this patch to 4.18 and earlier.  Tested good with
> > > > ltp/fanotify09.
> > >
> > > AFAICS this backport is identical to my v4.19 backport and yes, it looks fine.
> > > I just missed the fact that my v4.19 does apply cleanly on v4.18 or I would
> > > have asked Greg to apply it.
> >
> > Where's your v4.19 backport?  I don't see it only any list.
> 
> Hmm, I replied to this message with a patch, but my reply is not in the archive:
> https://www.spinics.net/lists/stable-commits/msg103743.html
> 
> So I guess my backport patch is nowhere....
> I did also CC Greg and Jan, so maybe Greg can say what went wrong?

Yes, and at least I have received your email.

								Honza
Sasha Levin Nov. 27, 2018, 4:38 p.m. UTC | #5
On Tue, Nov 27, 2018 at 04:25:41PM +0200, Amir Goldstein wrote:
>On Tue, Nov 27, 2018 at 1:42 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> On Tue, Nov 27, 2018 at 12:24 PM Amir Goldstein <amir73il@gmail.com> wrote:
>> >
>> > On Tue, Nov 27, 2018 at 11:09 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>> > >
>> > > Hi Amir,
>> > >
>> > > Here's a backport of this patch to 4.18 and earlier.  Tested good with
>> > > ltp/fanotify09.
>> >
>> > AFAICS this backport is identical to my v4.19 backport and yes, it looks fine.
>> > I just missed the fact that my v4.19 does apply cleanly on v4.18 or I would
>> > have asked Greg to apply it.
>>
>> Where's your v4.19 backport?  I don't see it only any list.
>
>Hmm, I replied to this message with a patch, but my reply is not in the archive:
>https://www.spinics.net/lists/stable-commits/msg103743.html
>
>So I guess my backport patch is nowhere....
>I did also CC Greg and Jan, so maybe Greg can say what went wrong?

I'm guessing you sent it to stable@kernel.org rather than
stable@vger.kernel.org? :)

--
Thanks,
Sasha
Amir Goldstein Nov. 27, 2018, 4:49 p.m. UTC | #6
On Tue, Nov 27, 2018 at 6:38 PM Sasha Levin <sashal@kernel.org> wrote:
>
> On Tue, Nov 27, 2018 at 04:25:41PM +0200, Amir Goldstein wrote:
> >On Tue, Nov 27, 2018 at 1:42 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >>
> >> On Tue, Nov 27, 2018 at 12:24 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >> >
> >> > On Tue, Nov 27, 2018 at 11:09 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >> > >
> >> > > Hi Amir,
> >> > >
> >> > > Here's a backport of this patch to 4.18 and earlier.  Tested good with
> >> > > ltp/fanotify09.
> >> >
> >> > AFAICS this backport is identical to my v4.19 backport and yes, it looks fine.
> >> > I just missed the fact that my v4.19 does apply cleanly on v4.18 or I would
> >> > have asked Greg to apply it.
> >>
> >> Where's your v4.19 backport?  I don't see it only any list.
> >
> >Hmm, I replied to this message with a patch, but my reply is not in the archive:
> >https://www.spinics.net/lists/stable-commits/msg103743.html
> >
> >So I guess my backport patch is nowhere....
> >I did also CC Greg and Jan, so maybe Greg can say what went wrong?
>
> I'm guessing you sent it to stable@kernel.org rather than
> stable@vger.kernel.org? :)
>

Nope.
I did not compose this email.
I replied to Greg's email (the one the I linked above).

Thanks,
Amir.
Greg Kroah-Hartman Nov. 28, 2018, 11:02 a.m. UTC | #7
On Tue, Nov 27, 2018 at 05:23:54PM +0100, Jan Kara wrote:
> On Tue 27-11-18 16:25:41, Amir Goldstein wrote:
> > On Tue, Nov 27, 2018 at 1:42 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Tue, Nov 27, 2018 at 12:24 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Tue, Nov 27, 2018 at 11:09 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > >
> > > > > Hi Amir,
> > > > >
> > > > > Here's a backport of this patch to 4.18 and earlier.  Tested good with
> > > > > ltp/fanotify09.
> > > >
> > > > AFAICS this backport is identical to my v4.19 backport and yes, it looks fine.
> > > > I just missed the fact that my v4.19 does apply cleanly on v4.18 or I would
> > > > have asked Greg to apply it.
> > >
> > > Where's your v4.19 backport?  I don't see it only any list.
> > 
> > Hmm, I replied to this message with a patch, but my reply is not in the archive:
> > https://www.spinics.net/lists/stable-commits/msg103743.html
> > 
> > So I guess my backport patch is nowhere....
> > I did also CC Greg and Jan, so maybe Greg can say what went wrong?
> 
> Yes, and at least I have received your email.

I have it too, it's in my queue.  So, should I take Amir's patch or this
one?

thanks,

greg k-h
Greg Kroah-Hartman Nov. 28, 2018, 11:03 a.m. UTC | #8
On Wed, Nov 28, 2018 at 12:02:07PM +0100, Greg KH wrote:
> On Tue, Nov 27, 2018 at 05:23:54PM +0100, Jan Kara wrote:
> > On Tue 27-11-18 16:25:41, Amir Goldstein wrote:
> > > On Tue, Nov 27, 2018 at 1:42 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Tue, Nov 27, 2018 at 12:24 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > On Tue, Nov 27, 2018 at 11:09 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > > >
> > > > > > Hi Amir,
> > > > > >
> > > > > > Here's a backport of this patch to 4.18 and earlier.  Tested good with
> > > > > > ltp/fanotify09.
> > > > >
> > > > > AFAICS this backport is identical to my v4.19 backport and yes, it looks fine.
> > > > > I just missed the fact that my v4.19 does apply cleanly on v4.18 or I would
> > > > > have asked Greg to apply it.
> > > >
> > > > Where's your v4.19 backport?  I don't see it only any list.
> > > 
> > > Hmm, I replied to this message with a patch, but my reply is not in the archive:
> > > https://www.spinics.net/lists/stable-commits/msg103743.html
> > > 
> > > So I guess my backport patch is nowhere....
> > > I did also CC Greg and Jan, so maybe Greg can say what went wrong?
> > 
> > Yes, and at least I have received your email.
> 
> I have it too, it's in my queue.  So, should I take Amir's patch or this
> one?

Oh nevermind, they are the same, I've added Amir's patch now...
Amir Goldstein Nov. 28, 2018, 11:58 a.m. UTC | #9
On Wed, Nov 28, 2018 at 1:03 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Nov 28, 2018 at 12:02:07PM +0100, Greg KH wrote:
> > On Tue, Nov 27, 2018 at 05:23:54PM +0100, Jan Kara wrote:
> > > On Tue 27-11-18 16:25:41, Amir Goldstein wrote:
> > > > On Tue, Nov 27, 2018 at 1:42 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > >
> > > > > On Tue, Nov 27, 2018 at 12:24 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Nov 27, 2018 at 11:09 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > > > >
> > > > > > > Hi Amir,
> > > > > > >
> > > > > > > Here's a backport of this patch to 4.18 and earlier.  Tested good with
> > > > > > > ltp/fanotify09.
> > > > > >
> > > > > > AFAICS this backport is identical to my v4.19 backport and yes, it looks fine.
> > > > > > I just missed the fact that my v4.19 does apply cleanly on v4.18 or I would
> > > > > > have asked Greg to apply it.
> > > > >
> > > > > Where's your v4.19 backport?  I don't see it only any list.
> > > >
> > > > Hmm, I replied to this message with a patch, but my reply is not in the archive:
> > > > https://www.spinics.net/lists/stable-commits/msg103743.html
> > > >
> > > > So I guess my backport patch is nowhere....
> > > > I did also CC Greg and Jan, so maybe Greg can say what went wrong?
> > >
> > > Yes, and at least I have received your email.
> >
> > I have it too, it's in my queue.  So, should I take Amir's patch or this
> > one?
>
> Oh nevermind, they are the same, I've added Amir's patch now...

Two notes:
1. You can apply same patch to v4.18 (Miklos tested it).
2. Patches are not the same on the line:
Miklos:
-       else if (p_inode->i_fsnotify_mask & mask) {
+       } else if (p_inode->i_fsnotify_mask & mask & ~FS_EVENT_ON_CHILD) {
Amir:
-       else if (p_inode->i_fsnotify_mask & mask) {
+       } else if (p_inode->i_fsnotify_mask & mask & ALL_FSNOTIFY_EVENTS) {

Miklos' patch is more accurate than mine.
OTOH, my patch keeps the code more similar in master and stable.
The difference is insignificant from user perspective.
Its a very very minor optimization that my backport did not get right.

If we want to have the best of both worlds (correctness and similar to master)
you may apply this commit from upstream before my backport patch:
007d1e8395ea fsnotify: generalize handling of extra event flags

This patch removes FS_EVENT_ON_CHILD from ALL_FSNOTIFY_EVENTS.
I verified that it applies cleanly on 4.19.y and 4.18.y.

That would be my preferred option.

Thanks,
Amir.
Greg Kroah-Hartman Nov. 28, 2018, 1:19 p.m. UTC | #10
On Wed, Nov 28, 2018 at 01:58:15PM +0200, Amir Goldstein wrote:
> On Wed, Nov 28, 2018 at 1:03 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Nov 28, 2018 at 12:02:07PM +0100, Greg KH wrote:
> > > On Tue, Nov 27, 2018 at 05:23:54PM +0100, Jan Kara wrote:
> > > > On Tue 27-11-18 16:25:41, Amir Goldstein wrote:
> > > > > On Tue, Nov 27, 2018 at 1:42 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > > >
> > > > > > On Tue, Nov 27, 2018 at 12:24 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > > >
> > > > > > > On Tue, Nov 27, 2018 at 11:09 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > > > > >
> > > > > > > > Hi Amir,
> > > > > > > >
> > > > > > > > Here's a backport of this patch to 4.18 and earlier.  Tested good with
> > > > > > > > ltp/fanotify09.
> > > > > > >
> > > > > > > AFAICS this backport is identical to my v4.19 backport and yes, it looks fine.
> > > > > > > I just missed the fact that my v4.19 does apply cleanly on v4.18 or I would
> > > > > > > have asked Greg to apply it.
> > > > > >
> > > > > > Where's your v4.19 backport?  I don't see it only any list.
> > > > >
> > > > > Hmm, I replied to this message with a patch, but my reply is not in the archive:
> > > > > https://www.spinics.net/lists/stable-commits/msg103743.html
> > > > >
> > > > > So I guess my backport patch is nowhere....
> > > > > I did also CC Greg and Jan, so maybe Greg can say what went wrong?
> > > >
> > > > Yes, and at least I have received your email.
> > >
> > > I have it too, it's in my queue.  So, should I take Amir's patch or this
> > > one?
> >
> > Oh nevermind, they are the same, I've added Amir's patch now...
> 
> Two notes:
> 1. You can apply same patch to v4.18 (Miklos tested it).

4.18 is end-of-life, sorry, not touching that anymore.

> 2. Patches are not the same on the line:
> Miklos:
> -       else if (p_inode->i_fsnotify_mask & mask) {
> +       } else if (p_inode->i_fsnotify_mask & mask & ~FS_EVENT_ON_CHILD) {
> Amir:
> -       else if (p_inode->i_fsnotify_mask & mask) {
> +       } else if (p_inode->i_fsnotify_mask & mask & ALL_FSNOTIFY_EVENTS) {
> 
> Miklos' patch is more accurate than mine.
> OTOH, my patch keeps the code more similar in master and stable.

I would prefer to keep things aligned with what is in Linus's tree where
ever possible.

> The difference is insignificant from user perspective.
> Its a very very minor optimization that my backport did not get right.
> 
> If we want to have the best of both worlds (correctness and similar to master)
> you may apply this commit from upstream before my backport patch:
> 007d1e8395ea fsnotify: generalize handling of extra event flags
> 
> This patch removes FS_EVENT_ON_CHILD from ALL_FSNOTIFY_EVENTS.
> I verified that it applies cleanly on 4.19.y and 4.18.y.
> 
> That would be my preferred option.

Ok, I've now done that, thanks.

greg k-h

Patch
diff mbox series

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index f90842efea13..78126bd7c162 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -113,12 +113,12 @@  static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
 			continue;
 		mark = iter_info->marks[type];
 		/*
-		 * if the event is for a child and this inode doesn't care about
-		 * events on the child, don't send it!
+		 * If the event is for a child and this mark doesn't care about
+		 * events on a child, don't send it!
 		 */
-		if (type == FSNOTIFY_OBJ_TYPE_INODE &&
-		    (event_mask & FS_EVENT_ON_CHILD) &&
-		    !(mark->mask & FS_EVENT_ON_CHILD))
+		if (event_mask & FS_EVENT_ON_CHILD &&
+		    (type != FSNOTIFY_OBJ_TYPE_INODE ||
+		     !(mark->mask & FS_EVENT_ON_CHILD)))
 			continue;
 
 		marks_mask |= mark->mask;
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index ababdbfab537..46d27b357226 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -158,9 +158,9 @@  int __fsnotify_parent(const struct path *path, struct dentry *dentry, __u32 mask
 	parent = dget_parent(dentry);
 	p_inode = parent->d_inode;
 
-	if (unlikely(!fsnotify_inode_watches_children(p_inode)))
+	if (unlikely(!fsnotify_inode_watches_children(p_inode))) {
 		__fsnotify_update_child_dentry_flags(p_inode);
-	else if (p_inode->i_fsnotify_mask & mask) {
+	} else if (p_inode->i_fsnotify_mask & mask & ~FS_EVENT_ON_CHILD) {
 		struct name_snapshot name;
 
 		/* we are notifying a parent so come up with the new mask which
@@ -329,6 +329,10 @@  int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	else
 		mnt = NULL;
 
+	/* An event "on child" is not intended for a mount mark */
+	if (mask & FS_EVENT_ON_CHILD)
+		mnt = NULL;
+
 	/*
 	 * Optimization: srcu_read_lock() has a memory barrier which can
 	 * be expensive.  It protects walking the *_fsnotify_marks lists.