| Message ID | 20230910045445.41632-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/1] efivarfs: fix statfs() on efivarfs | expand |
On Sun, 10 Sept 2023 at 06:53, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> 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@canonical.com> > --- > 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)
On Sun, 10 Sept 2023 at 15:08, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Sun, 10 Sept 2023 at 06:53, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> 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@canonical.com> > > --- > > 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) 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); - status = efivar_query_variable_info(attr, &storage_space, &remaining_space, - &max_variable_size); if (status != EFI_SUCCESS) return efi_status_to_err(status);
On 9/10/23 15:54, Ard Biesheuvel wrote: > On Sun, 10 Sept 2023 at 15:08, Ard Biesheuvel <ardb@kernel.org> wrote: >> >> On Sun, 10 Sept 2023 at 06:53, Heinrich Schuchardt >> <heinrich.schuchardt@canonical.com> 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@canonical.com> >>> --- >>> 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);
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. > > 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. Best regards Heinrich
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); + } /* * This is not a normal filesystem, so no point in pretending it has a block
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
On Mon, 11 Sept 2023 at 09:46, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > 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; > + } > The else branch is redundant if we leave the assignments of f_blocks and f_bfree where they were before.
Hi Ard, On Mon, 11 Sept 2023 at 09:45, Ard Biesheuvel <ardb@kernel.org> 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); > + } I think this is better, but shouldn't we initialize the status variable now? Or is there more code following that I am missing? Thanks /Ilias > > /* > * This is not a normal filesystem, so no point in pretending > it has a block
On Mon, 11 Sept 2023 at 10:04, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Ard, > > On Mon, 11 Sept 2023 at 09:45, Ard Biesheuvel <ardb@kernel.org> 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); > > + } > > 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.
On Mon, 11 Sept 2023 at 11:06, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Mon, 11 Sept 2023 at 10:04, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Ard, > > > > On Mon, 11 Sept 2023 at 09:45, Ard Biesheuvel <ardb@kernel.org> 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); > > > + } > > > > 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@linaro.org>
On 2023-09-10 00:54, Heinrich Schuchardt 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@canonical.com> > --- > v2: > initialize remaining_space to 0 > --- > 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; > 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; > + } > + } > + > /* > * In f_bavail we declare the free space that the kernel will allow > writing > * when the storage_paranoia x86 quirk is active. To use more, users FYI, this issue/fix affects ia64 EFI implementation as well, so adding a CC.
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; 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; + } + } + /* * In f_bavail we declare the free space that the kernel will allow writing * when the storage_paranoia x86 quirk is active. To use more, users
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@canonical.com> --- v2: initialize remaining_space to 0 --- fs/efivarfs/super.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)