On 9/10/23 15:54, Ard Biesheuvel wrote:
On Sun, 10 Sept 2023 at 15:08, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
On Sun, 10 Sept 2023 at 06:53, Heinrich Schuchardt
<heinrich.schuchardt@xxxxxxxxxxxxx> wrote:
Some firmware (notably U-Boot) provides GetVariable() and
GetNextVariableName() but not QueryVariableInfo().
With commit d86ff3333cb1 ("efivarfs: expose used and total size") the
statfs syscall was broken for such firmware.
If QueryVariableInfo() does not exist or returns an error, just report the
file-system size as 0 as statfs_simple() previously did.
Fixes: d86ff3333cb1 ("efivarfs: expose used and total size")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@xxxxxxxxxxxxx>
---
v2:
initialize remaining_space to 0
Thanks for the patch, and apologies for the oversight.
---
fs/efivarfs/super.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index e028fafa04f3..3893aae6a9be 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -29,14 +29,9 @@ 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 storage_space, remaining_space = 0, max_variable_size;
Shouldn't storage_space be set to 0 too?
efi_status_t status;
- status = efivar_query_variable_info(attr, &storage_space, &remaining_space,
- &max_variable_size);
- if (status != EFI_SUCCESS)
- return efi_status_to_err(status);
-
/*
* This is not a normal filesystem, so no point in pretending it has a block
* size; we declare f_bsize to 1, so that we can then report the exact value
@@ -44,10 +39,19 @@ 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_type = dentry->d_sb->s_magic;
+ /* Some UEFI firmware does not implement QueryVariable() */
+ if (efi_rt_services_supported(EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO)) {
+ status = efivar_query_variable_info(attr, &storage_space,
+ &remaining_space,
+ &max_variable_size);
+ if (status == EFI_SUCCESS) {
+ buf->f_blocks = storage_space;
+ buf->f_bfree = remaining_space;
+ }
+ }
+
I'd prefer not to ignore the error completely here. How about we do
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -32,10 +32,15 @@ static int efivarfs_statfs(struct dentry *dentry,
struct kstatfs *buf)
u64 storage_space, remaining_space, max_variable_size;
efi_status_t status;
- status = efivar_query_variable_info(attr, &storage_space,
&remaining_space,
- &max_variable_size);
- if (status != EFI_SUCCESS)
- return efi_status_to_err(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,
+ &max_variable_size);
+ if (status != EFI_SUCCESS && status != EFI_UNSUPPORTED)
+ return efi_status_to_err(status);
+ }
/*
* This is not a normal filesystem, so no point in pretending
it has a block
(no need to resend if you agree)
Hello Ard,
thanks for reviewing.
Raising an error in such a rare circumstance would be ok for me.
Actually, perhaps it would be better to simply fall back to the old
logic if QueryVariableInfo is not provided:
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index e028fafa04f3..50b05f1fa974 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -30,10 +30,15 @@ static int efivarfs_statfs(struct dentry *dentry,
struct kstatfs *buf)
EFI_VARIABLE_BOOTSERVICE_ACCESS |
EFI_VARIABLE_RUNTIME_ACCESS;
u64 storage_space, remaining_space, max_variable_size;
- efi_status_t status;
+ efi_status_t status = EFI_UNSUPPORTED;
+
+ if (efi_rt_services_supported(EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO))
+ status = efivar_query_variable_info(attr, &storage_space,
+ &remaining_space,
+ &max_variable_size);
+ if (status == EFI_UNSUPPORTED)
+ return simple_statfs(dentry, buf);
I would not know why the block size (buf->f_bsize) should depend on the
availability of QueryVariableInfo() (1 vs PAGE_SIZE).
Hence I would prefer your suggestion to just add the error handling
suggested in your previous mail.
Best regards
Heinrich
- status = efivar_query_variable_info(attr, &storage_space,
&remaining_space,
- &max_variable_size);
if (status != EFI_SUCCESS)
return efi_status_to_err(status);
