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...
> - FMODE_NOTIFY_HSM() send pre-conent permission events
^^^ content
Otherwise neat trick, I like it. Some nitty comments below.
> @@ -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.
> @@ -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().
> 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
FSNOTIFY_CONTENT_PERM_EVENTS completely as that has only single use in
ALL_FSNOTIFY_PERM_EVENTS instead of adding more practically unused defines.
> /* 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...
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
