[v5,12/22] inotify: report both events on parent and child with single callback
diff mbox series

Message ID 20200716084230.30611-13-amir73il@gmail.com
State New
Headers show
Series
  • fanotify events with name info
Related show

Commit Message

Amir Goldstein July 16, 2020, 8:42 a.m. UTC
fsnotify usually calls inotify_handle_event() once for watching parent
to report event with child's name and once for watching child to report
event without child's name.

Do the same thing with a single callback instead of two callbacks when
marks iterator contains both inode and child entries.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/inotify/inotify_fsnotify.c | 44 ++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 9 deletions(-)

Comments

Jan Kara July 16, 2020, 12:52 p.m. UTC | #1
On Thu 16-07-20 11:42:20, Amir Goldstein wrote:
> fsnotify usually calls inotify_handle_event() once for watching parent
> to report event with child's name and once for watching child to report
> event without child's name.
> 
> Do the same thing with a single callback instead of two callbacks when
> marks iterator contains both inode and child entries.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Another idea for possible future cleanup here: Everybody except for
fanotify cares only about inode marks and reporting both parent and child
only complicates things for them (and I can imagine bugs being created by
in-kernel fsnotify users because they misunderstand inode-vs-child mark
types etc.). So maybe we can create another fsnotify_group operation
similar to ->handle_event but with simpler signature for these simple
notification handlers and send_to_group() will take care of translating 
the complex fsnotify() call into a sequence of these simple callbacks.

								Honza
Amir Goldstein July 16, 2020, 2:25 p.m. UTC | #2
On Thu, Jul 16, 2020 at 3:52 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 16-07-20 11:42:20, Amir Goldstein wrote:
> > fsnotify usually calls inotify_handle_event() once for watching parent
> > to report event with child's name and once for watching child to report
> > event without child's name.
> >
> > Do the same thing with a single callback instead of two callbacks when
> > marks iterator contains both inode and child entries.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Another idea for possible future cleanup here: Everybody except for
> fanotify cares only about inode marks and reporting both parent and child
> only complicates things for them (and I can imagine bugs being created by
> in-kernel fsnotify users because they misunderstand inode-vs-child mark
> types etc.). So maybe we can create another fsnotify_group operation
> similar to ->handle_event but with simpler signature for these simple
> notification handlers and send_to_group() will take care of translating
> the complex fsnotify() call into a sequence of these simple callbacks.
>

Yeh we could do that.
But then it's not every day that a new in-kernel fsnotify_group is added...

Thanks,
Amir.
Jan Kara July 16, 2020, 3:17 p.m. UTC | #3
On Thu 16-07-20 17:25:27, Amir Goldstein wrote:
> On Thu, Jul 16, 2020 at 3:52 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 16-07-20 11:42:20, Amir Goldstein wrote:
> > > fsnotify usually calls inotify_handle_event() once for watching parent
> > > to report event with child's name and once for watching child to report
> > > event without child's name.
> > >
> > > Do the same thing with a single callback instead of two callbacks when
> > > marks iterator contains both inode and child entries.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Another idea for possible future cleanup here: Everybody except for
> > fanotify cares only about inode marks and reporting both parent and child
> > only complicates things for them (and I can imagine bugs being created by
> > in-kernel fsnotify users because they misunderstand inode-vs-child mark
> > types etc.). So maybe we can create another fsnotify_group operation
> > similar to ->handle_event but with simpler signature for these simple
> > notification handlers and send_to_group() will take care of translating
> > the complex fsnotify() call into a sequence of these simple callbacks.
> >
> 
> Yeh we could do that.
> But then it's not every day that a new in-kernel fsnotify_group is added...

Definitely. But then we often do not notice when it is added (to review the
usage) or when e.g. audit decides to tweak its event mask and things
suddently subtly break for it...

								Honza

Patch
diff mbox series

diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index dfd455798a1b..a65cf8c9f600 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -55,13 +55,11 @@  static int inotify_merge(struct list_head *list,
 	return event_compare(last_event, event);
 }
 
-int inotify_handle_event(struct fsnotify_group *group, u32 mask,
-			 const void *data, int data_type, struct inode *dir,
-			 const struct qstr *file_name, u32 cookie,
-			 struct fsnotify_iter_info *iter_info)
+static int inotify_one_event(struct fsnotify_group *group, u32 mask,
+			     struct fsnotify_mark *inode_mark,
+			     const struct path *path,
+			     const struct qstr *file_name, u32 cookie)
 {
-	const struct path *path = fsnotify_data_path(data, data_type);
-	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
 	struct inotify_inode_mark *i_mark;
 	struct inotify_event_info *event;
 	struct fsnotify_event *fsn_event;
@@ -69,9 +67,6 @@  int inotify_handle_event(struct fsnotify_group *group, u32 mask,
 	int len = 0;
 	int alloc_len = sizeof(struct inotify_event_info);
 
-	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
-		return 0;
-
 	if ((inode_mark->mask & FS_EXCL_UNLINK) &&
 	    path && d_unlinked(path->dentry))
 		return 0;
@@ -135,6 +130,37 @@  int inotify_handle_event(struct fsnotify_group *group, u32 mask,
 	return 0;
 }
 
+int inotify_handle_event(struct fsnotify_group *group, u32 mask,
+			 const void *data, int data_type, struct inode *dir,
+			 const struct qstr *file_name, u32 cookie,
+			 struct fsnotify_iter_info *iter_info)
+{
+	const struct path *path = fsnotify_data_path(data, data_type);
+	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
+	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
+	int ret = 0;
+
+	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
+		return 0;
+
+	/*
+	 * Some events cannot be sent on both parent and child marks
+	 * (e.g. IN_CREATE).  Those events are always sent on inode_mark.
+	 * For events that are possible on both parent and child (e.g. IN_OPEN),
+	 * event is sent on inode_mark with name if the parent is watching and
+	 * is sent on child_mark without name if child is watching.
+	 * If both parent and child are watching, report the event with child's
+	 * name here and report another event without child's name below.
+	 */
+	if (inode_mark)
+		ret = inotify_one_event(group, mask, inode_mark, path,
+					file_name, cookie);
+	if (ret || !child_mark)
+		return ret;
+
+	return inotify_one_event(group, mask, child_mark, path, NULL, 0);
+}
+
 static void inotify_freeing_mark(struct fsnotify_mark *fsn_mark, struct fsnotify_group *group)
 {
 	inotify_ignored_and_remove_idr(fsn_mark, group);