On Thu, Jun 29, 2023 at 1:18 PM Jan Kara <jack@xxxxxxx> 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@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... > 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.
