Re: [PATCH v7 01/18] fsnotify: opt-in for permission events at file_open_perm() time

[Linus Torvalds <torvalds@linux-foundation.org>]

On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@toxicpanda.com> wrote:
>
> @@ -119,14 +118,37 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
>          * handle creation / destruction events and not "real" file events.
>          */
>         if (file->f_mode & (FMODE_NONOTIFY | FMODE_PATH))
> +               return false;
> +
> +       /* Permission events require that watches are set before FS_OPEN_PERM */
> +       if (mask & ALL_FSNOTIFY_PERM_EVENTS & ~FS_OPEN_PERM &&
> +           !(file->f_mode & FMODE_NOTIFY_PERM))
> +               return false;

This still all looks very strange.

As far as I can tell, there is exactly one user of FS_OPEN_PERM in
'mask', and that's fsnotify_open_perm(). Which is called in exactly
one place: security_file_open(), which is the wrong place to call it
anyway and is the only place where fsnotify is called from the
security layer.

In fact, that looks like an active bug: if you enable FSNOTIFY, but
you *don't* enable CONFIG_SECURITY, the whole fsnotify_open_perm()
will never be called at all.

And I just verified that yes, you can very much generate such a config.

So the whole FS_OPEN_PERM thing looks like a special case, called from
a (broken) special place, and now polluting this "fsnotify_file()"
logic for no actual reason and making it all look unnecessarily messy.

I'd suggest that the whole fsnotify_open_perm() simply be moved to
where it *should* be - in the open path - and not make a bad and
broken attempt at hiding inside the security layer, and not use this
"fsnotify_file()" logic at all.

The open-time logic is different. It shouldn't even attempt - badly -
to look like it's the same thing as some regular file access.

              Linus