Re: [RFC][PATCH] fanotify: disallow mount/sb marks on kernel internal pseudo fs

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Jun 29, 2023 at 1:18 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 29-06-23 07:20:44, Amir Goldstein wrote:
> > Hopefully, nobody is trying to abuse mount/sb marks for watching all
> > anonymous pipes/inodes.
> >
> > I cannot think of a good reason to allow this - it looks like an
> > oversight that dated back to the original fanotify API.
> >
> > Link: https://lore.kernel.org/linux-fsdevel/20230628101132.kvchg544mczxv2pm@quack3/
> > Fixes: d54f4fba889b ("fanotify: add API to attach/detach super block mark")
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Jan,
> >
> > As discussed, allowing sb/mount mark on anonymous pipes
> > makes no sense and we should not allow it.
> >
> > I've noted FAN_MARK_FILESYSTEM as the Fixes commit as a trigger to
> > backport to maintained LTS kernels event though this dates back to day one
> > with FAN_MARK_MOUNT. Not sure if we should keep the Fixes tag or not.
>
> I can add CC to stable. We can also modify the Fixes tag to:
>
> Fixes: 0ff21db9fcc3 ("fanotify: hooks the fanotify_mark syscall to the vfsmount code")
>
> to make things a bit more accurate. Not that it would matter much...
>

Whatever you decide.
I guess that this could wait for 6.6?
but maybe before, because I wouldn't want to additional
fsnotify splice hooks to be added without this, so then
this restriction can be in place by the time vfs maintainers
merge the splice patches.

> > The reason this is an RFC and that I have not included also the
> > optimization patch is because we may want to consider banning kernel
> > internal inodes from fanotify and/or inotify altogether.
>
> So here I guess you mean to ban also inode marks for them? And by
> kernel-internal I suppose you mean on SB_NOUSER superblock?
>

Yes and yes.

> > The tricky point in banning anonymous pipes from inotify, which
> > could have existing users (?), but maybe not, so maybe this is
> > something that we need to try out.
> >
> > I think we can easily get away with banning anonymous pipes from
> > fanotify altogeter, but I would not like to get to into a situation
> > where new applications will be written to rely on inotify for
> > functionaly that fanotify is never going to have.
>
> Yeah, so didn't we try to already disable inotify on some virtual inodes
> and didn't it break something? I have a vague feeling we've already tried
> that in the past and it didn't quite fly but searching the history didn't
> reveal anything so maybe I'm mistaking it with something else.
>

I do have the same memory now that you mention it.
I will try to track it down.

> I guess you are looking for this so that fsnotify code can bail early when
> it sees such inodes and thus improve performance?
>

Not exactly.

Bailing early is easy even if we allow a mark on anonymous inode.
That is what the optimization patch looks like:

 /* Could the inode be watched by inode/mount/sb mark? */
 static inline bool fsnotify_inode_has_watchers(struct inode *inode, __u32 mask)
 {
+       /*
+        * For objects that are not mapped into user accessible path like
+        * anonymous pipes/inodes, we do not need to check for watchers on
+        * parent/mount/sb and the sb watchers optimizations below are
+        * not as effective, so check the inode mask directly.
+        */
+       if (inode->i_sb->s_flags & SB_NOUSER &&
+           !(mask & inode->i_fsnotify_mask))
+               return 0;
+
        if (mask & ALL_FSNOTIFY_PERM_EVENTS)
                return atomic_long_read(&inode->i_sb->s_fsnotify_perm_watchers);

        return atomic_long_read(&inode->i_sb->s_fsnotify_connectors);
}

My question was about: do we need this optimization patch or could
we just ban SB_NOUSER from inotify and fanotify altogether and then
s_fsnotify_connectors will be zero on the pseudo fs anyway.

Thanks,
Amir.