Re: [PATCH v2 08/10] mm/shmem_luo: Allow preserving tmpfs files that are still linked

[David Matlack <dmatlack@google.com>]

On 2026-10-23 10:51 PM, sashiko-bot@kernel.org wrote:

> > +static int shmem_luo_may_preserve(struct file *file)
> > +{
> > +	struct mnt_idmap *idmap = file_mnt_idmap(file);
> > +	struct inode *inode = file_inode(file);
> > +	int err;
> > +
> > +	err = inode_permission(idmap, inode, MAY_WRITE);
> 
> [Severity: High]
> Should shmem_luo_may_preserve() in mm/shmem_luo.c also verify that the file
> is open for reading (file->f_mode & FMODE_READ) or that the caller has
> MAY_READ permission?

> Wouldn't that allow a caller with only write access to read the file's
> contents after retrieval?

Yes. I will fix this in v3 to require the file is open for reading and
update the selftest to cover this scenario.

> > +	if (err && (sb_rdonly(inode->i_sb) || IS_IMMUTABLE(inode)) &&
> > +	    inode_owner_or_capable(idmap, inode))
> > +		return 0;
> > +
> > +	return err;
> > +}
> 
> [Severity: Medium]
> What happens if shmem_luo_may_preserve() in mm/shmem_luo.c is called on an
> append-only (S_APPEND) tmpfs inode?
> 
> Tmpfs supports setting S_APPEND via FS_IOC_SETFLAGS in
> shmem_fileattr_set() -> shmem_set_inode_flags() in mm/shmem.c:
> 
>     if (fsflags & FS_APPEND_FL)
>         i_flags |= S_APPEND;
> 
> Unlike IS_IMMUTABLE(inode), inode_permission(idmap, inode, MAY_WRITE) does
> not reject S_APPEND inodes. In fs/verity/enable.c, fsverity_ioctl_enable()
> explicitly checks IS_APPEND(inode) after checking MAY_WRITE:
> 
>     if (IS_APPEND(inode))
>         return -EPERM;
> 
> When a caller with MAY_WRITE permission on an S_APPEND tmpfs file preserves
> it via LIVEUPDATE_SESSION_PRESERVE_FD -> luo_preserve_file() ->
> shmem_luo_preserve(), shmem_freeze(inode, true) sets SHMEM_F_MAPPING_FROZEN.
> Because all writes to an S_APPEND file extend past i_size,
> shmem_write_begin() and shmem_fallocate() in mm/shmem.c will then fail with
> -EPERM:
> 
>     if (unlikely((info->flags & SHMEM_F_MAPPING_FROZEN) &&
>                  pos + len > inode->i_size))
>         return -EPERM;
> 
> Should shmem_luo_may_preserve() also reject IS_APPEND(inode) with -EPERM so
> an append-only writer cannot freeze the file and block future appends by all
> users?

I am not sure on this one, but rejecting S_APPEND inodes for now does
seem like a reasonable place to start.

A writer of an append-only file can only add data. It can't destroy
existing data or stop others from adding more. preserve() blocks all
future appends, so arguably it could be abused.

I will update v3 to reject S_APPEND inodes. If someone has a use-case in
the future for preserving such files we can revisit this and look at how
to support them.