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@xxxxxxxx>
> Link: https://lore.kernel.org/linux-fsdevel/20220314113337.j7slrb5srxukztje@xxxxxxxxxx/
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
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@xxxxxxxx>
SUSE Labs, CR
