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 > 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 > > #include > > #include > > #include > > #include > > 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