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

[Hugh Dickins <hughd@google.com>]

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

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?).

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.

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!

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