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

Hi,

Thanks for the review.

On Tue, 4 Aug 2026, at 19:01, Anisse Astier wrote:
> 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: <[email protected]>
>> Reported-by: Ravi Bangoria <[email protected]>
>> Fixes: d86ff3333cb1 ("efivarfs: expose used and total size")
>> Signed-off-by: Ard Biesheuvel <[email protected]>
>> ---
>>  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.
>

Right. So we might inadvertently report zero size and zero available
space even though QueryVariableInfo() is supported and working correctly.
Not the end of the world imo, but better avoided if we can.


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

This is a pre-existing issue, right? The only difference being the fact that
the variables are now globals?

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

Right. So under the assumption that the state of the EFI variable store
might change even after a failed SetVariable(), the cached values may
have become inaccurate. Is that what you are saying?

>>  
>>  	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?
>

Yeah, that is a very good point.