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

[Anisse Astier <anisse@astier.eu>]

Hi,

On Mon, Aug 17, 2026 at 03:11:40PM +0300, Ard Biesheuvel wrote:
> 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:
[snip]
> >> 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.

Yes, I don't think it's that performance sensitive (famous last words),
so putting it all behind a lock is a cheap way to avoid this type of
issue (unless you have a better idea?).

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

Yes and no. Before, a bad value might be returned once. Now, it might
get inadvertently cached and returned until the next variable
modification.

Yes, this is an edge case, but I think it can be worked around very
easily by either implementing the suggestion above, or simply writing
the values to other temporary variables, and updating the global ones on
success.

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

Yes, the actual free space might change but failed to be updated.

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

FYI, since this patch was initially merged, it helped us catch errors in
production *twice* thanks to existing monitoring on filesystem free
space: bad configurations or logic that wrote too many variables.

I can also see why this type of regular monitoring triggering SMM
rendez-vous of all CPUs might be bad for performance.

Regards,

Anisse