Re: [PATCH RFC 07/28] exfat: keep the current mount options on remount

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] exfat: data race and torn struct read during remount options copy
--

commit 2d6a9e734992913f68b1b7acddc9bc5d1eb92428
Author: Christian Brauner <brauner@kernel.org>

exfat: keep the current mount options on remount

This commit modifies exfat_init_fs_context() to initialize the filesystem
context with the current superblock mount options during a reconfiguration.
It resolves an issue where remounting without options, such as an emergency
remount, would inadvertently reset mount options like errors=continue to
defaults.

> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index a9ea36ba26938..71a348d10c26a 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
[ ... ]
> @@ -805,6 +807,11 @@ static int exfat_reconfigure(struct fs_context *fc)
>  	}
>  
>  	swap(*cur_opts, *new_opts);

[Severity: Medium]
Could this swap operation race with a concurrent read?

In exfat_init_fs_context(), the mount options struct is read locklessly
before the s_umount lock is acquired. Concurrently modifying the same
struct here via swap(*cur_opts, *new_opts) allows the compiler to use
non-atomic memory operations, which might lead to a torn read.

> +	/*
> +	 * The iocharset can't change, keep the current string. The new
> +	 * options have none if the remount didn't specify one.
> +	 */
> +	swap(cur_opts->iocharset, new_opts->iocharset);
[ ... ]
> @@ -831,23 +838,27 @@ static int exfat_init_fs_context(struct fs_context *fc)
>  
>  	if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE && fc->root) {
>  		struct super_block *sb = fc->root->d_sb;
> -		struct exfat_mount_options *cur_opts = &EXFAT_SB(sb)->options;
>  
> -		sbi->options.fs_uid = cur_opts->fs_uid;
> -		sbi->options.fs_gid = cur_opts->fs_gid;
> -		sbi->options.fs_fmask = cur_opts->fs_fmask;
> -		sbi->options.fs_dmask = cur_opts->fs_dmask;
> +		/*
> +		 * Start from the current options so a remount only changes
> +		 * what it specifies. exfat_reconfigure() refuses to change
> +		 * most of them, so a remount that doesn't repeat them all
> +		 * would fail, e.g. emergency remount which passes none. The
> +		 * iocharset string stays with the superblock and is only
> +		 * compared if the remount names one, see exfat_reconfigure().
> +		 */
> +		sbi->options = EXFAT_SB(sb)->options;

[Severity: Medium]
Can this struct assignment race with a concurrent remount?

When a user process calls mount -o remount or fsconfig, exfat_init_fs_context()
is invoked and locklessly reads EXFAT_SB(sb)->options. Concurrently, another
thread executing a remount might be modifying this same struct via
swap(*cur_opts, *new_opts) in exfat_reconfigure().

Since exfat_init_fs_context() executes before s_umount is acquired, this
concurrent access allows the compiler to use non-atomic memory operations.
Could this result in a torn read of the 32-bit word containing the bitfields
such as utf8, sys_tz, discard, or keep_last_dots, yielding an unintended
mixture of old and new options during the remount?

> +		sbi->options.iocharset = NULL;
>  	} else {
>  		sbi->options.fs_uid = current_uid();
>  		sbi->options.fs_gid = current_gid();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-work-fs-sb_rdonly-v1-0-60743a3dc326@kernel.org?part=7