Re: [PATCH] tracefs: avoid setting i_mode to a temp value

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 10 Aug 2023 20:59:26 -0400
Sishuai Gong <sishuai.system@gmail.com> wrote:

> Right now inode->i_mode is updated twice to reach the desired value
> in tracefs_apply_options(). Because there is no lock protecting the two
> writes, other threads might read the intermediate value of inode->i_mode.
> 
> Thread-1			Thread-2
> // tracefs_apply_options()	//e.g., acl_permission_check
> inode->i_mode &= ~S_IALLUGO;
> 				unsigned int mode = inode->i_mode;
> inode->i_mode |= opts->mode;
> 
> I think there is no need to introduce a lock but it is better to
> only update inode->i_mode ONCE, so the readers will either see the old
> or latest value, rather than an intermediate/temporary value.
> 
> Signed-off-by: Sishuai Gong <sishuai.system@gmail.com>
> ---
>  fs/tracefs/inode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index 57ac8aa4a724..dca84ebb62fa 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -297,8 +297,7 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
>  	 */
>  
>  	if (!remount || opts->opts & BIT(Opt_mode)) {
> -		inode->i_mode &= ~S_IALLUGO;
> -		inode->i_mode |= opts->mode;
> +		inode->i_mode = (inode->i_mode & ~S_IALLUGO) | opts->mode;

You do realize that the compiler could decide to keep the original logic
even with this change? If it was crucial that the compiler did not, you
would need to have:

	if (!remount || opts->opts & BIT(Opt_mode)) {
		umode_t tmp = READ_ONCE(inode->i_mode);

		tmp &= ~S_IALLUGO
		tmp |= opts->mode;
		WRITE_ONCE(inode->i_mode, tmp);
	}		

And if you notice the !remount flag, this is only preformed when the file
system is actually mounted. Are the files visible before then?

Can you produce this race?

-- Steve



>  	}
>  
>  	if (!remount || opts->opts & BIT(Opt_uid))