Re: [PATCH 0/9] fs: harden anon inodes

[Christian Brauner <brauner@kernel.org>]

On Mon, Apr 07, 2025 at 12:19:45PM +0200, Mateusz Guzik wrote:
> On Mon, Apr 7, 2025 at 11:54 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > * Anonymous inodes currently don't come with a proper mode causing
> >   issues in the kernel when we want to add useful VFS debug assert. Fix
> >   that by giving them a proper mode and masking it off when we report it
> >   to userspace which relies on them not having any mode.
> >
> > * Anonymous inodes currently allow to change inode attributes because
> >   the VFS falls back to simple_setattr() if i_op->setattr isn't
> >   implemented. This means the ownership and mode for every single user
> >   of anon_inode_inode can be changed. Block that as it's either useless
> >   or actively harmful. If specific ownership is needed the respective
> >   subsystem should allocate anonymous inodes from their own private
> >   superblock.
> >
> > * Port pidfs to the new anon_inode_{g,s}etattr() helpers.
> >
> > * Add proper tests for anonymous inode behavior.
> >
> > The anonymous inode specific fixes should ideally be backported to all
> > LTS kernels.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> > Christian Brauner (9):
> >       anon_inode: use a proper mode internally
> >       pidfs: use anon_inode_getattr()
> >       anon_inode: explicitly block ->setattr()
> >       pidfs: use anon_inode_setattr()
> >       anon_inode: raise SB_I_NODEV and SB_I_NOEXEC
> >       selftests/filesystems: add first test for anonymous inodes
> >       selftests/filesystems: add second test for anonymous inodes
> >       selftests/filesystems: add third test for anonymous inodes
> >       selftests/filesystems: add fourth test for anonymous inodes
> >
> 
> I have two nits, past that LGTM
> 
> 1. I would add a comment explaining why S_IFREG in alloc_anon_inode()

        /*
         * Historically anonymous inodes didn't have a type at all and
         * userspace has come to rely on this. Internally they're just
         * regular files but S_IFREG is masked off when reporting
         * information to userspace.
         */

> 2. commit messages for selftests could spell out what's being added
> instead of being counted, it's all one-liners

I've replaced the count with chown(), chmod(), exec(), and open().

Thanks!