Re: [PATCH v2] tmpfs: enforce the immutable flag on open files

["Ahelenia Ziemiańska" <nabijaczleweli@nabijaczleweli.xyz>]

[-- Attachment #1: Type: text/plain, Size: 7111 bytes --]

On Mon, Dec 08, 2025 at 08:14:44PM -0800, Hugh Dickins wrote:
> On Mon, 8 Dec 2025, Ahelenia Ziemiańska wrote:
> > This useful behaviour is implemented for most filesystems,
> > and wants to be implemented for every filesystem, quoth ref:
> >   There is general agreement that we should standardize all file systems
> >   to prevent modifications even for files that were opened at the time
> >   the immutable flag is set.  Eventually, a change to enforce this at
> >   the VFS layer should be landing in mainline.
> > 
> > References: commit 02b016ca7f99 ("ext4: enforce the immutable flag on
> >  open files")
> > Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> Sorry: thanks, but no thanks.
> 
> Supporting page_mkwrite() comes at a cost (an additional fault on first
> write to a folio in a shared mmap).  It's important for space allocation
> (and more) in the case of persistent writeback filesystems, but unwelcome
> overhead in the case of tmpfs (and ramfs and hugetlbfs - others?).

Yeah, from the way page_mkwrite() was implemented it looked like
enough of a pessimisation to be significant, and with how common
an operation this is, I kinda expected this result.

(I was also gonna post the same for ramfs,
 but it doesn't support FS_IOC_SETFLAGS attributes at all.)

> tmpfs has always preferred not to support page_mkwrite(), and just fail
> fstests generic/080: we shall not slow down to change that, without a
> much stronger justification than "useful behaviour" which we've got
> along well enough without.

How do we feel about just the VFS half of this,
i.e. open(WR)/chattr +i/write() = -EPERM?
That shouldn't have a performance impact.

(I'll admit that this is the behaviour I find to be useful,
 and I was surprised that the ext4 implementation also made mappings
 SIGBUS, but I implemented both out of an undue sense of completionism.)

> But it is interesting that tmpfs supports IMMUTABLE, and passes all
> the chattr fstests, without this patch.  Perhaps you should be adding
> a new fstest, for tmpfs to fail: I won't thank you for that, but it
> would be a fair response!

I rather think having IMMUTABLE but not atomically perfusing it
to file descriptions is worthy of a test failure.
The mmap behaviour, not so much.

> Hugh
> 
> > ---
> > v1: https://lore.kernel.org/linux-fsdevel/znhu3eyffewvvhleewehuvod2wrf4tz6vxrouoakiarjtxt5uy@tarta.nabijaczleweli.xyz/t/#u
> > 
> > shmem_page_mkwrite()'s return 0; falls straight into do_page_mkwrite()'s
> > 	if (unlikely(!(ret & VM_FAULT_LOCKED))) {
> > 		folio_lock(folio);
> > Given the unlikely, is it better to folio_lock(folio); return VM_FAULT_LOCKED; instead?
> > 
> > /ext4# uname -a
> > Linux tarta 6.18.0-10912-g416f99c3b16f-dirty #1 SMP PREEMPT_DYNAMIC Sat Dec  6 12:14:41 CET 2025 x86_64 GNU/Linux
> > /ext4# while sleep 1; do echo $$; done > file &
> > [1] 262
> > /ext4# chattr +i file
> > /ext4# sh: line 25: echo: write error: Operation not permitted
> > sh: line 25: echo: write error: Operation not permitted
> > sh: line 25: echo: write error: Operation not permitted
> > sh: line 25: echo: write error: Operation not permitted
> > fg
> > while sleep 1; do
> >     echo $$;
> > done > file
> > ^C
> > /ext4# mount -t tmpfs tmpfs /tmp
> > /ext4# cd /tmp
> > /tmp# while sleep 1; do echo $$; done > file &
> > [1] 284
> > /tmp# chattr +i file
> > /tmp# sh: line 35: echo: write error: Operation not permitted
> > sh: line 35: echo: write error: Operation not permitted
> > sh: line 35: echo: write error: Operation not permitted
> > 
> > $ cat test.c
> > #include <unistd.h>
> > #include <fcntl.h>
> > #include <sys/ioctl.h>
> > #include <linux/fs.h>
> > #include <sys/mman.h>
> > int main(int, char **argv) {
> > 	int fd = open(argv[1], O_RDWR | O_CREAT | O_TRUNC, 0666);
> > 	ftruncate(fd, 1024 * 1024);
> > 	char *addr = mmap(NULL, 1024 * 1024, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> > 	addr[0] = 0x69;
> > 	int attrs = FS_IMMUTABLE_FL;
> > 	ioctl(3, FS_IOC_SETFLAGS, &attrs);
> > 	addr[1024 * 1024 - 1] = 0x69;
> > }
> > 
> > # strace ./test /tmp/file
> > execve("./test", ["./test", "/tmp/file"], 0x7ffc720bead8 /* 22 vars */) = 0
> > ...
> > openat(AT_FDCWD, "/tmp/file", O_RDWR|O_CREAT|O_TRUNC, 0666) = 3
> > ftruncate(3, 1048576)                   = 0
> > mmap(NULL, 1048576, PROT_READ|PROT_WRITE, MAP_SHARED, 3, 0) = 0x7f09bbf2a000
> > ioctl(3, FS_IOC_SETFLAGS, [FS_IMMUTABLE_FL]) = 0
> > --- SIGBUS {si_signo=SIGBUS, si_code=BUS_ADRERR, si_addr=0x7f09bc029fff} ---
> > +++ killed by SIGBUS +++
> > Bus error
> > # tr -d \\0 < /tmp/file; echo
> > i
> > 
> >  mm/shmem.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index d578d8e765d7..432935f79f35 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1294,6 +1294,14 @@ static int shmem_setattr(struct mnt_idmap *idmap,
> >  	bool update_mtime = false;
> >  	bool update_ctime = true;
> >  
> > +	if (unlikely(IS_IMMUTABLE(inode)))
> > +		return -EPERM;
> > +
> > +	if (unlikely(IS_APPEND(inode) &&
> > +		     (attr->ia_valid & (ATTR_MODE | ATTR_UID |
> > +					ATTR_GID | ATTR_TIMES_SET))))
> > +		return -EPERM;
> > +
> >  	error = setattr_prepare(idmap, dentry, attr);
> >  	if (error)
> >  		return error;
> > @@ -2763,6 +2771,17 @@ static vm_fault_t shmem_fault(struct vm_fault *vmf)
> >  	return ret;
> >  }
> >  
> > +static vm_fault_t shmem_page_mkwrite(struct vm_fault *vmf)
> > +{
> > +	struct file *file = vmf->vma->vm_file;
> > +
> > +	if (unlikely(IS_IMMUTABLE(file_inode(file))))
> > +		return VM_FAULT_SIGBUS;
> > +
> > +	file_update_time(file);
> > +	return 0;
> > +}
> > +
> >  unsigned long shmem_get_unmapped_area(struct file *file,
> >  				      unsigned long uaddr, unsigned long len,
> >  				      unsigned long pgoff, unsigned long flags)
> > @@ -3475,6 +3494,10 @@ static ssize_t shmem_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> >  	ret = generic_write_checks(iocb, from);
> >  	if (ret <= 0)
> >  		goto unlock;
> > +	if (unlikely(IS_IMMUTABLE(inode))) {
> > +		ret = -EPERM;
> > +		goto unlock;
> > +	}
> >  	ret = file_remove_privs(file);
> >  	if (ret)
> >  		goto unlock;
> > @@ -5286,6 +5309,7 @@ static const struct super_operations shmem_ops = {
> >  static const struct vm_operations_struct shmem_vm_ops = {
> >  	.fault		= shmem_fault,
> >  	.map_pages	= filemap_map_pages,
> > +	.page_mkwrite	= shmem_page_mkwrite,
> >  #ifdef CONFIG_NUMA
> >  	.set_policy     = shmem_set_policy,
> >  	.get_policy     = shmem_get_policy,
> > @@ -5295,6 +5319,7 @@ static const struct vm_operations_struct shmem_vm_ops = {
> >  static const struct vm_operations_struct shmem_anon_vm_ops = {
> >  	.fault		= shmem_fault,
> >  	.map_pages	= filemap_map_pages,
> > +	.page_mkwrite	= shmem_page_mkwrite,
> >  #ifdef CONFIG_NUMA
> >  	.set_policy     = shmem_set_policy,
> >  	.get_policy     = shmem_get_policy,
> > -- 
> > 2.39.5


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]