On Tue, Nov 27, 2018 at 11:09 AM Miklos Szeredi <miklos@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx> # v4.19
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> (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
>
