[PATCH 0/2] Fixes for fanotify parent dir ignore mask logic

[PATCH 0/2] Fixes for fanotify parent dir ignore mask logic

[Amir Goldstein]

Jan,

The following two patches are a prelude to FAN_MARK_IGNORE patch set [1].
I have written tests [2] and man page draft [3] for FAN_MARK_IGNORE, but
not proposing it for next, because one big UAPI change is enough and it
is too late in the cycle anyway.

However, I though you may want to consider these two patches for next.
The test fanotify09 on [2] has two new test cases for the fixes in these
patches.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fan_mark_ignore
[2] https://github.com/amir73il/ltp/commits/fan_mark_ignore
[3] https://github.com/amir73il/man-pages/commits/fan_mark_ignore

Amir Goldstein (2):
  fsnotify: introduce mark type iterator
  fsnotify: consistent behavior for parent not watching children

 fs/notify/fanotify/fanotify.c    | 24 ++-------
 fs/notify/fsnotify.c             | 88 +++++++++++++++++---------------
 include/linux/fsnotify_backend.h | 17 +++---
 3 files changed, 61 insertions(+), 68 deletions(-)

-- 
2.25.1

[PATCH 1/2] fsnotify: introduce mark type iterator

[Amir Goldstein]

fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code
of iteratating all marks of a specific group interested in an event
by consulting the iterator report_mask.

Use an open coded version of that iterator in fsnotify_iter_next()
that collects all marks of the current iteration group without
consulting the iterator report_mask.

At the moment, the two iterator variants are the same, but this
decoupling will allow us to exclude some of the group's marks from
reporting the event, for example for event on child and inode marks
on parent did not request to watch events on children.

Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
Reported-by: Jan Kara <jack@suse.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c    | 14 +++-------
 fs/notify/fsnotify.c             | 48 ++++++++++++++------------------
 include/linux/fsnotify_backend.h | 17 ++++++-----
 3 files changed, 35 insertions(+), 44 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 985e995d2a39..263d303d8f8f 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -319,11 +319,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 			return 0;
 	}
 
