On Mon, 3 Feb 2025 21:39:27 +0100 Amir Goldstein <amir73il@xxxxxxxxx> wrote: > On Mon, Feb 3, 2025 at 1:41 PM Jan Kara <jack@xxxxxxx> wrote: > > > > On Sun 02-02-25 11:04:02, Christian Brauner wrote: > > > 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. > > > > Agreed, that would be a saner default. > > > > > 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]", > > > > This is just a helper file for managing aio context so I don't think any > > notification makes sense there (events are not well defined). So I'd say > > FMODE_NONOTIFY here as well. > > > > > fs/pipe.c: f = alloc_file_pseudo(inode, pipe_mnt, "", > > > mm/shmem.c: res = alloc_file_pseudo(inode, mnt, name, O_RDWR, > > > > This is actually used for stuff like IPC SEM where notification doesn't > > make sense. It's also used when mmapping /dev/zero but that struct file > > isn't easily accessible to userspace so overall I'd say this should be > > FMODE_NONOTIFY as well. > > I think there is another code path that the audit missed for getting these > pseudo files not via alloc_file_pseudo(): > ipc/shm.c: file = alloc_file_clone(base, f_flags, > > which does not copy f_mode as far as I can tell. > > > > > > // Unsure: > > > fs/nfs/nfs4file.c: filep = alloc_file_pseudo(r_ino, ss_mnt, read_name, O_RDONLY, > > > > AFAICS this struct file is for copy offload and doesn't leave the kernel. > > Hence FMODE_NONOTIFY should be fine. > > > > > net/socket.c: file = alloc_file_pseudo(SOCK_INODE(sock), sock_mnt, dname, > > > > In this case I think we need to be careful. It's a similar case as pipes so > > probably we should use ~FMODE_NONOTIFY here from pure caution. > > > > I tried this approach with patch: > "fsnotify: disable notification by default for all pseudo files" > > But I also added another patch: > "fsnotify: disable pre-content and permission events by default" > > So that code paths that we missed such as alloc_file_clone() > will not have pre-content events enabled. > > Alex, > > Can you please try this branch: > > https://github.com/amir73il/linux/commits/fsnotify-fixes/ > > and verify that it fixes your issue. > > The branch contains one prep patch: > "fsnotify: use accessor to set FMODE_NONOTIFY_*" > and two independent Fixes patches. > > Assuming that it fixes your issue, can you please test each of the > Fixes patches individually, because every one of them should be fixing > the issue independently and every one of them could break something, > so we may end up reverting it later on. Test #1: fsnotify: disable pre-content and permission events by default fsnotify: disable notification by default for all pseudo files fsnotify: use accessor to set FMODE_NONOTIFY_* Result: Pass, vfio-pci huge_fault observed Test #2: fsnotify: disable notification by default for all pseudo files fsnotify: use accessor to set FMODE_NONOTIFY_* Result: Pass, vfio-pci huge_fault observed Test #3: fsnotify: disable pre-content and permission events by default fsnotify: use accessor to set FMODE_NONOTIFY_* Result: Pass, vfio-pci huge_fault observed Test #4 (control): fsnotify: use accessor to set FMODE_NONOTIFY_* Result: Fail, no vfio-pci huge_fault observed For any combination of the Fixes patches: Tested-by: Alex Williamson <alex.williamson@xxxxxxxxxx> Thanks! Alex
