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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <l5apiabdjosyy4gfuenr4oqdfio3zdiajzxoekdgtsohzpn3mj@dcmvayncbye4>
Date: Mon, 3 Feb 2025 13:41:45 +0100
From: Jan Kara <jack@...e.cz>
To: Christian Brauner <brauner@...nel.org>
Cc: Amir Goldstein <amir73il@...il.com>, 
	Linus Torvalds <torvalds@...ux-foundation.org>, Peter Xu <peterx@...hat.com>, 
	Alex Williamson <alex.williamson@...hat.com>, Josef Bacik <josef@...icpanda.com>, kernel-team@...com, 
	linux-fsdevel@...r.kernel.org, jack@...e.cz, viro@...iv.linux.org.uk, linux-xfs@...r.kernel.org, 
	linux-btrfs@...r.kernel.org, linux-mm@...ck.org, linux-ext4@...r.kernel.org, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: Re: [REGRESSION] Re: [PATCH v8 15/19] mm: don't allow huge faults
 for files with pre content watches

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@...ux-foundation.org> wrote:
> > >
> > > On Sat, 1 Feb 2025 at 06:38, Christian Brauner <brauner@...nel.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.

> // 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.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR