Re: [PATCH] efivarfs: Cache occupied and available space

[Anisse Astier <anisse@astier.eu>]

Hi Ard,

Please find a few comments below,

On Sat, Aug 01, 2026 at 04:42:59PM +0200, Ard Biesheuvel wrote:
> Ravi reports that statfs() may be called by unprivileged users on the
> efivarfs mount point, resulting in repeated calls to QueryVariableInfo()
> which are disproportionately costly on x86 systems where the variable
> store is backed by SMM, as each SMM entry requires a rendez-vous of all
> the CPUs.
> 
> So cache the output of QueryVariableInfo(), and invalidate the cached
> values when an entry is added or deleted, or the variable store is
> synchronized after a suspend/resume sequence.
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Ravi Bangoria <ravi.bangoria@amd.com>
> Fixes: d86ff3333cb1 ("efivarfs: expose used and total size")
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  fs/efivarfs/internal.h |  2 ++
>  fs/efivarfs/super.c    | 21 +++++++++++---------
>  fs/efivarfs/vars.c     |  9 +++++++++
>  3 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
> index f913b6824289..d43789bc7839 100644
> --- a/fs/efivarfs/internal.h
> +++ b/fs/efivarfs/internal.h
> @@ -31,6 +31,8 @@ struct efivar_entry {
>  	bool removed;
>  };
>  
> +extern u64 efivar_storage_space, efivar_remaining_space;
> +
>  static inline struct efivar_entry *efivar_entry(struct inode *inode)
>  {
>  	return container_of(inode, struct efivar_entry, vfs_inode);
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index 733c19571f1c..3691a2a087a0 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -23,6 +23,8 @@
>  #include "internal.h"
>  #include "../internal.h"
>  
> +u64 efivar_storage_space, efivar_remaining_space;
> +
Since those are globals (before they were only local to the function),
I'd put them behind a lock now, probably efivar_lock, because we need
something that's globally available.

statfs could be raced from userspace vs the rest of the code; and
efivar_invalidate_cached_storage_space is only called outside the lock.

>  static int efivarfs_ops_notifier(struct notifier_block *nb, unsigned long event,
>  				 void *data)
>  {
> @@ -82,15 +84,16 @@ static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	const u32 attr = EFI_VARIABLE_NON_VOLATILE |
>  			 EFI_VARIABLE_BOOTSERVICE_ACCESS |
>  			 EFI_VARIABLE_RUNTIME_ACCESS;
> -	u64 storage_space, remaining_space, max_variable_size;
>  	u64 id = huge_encode_dev(dentry->d_sb->s_dev);
> +	u64 max_variable_size;
>  	efi_status_t status;
>  
>  	/* Some UEFI firmware does not implement QueryVariableInfo() */
> -	storage_space = remaining_space = 0;
> -	if (efi_rt_services_supported(EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO)) {
> -		status = efivar_query_variable_info(attr, &storage_space,
> -						    &remaining_space,
> +	if (efivar_storage_space == 0 &&
> +	    efivar_remaining_space == 0 &&
> +	    efi_rt_services_supported(EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO)) {
> +		status = efivar_query_variable_info(attr, &efivar_storage_space,
> +						    &efivar_remaining_space,
>  						    &max_variable_size);
>  		if (status != EFI_SUCCESS && status != EFI_UNSUPPORTED)
>  			pr_warn_ratelimited("query_variable_info() failed: 0x%lx\n",

We've had issues with firmware in the past, shouldn't we be a bit more
defensive here in case of errors? I'd reset both variables on error in
case an incomplete write to one of the variables gives us an invalid state.

> @@ -104,8 +107,8 @@ static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	 */
>  	buf->f_bsize	= 1;
>  	buf->f_namelen	= NAME_MAX;
> -	buf->f_blocks	= storage_space;
> -	buf->f_bfree	= remaining_space;
> +	buf->f_blocks	= efivar_storage_space;
> +	buf->f_bfree	= efivar_remaining_space;
>  	buf->f_type	= dentry->d_sb->s_magic;
>  	buf->f_fsid	= u64_to_fsid(id);
>  
> @@ -114,8 +117,8 @@ static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	 * when the storage_paranoia x86 quirk is active. To use more, users
>  	 * should boot the kernel with efi_no_storage_paranoia.
>  	 */
> -	if (remaining_space > efivar_reserved_space())
> -		buf->f_bavail = remaining_space - efivar_reserved_space();
> +	if (efivar_remaining_space > efivar_reserved_space())
> +		buf->f_bavail = efivar_remaining_space - efivar_reserved_space();
>  	else
>  		buf->f_bavail = 0;
>  
> diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
> index 6833c3d24b54..4196963695bd 100644
> --- a/fs/efivarfs/vars.c
> +++ b/fs/efivarfs/vars.c
> @@ -361,6 +361,11 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
>  	kfree(str8);
>  }
>  
> +static void efivar_invalidate_cached_storage_space(void)
> +{
> +	efivar_storage_space = efivar_remaining_space = 0;
> +}
> +
>  /**
>   * efivar_init - build the initial list of EFI variables
>   * @func: callback function to invoke for every variable
> @@ -450,6 +455,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
>  	} while (status != EFI_NOT_FOUND);
>  
>  	efivar_unlock();
> +	efivar_invalidate_cached_storage_space();
>  free:
>  	kfree(variable_name);
>  
> @@ -480,6 +486,8 @@ int efivar_entry_delete(struct efivar_entry *entry)
>  					    &entry->var.VendorGuid,
>  					    0, 0, NULL, false);
>  	efivar_unlock();
> +	efivar_invalidate_cached_storage_space();
> +
>  	if (!(status == EFI_SUCCESS || status == EFI_NOT_FOUND))
>  		return efi_status_to_err(status);
>  
> @@ -620,6 +628,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
>  				    NULL, size, NULL);
>  
>  	efivar_unlock();
> +	efivar_invalidate_cached_storage_space();

In this function, the error path for efi_set_variable_locked (that could
have triggered an EFI_OUT_OF_RESOURCES) is not covered by the
invalidation.

>  
>  	if (status && status != EFI_BUFFER_TOO_SMALL)
>  		return efi_status_to_err(status);
> -- 

Couldn't the variables be written outside of efivarfs as well?
efivar_set_variable can be called from other code? pstore could maybe be
ignored if it's only used during crashes (I'm not sure), but I see at
least one other driver calling it as well. Wouldn't that change the
available/free size?

Regards,

Anisse