Re: [PATCH] tmpfs: preserve SB_I_VERSION on remount

Re: [PATCH] tmpfs: preserve SB_I_VERSION on remount

[Hugh Dickins]

On Tue, 19 Aug 2025, Jeff Layton wrote:
> On Tue, 2025-08-19 at 14:18 +0800, libaokun@huaweicloud.com wrote:
> > From: Baokun Li <libaokun1@huawei.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.  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.

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.

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

> > 
> > Fixes: 36f05cab0a2c ("tmpfs: add support for an i_version counter")
> > Signed-off-by: Baokun Li <libaokun1@huawei.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.

> > +	fc->sb_flags |= SB_I_VERSION;
> > +#endif
> >  	return 0;
> >  }
> >  
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Acked-by: Hugh Dickins <hughd@google.com>

Re: [PATCH] tmpfs: preserve SB_I_VERSION on remount

[Baokun Li]

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@huaweicloud.com wrote:
>>> From: Baokun Li <libaokun1@huawei.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@huawei.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@kernel.org>
> Acked-by: Hugh Dickins <hughd@google.com>
>
Thanks,
Baokun