On Thu, Jun 29, 2023 at 3:20 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > 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. > Here it is: https://lore.kernel.org/linux-fsdevel/20200629130915.GF26507@xxxxxxxxxxxxxx/ A regression report on Mel's patch: e9c15badbb7b ("fs: Do not check if there is a fsnotify watcher on pseudo inodes") Chromium needs IN_OPEN/IN_CLOSE on anon pipes. It does not need IN_ACCESS/IN_MODIFY, but the value of eliminating those was deemed as marginal after the alternative optimizations by Mel: 71d734103edf ("fsnotify: Rearrange fast path to minimise overhead when there is no watcher") The reason I would like to ban the "global" watch on all anon inodes is because it is just wrong and an oversight of sb/mount marks that needs to be fixed. The SB_NOUSER optimization is something that we can consider later. It's not critical, but just a very low hanging fruit to pick. Based on this finding, I would go with this RFC patch as is. I will let you decide how to CC stable and about the timing of sending this to Linus. Thanks, Amir.
