On Mon, Apr 11, 2022 at 1:53 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Tue 29-03-22 10:49:00, Amir Goldstein wrote:
> > Handle FAN_MARK_IGNORED_SURV_MODIFY flag change in a helper that
> > is called after updating the mark mask.
> >
> > Move recalc of object mask inside fanotify_mark_add_to_mask() which
> > makes the code a bit simpler to follow.
> >
> > Add also helper to translate fsnotify mark flags to user visible
> > fanotify mark flags.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> > fs/notify/fanotify/fanotify.h | 10 ++++++++
> > fs/notify/fanotify/fanotify_user.c | 39 +++++++++++++++++-------------
> > fs/notify/fdinfo.c | 6 ++---
> > 3 files changed, 34 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> > index a3d5b751cac5..87142bc0131a 100644
> > --- a/fs/notify/fanotify/fanotify.h
> > +++ b/fs/notify/fanotify/fanotify.h
> > @@ -490,3 +490,13 @@ static inline unsigned int fanotify_event_hash_bucket(
> > {
> > return event->hash & FANOTIFY_HTABLE_MASK;
> > }
> > +
> > +static inline unsigned int fanotify_mark_user_flags(struct fsnotify_mark *mark)
> > +{
> > + unsigned int mflags = 0;
> > +
> > + if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)
> > + mflags |= FAN_MARK_IGNORED_SURV_MODIFY;
> > +
> > + return mflags;
> > +}
>
> This, together with fdinfo change should probably be a separate commit. I
> don't see a good reason to mix these two changes...
>
True.
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 0f0db1efa379..6e78ea12239c 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -1081,42 +1081,50 @@ static int fanotify_remove_inode_mark(struct fsnotify_group *group,
> > flags, umask);
> > }
> >
> > -static void fanotify_mark_add_ignored_mask(struct fsnotify_mark *fsn_mark,
> > - __u32 mask, unsigned int flags,
> > - __u32 *removed)
> > +static int fanotify_mark_update_flags(struct fsnotify_mark *fsn_mark,
> > + unsigned int flags, bool *recalc)
> > {
> > - fsn_mark->ignored_mask |= mask;
> > -
> > /*
> > * Setting FAN_MARK_IGNORED_SURV_MODIFY for the first time may lead to
> > * the removal of the FS_MODIFY bit in calculated mask if it was set
> > * because of an ignored mask that is now going to survive FS_MODIFY.
> > */
> > if ((flags & FAN_MARK_IGNORED_SURV_MODIFY) &&
> > + (flags & FAN_MARK_IGNORED_MASK) &&
> > !(fsn_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)) {
> > fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY;
> > if (!(fsn_mark->mask & FS_MODIFY))
> > - *removed = FS_MODIFY;
> > + *recalc = true;
> > }
> > +
> > + return 0;
> > }
> >
> > -static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
> > - __u32 mask, unsigned int flags,
> > - __u32 *removed)
> > +static int fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
> > + __u32 mask, unsigned int flags)
> > {
> > - __u32 oldmask, newmask;
> > + __u32 oldmask;
> > + bool recalc = false;
> > + int ret;
> >
> > spin_lock(&fsn_mark->lock);
> > oldmask = fsnotify_calc_mask(fsn_mark);
> > if (!(flags & FAN_MARK_IGNORED_MASK)) {
> > fsn_mark->mask |= mask;
> > } else {
> > - fanotify_mark_add_ignored_mask(fsn_mark, mask, flags, removed);
> > + fsn_mark->ignored_mask |= mask;
> > }
> > - newmask = fsnotify_calc_mask(fsn_mark);
> > +
> > + recalc = fsnotify_calc_mask(fsn_mark) & ~oldmask &
> > + ~fsnotify_conn_mask(fsn_mark->connector);
>
> oldmask should be a subset of fsnotify_conn_mask() so the above should be
> equivalent to:
>
> recalc = fsnotify_calc_mask(fsn_mark) & ~fsnotify_conn_mask(fsn_mark->connector)
>
> shouldn't it?
I just translated the old expression of 'added',
but I guess there is no reason to look at the oldmask
only the newmask matters, so I can drop oldmask variable altogether.
Thanks,
Amir.
