Re: [PATCH v2 1/1] efivarfs: fix statfs() on efivarfs

[Ard Biesheuvel]

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);
+       }

        /*
         * This is not a normal filesystem, so no point in pretending
it has a block