-	fsnotify_foreach_iter_type(type) {
-		if (!fsnotify_iter_should_report_type(iter_info, type))
-			continue;
-		mark = iter_info->marks[type];
-
+	fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
 		/* Apply ignore mask regardless of ISDIR and ON_CHILD flags */
 		marks_ignored_mask |= mark->ignored_mask;
 
@@ -849,16 +845,14 @@ static struct fanotify_event *fanotify_alloc_event(
  */
 static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)
 {
+	struct fsnotify_mark *mark;
 	int type;
 	__kernel_fsid_t fsid = {};
 
-	fsnotify_foreach_iter_type(type) {
+	fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
 		struct fsnotify_mark_connector *conn;
 
-		if (!fsnotify_iter_should_report_type(iter_info, type))
-			continue;
-
-		conn = READ_ONCE(iter_info->marks[type]->connector);
+		conn = READ_ONCE(mark->connector);
 		/* Mark is just getting destroyed or created? */
 		if (!conn)
 			continue;
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 6eee19d15e8c..00f11ef4c0ed 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -335,31 +335,23 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
 	struct fsnotify_mark *mark;
 	int type;
 
-	if (WARN_ON(!iter_info->report_mask))
+	if (!iter_info->report_mask)
 		return 0;
 
 	/* clear ignored on inode modification */
 	if (mask & FS_MODIFY) {
-		fsnotify_foreach_iter_type(type) {
-			if (!fsnotify_iter_should_report_type(iter_info, type))
-				continue;
-			mark = iter_info->marks[type];
-			if (mark &&
-			    !(mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
+		fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
+			if (!(mark->flags &
+			      FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
 				mark->ignored_mask = 0;
 		}
 	}
 
-	fsnotify_foreach_iter_type(type) {
-		if (!fsnotify_iter_should_report_type(iter_info, type))
-			continue;
-		mark = iter_info->marks[type];
-		/* does the object mark tell us to do something? */
-		if (mark) {
-			group = mark->group;
-			marks_mask |= mark->mask;
-			marks_ignored_mask |= mark->ignored_mask;
-		}
+	/* Are any of the group marks interested in this event? */
+	fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
+		group = mark->group;
+		marks_mask |= mark->mask;
+		marks_ignored_mask |= mark->ignored_mask;
 	}
 
 	pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignored_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
@@ -403,11 +395,11 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
 
 /*
  * iter_info is a multi head priority queue of marks.
- * Pick a subset of marks from queue heads, all with the
- * same group and set the report_mask for selected subset.
- * Returns the report_mask of the selected subset.
+ * Pick a subset of marks from queue heads, all with the same group
+ * and set the report_mask to a subset of the selected marks.
+ * Returns false if there are no more groups to iterate.
  */
-static unsigned int fsnotify_iter_select_report_types(
+static bool fsnotify_iter_select_report_types(
 		struct fsnotify_iter_info *iter_info)
 {
 	struct fsnotify_group *max_prio_group = NULL;
@@ -423,30 +415,32 @@ static unsigned int fsnotify_iter_select_report_types(
 	}
 
 	if (!max_prio_group)
-		return 0;
+		return false;
 
 	/* Set the report mask for marks from same group as max prio group */
+	iter_info->current_group = max_prio_group;
 	iter_info->report_mask = 0;
 	fsnotify_foreach_iter_type(type) {
 		mark = iter_info->marks[type];
-		if (mark &&
-		    fsnotify_compare_groups(max_prio_group, mark->group) == 0)
+		if (mark && mark->group == iter_info->current_group)
 			fsnotify_iter_set_report_type(iter_info, type);
 	}
 
-	return iter_info->report_mask;
+	return true;
 }
 
 /*
- * Pop from iter_info multi head queue, the marks that were iterated in the
+ * Pop from iter_info multi head queue, the marks that belong to the group of
  * current iteration step.
  */
 static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
 {
+	struct fsnotify_mark *mark;
 	int type;
 
 	fsnotify_foreach_iter_type(type) {
-		if (fsnotify_iter_should_report_type(iter_info, type))
+		mark = iter_info->marks[type];
+		if (mark && mark->group == iter_info->current_group)
 			iter_info->marks[type] =
 				fsnotify_next_mark(iter_info->marks[type]);
 	}
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 9a1a9e78f69f..a21dccc255ee 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -399,6 +399,7 @@ static inline bool fsnotify_valid_obj_type(unsigned int obj_type)
 
 struct fsnotify_iter_info {
 	struct fsnotify_mark *marks[FSNOTIFY_ITER_TYPE_COUNT];
+	struct fsnotify_group *current_group;
 	unsigned int report_mask;
 	int srcu_idx;
 };
@@ -415,20 +416,19 @@ static inline void fsnotify_iter_set_report_type(
 	iter_info->report_mask |= (1U << iter_type);
 }
 
-static inline void fsnotify_iter_set_report_type_mark(
-		struct fsnotify_iter_info *iter_info, int iter_type,
-		struct fsnotify_mark *mark)
+static inline struct fsnotify_mark *fsnotify_iter_mark(
+		struct fsnotify_iter_info *iter_info, int iter_type)
 {
-	iter_info->marks[iter_type] = mark;
-	iter_info->report_mask |= (1U << iter_type);
+	if (fsnotify_iter_should_report_type(iter_info, iter_type))
+		return iter_info->marks[iter_type];
+	return NULL;
 }
 
 #define FSNOTIFY_ITER_FUNCS(name, NAME) \
 static inline struct fsnotify_mark *fsnotify_iter_##name##_mark( \
 		struct fsnotify_iter_info *iter_info) \
 { \
-	return (iter_info->report_mask & (1U << FSNOTIFY_ITER_TYPE_##NAME)) ? \
-		iter_info->marks[FSNOTIFY_ITER_TYPE_##NAME] : NULL; \
+	return fsnotify_iter_mark(iter_info, FSNOTIFY_ITER_TYPE_##NAME); \
 }
 
 FSNOTIFY_ITER_FUNCS(inode, INODE)
@@ -438,6 +438,9 @@ FSNOTIFY_ITER_FUNCS(sb, SB)
 
 #define fsnotify_foreach_iter_type(type) \
 	for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++)
+#define fsnotify_foreach_iter_mark_type(iter, mark, type) \
+	for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) \
+		if (!(mark = fsnotify_iter_mark(iter, type))) {} else
 
 /*
  * fsnotify_connp_t is what we embed in objects which connector can be attached
-- 
2.25.1

[PATCH 2/2] fsnotify: consistent behavior for parent not watching children

[Amir Goldstein]

The logic for handling events on child in groups that have a mark on
the parent inode, but without FS_EVENT_ON_CHILD flag in the mask is
duplicated in several places and inconsistent.

Move the logic into the preparation of mark type iterator, so that the
parent mark type will be excluded from all mark type iterations in that
case.

This results in several subtle changes of behavior, hopefully all
desired changes of behavior, for example:

- Group A has a mount mark with FS_MODIFY in mask
- Group A has a mark with ignore mask that does not survive FS_MODIFY
  and does not watch children on directory D.
- Group B has a mark with FS_MODIFY in mask that does watch children
  on directory D.
- FS_MODIFY event on file D/foo should not clear the ignore mask of
  group A, but before this change it does

And if group A ignore mask was set to survive FS_MODIFY:
- FS_MODIFY event on file D/foo should be reported to group A on account
  of the mount mark, but before this change it is wrongly ignored

Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
Reported-by: Jan Kara <jack@suse.com>
Link: https://lore.kernel.org/linux-fsdevel/20220314113337.j7slrb5srxukztje@quack3.lan/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 10 +--------
 fs/notify/fsnotify.c          | 40 ++++++++++++++++++++++-------------
 2 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 263d303d8f8f..4f897e109547 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -320,7 +320,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 	}
 
 	fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
-		/* Apply ignore mask regardless of ISDIR and ON_CHILD flags */
+		/* Apply ignore mask regardless of mark's ISDIR flag */
 		marks_ignored_mask |= mark->ignored_mask;
 
 		/*
@@ -330,14 +330,6 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 		if (event_mask & FS_ISDIR && !(mark->mask & FS_ISDIR))
 			continue;
 
-		/*
-		 * If the event is on a child and this mark is on a parent not
-		 * watching children, don't send it!
-		 */
-		if (type == FSNOTIFY_ITER_TYPE_PARENT &&
-		    !(mark->mask & FS_EVENT_ON_CHILD))
-			continue;
-
 		marks_mask |= mark->mask;
 
 		/* Record the mark types of this group that matched the event */
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 00f11ef4c0ed..985cc56f4c7d 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -290,22 +290,15 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
 	}
 
 	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;
+		ret = fsnotify_handle_inode_event(group, parent_mark, mask,
+						  data, data_type, dir, name, 0);
+		if (ret)
+			return ret;
 	}
 
+	if (!inode_mark)
+		return 0;
+
 	if (mask & FS_EVENT_ON_CHILD) {
 		/*
 		 * Some events can be sent on both parent dir and child marks
@@ -393,6 +386,23 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
 	return hlist_entry_safe(node, struct fsnotify_mark, obj_list);
 }
 
+static void fsnotify_iter_set_report_mark_type(
+		struct fsnotify_iter_info *iter_info, int iter_type)
+{
+	struct fsnotify_mark *mark = iter_info->marks[iter_type];
+
+	/*
+	 * FSNOTIFY_ITER_TYPE_PARENT indicates that this inode is watching
+	 * children and interested in this event, which is an event
+	 * possible on child. But is *this mark* watching children?
+	 */
+	if (iter_type == FSNOTIFY_ITER_TYPE_PARENT &&
+	    !(mark->mask & FS_EVENT_ON_CHILD))
+		return;
+
+	fsnotify_iter_set_report_type(iter_info, iter_type);
+}
+
 /*
  * iter_info is a multi head priority queue of marks.
  * Pick a subset of marks from queue heads, all with the same group
@@ -423,7 +433,7 @@ static bool fsnotify_iter_select_report_types(
 	fsnotify_foreach_iter_type(type) {
 		mark = iter_info->marks[type];
 		if (mark && mark->group == iter_info->current_group)
-			fsnotify_iter_set_report_type(iter_info, type);
+			fsnotify_iter_set_report_mark_type(iter_info, type);
 	}
 
 	return true;
-- 
2.25.1

Re: [PATCH 1/2] fsnotify: introduce mark type iterator

[Jan Kara]

On Wed 11-05-22 12:29:13, Amir Goldstein wrote:
> fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code
> of iteratating all marks of a specific group interested in an event
> by consulting the iterator report_mask.
> 
> Use an open coded version of that iterator in fsnotify_iter_next()
> that collects all marks of the current iteration group without
> consulting the iterator report_mask.
> 
> At the moment, the two iterator variants are the same, but this
> decoupling will allow us to exclude some of the group's marks from
> reporting the event, for example for event on child and inode marks
> on parent did not request to watch events on children.
> 
> Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
> Reported-by: Jan Kara <jack@suse.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Mostly looks good. Two nits below.

>  /*
> - * Pop from iter_info multi head queue, the marks that were iterated in the
> + * Pop from iter_info multi head queue, the marks that belong to the group of
>   * current iteration step.
>   */
>  static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
>  {
> +	struct fsnotify_mark *mark;
>  	int type;
>  
>  	fsnotify_foreach_iter_type(type) {
> -		if (fsnotify_iter_should_report_type(iter_info, type))
> +		mark = iter_info->marks[type];
> +		if (mark && mark->group == iter_info->current_group)
>  			iter_info->marks[type] =
>  				fsnotify_next_mark(iter_info->marks[type]);

Wouldn't it be more natural here to use the new helper
fsnotify_foreach_iter_mark_type()? In principle we want to advance mark
types which were already reported...

> @@ -438,6 +438,9 @@ FSNOTIFY_ITER_FUNCS(sb, SB)
>  
>  #define fsnotify_foreach_iter_type(type) \
>  	for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++)
> +#define fsnotify_foreach_iter_mark_type(iter, mark, type) \
> +	for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) \
> +		if (!(mark = fsnotify_iter_mark(iter, type))) {} else

Hum, you're really inventive here ;) I'd rather go for something a bit more
conservative and readable like:

static inline int fsnotify_iter_step(struct fsnotify_iter_info *iter, int type,
				     struct fsnotify_mark **markp)
{
	while (type < FSNOTIFY_ITER_TYPE_COUNT) {
		*markp = fsnotify_iter_mark(iter, type);
		if (*markp)
			break;
		type++;
	}
	return type;
}

#define fsnotify_foreach_iter_mark_type(iter, mark, type) \
	for (type = 0; \
	     (type = fsnotify_iter_step(iter, type, &mark)) < FSNOTIFY_ITER_TYPE_COUNT; \
	     type++)


								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] fsnotify: consistent behavior for parent not watching children

[Jan Kara]

On Wed 11-05-22 12:29:14, Amir Goldstein wrote:
> The logic for handling events on child in groups that have a mark on
> the parent inode, but without FS_EVENT_ON_CHILD flag in the mask is
> duplicated in several places and inconsistent.
> 
> Move the logic into the preparation of mark type iterator, so that the
> parent mark type will be excluded from all mark type iterations in that
> case.
> 
> This results in several subtle changes of behavior, hopefully all
> desired changes of behavior, for example:
> 
> - Group A has a mount mark with FS_MODIFY in mask
> - Group A has a mark with ignore mask that does not survive FS_MODIFY
>   and does not watch children on directory D.
> - Group B has a mark with FS_MODIFY in mask that does watch children
>   on directory D.
> - FS_MODIFY event on file D/foo should not clear the ignore mask of
>   group A, but before this change it does

Since FS_MODIFY of directory never happens I guess the ignore mask is never
cleared? Am I missing something?

> And if group A ignore mask was set to survive FS_MODIFY:
> - FS_MODIFY event on file D/foo should be reported to group A on account
>   of the mount mark, but before this change it is wrongly ignored
> 
> Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
> Reported-by: Jan Kara <jack@suse.com>
> Link: https://lore.kernel.org/linux-fsdevel/20220314113337.j7slrb5srxukztje@quack3.lan/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Just one nit below.

> @@ -393,6 +386,23 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
>  	return hlist_entry_safe(node, struct fsnotify_mark, obj_list);
>  }
>  
> +static void fsnotify_iter_set_report_mark_type(
> +		struct fsnotify_iter_info *iter_info, int iter_type)
> +{
> +	struct fsnotify_mark *mark = iter_info->marks[iter_type];
> +
> +	/*
> +	 * FSNOTIFY_ITER_TYPE_PARENT indicates that this inode is watching
> +	 * children and interested in this event, which is an event
> +	 * possible on child. But is *this mark* watching children?
> +	 */
> +	if (iter_type == FSNOTIFY_ITER_TYPE_PARENT &&
> +	    !(mark->mask & FS_EVENT_ON_CHILD))
> +		return;
> +
> +	fsnotify_iter_set_report_type(iter_info, iter_type);
> +}
> +
>  /*
>   * iter_info is a multi head priority queue of marks.
>   * Pick a subset of marks from queue heads, all with the same group
> @@ -423,7 +433,7 @@ static bool fsnotify_iter_select_report_types(
>  	fsnotify_foreach_iter_type(type) {
>  		mark = iter_info->marks[type];
>  		if (mark && mark->group == iter_info->current_group)
> -			fsnotify_iter_set_report_type(iter_info, type);
> +			fsnotify_iter_set_report_mark_type(iter_info, type);
>  	}

I think it is confusing to hide another condition in
fsnotify_iter_set_report_mark_type() I'd rather have it explicitely here in
fsnotify_iter_select_report_types().

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] fsnotify: consistent behavior for parent not watching children

[Amir Goldstein]

On Wed, May 11, 2022 at 4:09 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 11-05-22 12:29:14, Amir Goldstein wrote:
> > The logic for handling events on child in groups that have a mark on
> > the parent inode, but without FS_EVENT_ON_CHILD flag in the mask is
> > duplicated in several places and inconsistent.
> >
> > Move the logic into the preparation of mark type iterator, so that the
> > parent mark type will be excluded from all mark type iterations in that
> > case.
> >
> > This results in several subtle changes of behavior, hopefully all
> > desired changes of behavior, for example:
> >
> > - Group A has a mount mark with FS_MODIFY in mask
> > - Group A has a mark with ignore mask that does not survive FS_MODIFY
> >   and does not watch children on directory D.
> > - Group B has a mark with FS_MODIFY in mask that does watch children
> >   on directory D.
> > - FS_MODIFY event on file D/foo should not clear the ignore mask of
> >   group A, but before this change it does
>
> Since FS_MODIFY of directory never happens I guess the ignore mask is never
> cleared? Am I missing something?

According to the code in send_to_group()
If D has FS_EVENT_ON_CHILD in mask then
The the inode mask on D would get events on D/foo
therefore
The ignore mask on D should ignore events (e.g. from mount mark) on D/foo
therefore
A MODIFY event on D/foo should clear the ignore mask on D

This is expected. The bug is that the ignore mask is cleared also
when D does not have FS_EVENT_ON_CHILD in the mask.

>
> > And if group A ignore mask was set to survive FS_MODIFY:
> > - FS_MODIFY event on file D/foo should be reported to group A on account
> >   of the mount mark, but before this change it is wrongly ignored
> >
> > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
> > Reported-by: Jan Kara <jack@suse.com>
> > Link: https://lore.kernel.org/linux-fsdevel/20220314113337.j7slrb5srxukztje@quack3.lan/
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Just one nit below.
>
> > @@ -393,6 +386,23 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
> >       return hlist_entry_safe(node, struct fsnotify_mark, obj_list);
> >  }
> >
> > +static void fsnotify_iter_set_report_mark_type(
> > +             struct fsnotify_iter_info *iter_info, int iter_type)
> > +{
> > +     struct fsnotify_mark *mark = iter_info->marks[iter_type];
> > +
> > +     /*
> > +      * FSNOTIFY_ITER_TYPE_PARENT indicates that this inode is watching
> > +      * children and interested in this event, which is an event
> > +      * possible on child. But is *this mark* watching children?
> > +      */
> > +     if (iter_type == FSNOTIFY_ITER_TYPE_PARENT &&
> > +         !(mark->mask & FS_EVENT_ON_CHILD))
> > +             return;
> > +
> > +     fsnotify_iter_set_report_type(iter_info, iter_type);
> > +}
> > +
> >  /*
> >   * iter_info is a multi head priority queue of marks.
> >   * Pick a subset of marks from queue heads, all with the same group
> > @@ -423,7 +433,7 @@ static bool fsnotify_iter_select_report_types(
> >       fsnotify_foreach_iter_type(type) {
> >               mark = iter_info->marks[type];
> >               if (mark && mark->group == iter_info->current_group)
> > -                     fsnotify_iter_set_report_type(iter_info, type);
> > +                     fsnotify_iter_set_report_mark_type(iter_info, type);
> >       }
>
> I think it is confusing to hide another condition in
> fsnotify_iter_set_report_mark_type() I'd rather have it explicitely here in
> fsnotify_iter_select_report_types().

OK.

Thanks,
Amir.

Re: [PATCH 2/2] fsnotify: consistent behavior for parent not watching children

[Jan Kara]

On Wed 11-05-22 16:37:36, Amir Goldstein wrote:
> On Wed, May 11, 2022 at 4:09 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 11-05-22 12:29:14, Amir Goldstein wrote:
> > > The logic for handling events on child in groups that have a mark on
> > > the parent inode, but without FS_EVENT_ON_CHILD flag in the mask is
> > > duplicated in several places and inconsistent.
> > >
> > > Move the logic into the preparation of mark type iterator, so that the
> > > parent mark type will be excluded from all mark type iterations in that
> > > case.
> > >
> > > This results in several subtle changes of behavior, hopefully all
> > > desired changes of behavior, for example:
> > >
> > > - Group A has a mount mark with FS_MODIFY in mask
> > > - Group A has a mark with ignore mask that does not survive FS_MODIFY
> > >   and does not watch children on directory D.
> > > - Group B has a mark with FS_MODIFY in mask that does watch children
> > >   on directory D.
> > > - FS_MODIFY event on file D/foo should not clear the ignore mask of
> > >   group A, but before this change it does
> >
> > Since FS_MODIFY of directory never happens I guess the ignore mask is never
> > cleared? Am I missing something?
> 
> According to the code in send_to_group()
> If D has FS_EVENT_ON_CHILD in mask then
> The the inode mask on D would get events on D/foo
> therefore
> The ignore mask on D should ignore events (e.g. from mount mark) on D/foo
> therefore
> A MODIFY event on D/foo should clear the ignore mask on D
> 
> This is expected. The bug is that the ignore mask is cleared also
> when D does not have FS_EVENT_ON_CHILD in the mask.

Ah, now I understand. Thanks for explanation.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] fsnotify: introduce mark type iterator

[Amir Goldstein]

On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 11-05-22 12:29:13, Amir Goldstein wrote:
> > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code
> > of iteratating all marks of a specific group interested in an event
> > by consulting the iterator report_mask.
> >
> > Use an open coded version of that iterator in fsnotify_iter_next()
> > that collects all marks of the current iteration group without
> > consulting the iterator report_mask.
> >
> > At the moment, the two iterator variants are the same, but this
> > decoupling will allow us to exclude some of the group's marks from
> > reporting the event, for example for event on child and inode marks
> > on parent did not request to watch events on children.
> >
> > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
> > Reported-by: Jan Kara <jack@suse.com>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Mostly looks good. Two nits below.
>
> >  /*
> > - * Pop from iter_info multi head queue, the marks that were iterated in the
> > + * Pop from iter_info multi head queue, the marks that belong to the group of
> >   * current iteration step.
> >   */
> >  static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
> >  {
> > +     struct fsnotify_mark *mark;
> >       int type;
> >
> >       fsnotify_foreach_iter_type(type) {
> > -             if (fsnotify_iter_should_report_type(iter_info, type))
> > +             mark = iter_info->marks[type];
> > +             if (mark && mark->group == iter_info->current_group)
> >                       iter_info->marks[type] =
> >                               fsnotify_next_mark(iter_info->marks[type]);
>
> Wouldn't it be more natural here to use the new helper
> fsnotify_foreach_iter_mark_type()? In principle we want to advance mark
> types which were already reported...

Took me an embarrassing amount of time to figure out why this would be wrong
and I must have known this a few weeks ago when I wrote the patch, so
a comment is in order:

        /*
         * We cannot use fsnotify_foreach_iter_mark_type() here because we
         * may need to check if next group has a mark of type X even if current
         * group did not have a mark of type X.
         */

>
> > @@ -438,6 +438,9 @@ FSNOTIFY_ITER_FUNCS(sb, SB)
> >
> >  #define fsnotify_foreach_iter_type(type) \
> >       for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++)
> > +#define fsnotify_foreach_iter_mark_type(iter, mark, type) \
> > +     for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) \
> > +             if (!(mark = fsnotify_iter_mark(iter, type))) {} else
>
> Hum, you're really inventive here ;) I'd rather go for something a bit more
> conservative and readable like:

It's good that you are here to restrain me ;-)

>
> static inline int fsnotify_iter_step(struct fsnotify_iter_info *iter, int type,
>                                      struct fsnotify_mark **markp)
> {
>         while (type < FSNOTIFY_ITER_TYPE_COUNT) {
>                 *markp = fsnotify_iter_mark(iter, type);
>                 if (*markp)
>                         break;
>                 type++;
>         }
>         return type;
> }
>
> #define fsnotify_foreach_iter_mark_type(iter, mark, type) \
>         for (type = 0; \
>              (type = fsnotify_iter_step(iter, type, &mark)) < FSNOTIFY_ITER_TYPE_COUNT; \
>              type++)
>
>

That looks nicer.

Thanks,
Amir.

Re: [PATCH 1/2] fsnotify: introduce mark type iterator

[Jan Kara]

On Wed 11-05-22 21:26:16, Amir Goldstein wrote:
> On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 11-05-22 12:29:13, Amir Goldstein wrote:
> > > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code
> > > of iteratating all marks of a specific group interested in an event
> > > by consulting the iterator report_mask.
> > >
> > > Use an open coded version of that iterator in fsnotify_iter_next()
> > > that collects all marks of the current iteration group without
> > > consulting the iterator report_mask.
> > >
> > > At the moment, the two iterator variants are the same, but this
> > > decoupling will allow us to exclude some of the group's marks from
> > > reporting the event, for example for event on child and inode marks
> > > on parent did not request to watch events on children.
> > >
> > > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
> > > Reported-by: Jan Kara <jack@suse.com>
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Mostly looks good. Two nits below.
> >
> > >  /*
> > > - * Pop from iter_info multi head queue, the marks that were iterated in the
> > > + * Pop from iter_info multi head queue, the marks that belong to the group of
> > >   * current iteration step.
> > >   */
> > >  static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
> > >  {
> > > +     struct fsnotify_mark *mark;
> > >       int type;
> > >
> > >       fsnotify_foreach_iter_type(type) {
> > > -             if (fsnotify_iter_should_report_type(iter_info, type))
> > > +             mark = iter_info->marks[type];
> > > +             if (mark && mark->group == iter_info->current_group)
> > >                       iter_info->marks[type] =
> > >                               fsnotify_next_mark(iter_info->marks[type]);
> >
> > Wouldn't it be more natural here to use the new helper
> > fsnotify_foreach_iter_mark_type()? In principle we want to advance mark
> > types which were already reported...
> 
> Took me an embarrassing amount of time to figure out why this would be wrong
> and I must have known this a few weeks ago when I wrote the patch, so
> a comment is in order:
> 
>         /*
>          * We cannot use fsnotify_foreach_iter_mark_type() here because we
>          * may need to check if next group has a mark of type X even if current
>          * group did not have a mark of type X.
>          */

Well, but this function is just advancing the lists for marks we have
already processed. And processed marks are exactly those set in report_mask.
So your code should be equivalent to the old one but using
fsnotify_foreach_iter_mark_type() should work as well AFAICT.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] fsnotify: introduce mark type iterator

[Amir Goldstein]

On Thu, May 12, 2022 at 8:20 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 11-05-22 21:26:16, Amir Goldstein wrote:
> > On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Wed 11-05-22 12:29:13, Amir Goldstein wrote:
> > > > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code
> > > > of iteratating all marks of a specific group interested in an event
> > > > by consulting the iterator report_mask.
> > > >
> > > > Use an open coded version of that iterator in fsnotify_iter_next()
> > > > that collects all marks of the current iteration group without
> > > > consulting the iterator report_mask.
> > > >
> > > > At the moment, the two iterator variants are the same, but this
> > > > decoupling will allow us to exclude some of the group's marks from
> > > > reporting the event, for example for event on child and inode marks
> > > > on parent did not request to watch events on children.
> > > >
> > > > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
> > > > Reported-by: Jan Kara <jack@suse.com>
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > Mostly looks good. Two nits below.
> > >
> > > >  /*
> > > > - * Pop from iter_info multi head queue, the marks that were iterated in the
> > > > + * Pop from iter_info multi head queue, the marks that belong to the group of
> > > >   * current iteration step.
> > > >   */
> > > >  static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
> > > >  {
> > > > +     struct fsnotify_mark *mark;
> > > >       int type;
> > > >
> > > >       fsnotify_foreach_iter_type(type) {
> > > > -             if (fsnotify_iter_should_report_type(iter_info, type))
> > > > +             mark = iter_info->marks[type];
> > > > +             if (mark && mark->group == iter_info->current_group)
> > > >                       iter_info->marks[type] =
> > > >                               fsnotify_next_mark(iter_info->marks[type]);
> > >
> > > Wouldn't it be more natural here to use the new helper
> > > fsnotify_foreach_iter_mark_type()? In principle we want to advance mark
> > > types which were already reported...
> >
> > Took me an embarrassing amount of time to figure out why this would be wrong
> > and I must have known this a few weeks ago when I wrote the patch, so
> > a comment is in order:
> >
> >         /*
> >          * We cannot use fsnotify_foreach_iter_mark_type() here because we
> >          * may need to check if next group has a mark of type X even if current
> >          * group did not have a mark of type X.
> >          */
>
> Well, but this function is just advancing the lists for marks we have
> already processed. And processed marks are exactly those set in report_mask.
> So your code should be equivalent to the old one but using
> fsnotify_foreach_iter_mark_type() should work as well AFAICT.

Yes, you are right about that. But,
With the 2 patches applied, fanotify09 test gets into livelock here.
Fixing the livelock requires that fsnotify_iter_should_report_type()
condition is removed.

I am assuming that it is the next patch that introduces the livelock,
but I cannot figure out why the comment I have written above is not true
in the code in master.

Could it be that the livelock already exists but we do not have a test case
to trigger it and the test case in fanotify09 which checks the next patch
is just a private case?

In any case, even if the logic change here is not needed before the next
patch, it would make no sense to convert this function to
fsnotify_foreach_iter_mark_type() and change it back.
The most that could make sense is to add this comment and remove
fsnotify_iter_should_report_type() together with the next patch.

Can you wrap your head around this? Because I can't.

Thanks,
Amir.

Re: [PATCH 1/2] fsnotify: introduce mark type iterator

[Amir Goldstein]

On Thu, May 12, 2022 at 8:50 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, May 12, 2022 at 8:20 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 11-05-22 21:26:16, Amir Goldstein wrote:
> > > On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Wed 11-05-22 12:29:13, Amir Goldstein wrote:
> > > > > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code
> > > > > of iteratating all marks of a specific group interested in an event
> > > > > by consulting the iterator report_mask.
> > > > >
> > > > > Use an open coded version of that iterator in fsnotify_iter_next()
> > > > > that collects all marks of the current iteration group without
> > > > > consulting the iterator report_mask.
> > > > >
> > > > > At the moment, the two iterator variants are the same, but this
> > > > > decoupling will allow us to exclude some of the group's marks from
> > > > > reporting the event, for example for event on child and inode marks
> > > > > on parent did not request to watch events on children.
> > > > >
> > > > > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
> > > > > Reported-by: Jan Kara <jack@suse.com>
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > >
> > > > Mostly looks good. Two nits below.
> > > >
> > > > >  /*
> > > > > - * Pop from iter_info multi head queue, the marks that were iterated in the
> > > > > + * Pop from iter_info multi head queue, the marks that belong to the group of
> > > > >   * current iteration step.
> > > > >   */
> > > > >  static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
> > > > >  {
> > > > > +     struct fsnotify_mark *mark;
> > > > >       int type;
> > > > >
> > > > >       fsnotify_foreach_iter_type(type) {
> > > > > -             if (fsnotify_iter_should_report_type(iter_info, type))
> > > > > +             mark = iter_info->marks[type];
> > > > > +             if (mark && mark->group == iter_info->current_group)
> > > > >                       iter_info->marks[type] =
> > > > >                               fsnotify_next_mark(iter_info->marks[type]);
> > > >
> > > > Wouldn't it be more natural here to use the new helper
> > > > fsnotify_foreach_iter_mark_type()? In principle we want to advance mark
> > > > types which were already reported...
> > >
> > > Took me an embarrassing amount of time to figure out why this would be wrong
> > > and I must have known this a few weeks ago when I wrote the patch, so
> > > a comment is in order:
> > >
> > >         /*
> > >          * We cannot use fsnotify_foreach_iter_mark_type() here because we
> > >          * may need to check if next group has a mark of type X even if current
> > >          * group did not have a mark of type X.
> > >          */
> >
> > Well, but this function is just advancing the lists for marks we have
> > already processed. And processed marks are exactly those set in report_mask.
> > So your code should be equivalent to the old one but using
> > fsnotify_foreach_iter_mark_type() should work as well AFAICT.
>
> Yes, you are right about that. But,
> With the 2 patches applied, fanotify09 test gets into livelock here.
> Fixing the livelock requires that fsnotify_iter_should_report_type()
> condition is removed.
>
> I am assuming that it is the next patch that introduces the livelock,
> but I cannot figure out why the comment I have written above is not true
> in the code in master.

Nevermind. I got it.

>
> Could it be that the livelock already exists but we do not have a test case
> to trigger it and the test case in fanotify09 which checks the next patch
> is just a private case?
>

No. This is because of the change of behavior in the next patch.

> In any case, even if the logic change here is not needed before the next
> patch, it would make no sense to convert this function to
> fsnotify_foreach_iter_mark_type() and change it back.
> The most that could make sense is to add this comment and remove
> fsnotify_iter_should_report_type() together with the next patch.

Yes. I will do that and better explain in the comment.

Thanks,
Amir.

Re: [PATCH 1/2] fsnotify: introduce mark type iterator

[Amir Goldstein]

On Fri, May 13, 2022 at 6:07 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, May 12, 2022 at 8:50 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Thu, May 12, 2022 at 8:20 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Wed 11-05-22 21:26:16, Amir Goldstein wrote:
> > > > On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote:
> > > > >
> > > > > On Wed 11-05-22 12:29:13, Amir Goldstein wrote:
> > > > > > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code
> > > > > > of iteratating all marks of a specific group interested in an event
> > > > > > by consulting the iterator report_mask.
> > > > > >
> > > > > > Use an open coded version of that iterator in fsnotify_iter_next()
> > > > > > that collects all marks of the current iteration group without
> > > > > > consulting the iterator report_mask.
> > > > > >
> > > > > > At the moment, the two iterator variants are the same, but this
> > > > > > decoupling will allow us to exclude some of the group's marks from
> > > > > > reporting the event, for example for event on child and inode marks
> > > > > > on parent did not request to watch events on children.
> > > > > >
> > > > > > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
> > > > > > Reported-by: Jan Kara <jack@suse.com>
> > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > >
> > > > > Mostly looks good. Two nits below.
> > > > >
> > > > > >  /*
> > > > > > - * Pop from iter_info multi head queue, the marks that were iterated in the
> > > > > > + * Pop from iter_info multi head queue, the marks that belong to the group of
> > > > > >   * current iteration step.
> > > > > >   */
> > > > > >  static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
> > > > > >  {
> > > > > > +     struct fsnotify_mark *mark;
> > > > > >       int type;
> > > > > >
> > > > > >       fsnotify_foreach_iter_type(type) {
> > > > > > -             if (fsnotify_iter_should_report_type(iter_info, type))
> > > > > > +             mark = iter_info->marks[type];
> > > > > > +             if (mark && mark->group == iter_info->current_group)
> > > > > >                       iter_info->marks[type] =
> > > > > >                               fsnotify_next_mark(iter_info->marks[type]);
> > > > >
> > > > > Wouldn't it be more natural here to use the new helper
> > > > > fsnotify_foreach_iter_mark_type()? In principle we want to advance mark
> > > > > types which were already reported...
> > > >
> > > > Took me an embarrassing amount of time to figure out why this would be wrong
> > > > and I must have known this a few weeks ago when I wrote the patch, so
> > > > a comment is in order:
> > > >
> > > >         /*
> > > >          * We cannot use fsnotify_foreach_iter_mark_type() here because we
> > > >          * may need to check if next group has a mark of type X even if current
> > > >          * group did not have a mark of type X.
> > > >          */
> > >
> > > Well, but this function is just advancing the lists for marks we have
> > > already processed. And processed marks are exactly those set in report_mask.
> > > So your code should be equivalent to the old one but using
> > > fsnotify_foreach_iter_mark_type() should work as well AFAICT.
> >
> > Yes, you are right about that. But,
> > With the 2 patches applied, fanotify09 test gets into livelock here.
> > Fixing the livelock requires that fsnotify_iter_should_report_type()
> > condition is removed.
> >
> > I am assuming that it is the next patch that introduces the livelock,
> > but I cannot figure out why the comment I have written above is not true
> > in the code in master.
>
> Nevermind. I got it.
>
> >
> > Could it be that the livelock already exists but we do not have a test case
> > to trigger it and the test case in fanotify09 which checks the next patch
> > is just a private case?
> >
>
> No. This is because of the change of behavior in the next patch.

Bah, no it is not.
Emailing before coffee is not a good idea...

It is *this* patch that changes the behavior:
fsnotify_iter_select_report_types() can return true with 0 report_mask
and it is valid.
This is documented in the updated comments above fsnotify_iter_next()
and fsnotify_iter_select_report_types() and also in commit message:

    Use an open coded version of that iterator in fsnotify_iter_next()
    that collects all marks of the current iteration group without
    consulting the iterator report_mask.

The comment I added to v2 could be better phrased as:

 /*
  * We cannot use fsnotify_foreach_iter_mark_type() here because we
  * may need to advance a mark of type X that belongs to current_group
  * but was not selected for reporting.
  */

Is that better? Do you want me to re-post or can you fix that on commit?
Maybe even phrase it better than I did.

Thanks,
Amir.