Re: [REGRESSION] Re: [PATCH v8 15/19] mm: don't allow huge faults for files with pre content watches

Amir Goldstein
2025-02-03T22:05:06Z

On Mon, Feb 3, 2025 at 10:41 PM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Mon, 3 Feb 2025 21:39:27 +0100
> Amir Goldstein <amir73il@gmail.com> wrote:
>
> > On Mon, Feb 3, 2025 at 1:41 PM Jan Kara <jack@suse.cz> 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@linux-foundation.org> wrote:
> > > > > >
> > > > > > On Sat, 1 Feb 2025 at 06:38, Christian Brauner <brauner@kernel.org> 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@redhat.com>
>

That was fast.
I will post the patches.

Thanks!
Amir.