On Wed, May 11, 2022 at 4:09 PM Jan Kara <jack@xxxxxxx> 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@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().
OK.
Thanks,
Amir.
