Re: [PATCH v3] efivarfs: add nostatfs mount option to skip QueryVariableInfo()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] efivarfs: data race in efivarfs_show_options() on opts->nostatfs
--

commit 33ecb04473d9adc8fe8bcd4b68acea9d071545b7
Author: Prashant Singh <singhpra@juniper.net>

efivarfs: add nostatfs mount option to skip QueryVariableInfo()

QueryVariableInfo() is an EFI runtime service that, on some firmware,
takes tens of milliseconds and runs with preemption disabled, stalling
the CPU that services it. efivarfs_statfs() calls it (rate-limited since
commit b2326338dc68 ("efivarfs: Rate limit statfs() handler")) to report
the variable-store used/available capacity, so any statfs(2) -- e.g.
every "df" -- can inject that stall into unrelated latency-sensitive
workloads on the same CPU.

Add a negatable "nostatfs" mount option: with nostatfs, statfs(2) skips
QueryVariableInfo() and reports zero used/available; with statfs it
reports the capacity as before.

> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index 8d33f11db2a1b..d2e1f31a72d1e 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -74,6 +74,15 @@ static int efivarfs_show_options(struct seq_file *m, struct dentry *root)
>  	if (!gid_eq(opts->gid, GLOBAL_ROOT_GID))
>  		seq_printf(m, ",gid=%u",
>  				from_kgid_munged(&init_user_ns, opts->gid));
> +	/*
> +	 * Always show nostatfs (it makes statfs() report zero capacity, which
> +	 * is otherwise surprising); only show statfs where it is not the
> +	 * default, i.e. on PREEMPT_RT.
> +	 */
> +	if (opts->nostatfs)

[Severity: Low]
Does this code risk a data race and potential KCSAN warning?

Since opts->nostatfs can be updated concurrently by efivarfs_reconfigure()
using WRITE_ONCE(), reading it here without READ_ONCE() could result in
undefined behavior.

> +		seq_puts(m, ",nostatfs");
> +	else if (IS_ENABLED(CONFIG_PREEMPT_RT))
> +		seq_puts(m, ",statfs");
>  	return 0;
>  }

[ ... ]

> @@ -402,11 +421,17 @@ static int efivarfs_get_tree(struct fs_context *fc)
>  
>  static int efivarfs_reconfigure(struct fs_context *fc)
>  {
> +	struct efivarfs_fs_info *sfi = fc->root->d_sb->s_fs_info;
> +	struct efivarfs_fs_info *new_sfi = fc->s_fs_info;
> +
>  	if (!efivar_supports_writes() && !(fc->sb_flags & SB_RDONLY)) {
>  		pr_err("Firmware does not support SetVariableRT. Can not remount with rw\n");
>  		return -EINVAL;
>  	}
>  
> +	/* statfs() reads nostatfs locklessly (no s_umount); pair with WRITE_ONCE. */
> +	WRITE_ONCE(sfi->mount_opts.nostatfs, new_sfi->mount_opts.nostatfs);

[Severity: Medium]
This is a pre-existing issue, but does this function silently discard newly
parsed uid and gid mount options during a remount?

It appears that efivarfs_reconfigure() only copies the nostatfs option into
the active superblock context, ignoring any changes to the uid and gid
options that were parsed into new_sfi.

> +
>  	return 0;
>  }
>  
> @@ -524,8 +549,19 @@ static int efivarfs_init_fs_context(struct fs_context *fc)
>  	if (!sfi)
>  		return -ENOMEM;
>  
> -	sfi->mount_opts.uid = GLOBAL_ROOT_UID;
> -	sfi->mount_opts.gid = GLOBAL_ROOT_GID;
> +	if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
> +		/* Preserve options not respecified on remount. */
> +		struct efivarfs_fs_info *old = fc->root->d_sb->s_fs_info;
> +
> +		sfi->mount_opts.uid = old->mount_opts.uid;
> +		sfi->mount_opts.gid = old->mount_opts.gid;

[Severity: Medium]
This is a pre-existing issue, but is this intended to carry over the uid and
gid options so they can be modified by the parser?

Because efivarfs_reconfigure() never applies the parsed uid and gid back to
the live superblock, the preservation here acts as dead code. Administrators
who run "mount -o remount,uid=1000" will find their requested ownership ignored
when creating new variables.

> +		sfi->mount_opts.nostatfs = READ_ONCE(old->mount_opts.nostatfs);
> +	} else {
> +		sfi->mount_opts.uid = GLOBAL_ROOT_UID;
> +		sfi->mount_opts.gid = GLOBAL_ROOT_GID;
> +		/* QueryVariableInfo() stalls the CPU; default nostatfs on PREEMPT_RT. */
> +		sfi->mount_opts.nostatfs = IS_ENABLED(CONFIG_PREEMPT_RT);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925113145.7396-1-singhpra@juniper.net?part=1