[PATCH] efivarfs: Cache occupied and available space

[PATCH] efivarfs: Cache occupied and available space

[Ard Biesheuvel]

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;
+
 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",
@@ -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();
 
 	if (status && status != EFI_BUFFER_TOO_SMALL)
 		return efi_status_to_err(status);
-- 
2.55.0.508.g3f0d502094-goog

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

[Ard Biesheuvel]

On Sat, 1 Aug 2026, at 17:42, 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(-)
>

Applied to efi/next

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

[Anisse Astier]

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

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

[Ard Biesheuvel]

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

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.

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

[Anisse Astier]

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