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@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. 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... > 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? > 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 guess you are looking for this so that fsnotify code can bail early when it sees such inodes and thus improve performance? Honza > 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. > + * > + * 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; > + > /* > * We shouldn't have allowed setting dirent events and the directory > * flags FAN_ONDIR and FAN_EVENT_ON_CHILD in mask of non-dir inode, > -- > 2.34.1 > -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
