On Mon, 11 Sept 2023 at 11:06, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > On Mon, 11 Sept 2023 at 10:04, Ilias Apalodimas > <ilias.apalodimas@xxxxxxxxxx> wrote: > > > > Hi Ard, > > > > On Mon, 11 Sept 2023 at 09:45, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > > > > > On Sun, 10 Sept 2023 at 22:42, Heinrich Schuchardt > > > <heinrich.schuchardt@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx/ > > > > > > > > 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); > > > + } > > > > I think this is better, but shouldn't we initialize the status > > variable now? Or is there more code following that I am missing? > > > > status is not referenced again after this. Ah fair enough. I'd still initialize it for sanity, but with or without Reviewed-by: Ilias Apalodimas <ilias.apalodimas@xxxxxxxxxx>
