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

[Christian Brauner]

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.