Re: [PATCH v2 7/9] fanotify: record either old name new name or both for FAN_RENAME

[Amir Goldstein <amir73il@gmail.com>]

On Fri, Nov 26, 2021 at 5:14 PM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 19-11-21 09:17:36, Amir Goldstein wrote:
> > We do not want to report the dirfid+name of a directory whose
> > inode/sb are not watched, because watcher may not have permissions
> > to see the directory content.
> >
> > The FAN_MOVED_FROM/TO flags are used internally to indicate to
> > fanotify_alloc_event() if we need to record only the old parent+name,
> > only the new parent+name or both.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/notify/fanotify/fanotify.c | 57 ++++++++++++++++++++++++++++++-----
> >  1 file changed, 50 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 018b32a57702..c0a3fb1dd066 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -290,6 +290,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >       __u32 marks_mask = 0, marks_ignored_mask = 0;
> >       __u32 test_mask, user_mask = FANOTIFY_OUTGOING_EVENTS |
> >                                    FANOTIFY_EVENT_FLAGS;
> > +     __u32 moved_mask = 0;
> >       const struct path *path = fsnotify_data_path(data, data_type);
> >       unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
> >       struct fsnotify_mark *mark;
> > @@ -327,17 +328,44 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >                       continue;
> >
> >               /*
> > -              * If the event is on a child and this mark is on a parent not
> > -              * watching children, don't send it!
> > +              * In the special case of FAN_RENAME event, inode mark is the
> > +              * mark on the old dir and parent mark is the mark on the new
> > +              * dir.  We do not want to report the dirfid+name of a directory
> > +              * whose inode/sb are not watched.  The FAN_MOVE flags
> > +              * are used internally to indicate if we need to report only
> > +              * the old parent+name, only the new parent+name or both.
> >                */
> > -             if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
> > -                 !(mark->mask & FS_EVENT_ON_CHILD))
> > +             if (event_mask & FAN_RENAME) {
> > +                     /* Old dir sb are watched - report old info */
> > +                     if (type != FSNOTIFY_OBJ_TYPE_PARENT &&
> > +                         (mark->mask & FAN_RENAME))
> > +                             moved_mask |= FAN_MOVED_FROM;
> > +                     /* New dir sb are watched - report new info */
> > +                     if (type != FSNOTIFY_OBJ_TYPE_INODE &&
> > +                         (mark->mask & FAN_RENAME))
> > +                             moved_mask |= FAN_MOVED_TO;
> > +             } else if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
> > +                        !(mark->mask & FS_EVENT_ON_CHILD)) {
> > +                     /*
> > +                      * If the event is on a child and this mark is on
> > +                      * a parent not watching children, don't send it!
> > +                      */
> >                       continue;
> > +             }
>
> It feels a bit hacky to mix the "what info to report" into the mask
> especially as otherwise perfectly valid flags. Can we perhaps have a
> separate function to find this out (like fanotify_rename_info_report_mask()
> or something like that) and use it in fanotify_alloc_event() or directly in
> fanotify_handle_event() and pass the result to fanotify_alloc_event()?
> That would seem a bit cleaner to me.

I used fsnotify_iter_info *match_info arg to fanotify_group_event_mask()
to indicate the marks of this group that matched the event and passed
it into fanotify_alloc_event().

Thanks,
Amir.