| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <mmfrclxjf2mmmohiwdbgqhyyrlab33tpnmtuzatk2xsuyiglrp@tarta.nabijaczleweli.xyz>
Date: Tue, 9 Dec 2025 15:00:35 +0100
From:
Ahelenia Ziemiańska <nabijaczleweli@...ijaczleweli.xyz>
To: Hugh Dickins <hughd@...gle.com>
Cc: Baolin Wang <baolin.wang@...ux.alibaba.com>,
Andrew Morton <akpm@...ux-foundation.org>, Matthew Wilcox <willy@...radead.org>,
Christian Brauner <brauner@...nel.org>, Theodore Ts'o <tytso@....edu>, linux-fsdevel@...r.kernel.org,
linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] tmpfs: enforce the immutable flag on open files
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@...ijaczleweli.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
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists