diff mbox series

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

Message ID 20220511092914.731897-3-amir73il@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fixes for fanotify parent dir ignore mask logic | expand

Commit Message

Amir Goldstein May 11, 2022, 9:29 a.m. UTC 
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(-)

Comments

Jan Kara May 11, 2022, 1:09 p.m. UTC | #1 
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
Amir Goldstein May 11, 2022, 1:37 p.m. UTC | #2 
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.
Jan Kara May 11, 2022, 1:49 p.m. UTC | #3 
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
diff mbox series

Patch

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;