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

[Heinrich Schuchardt]

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