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

[Alex Williamson <alex.williamson@redhat.com>]

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>

Thanks!
Alex