On Thu, Jun 29, 2023 at 07:20:44AM +0300, 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@xxxxxxxxx> > --- > > 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. > > 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. > > 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. > > Thoughts? > Am I over thinking this? > > Amir. > > fs/notify/fanotify/fanotify_user.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 95d7d8790bc3..8240a3fdbef0 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -1622,6 +1622,20 @@ static int fanotify_events_supported(struct fsnotify_group *group, > path->mnt->mnt_sb->s_type->fs_flags & FS_DISALLOW_NOTIFY_PERM) > return -EINVAL; > > + /* > + * mount and sb marks are not allowed on kernel internal pseudo fs, > + * like pipe_mnt, because that would subscribe to events on all the > + * anonynous pipes in the system. s/anonynous/anonymous/ > + * > + * XXX: SB_NOUSER covers all of the internal pseudo fs whose objects > + * are not exposed to user's mount namespace, but there are other > + * SB_KERNMOUNT fs, like nsfs, debugfs, for which the value of > + * allowing sb and mount mark is questionable. > + */ > + if (mark_type != FAN_MARK_INODE && > + path->mnt->mnt_sb->s_flags & SB_NOUSER) > + return -EINVAL; It's a good starting point. Looks good to me, Reviewed-by: Christian Brauner <brauner@xxxxxxxxxx>
