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

[PATCH v2 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.

Changes since v1:
- Change hacky mark iterator macros
- Clarify mark iterator in fsnotify_iter_next()
- Open code parent mark type logic in
  fsnotify_iter_select_report_types()

[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             | 85 +++++++++++++++++---------------
 include/linux/fsnotify_backend.h | 31 +++++++++---
 3 files changed, 73 insertions(+), 67 deletions(-)

-- 
2.25.1

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

[Amir Goldstein]

fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code
of iterating 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             | 53 ++++++++++++++++----------------
 include/linux/fsnotify_backend.h | 31 ++++++++++++++-----
 3 files changed, 54 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..c5bb2405ead3 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,37 @@ 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;
 
+	/*
+	 * 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.
+	 */
 	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..9560734759fa 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,31 @@ 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;
+}
+
+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_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 +450,11 @@ 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_step(iter, type, &mark), \
+	     type < FSNOTIFY_ITER_TYPE_COUNT; \
+	     type++)
 
 /*
  * fsnotify_connp_t is what we embed in objects which connector can be attached
-- 
2.25.1

[PATCH v2 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          | 34 +++++++++++++++++++---------------
 2 files changed, 20 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 c5bb2405ead3..2c9a13c31ac9 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
@@ -422,8 +415,19 @@ static bool fsnotify_iter_select_report_types(
 	iter_info->report_mask = 0;
 	fsnotify_foreach_iter_type(type) {
 		mark = iter_info->marks[type];
-		if (mark && mark->group == iter_info->current_group)
+		if (mark && mark->group == iter_info->current_group) {
+			/*
+			 * 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 (type == FSNOTIFY_ITER_TYPE_PARENT &&
+			    !(mark->mask & FS_EVENT_ON_CHILD))
+				continue;
+
 			fsnotify_iter_set_report_type(iter_info, type);
+		}
 	}
 
 	return true;
-- 
2.25.1

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

[Jan Kara]

On Wed 11-05-22 22:02:11, Amir Goldstein wrote:
> 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.
> 
> Changes since v1:
> - Change hacky mark iterator macros
> - Clarify mark iterator in fsnotify_iter_next()
> - Open code parent mark type logic in
>   fsnotify_iter_select_report_types()


Thanks! I've pushed the changes into my tree and updated the comment in
patch 1 as you suggested.

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

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

[Amir Goldstein]

On Wed, May 11, 2022 at 10:02 PM Amir Goldstein <amir73il@gmail.com> 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
>
> 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>
> ---

Greg,

FYI, this needs the previous commit to apply to 5.18.y:

e730558adffb fsnotify: consistent behavior for parent not watching children
14362a254179 fsnotify: introduce mark type iterator

They won't apply to earlier versions and this is a fix for a very minor bug
that existed forever, so no need to bother.

Thanks,
Amir.

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

[Greg KH]

On Mon, Jun 20, 2022 at 05:16:16PM +0300, Amir Goldstein wrote:
> On Wed, May 11, 2022 at 10:02 PM Amir Goldstein <amir73il@gmail.com> 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
> >
> > 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>
> > ---
> 
> Greg,
> 
> FYI, this needs the previous commit to apply to 5.18.y:

What is "this" here?  What git id?

> e730558adffb fsnotify: consistent behavior for parent not watching children
> 14362a254179 fsnotify: introduce mark type iterator
> 
> They won't apply to earlier versions and this is a fix for a very minor bug
> that existed forever, so no need to bother.

So what exactly needs to be applied in what order and to what trees?

confused,

greg k-h

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

[Amir Goldstein]

On Mon, Jun 20, 2022 at 11:34 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 20, 2022 at 05:16:16PM +0300, Amir Goldstein wrote:
> > On Wed, May 11, 2022 at 10:02 PM Amir Goldstein <amir73il@gmail.com> 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
> > >
> > > 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>
> > > ---
> >
> > Greg,
> >
> > FYI, this needs the previous commit to apply to 5.18.y:
>
> What is "this" here?  What git id?

Sorry, this commit:

> > e730558adffb fsnotify: consistent behavior for parent not watching children

Needs this previous commit:

> > 14362a254179 fsnotify: introduce mark type iterator

> > They won't apply to earlier versions and this is a fix for a very minor bug
> > that existed forever, so no need to bother.
>
> So what exactly needs to be applied in what order and to what trees?
>

To apply to 5.18.y.

Don't bother trying to apply either to earlier trees.

Thanks,
Amir.

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

[Greg KH]

On Tue, Jun 21, 2022 at 06:04:33AM +0300, Amir Goldstein wrote:
> On Mon, Jun 20, 2022 at 11:34 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jun 20, 2022 at 05:16:16PM +0300, Amir Goldstein wrote:
> > > On Wed, May 11, 2022 at 10:02 PM Amir Goldstein <amir73il@gmail.com> 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
> > > >
> > > > 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>
> > > > ---
> > >
> > > Greg,
> > >
> > > FYI, this needs the previous commit to apply to 5.18.y:
> >
> > What is "this" here?  What git id?
> 
> Sorry, this commit:
> 
> > > e730558adffb fsnotify: consistent behavior for parent not watching children
> 
> Needs this previous commit:
> 
> > > 14362a254179 fsnotify: introduce mark type iterator
> 
> > > They won't apply to earlier versions and this is a fix for a very minor bug
> > > that existed forever, so no need to bother.
> >
> > So what exactly needs to be applied in what order and to what trees?
> >
> 
> To apply to 5.18.y.

Now queued up, thanks.

> Don't bother trying to apply either to earlier trees.

So the Fixes: tag lied?  No wonder I was confused :)

thanks,

greg k-h

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

[Amir Goldstein]

On Thu, Jun 23, 2022 at 7:09 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jun 21, 2022 at 06:04:33AM +0300, Amir Goldstein wrote:
> > On Mon, Jun 20, 2022 at 11:34 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Jun 20, 2022 at 05:16:16PM +0300, Amir Goldstein wrote:
> > > > On Wed, May 11, 2022 at 10:02 PM Amir Goldstein <amir73il@gmail.com> 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
> > > > >
> > > > > 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>
> > > > > ---
> > > >
> > > > Greg,
> > > >
> > > > FYI, this needs the previous commit to apply to 5.18.y:
> > >
> > > What is "this" here?  What git id?
> >
> > Sorry, this commit:
> >
> > > > e730558adffb fsnotify: consistent behavior for parent not watching children
> >
> > Needs this previous commit:
> >
> > > > 14362a254179 fsnotify: introduce mark type iterator
> >
> > > > They won't apply to earlier versions and this is a fix for a very minor bug
> > > > that existed forever, so no need to bother.
> > >
> > > So what exactly needs to be applied in what order and to what trees?
> > >
> >
> > To apply to 5.18.y.
>
> Now queued up, thanks.
>
> > Don't bother trying to apply either to earlier trees.
>
> So the Fixes: tag lied?  No wonder I was confused :)

No it hasn't lied.

The fix could be backported to an earlier kernel, but it is not trivial
and I don't think it is worth the effort, because the behavior of this
corner case was undefined for the entire lifetime of fanotify.

IOW, if stable scripts send me the message that the patch
does not apply cleanly, I won't be doing anything about it.

Furthermore, I instrumented the LTP regression test for this
bug with:

        if (tc->ignore && tst_kvercmp(5, 19, 0) < 0) {
                tst_res(TCONF, "ignored mask on parent dir has undefined "
                                "behavior on kernel < 5.19");
                return;
        }

So no one is going to be asking for this backport, unless they really
encounter the problem in real world use cases.

Thanks,
Amir.