linux-kernel - Re: [PATCH] tmpfs: preserve SB_I

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b5e588cf-1408-4b67-b21f-04ff94314936@huawei.com>
Date: Fri, 22 Aug 2025 12:03:58 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Hugh Dickins <hughd@...gle.com>
CC: Jeff Layton <jlayton@...nel.org>, <libaokun@...weicloud.com>,
	<linux-mm@...ck.org>, <baolin.wang@...ux.alibaba.com>,
	<akpm@...ux-foundation.org>, <linux-kernel@...r.kernel.org>,
	<linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] tmpfs: preserve SB_I_VERSION on remount

On 2025-08-22 10:49, Hugh Dickins wrote:
> On Tue, 19 Aug 2025, Jeff Layton wrote:
>> On Tue, 2025-08-19 at 14:18 +0800, libaokun@...weicloud.com wrote:
>>> From: Baokun Li <libaokun1@...wei.com>
>>>
>>> Now tmpfs enables i_version by default and tmpfs does not modify it. But
>>> SB_I_VERSION can also be modified via sb_flags, and reconfigure_super()
>>> always overwrites the existing flags with the latest ones. This means that
>>> if tmpfs is remounted without specifying iversion, the default i_version
>>> will be unexpectedly disabled.
> Wow, what a surprise! Thank you so much for finding and fixing this.
>
>>> To ensure iversion remains enabled, SB_I_VERSION is now always set for
>>> fc->sb_flags in shmem_init_fs_context(), instead of for sb->s_flags in
>>> shmem_fill_super().
> I have to say that your patch looks to me like a hacky workaround. But 
> after spending ages trying to work out how this came about, have concluded
> that it's an artifact of "iversion" and/or "noiversion" being or having
> been a mount option in some filesystems, with MS_I_VERSION in MS_RMT_MASK
> getting propagated to sb_flags_mask, implying that the remounter is
> changing the option when they have no such intention. 
Exactly!
>  And any attempt
> to fix this in a better way would be too likely to cause more trouble
> than it's worth - unless other filesystems are also still surprised.
Other filesystems supporting i_version (ext4, xfs, btrfs) have encountered
similar issues. The solution adopted was either resetting SB_I_VERSION
during remount operations or setting SB_I_VERSION in init_fs_context().

Given that the overhead of iversion is now minimal, all supported
filesystems in the kernel enable it by default. I previously considered
converting SB_I_VERSION to FS_I_VERSION and setting it in
file_system_type->fs_flags, but since XFS only supports iversion in v5,
this idea was ultimately abandoned. Alternatively, removing MS_I_VERSION
from MS_RMT_MASK might also be a viable approach.
> I had to worry, does the same weird disappearance-on-remount happen to
> tmpfs's SB_POSIXACL too?  But it looks like not, because MS_POSIXACL is
> not in MS_RMT_MASK - a relic of history why one in but not the other.
Yes.
> But I've added linux-fsdevel to the Ccs, mainly as a protest at this
> unexpected interface (though no work for Christian to do: Andrew has
> already taken the patch, thanks).
Okay.
>
>>> Fixes: 36f05cab0a2c ("tmpfs: add support for an i_version counter")
>>> Signed-off-by: Baokun Li <libaokun1@...wei.com>
>>> ---
>>>  mm/shmem.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index e2c76a30802b..eebe12ff5bc6 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -5081,7 +5081,7 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
>>>  		sb->s_flags |= SB_NOUSER;
>>>  	}
>>>  	sb->s_export_op = &shmem_export_ops;
>>> -	sb->s_flags |= SB_NOSEC | SB_I_VERSION;
>>> +	sb->s_flags |= SB_NOSEC;
>>>  
>>>  #if IS_ENABLED(CONFIG_UNICODE)
>>>  	if (!ctx->encoding && ctx->strict_encoding) {
>>> @@ -5385,6 +5385,9 @@ int shmem_init_fs_context(struct fs_context *fc)
>>>  
>>>  	fc->fs_private = ctx;
>>>  	fc->ops = &shmem_fs_context_ops;
>>> +#ifdef CONFIG_TMPFS
> Ah, you're being very punctilious with that #ifdef: yes, the original
> code happened not to set it in the #ifndef CONFIG_TMPFS case (when the
> i_version would be invisible anyway).  But I bet that if we had done it
> this way originally, we would have preferred not to clutter the source
> with #ifdef and #else here.  Oh well, perhaps they will vanish in the
> night sometime, it's a nit not worth you resending.
Yes, I kept this macro to maintain consistency with shmem_fill_super().
If i_version is not supported but SB_I_VERSION is set, it may cause
confusion for IMA or NFS.
>
>>> +	fc->sb_flags |= SB_I_VERSION;
>>> +#endif
>>>  	return 0;
>>>  }
>>>  
>> Reviewed-by: Jeff Layton <jlayton@...nel.org>
> Acked-by: Hugh Dickins <hughd@...gle.com>
>
Thanks,
Baokun