Messages in this thread | | | Date | Mon, 11 Sep 2023 09:47:43 +0200 | Subject | Re: [PATCH v2 1/1] efivarfs: fix statfs() on efivarfs | From | Heinrich Schuchardt <> |
| |
On 9/11/23 08:45, Ard Biesheuvel wrote: > On Sun, 10 Sept 2023 at 22:42, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> On 9/10/23 20:53, Anisse Astier wrote: >>> Hi Heinrich, >>> >>> On Sun, Sep 10, 2023 at 06:54:45AM +0200, Heinrich Schuchardt wrote: >>>> Some firmware (notably U-Boot) provides GetVariable() and >>>> GetNextVariableName() but not QueryVariableInfo(). >>> >>> From a quick search, it seems u-boot, does support QueryVariableInfo, is >>> it on a given version ? >>> >>> https://elixir.bootlin.com/u-boot/v2023.07.02/source/lib/efi_loader/efi_variable.c#L391 >> >> QueryVariableInfo() and SetVariable() are available before >> ExitBootServices(), i.e. in Linux' EFI stub. >> >> ExitBootServices() results in calling efi_variables_boot_exit_notify() >> which disables these services during the UEFI runtime. >> >>> >>>> >>>> With commit d86ff3333cb1 ("efivarfs: expose used and total size") the >>>> statfs syscall was broken for such firmware. >>> >>> Could you be more specific ? What breaks, and what regressed ? I imagine >>> it could be some scripts running df, but maybe you had something else in >>> mind ? >> >> Some more details can be found in >> https://bugs.launchpad.net/ubuntu/+source/linux-meta-riscv/+bug/2034705. >> >> Though EFI variables are exposed via GetVariable() and >> GetNextVariableName() the efivar command refuses to display variables >> when statfs() reports an error. >> >>> >>>> >>>> If QueryVariableInfo() does not exist or returns an error, just report the >>>> file-system size as 0 as statfs_simple() previously did. >>> >>> I considered doing this [2] , but we settled on returning an error >>> instead for clarity: >>> https://lore.kernel.org/linux-efi/20230515-vorgaben-portrait-bb1b4255d31a@brauner/ >>> >>> I still think it would be a good idea if necessary. >> >> We should never break user APIs. >> > > Indeed. > >>> >>> On the approach, I prefer what Ard proposed, to fall back to the old >>> approach. I think the difference in block size could also be a good >>> marker that something wrong is happening: >>> https://lore.kernel.org/linux-efi/CAMj1kXEkNSoqG4zWfCZ8Ytte5b2SzwXggZp21Xt17Pszd-q0dg@mail.gmail.com/ >> >> This will allow user code making assumptions based on block size: >> If block size > 1, assume setting variables is possible. >> >> We should really avoid this. >> > > I agree that having different block sizes depending on which code path > is taken is not great. But that is the situation we are already in, > given that older kernels will always report PAGE_SIZE. And actually, > PAGE_SIZE does not make sense either - PAGE_SIZE could be larger than > 4k on ARM for instance, so the efivarfs block size will be dependent > on the page size of the kernel you happened to boot. > > So I think we should go with the below: > > --- a/fs/efivarfs/super.c > +++ b/fs/efivarfs/super.c > @@ -32,10 +32,16 @@ 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) > + pr_warn_ratelimited("query_variable_info() > failed: 0x%lx\n", > + status);
Adding a warning here is helpful. The else branch would be:
+ } else { + buf->f_blocks = storage_space; + buf->f_bfree = remaining_space; + }
Best regards
Heinrich
> + } > > /* > * This is not a normal filesystem, so no point in pretending > it has a block
| |