On Tue, Jul 8, 2025 at 1:26 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Mon 07-07-25 19:07:04, Amir Goldstein wrote:
> > The most unlikely watched permission event is FAN_ACCESS_PERM, because
> > at the time that it was introduced there were no evictable ignore mark,
> > so subscribing to FAN_ACCESS_PERM would have incured a very high
> > overhead.
> >
> > Yet, when we set the fmode to FMODE_NOTIFY_HSM(), we never skip trying
> > to send FAN_ACCESS_PERM, which is almost always a waste of cycles.
> >
> > We got to this logic because of bundling open permisson events and access
> > permission events in the same category and because FAN_OPEN_PERM is a
> > commonly used event.
> >
> > By open coding fsnotify_open_perm() in fsnotify_open_perm_and_set_mode(),
> > we no longer need to regard FAN_OPEN*_PERM when calculating fmode.
> >
> > This leaves the case of having pre-content events and not having access
> > permission events in the object masks a more likely case than the other
> > way around.
> >
> > Rework the fmode macros and code so that their meaning now refers only
> > to hooks on an already open file:
> >
> > - FMODE_NOTIFY_NONE() skip all events
> > - FMODE_NOTIFY_PERM() send all access permission events
>
> I was a bit confused here but AFAIU you mean "send pre-content events and
> FAN_ACCESS_PERM". And perhaps I'd call this macro
> FMODE_NOTIFY_ACCESS_PERM() because that's the only place where it's going
> to be used...
Yes. agree.
>
> > - FMODE_NOTIFY_HSM() send pre-conent permission events
> ^^^ content
>
>
> Otherwise neat trick, I like it. Some nitty comments below.
Thanks.
>
> > @@ -683,45 +683,70 @@ int fsnotify_open_perm_and_set_mode(struct file *file)
> > }
> >
> > /*
> > - * If there are permission event watchers but no pre-content event
> > - * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that.
> > + * OK, there are some permission event watchers. Check if anybody is
> > + * watching for permission events on *this* file.
> > */
> > - if ((!d_is_dir(dentry) && !d_is_reg(dentry)) ||
> > - likely(!fsnotify_sb_has_priority_watchers(sb,
> > - FSNOTIFY_PRIO_PRE_CONTENT))) {
> > - file_set_fsnotify_mode(file, FMODE_NONOTIFY |
> > - FMODE_NONOTIFY_PERM);
> > + mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
> > + p_mask = fsnotify_object_watched(d_inode(dentry), mnt_mask,
> > + ALL_FSNOTIFY_PERM_EVENTS);
> > + if (dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) {
> > + parent = dget_parent(dentry);
> > + p_mask |= fsnotify_inode_watches_children(d_inode(parent));
> > + dput(parent);
> > + }
> > +
> > + /*
> > + * Without any access permission events, we only need to call the
> > + * open perm hook and no further permission hooks on the open file.
> > + * That is the common case with Anti-Malware protection service.
> > + */
> > + if (likely(!(p_mask & FSNOTIFY_ACCESS_PERM_EVENTS))) {
> > + file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM);
> > goto open_perm;
> > }
>
> Why is the above if needed? It seems to me all the cases are properly
> handled below already? And they are very cheap to check...
>
> > /*
> > - * OK, there are some pre-content watchers. Check if anybody is
> > - * watching for pre-content events on *this* file.
> > + * Legacy FAN_ACCESS_PERM events have very high performance overhead,
> > + * so unlikely to be used in the wild. If they are used there will be
> > + * no optimizations at all.
> > */
> > - mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
> > - if (unlikely(fsnotify_object_watched(d_inode(dentry), mnt_mask,
> > - FSNOTIFY_PRE_CONTENT_EVENTS))) {
> > - /* Enable pre-content events */
> > + if (unlikely(p_mask & FS_ACCESS_PERM)) {
> > + /* Enable all permission and pre-content events */
> > file_set_fsnotify_mode(file, 0);
> > goto open_perm;
> > }
> >
> > - /* Is parent watching for pre-content events on this file? */
> > - if (dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) {
> > - parent = dget_parent(dentry);
> > - p_mask = fsnotify_inode_watches_children(d_inode(parent));
> > - dput(parent);
> > - if (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS) {
> > - /* Enable pre-content events */
> > - file_set_fsnotify_mode(file, 0);
> > - goto open_perm;
> > - }
> > + /*
> > + * Pre-content events are only supported on regular files.
> > + * If there are pre-content event watchers and no permission access
> > + * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that.
> > + * That is the common case with HSM service.
> > + */
> > + if (d_is_reg(dentry) && (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) {
> > + file_set_fsnotify_mode(file, FMODE_NONOTIFY |
> > + FMODE_NONOTIFY_PERM);
> > + goto open_perm;
> > }
> > - /* Nobody watching for pre-content events from this file */
> > - file_set_fsnotify_mode(file, FMODE_NONOTIFY | FMODE_NONOTIFY_PERM);
> > +
> > + /* Nobody watching permission and pre-content events on this file */
> > + file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM);
>
> <snip>
>
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 45fe8f833284..1d54d323d9de 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -205,7 +205,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> > *
> > * FMODE_NONOTIFY - suppress all (incl. non-permission) events.
> > * FMODE_NONOTIFY_PERM - suppress permission (incl. pre-content) events.
> > - * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - suppress only pre-content events.
> > + * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - .. (excl. pre-content) events.
> ^^ I'd write here "suppress
> FAN_ACCESS_PERM" to be explicit what this is about.
ok.
>
> > @@ -213,10 +213,10 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> > #define FMODE_FSNOTIFY_NONE(mode) \
> > ((mode & FMODE_FSNOTIFY_MASK) == FMODE_NONOTIFY)
> > #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> > -#define FMODE_FSNOTIFY_PERM(mode) \
> > +#define FMODE_FSNOTIFY_HSM(mode) \
> > ((mode & FMODE_FSNOTIFY_MASK) == 0 || \
> > (mode & FMODE_FSNOTIFY_MASK) == (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM))
> > -#define FMODE_FSNOTIFY_HSM(mode) \
> > +#define FMODE_FSNOTIFY_PERM(mode) \
> > ((mode & FMODE_FSNOTIFY_MASK) == 0)
>
> As mentioned above I'd call this FMODE_FSNOTIFY_ACCESS_PERM().
ok.
>
> > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> > index 832d94d783d9..557f9b127960 100644
> > --- a/include/linux/fsnotify_backend.h
> > +++ b/include/linux/fsnotify_backend.h
> > @@ -87,14 +87,18 @@
> > /* Mount namespace events */
> > #define FSNOTIFY_MNT_EVENTS (FS_MNT_ATTACH | FS_MNT_DETACH)
> >
> > +#define FSNOTIFY_OPEN_PERM_EVENTS (FS_OPEN_PERM | FS_OPEN_EXEC_PERM)
> > /* Content events can be used to inspect file content */
> > -#define FSNOTIFY_CONTENT_PERM_EVENTS (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | \
> > +#define FSNOTIFY_CONTENT_PERM_EVENTS (FSNOTIFY_OPEN_PERM_EVENTS | \
> > FS_ACCESS_PERM)
>
> You don't use FSNOTIFY_OPEN_PERM_EVENTS anywhere. If anything I'd drop
Right, I will drop it.
> FSNOTIFY_CONTENT_PERM_EVENTS completely as that has only single use in
> ALL_FSNOTIFY_PERM_EVENTS instead of adding more practically unused defines.
>
It is going to be used down the road.
In my followup fa_pre_dir_access patches, I split the dir/file macros:
#define FSNOTIFY_PRE_CONTENT_EVENTS \
(FSNOTIFY_PRE_FILE_CONTENT_EVENTS | \
FSNOTIFY_PRE_DIR_CONTENT_EVENTS)
Because the pre-dir-content events are not applicable ON_CHILD:
#define FS_EVENTS_POSS_ON_CHILD (FSNOTIFY_CONTENT_PERM_EVENTS | \
FSNOTIFY_PRE_FILE_CONTENT_EVENTS | \
> > /* Pre-content events can be used to fill file content */
> > #define FSNOTIFY_PRE_CONTENT_EVENTS (FS_PRE_ACCESS)
> >
> > #define ALL_FSNOTIFY_PERM_EVENTS (FSNOTIFY_CONTENT_PERM_EVENTS | \
> > FSNOTIFY_PRE_CONTENT_EVENTS)
> > +/* Access permission events determine FMODE_NONOTIFY_PERM mode */
> > +#define FSNOTIFY_ACCESS_PERM_EVENTS (FS_ACCESS_PERM | \
> > + FSNOTIFY_PRE_CONTENT_EVENTS)
>
> I don't think this define is needed either so I'd drop it for now...
ok. v2 soon...
Thanks,
Amir.
