| Message ID | 20201202120713.702387-4-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsnotify fixes and cleanups | expand |
On Wed 02-12-20 14:07:09, Amir Goldstein wrote: > fsnotify_parent() used to send two separate events to backends when a > parent inode is watcing children and the child inode is also watching. ^^ watching > In an attempt to avoid duplicate events in fanotify, we unified the two > backend callbacks to a single callback and handled the reporting of the > two separate events for the relevant backends (inotify and dnotify). > > The unified event callback with two inode marks (parent and child) is > called when both parent and child inode are watched and interested in > the event, but they could each be watched by a different group. > > So before reporting the parent or child event flavor to backend we need > to check that the group is really interested in that event flavor. So I'm not 100% sure what is the actual visible problem - is it that we could deliver events a group didn't ask for? Also I'm confused by a "different group" argument above. AFAICT fsnotify_iter_select_report_types() makes sure we always select marks from a single group and only after that we look at mark's masks. That being said I agree that the loop in send_to_group() will 'or' parent and child masks and then check test_mask & marks_mask & ~marks_ignored_mask so if either parent *or* child was interested in the event, we'll deliver it to both parent and the child. Fanotify is not prone to this since it does its own checks. Dnotify also isn't prone to the problem because it has only events on directories (so there are never two inodes to deliver to). Inotify is prone to the problem although only because we have 'wd' in the event. So an inotify group can receive event also with a wrong 'wd'. After more pondering about your patch I think what I write above isn't actually a problem you were concerned about :) I think you were concerned about the situation when event mask gets FS_EVENT_ON_CHILD because some group has a mark on the parent which is interested in watching children (and so __fsnotify_parent() sets this flag). But then *another* group has a mark without FS_EVENT_ON_CHILD on the parent but we'll send the event to it regardless. This can actually result in completely spurious event on directory inode for inotify & dnotify. If I understood the problem correctly, I suggest modifying beginning of the changelog like below because I was able to figure it out but some poor distro guy deciding whether this could be fixing the problem his customer is hitting or not has a small chance... "fsnotify_parent() used to send two separate events to backends when a parent inode is watching children and the child inode is also watching. In an attempt to avoid duplicate events in fanotify, we unified the two backend callbacks to a single callback and handled the reporting of the two separate events for the relevant backends (inotify and dnotify). However the handling is buggy and can result in inotify and dnotify listeners receiving events of the type they never asked for or spurious events." > The semantics of INODE and CHILD marks were hard to follow and made the > logic more complicated than it should have been. Replace it with INODE > and PARENT marks semantics to hopefully make the logic more clear. Heh, wasn't I complaining about this when I was initially reviewing the changes? ;) > Fixes: eca4784cbb18 ("fsnotify: send event to parent and child with single callback") > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/notify/fanotify/fanotify.c | 7 ++- > fs/notify/fsnotify.c | 78 ++++++++++++++++++-------------- > include/linux/fsnotify_backend.h | 6 +-- > 3 files changed, 51 insertions(+), 40 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 9167884a61ec..1192c9953620 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -268,12 +268,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > continue; > > /* > - * If the event is for a child and this mark is on a parent not > + * If the event is on a child and this mark is on a parent not > * watching children, don't send it! > */ > - if (event_mask & FS_EVENT_ON_CHILD && > - type == FSNOTIFY_OBJ_TYPE_INODE && > - !(mark->mask & FS_EVENT_ON_CHILD)) > + if (type == FSNOTIFY_OBJ_TYPE_PARENT && > + !(mark->mask & FS_EVENT_ON_CHILD)) > continue; > > marks_mask |= mark->mask; > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index c5c68bcbaadf..0676ce4d3352 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -152,6 +152,13 @@ static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt, > if (mask & FS_ISDIR) > return false; > > + /* > + * All events that are possible on child can also may be reported with > + * parent/name info to inode/sb/mount. Otherwise, a watching parent > + * could result in events reported with unexpected name info to sb/mount. > + */ > + BUILD_BUG_ON(FS_EVENTS_POSS_ON_CHILD & ~FS_EVENTS_POSS_TO_PARENT); > + > /* Did either inode/sb/mount subscribe for events with parent/name? */ > marks_mask |= fsnotify_parent_needed_mask(inode->i_fsnotify_mask); > marks_mask |= fsnotify_parent_needed_mask(inode->i_sb->s_fsnotify_mask); > @@ -249,6 +256,10 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group, > path && d_unlinked(path->dentry)) > return 0; > > + /* Check interest of this mark in case event was sent with two marks */ > + if (!(mask & inode_mark->mask & ALL_FSNOTIFY_EVENTS)) > + return 0; > + > return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie); > } > > @@ -258,38 +269,40 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, > u32 cookie, struct fsnotify_iter_info *iter_info) > { > struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info); > - struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info); > + struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info); > int ret; > > if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) || > WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info))) > return 0; > > - /* > - * An event can be sent on child mark iterator instead of inode mark > - * iterator because of other groups that have interest of this inode > - * and have marks on both parent and child. We can simplify this case. > - */ > - if (!inode_mark) { > - inode_mark = child_mark; > - child_mark = NULL; > + if (parent_mark) { > + /* > + * parent_mark indicates that the parent inode is watching children > + * and interested in this event, which is an event possible on child. > + * But is this mark watching children and interested in this event? > + */ > + if (parent_mark->mask & FS_EVENT_ON_CHILD) { Is this really enough? I'd expect us to also check (mask & parent_mark->mask & ALL_FSNOTIFY_EVENTS) != 0... Honza
On Thu, Dec 3, 2020 at 1:53 PM Jan Kara <jack@suse.cz> wrote: > > On Wed 02-12-20 14:07:09, Amir Goldstein wrote: > > fsnotify_parent() used to send two separate events to backends when a > > parent inode is watcing children and the child inode is also watching. > ^^ watching > > > In an attempt to avoid duplicate events in fanotify, we unified the two > > backend callbacks to a single callback and handled the reporting of the > > two separate events for the relevant backends (inotify and dnotify). > > > > The unified event callback with two inode marks (parent and child) is > > called when both parent and child inode are watched and interested in > > the event, but they could each be watched by a different group. > > > > So before reporting the parent or child event flavor to backend we need > > to check that the group is really interested in that event flavor. > > So I'm not 100% sure what is the actual visible problem - is it that we > could deliver events a group didn't ask for? Sort of yes. See: https://github.com/amir73il/ltp/blob/fsnotify-fixes/testcases/kernel/syscalls/inotify/inotify11.c > > Also I'm confused by a "different group" argument above. AFAICT > fsnotify_iter_select_report_types() makes sure we always select marks from > a single group and only after that we look at mark's masks. The group will get an event it has ask for but on the wrong object. > > That being said I agree that the loop in send_to_group() will 'or' parent > and child masks and then check test_mask & marks_mask & ~marks_ignored_mask > so if either parent *or* child was interested in the event, we'll deliver > it to both parent and the child. Fanotify is not prone to this since it > does its own checks. Dnotify also isn't prone to the problem because it > has only events on directories (so there are never two inodes to deliver > to). FS_ATTRIB on dir will be delivered to parent watcher as well as child watcher I think. > Inotify is prone to the problem although only because we have 'wd' in > the event. So an inotify group can receive event also with a wrong 'wd'. > True wrong wd and with unexpected name or unexpected missing name. group1 could be watching a file foo and get events on file bar because group1 is watching the parent (for another event) and group2 is watching parent for this event. > After more pondering about your patch I think what I write above isn't > actually a problem you were concerned about :) I think you were concerned > about the situation when event mask gets FS_EVENT_ON_CHILD because some > group has a mark on the parent which is interested in watching children > (and so __fsnotify_parent() sets this flag). But then *another* group has > a mark without FS_EVENT_ON_CHILD on the parent but we'll send the event to > it regardless. This can actually result in completely spurious event on > directory inode for inotify & dnotify. > Haha lags in emails :) > If I understood the problem correctly, I suggest modifying beginning of the > changelog like below because I was able to figure it out but some poor > distro guy deciding whether this could be fixing the problem his customer > is hitting or not has a small chance... > > "fsnotify_parent() used to send two separate events to backends when a > parent inode is watching children and the child inode is also watching. > In an attempt to avoid duplicate events in fanotify, we unified the two > backend callbacks to a single callback and handled the reporting of the > two separate events for the relevant backends (inotify and dnotify). > However the handling is buggy and can result in inotify and dnotify listeners > receiving events of the type they never asked for or spurious events." > ACK > > The semantics of INODE and CHILD marks were hard to follow and made the > > logic more complicated than it should have been. Replace it with INODE > > and PARENT marks semantics to hopefully make the logic more clear. > > Heh, wasn't I complaining about this when I was initially reviewing the > changes? ;) You certainly did and rightfully so. It took me a long time to untangle this knot, so I hope you like the result. > > > Fixes: eca4784cbb18 ("fsnotify: send event to parent and child with single callback") > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > fs/notify/fanotify/fanotify.c | 7 ++- > > fs/notify/fsnotify.c | 78 ++++++++++++++++++-------------- > > include/linux/fsnotify_backend.h | 6 +-- > > 3 files changed, 51 insertions(+), 40 deletions(-) > > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > index 9167884a61ec..1192c9953620 100644 > > --- a/fs/notify/fanotify/fanotify.c > > +++ b/fs/notify/fanotify/fanotify.c > > @@ -268,12 +268,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > > continue; > > > > /* > > - * If the event is for a child and this mark is on a parent not > > + * If the event is on a child and this mark is on a parent not > > * watching children, don't send it! > > */ > > - if (event_mask & FS_EVENT_ON_CHILD && > > - type == FSNOTIFY_OBJ_TYPE_INODE && > > - !(mark->mask & FS_EVENT_ON_CHILD)) > > + if (type == FSNOTIFY_OBJ_TYPE_PARENT && > > + !(mark->mask & FS_EVENT_ON_CHILD)) > > continue; > > > > marks_mask |= mark->mask; > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > > index c5c68bcbaadf..0676ce4d3352 100644 > > --- a/fs/notify/fsnotify.c > > +++ b/fs/notify/fsnotify.c > > @@ -152,6 +152,13 @@ static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt, > > if (mask & FS_ISDIR) > > return false; > > > > + /* > > + * All events that are possible on child can also may be reported with > > + * parent/name info to inode/sb/mount. Otherwise, a watching parent > > + * could result in events reported with unexpected name info to sb/mount. > > + */ > > + BUILD_BUG_ON(FS_EVENTS_POSS_ON_CHILD & ~FS_EVENTS_POSS_TO_PARENT); > > + > > /* Did either inode/sb/mount subscribe for events with parent/name? */ > > marks_mask |= fsnotify_parent_needed_mask(inode->i_fsnotify_mask); > > marks_mask |= fsnotify_parent_needed_mask(inode->i_sb->s_fsnotify_mask); > > @@ -249,6 +256,10 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group, > > path && d_unlinked(path->dentry)) > > return 0; > > > > + /* Check interest of this mark in case event was sent with two marks */ > > + if (!(mask & inode_mark->mask & ALL_FSNOTIFY_EVENTS)) > > + return 0; > > + > > return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie); > > } > > > > @@ -258,38 +269,40 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, > > u32 cookie, struct fsnotify_iter_info *iter_info) > > { > > struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info); > > - struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info); > > + struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info); > > int ret; > > > > if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) || > > WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info))) > > return 0; > > > > - /* > > - * An event can be sent on child mark iterator instead of inode mark > > - * iterator because of other groups that have interest of this inode > > - * and have marks on both parent and child. We can simplify this case. > > - */ > > - if (!inode_mark) { > > - inode_mark = child_mark; > > - child_mark = NULL; > > + if (parent_mark) { > > + /* > > + * parent_mark indicates that the parent inode is watching children > > + * and interested in this event, which is an event possible on child. > > + * But is this mark watching children and interested in this event? > > + */ > > + if (parent_mark->mask & FS_EVENT_ON_CHILD) { > > Is this really enough? I'd expect us to also check (mask & > parent_mark->mask & ALL_FSNOTIFY_EVENTS) != 0... I put it up in fsnotify_event_needs_parent() because this check is needed for both parent and child. BTW, at first I was thinking we needed to check EVENTS_POSS_ON_CHILD here but we don't because if event is not EVENTS_POSS_ON_CHILD (a.k.a. !parent_interested) then flag ON_CHILD is not set and parent_mark is not iterated . Thanks, Amir.
On Thu 03-12-20 14:58:41, Amir Goldstein wrote: > On Thu, Dec 3, 2020 at 1:53 PM Jan Kara <jack@suse.cz> wrote: > > > The semantics of INODE and CHILD marks were hard to follow and made the > > > logic more complicated than it should have been. Replace it with INODE > > > and PARENT marks semantics to hopefully make the logic more clear. > > > > Heh, wasn't I complaining about this when I was initially reviewing the > > changes? ;) > > You certainly did and rightfully so. > It took me a long time to untangle this knot, so I hope you like the result. Yes, it is IMO more readable now. > > > Fixes: eca4784cbb18 ("fsnotify: send event to parent and child with single callback") > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > fs/notify/fanotify/fanotify.c | 7 ++- > > > fs/notify/fsnotify.c | 78 ++++++++++++++++++-------------- > > > include/linux/fsnotify_backend.h | 6 +-- > > > 3 files changed, 51 insertions(+), 40 deletions(-) > > > > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > > index 9167884a61ec..1192c9953620 100644 > > > --- a/fs/notify/fanotify/fanotify.c > > > +++ b/fs/notify/fanotify/fanotify.c > > > @@ -268,12 +268,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > > > continue; > > > > > > /* > > > - * If the event is for a child and this mark is on a parent not > > > + * If the event is on a child and this mark is on a parent not > > > * watching children, don't send it! > > > */ > > > - if (event_mask & FS_EVENT_ON_CHILD && > > > - type == FSNOTIFY_OBJ_TYPE_INODE && > > > - !(mark->mask & FS_EVENT_ON_CHILD)) > > > + if (type == FSNOTIFY_OBJ_TYPE_PARENT && > > > + !(mark->mask & FS_EVENT_ON_CHILD)) > > > continue; > > > > > > marks_mask |= mark->mask; > > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > > > index c5c68bcbaadf..0676ce4d3352 100644 > > > --- a/fs/notify/fsnotify.c > > > +++ b/fs/notify/fsnotify.c > > > @@ -152,6 +152,13 @@ static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt, > > > if (mask & FS_ISDIR) > > > return false; > > > > > > + /* > > > + * All events that are possible on child can also may be reported with > > > + * parent/name info to inode/sb/mount. Otherwise, a watching parent > > > + * could result in events reported with unexpected name info to sb/mount. > > > + */ > > > + BUILD_BUG_ON(FS_EVENTS_POSS_ON_CHILD & ~FS_EVENTS_POSS_TO_PARENT); > > > + > > > /* Did either inode/sb/mount subscribe for events with parent/name? */ > > > marks_mask |= fsnotify_parent_needed_mask(inode->i_fsnotify_mask); > > > marks_mask |= fsnotify_parent_needed_mask(inode->i_sb->s_fsnotify_mask); > > > @@ -249,6 +256,10 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group, > > > path && d_unlinked(path->dentry)) > > > return 0; > > > > > > + /* Check interest of this mark in case event was sent with two marks */ > > > + if (!(mask & inode_mark->mask & ALL_FSNOTIFY_EVENTS)) > > > + return 0; > > > + > > > return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie); > > > } > > > > > > @@ -258,38 +269,40 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, > > > u32 cookie, struct fsnotify_iter_info *iter_info) > > > { > > > struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info); > > > - struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info); > > > + struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info); > > > int ret; > > > > > > if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) || > > > WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info))) > > > return 0; > > > > > > - /* > > > - * An event can be sent on child mark iterator instead of inode mark > > > - * iterator because of other groups that have interest of this inode > > > - * and have marks on both parent and child. We can simplify this case. > > > - */ > > > - if (!inode_mark) { > > > - inode_mark = child_mark; > > > - child_mark = NULL; > > > + if (parent_mark) { > > > + /* > > > + * parent_mark indicates that the parent inode is watching children > > > + * and interested in this event, which is an event possible on child. > > > + * But is this mark watching children and interested in this event? > > > + */ > > > + if (parent_mark->mask & FS_EVENT_ON_CHILD) { > > > > Is this really enough? I'd expect us to also check (mask & > > parent_mark->mask & ALL_FSNOTIFY_EVENTS) != 0... > > I put it up in fsnotify_event_needs_parent() because this check is needed > for both parent and child. Right, I missed that. Thanks for explanation. > BTW, at first I was thinking we needed to check EVENTS_POSS_ON_CHILD > here but we don't because if event is not EVENTS_POSS_ON_CHILD > (a.k.a. !parent_interested) then flag ON_CHILD is not set and parent_mark > is not iterated . OK :). I'll just improve the changelog and pick this patch up. Honza
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 9167884a61ec..1192c9953620 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -268,12 +268,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, continue; /* - * If the event is for a child and this mark is on a parent not + * If the event is on a child and this mark is on a parent not * watching children, don't send it! */ - if (event_mask & FS_EVENT_ON_CHILD && - type == FSNOTIFY_OBJ_TYPE_INODE && - !(mark->mask & FS_EVENT_ON_CHILD)) + if (type == FSNOTIFY_OBJ_TYPE_PARENT && + !(mark->mask & FS_EVENT_ON_CHILD)) continue; marks_mask |= mark->mask; diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index c5c68bcbaadf..0676ce4d3352 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -152,6 +152,13 @@ static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt, if (mask & FS_ISDIR) return false; + /* + * All events that are possible on child can also may be reported with + * parent/name info to inode/sb/mount. Otherwise, a watching parent + * could result in events reported with unexpected name info to sb/mount. + */ + BUILD_BUG_ON(FS_EVENTS_POSS_ON_CHILD & ~FS_EVENTS_POSS_TO_PARENT); + /* Did either inode/sb/mount subscribe for events with parent/name? */ marks_mask |= fsnotify_parent_needed_mask(inode->i_fsnotify_mask); marks_mask |= fsnotify_parent_needed_mask(inode->i_sb->s_fsnotify_mask); @@ -249,6 +256,10 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group, path && d_unlinked(path->dentry)) return 0; + /* Check interest of this mark in case event was sent with two marks */ + if (!(mask & inode_mark->mask & ALL_FSNOTIFY_EVENTS)) + return 0; + return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie); } @@ -258,38 +269,40 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, u32 cookie, struct fsnotify_iter_info *iter_info) { struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info); - struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info); + struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info); int ret; if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) || WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info))) return 0; - /* - * An event can be sent on child mark iterator instead of inode mark - * iterator because of other groups that have interest of this inode - * and have marks on both parent and child. We can simplify this case. - */ - if (!inode_mark) { - inode_mark = child_mark; - child_mark = NULL; + if (parent_mark) { + /* + * parent_mark indicates that the parent inode is watching children + * and interested in this event, which is an event possible on child. + * But is this mark watching children and interested in this event? + */ + if (parent_mark->mask & FS_EVENT_ON_CHILD) { + ret = fsnotify_handle_inode_event(group, parent_mark, mask, + data, data_type, dir, name, 0); + if (ret) + return ret; + } + if (!inode_mark) + return 0; + + /* + * Some events can be sent on both parent dir and child marks + * (e.g. FS_ATTRIB). If both parent dir and child are watching, + * report the event once to parent dir with name (if interested) + * and once to child without name (if interested). + */ dir = NULL; name = NULL; } - ret = fsnotify_handle_inode_event(group, inode_mark, mask, data, data_type, - dir, name, cookie); - if (ret || !child_mark) - return ret; - - /* - * Some events can be sent on both parent dir and child marks - * (e.g. FS_ATTRIB). If both parent dir and child are watching, - * report the event once to parent dir with name and once to child - * without name. - */ - return fsnotify_handle_inode_event(group, child_mark, mask, data, data_type, - NULL, NULL, 0); + return fsnotify_handle_inode_event(group, inode_mark, mask, data, data_type, + dir, name, cookie); } static int send_to_group(__u32 mask, const void *data, int data_type, @@ -447,7 +460,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, struct fsnotify_iter_info iter_info = {}; struct super_block *sb; struct mount *mnt = NULL; - struct inode *child = NULL; + struct inode *parent = NULL; int ret = 0; __u32 test_mask, marks_mask; @@ -459,11 +472,10 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, inode = dir; } else if (mask & FS_EVENT_ON_CHILD) { /* - * Event on child - report on TYPE_INODE to dir if it is - * watching children and on TYPE_CHILD to child. + * Event on child - report on TYPE_PARENT to dir if it is + * watching children and on TYPE_INODE to child. */ - child = inode; - inode = dir; + parent = dir; } sb = inode->i_sb; @@ -477,7 +489,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, if (!sb->s_fsnotify_marks && (!mnt || !mnt->mnt_fsnotify_marks) && (!inode || !inode->i_fsnotify_marks) && - (!child || !child->i_fsnotify_marks)) + (!parent || !parent->i_fsnotify_marks)) return 0; marks_mask = sb->s_fsnotify_mask; @@ -485,8 +497,8 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, marks_mask |= mnt->mnt_fsnotify_mask; if (inode) marks_mask |= inode->i_fsnotify_mask; - if (child) - marks_mask |= child->i_fsnotify_mask; + if (parent) + marks_mask |= parent->i_fsnotify_mask; /* @@ -509,9 +521,9 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] = fsnotify_first_mark(&inode->i_fsnotify_marks); } - if (child) { - iter_info.marks[FSNOTIFY_OBJ_TYPE_CHILD] = - fsnotify_first_mark(&child->i_fsnotify_marks); + if (parent) { + iter_info.marks[FSNOTIFY_OBJ_TYPE_PARENT] = + fsnotify_first_mark(&parent->i_fsnotify_marks); } /* diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index 4ee3044eedd0..a2e42d3cd87c 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -278,7 +278,7 @@ static inline const struct path *fsnotify_data_path(const void *data, enum fsnotify_obj_type { FSNOTIFY_OBJ_TYPE_INODE, - FSNOTIFY_OBJ_TYPE_CHILD, + FSNOTIFY_OBJ_TYPE_PARENT, FSNOTIFY_OBJ_TYPE_VFSMOUNT, FSNOTIFY_OBJ_TYPE_SB, FSNOTIFY_OBJ_TYPE_COUNT, @@ -286,7 +286,7 @@ enum fsnotify_obj_type { }; #define FSNOTIFY_OBJ_TYPE_INODE_FL (1U << FSNOTIFY_OBJ_TYPE_INODE) -#define FSNOTIFY_OBJ_TYPE_CHILD_FL (1U << FSNOTIFY_OBJ_TYPE_CHILD) +#define FSNOTIFY_OBJ_TYPE_PARENT_FL (1U << FSNOTIFY_OBJ_TYPE_PARENT) #define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL (1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT) #define FSNOTIFY_OBJ_TYPE_SB_FL (1U << FSNOTIFY_OBJ_TYPE_SB) #define FSNOTIFY_OBJ_ALL_TYPES_MASK ((1U << FSNOTIFY_OBJ_TYPE_COUNT) - 1) @@ -331,7 +331,7 @@ static inline struct fsnotify_mark *fsnotify_iter_##name##_mark( \ } FSNOTIFY_ITER_FUNCS(inode, INODE) -FSNOTIFY_ITER_FUNCS(child, CHILD) +FSNOTIFY_ITER_FUNCS(parent, PARENT) FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT) FSNOTIFY_ITER_FUNCS(sb, SB)
fsnotify_parent() used to send two separate events to backends when a parent inode is watcing children and the child inode is also watching. In an attempt to avoid duplicate events in fanotify, we unified the two backend callbacks to a single callback and handled the reporting of the two separate events for the relevant backends (inotify and dnotify). The unified event callback with two inode marks (parent and child) is called when both parent and child inode are watched and interested in the event, but they could each be watched by a different group. So before reporting the parent or child event flavor to backend we need to check that the group is really interested in that event flavor. The semantics of INODE and CHILD marks were hard to follow and made the logic more complicated than it should have been. Replace it with INODE and PARENT marks semantics to hopefully make the logic more clear. Fixes: eca4784cbb18 ("fsnotify: send event to parent and child with single callback") Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fanotify/fanotify.c | 7 ++- fs/notify/fsnotify.c | 78 ++++++++++++++++++-------------- include/linux/fsnotify_backend.h | 6 +-- 3 files changed, 51 insertions(+), 40 deletions(-)