On Sun, Feb 02, 2025 at 08:46:21AM +0100, Amir Goldstein wrote: > On Sun, Feb 2, 2025 at 1:58 AM Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > On Sat, 1 Feb 2025 at 06:38, Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > > > Ok, but those "device fds" aren't really device fds in the sense that > > > they are character fds. They are regular files afaict from: > > > > > > vfio_device_open_file(struct vfio_device *device) > > > > > > (Well, it's actually worse as anon_inode_getfile() files don't have any > > > mode at all but that's beside the point.)? > > > > > > In any case, I think you're right that such files would (accidently?) > > > qualify for content watches afaict. So at least that should probably get > > > FMODE_NONOTIFY. > > > > Hmm. Can we just make all anon_inodes do that? I don't think you can > > sanely have pre-content watches on anon-inodes, since you can't really > > have access to them to _set_ the content watch from outside anyway.. > > > > In fact, maybe do it in alloc_file_pseudo()? > > > > The problem is that we cannot set FMODE_NONOTIFY - > we tried that once but it regressed some workloads watching > write on pipe fd or something. Ok, that might be true. But I would assume that most users of alloc_file_pseudo() or the anonymous inode infrastructure will not care about fanotify events. I would not go for a separate helper. It'd be nice to keep the number of file allocation functions low. I'd rather have the subsystems that want it explicitly opt-in to fanotify watches, i.e., remove FMODE_NONOTIFY. Because right now we have broken fanotify support for e.g., nsfs already. So make the subsystems think about whether they actually want to support it. I would disqualify all anonymous inodes and see what actually does break. I naively suspect that almost no one uses anonymous inodes + fanotify. I'd be very surprised. I'm currently traveling (see you later btw) but from a very cursory reading I would naively suspect the following: // Suspects for FMODE_NONOTIFY drivers/dma-buf/dma-buf.c: file = alloc_file_pseudo(inode, dma_buf_mnt, "dmabuf", drivers/misc/cxl/api.c: file = alloc_file_pseudo(inode, cxl_vfs_mount, name, drivers/scsi/cxlflash/ocxl_hw.c: file = alloc_file_pseudo(inode, ocxlflash_vfs_mount, name, fs/anon_inodes.c: file = alloc_file_pseudo(inode, anon_inode_mnt, name, fs/hugetlbfs/inode.c: file = alloc_file_pseudo(inode, mnt, name, O_RDWR, kernel/bpf/token.c: file = alloc_file_pseudo(inode, path.mnt, BPF_TOKEN_INODE_NAME, O_RDWR, &bpf_token_fops); mm/secretmem.c: file = alloc_file_pseudo(inode, secretmem_mnt, "secretmem", block/bdev.c: bdev_file = alloc_file_pseudo_noaccount(BD_INODE(bdev), drivers/tty/pty.c: static int ptmx_open(struct inode *inode, struct file *filp) // Suspects for ~FMODE_NONOTIFY fs/aio.c: file = alloc_file_pseudo(inode, aio_mnt, "[aio]", fs/pipe.c: f = alloc_file_pseudo(inode, pipe_mnt, "", mm/shmem.c: res = alloc_file_pseudo(inode, mnt, name, O_RDWR, // Unsure: fs/nfs/nfs4file.c: filep = alloc_file_pseudo(r_ino, ss_mnt, read_name, O_RDONLY, net/socket.c: file = alloc_file_pseudo(SOCK_INODE(sock), sock_mnt, dname, > > and the no-pre-content is a flag combination (to save FMODE_ flags) > which makes things a bit messy. > > We could try to initialize f_mode to FMODE_NONOTIFY_PERM > for anon_inode, which opts out of both permission and pre-content > events and leaves the legacy inotify workloads unaffected. > > But, then code like this will not do the right thing: > > /* We refuse fsnotify events on ptmx, since it's a shared resource */ > filp->f_mode |= FMODE_NONOTIFY; > > We will need to convert all those to use a helper. > I am traveling today so will be able to look closer tomorrow. > > Jan, > > What do you think? > > Amir.
