From: Anisse Astier <[email protected]>
When writing variables, one might get errors with no other message on
why it fails.
Being able to see how much is used by EFI variables helps analyzing such
issues.
Since this is not a conventionnal filesystem, block size is
intentionnally set to 1 instead of PAGE_SIZE.
x86 quirks of reserved size are taken into account and available and
free size can be different, further helping debugging space issues.
Signed-off-by: Anisse Astier <[email protected]>
---
Notes:
Patch isn't split per subsystem intentionally, for better understanding
of intent; split could be trivial in a later version.
I'm not sure whether statfs(2) should return an error if the efi request
fails; I think it could be ignored with maybe a WARN_ONCE; which would
be close to the current behaviour.
Regards,
Anisse
---
arch/x86/platform/efi/quirks.c | 8 ++++++++
drivers/firmware/efi/efi.c | 1 +
drivers/firmware/efi/vars.c | 12 ++++++++++++
fs/efivarfs/super.c | 26 +++++++++++++++++++++++++-
include/linux/efi.h | 10 ++++++++++
5 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index b0b848d6933a..587fa51230e2 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -114,6 +114,14 @@ void efi_delete_dummy_variable(void)
EFI_VARIABLE_RUNTIME_ACCESS, 0, NULL);
}
+u64 efi_reserved_space(void)
+{
+ if (efi_no_storage_paranoia)
+ return 0;
+ return EFI_MIN_RESERVE;
+}
+EXPORT_SYMBOL_GPL(efi_reserved_space);
+
/*
* In the nonblocking case we do not attempt to perform garbage
* collection if we do not have enough free space. Rather, we do the
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index abeff7dc0b58..d0dfa007bffc 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -211,6 +211,7 @@ static int generic_ops_register(void)
generic_ops.get_variable = efi.get_variable;
generic_ops.get_next_variable = efi.get_next_variable;
generic_ops.query_variable_store = efi_query_variable_store;
+ generic_ops.query_variable_info = efi.query_variable_info;
if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) {
generic_ops.set_variable = efi.set_variable;
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index bd75b87f5fc1..c5382d5c3073 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -245,3 +245,15 @@ efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor,
return status;
}
EXPORT_SYMBOL_NS_GPL(efivar_set_variable, EFIVAR);
+
+efi_status_t efivar_query_variable_info(u32 attr,
+ u64 *storage_space,
+ u64 *remaining_space,
+ u64 *max_variable_size)
+{
+ if (!__efivars->ops->query_variable_info)
+ return EFI_UNSUPPORTED;
+ return __efivars->ops->query_variable_info(attr, storage_space,
+ remaining_space, max_variable_size);
+}
+EXPORT_SYMBOL_NS_GPL(efivar_query_variable_info, EFIVAR);
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 482d612b716b..064bfc0243c9 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -13,6 +13,7 @@
#include <linux/ucs2_string.h>
#include <linux/slab.h>
#include <linux/magic.h>
+#include <linux/statfs.h>
#include "internal.h"
@@ -23,8 +24,31 @@ static void efivarfs_evict_inode(struct inode *inode)
clear_inode(inode);
}
+static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf)
+{
+ u64 storage_space, remaining_space, max_variable_size;
+ efi_status_t status;
+ const u32 attr = (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS);
+
+ buf->f_type = dentry->d_sb->s_magic;
+ buf->f_bsize = 1;
+ buf->f_namelen = NAME_MAX;
+
+ status = efivar_query_variable_info(attr, &storage_space, &remaining_space,
+ &max_variable_size);
+ if (status != EFI_SUCCESS)
+ return efi_status_to_err(status);
+ buf->f_blocks = storage_space;
+ buf->f_bfree = remaining_space;
+ if (remaining_space > efi_reserved_space())
+ buf->f_bavail = remaining_space - efi_reserved_space();
+ else
+ buf->f_bavail = 0;
+ return 0;
+}
static const struct super_operations efivarfs_ops = {
- .statfs = simple_statfs,
+ .statfs = efivarfs_statfs,
.drop_inode = generic_delete_inode,
.evict_inode = efivarfs_evict_inode,
};
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7aa62c92185f..d2b686191870 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -703,6 +703,7 @@ static inline void efi_enter_virtual_mode (void) {}
extern efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size,
bool nonblocking);
+extern u64 efi_reserved_space(void);
#else
static inline efi_status_t efi_query_variable_store(u32 attributes,
@@ -711,6 +712,10 @@ static inline efi_status_t efi_query_variable_store(u32 attributes,
{
return EFI_SUCCESS;
}
+static inline u64 efi_reserved_space(void)
+{
+ return 0;
+}
#endif
extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
@@ -1042,6 +1047,7 @@ struct efivar_operations {
efi_set_variable_t *set_variable;
efi_set_variable_t *set_variable_nonblocking;
efi_query_variable_store_t *query_variable_store;
+ efi_query_variable_info_t *query_variable_info;
};
struct efivars {
@@ -1087,6 +1093,10 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor,
u32 attr, unsigned long data_size, void *data);
+efi_status_t efivar_query_variable_info(u32 attr, u64 *storage_space,
+ u64 *remaining_space,
+ u64 *max_variable_size);
+
#if IS_ENABLED(CONFIG_EFI_CAPSULE_LOADER)
extern bool efi_capsule_pending(int *reset_type);
--
2.34.1
[Public]
> -----Original Message-----
> From: Anisse Astier <[email protected]>
> Sent: Wednesday, April 26, 2023 14:59
> To: [email protected]
> Cc: Ard Biesheuvel <[email protected]>; Johan Hovold
> <[email protected]>; Darren Hart <[email protected]>; Andy
> Shevchenko <[email protected]>; Thomas Gleixner <[email protected]>;
> Ingo Molnar <[email protected]>; Borislav Petkov <[email protected]>; Dave
> Hansen <[email protected]>; [email protected]; H. Peter Anvin
> <[email protected]>; Jeremy Kerr <[email protected]>; platform-driver-
> [email protected]; [email protected]; Anisse Astier
> <[email protected]>; Anisse Astier <[email protected]>
> Subject: [PATCH] efivarfs: expose used and total size
>
> From: Anisse Astier <[email protected]>
>
> When writing variables, one might get errors with no other message on
> why it fails.
>
> Being able to see how much is used by EFI variables helps analyzing such
> issues.
>
> Since this is not a conventionnal filesystem, block size is
> intentionnally set to 1 instead of PAGE_SIZE.
>
> x86 quirks of reserved size are taken into account and available and
> free size can be different, further helping debugging space issues.
+hughsie
This can be quite useful for fwupd to be capturing as well with some of the
weird issues that look like they're probably related to how much variable
space is left.
>
> Signed-off-by: Anisse Astier <[email protected]>
> ---
> Notes:
> Patch isn't split per subsystem intentionally, for better understanding
> of intent; split could be trivial in a later version.
>
> I'm not sure whether statfs(2) should return an error if the efi request
> fails; I think it could be ignored with maybe a WARN_ONCE; which would
> be close to the current behaviour.
>
> Regards,
>
> Anisse
>
> ---
> arch/x86/platform/efi/quirks.c | 8 ++++++++
> drivers/firmware/efi/efi.c | 1 +
> drivers/firmware/efi/vars.c | 12 ++++++++++++
> fs/efivarfs/super.c | 26 +++++++++++++++++++++++++-
> include/linux/efi.h | 10 ++++++++++
> 5 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index b0b848d6933a..587fa51230e2 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -114,6 +114,14 @@ void efi_delete_dummy_variable(void)
> EFI_VARIABLE_RUNTIME_ACCESS, 0,
> NULL);
> }
>
> +u64 efi_reserved_space(void)
> +{
> + if (efi_no_storage_paranoia)
> + return 0;
> + return EFI_MIN_RESERVE;
> +}
> +EXPORT_SYMBOL_GPL(efi_reserved_space);
> +
> /*
> * In the nonblocking case we do not attempt to perform garbage
> * collection if we do not have enough free space. Rather, we do the
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index abeff7dc0b58..d0dfa007bffc 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -211,6 +211,7 @@ static int generic_ops_register(void)
> generic_ops.get_variable = efi.get_variable;
> generic_ops.get_next_variable = efi.get_next_variable;
> generic_ops.query_variable_store = efi_query_variable_store;
> + generic_ops.query_variable_info = efi.query_variable_info;
>
> if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) {
> generic_ops.set_variable = efi.set_variable;
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index bd75b87f5fc1..c5382d5c3073 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -245,3 +245,15 @@ efi_status_t efivar_set_variable(efi_char16_t
> *name, efi_guid_t *vendor,
> return status;
> }
> EXPORT_SYMBOL_NS_GPL(efivar_set_variable, EFIVAR);
> +
> +efi_status_t efivar_query_variable_info(u32 attr,
> + u64 *storage_space,
> + u64 *remaining_space,
> + u64 *max_variable_size)
> +{
> + if (!__efivars->ops->query_variable_info)
> + return EFI_UNSUPPORTED;
> + return __efivars->ops->query_variable_info(attr, storage_space,
> + remaining_space, max_variable_size);
> +}
> +EXPORT_SYMBOL_NS_GPL(efivar_query_variable_info, EFIVAR);
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index 482d612b716b..064bfc0243c9 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -13,6 +13,7 @@
> #include <linux/ucs2_string.h>
> #include <linux/slab.h>
> #include <linux/magic.h>
> +#include <linux/statfs.h>
>
> #include "internal.h"
>
> @@ -23,8 +24,31 @@ static void efivarfs_evict_inode(struct inode *inode)
> clear_inode(inode);
> }
>
> +static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf)
> +{
> + u64 storage_space, remaining_space, max_variable_size;
> + efi_status_t status;
> + const u32 attr = (EFI_VARIABLE_NON_VOLATILE |
> EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS);
> +
> + buf->f_type = dentry->d_sb->s_magic;
> + buf->f_bsize = 1;
> + buf->f_namelen = NAME_MAX;
> +
> + status = efivar_query_variable_info(attr, &storage_space,
> &remaining_space,
> + &max_variable_size);
> + if (status != EFI_SUCCESS)
> + return efi_status_to_err(status);
> + buf->f_blocks = storage_space;
> + buf->f_bfree = remaining_space;
> + if (remaining_space > efi_reserved_space())
> + buf->f_bavail = remaining_space - efi_reserved_space();
> + else
> + buf->f_bavail = 0;
> + return 0;
> +}
> static const struct super_operations efivarfs_ops = {
> - .statfs = simple_statfs,
> + .statfs = efivarfs_statfs,
> .drop_inode = generic_delete_inode,
> .evict_inode = efivarfs_evict_inode,
> };
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 7aa62c92185f..d2b686191870 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -703,6 +703,7 @@ static inline void efi_enter_virtual_mode (void) {}
> extern efi_status_t efi_query_variable_store(u32 attributes,
> unsigned long size,
> bool nonblocking);
> +extern u64 efi_reserved_space(void);
> #else
>
> static inline efi_status_t efi_query_variable_store(u32 attributes,
> @@ -711,6 +712,10 @@ static inline efi_status_t
> efi_query_variable_store(u32 attributes,
> {
> return EFI_SUCCESS;
> }
> +static inline u64 efi_reserved_space(void)
> +{
> + return 0;
> +}
> #endif
> extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
>
> @@ -1042,6 +1047,7 @@ struct efivar_operations {
> efi_set_variable_t *set_variable;
> efi_set_variable_t *set_variable_nonblocking;
> efi_query_variable_store_t *query_variable_store;
> + efi_query_variable_info_t *query_variable_info;
> };
>
> struct efivars {
> @@ -1087,6 +1093,10 @@ efi_status_t
> efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
> efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> u32 attr, unsigned long data_size, void
> *data);
>
> +efi_status_t efivar_query_variable_info(u32 attr, u64 *storage_space,
> + u64 *remaining_space,
> + u64 *max_variable_size);
> +
> #if IS_ENABLED(CONFIG_EFI_CAPSULE_LOADER)
> extern bool efi_capsule_pending(int *reset_type);
>
> --
> 2.34.1
On Wed, 26 Apr 2023 at 22:50, Limonciello, Mario
<[email protected]> wrote:
> This can be quite useful for fwupd to be capturing as well with some of the
> weird issues that look like they're probably related to how much variable
> space is left.
Yes, agreed, this will be very useful indeed. Thanks!
Richard
On Wed, 26 Apr 2023 at 21:59, Anisse Astier <[email protected]> wrote:
>
> From: Anisse Astier <[email protected]>
>
> When writing variables, one might get errors with no other message on
> why it fails.
>
> Being able to see how much is used by EFI variables helps analyzing such
> issues.
>
> Since this is not a conventionnal filesystem, block size is
> intentionnally set to 1 instead of PAGE_SIZE.
>
> x86 quirks of reserved size are taken into account and available and
> free size can be different, further helping debugging space issues.
>
I have no objections to this, but I'm not much of a user space/ VFS
person, so adding some other folks that can chime in if they want.
The point of this patch is that user space can query this information
using statvfs(), right?
> Signed-off-by: Anisse Astier <[email protected]>
> ---
> Notes:
> Patch isn't split per subsystem intentionally, for better understanding
> of intent; split could be trivial in a later version.
>
> I'm not sure whether statfs(2) should return an error if the efi request
> fails; I think it could be ignored with maybe a WARN_ONCE; which would
> be close to the current behaviour.
>
> Regards,
>
> Anisse
>
> ---
> arch/x86/platform/efi/quirks.c | 8 ++++++++
> drivers/firmware/efi/efi.c | 1 +
> drivers/firmware/efi/vars.c | 12 ++++++++++++
> fs/efivarfs/super.c | 26 +++++++++++++++++++++++++-
> include/linux/efi.h | 10 ++++++++++
> 5 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index b0b848d6933a..587fa51230e2 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -114,6 +114,14 @@ void efi_delete_dummy_variable(void)
> EFI_VARIABLE_RUNTIME_ACCESS, 0, NULL);
> }
>
> +u64 efi_reserved_space(void)
> +{
> + if (efi_no_storage_paranoia)
> + return 0;
> + return EFI_MIN_RESERVE;
> +}
> +EXPORT_SYMBOL_GPL(efi_reserved_space);
> +
> /*
> * In the nonblocking case we do not attempt to perform garbage
> * collection if we do not have enough free space. Rather, we do the
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index abeff7dc0b58..d0dfa007bffc 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -211,6 +211,7 @@ static int generic_ops_register(void)
> generic_ops.get_variable = efi.get_variable;
> generic_ops.get_next_variable = efi.get_next_variable;
> generic_ops.query_variable_store = efi_query_variable_store;
> + generic_ops.query_variable_info = efi.query_variable_info;
>
> if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) {
> generic_ops.set_variable = efi.set_variable;
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index bd75b87f5fc1..c5382d5c3073 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -245,3 +245,15 @@ efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> return status;
> }
> EXPORT_SYMBOL_NS_GPL(efivar_set_variable, EFIVAR);
> +
> +efi_status_t efivar_query_variable_info(u32 attr,
> + u64 *storage_space,
> + u64 *remaining_space,
> + u64 *max_variable_size)
> +{
> + if (!__efivars->ops->query_variable_info)
> + return EFI_UNSUPPORTED;
> + return __efivars->ops->query_variable_info(attr, storage_space,
> + remaining_space, max_variable_size);
> +}
> +EXPORT_SYMBOL_NS_GPL(efivar_query_variable_info, EFIVAR);
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index 482d612b716b..064bfc0243c9 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -13,6 +13,7 @@
> #include <linux/ucs2_string.h>
> #include <linux/slab.h>
> #include <linux/magic.h>
> +#include <linux/statfs.h>
>
> #include "internal.h"
>
> @@ -23,8 +24,31 @@ static void efivarfs_evict_inode(struct inode *inode)
> clear_inode(inode);
> }
>
> +static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf)
> +{
> + u64 storage_space, remaining_space, max_variable_size;
> + efi_status_t status;
> + const u32 attr = (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS);
> +
> + buf->f_type = dentry->d_sb->s_magic;
> + buf->f_bsize = 1;
> + buf->f_namelen = NAME_MAX;
> +
> + status = efivar_query_variable_info(attr, &storage_space, &remaining_space,
> + &max_variable_size);
> + if (status != EFI_SUCCESS)
> + return efi_status_to_err(status);
> + buf->f_blocks = storage_space;
> + buf->f_bfree = remaining_space;
> + if (remaining_space > efi_reserved_space())
> + buf->f_bavail = remaining_space - efi_reserved_space();
> + else
> + buf->f_bavail = 0;
> + return 0;
> +}
> static const struct super_operations efivarfs_ops = {
> - .statfs = simple_statfs,
> + .statfs = efivarfs_statfs,
> .drop_inode = generic_delete_inode,
> .evict_inode = efivarfs_evict_inode,
> };
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 7aa62c92185f..d2b686191870 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -703,6 +703,7 @@ static inline void efi_enter_virtual_mode (void) {}
> extern efi_status_t efi_query_variable_store(u32 attributes,
> unsigned long size,
> bool nonblocking);
> +extern u64 efi_reserved_space(void);
> #else
>
> static inline efi_status_t efi_query_variable_store(u32 attributes,
> @@ -711,6 +712,10 @@ static inline efi_status_t efi_query_variable_store(u32 attributes,
> {
> return EFI_SUCCESS;
> }
> +static inline u64 efi_reserved_space(void)
> +{
> + return 0;
> +}
> #endif
> extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
>
> @@ -1042,6 +1047,7 @@ struct efivar_operations {
> efi_set_variable_t *set_variable;
> efi_set_variable_t *set_variable_nonblocking;
> efi_query_variable_store_t *query_variable_store;
> + efi_query_variable_info_t *query_variable_info;
> };
>
> struct efivars {
> @@ -1087,6 +1093,10 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
> efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> u32 attr, unsigned long data_size, void *data);
>
> +efi_status_t efivar_query_variable_info(u32 attr, u64 *storage_space,
> + u64 *remaining_space,
> + u64 *max_variable_size);
> +
> #if IS_ENABLED(CONFIG_EFI_CAPSULE_LOADER)
> extern bool efi_capsule_pending(int *reset_type);
>
> --
> 2.34.1
>
Hi,
On Wed, May 10, 2023 at 05:13:36PM +0200, Ard Biesheuvel wrote:
> On Wed, 26 Apr 2023 at 21:59, Anisse Astier <[email protected]> wrote:
> >
> > From: Anisse Astier <[email protected]>
> >
> > When writing variables, one might get errors with no other message on
> > why it fails.
> >
> > Being able to see how much is used by EFI variables helps analyzing such
> > issues.
> >
> > Since this is not a conventionnal filesystem, block size is
> > intentionnally set to 1 instead of PAGE_SIZE.
> >
> > x86 quirks of reserved size are taken into account and available and
> > free size can be different, further helping debugging space issues.
> >
>
> I have no objections to this, but I'm not much of a user space/ VFS
> person, so adding some other folks that can chime in if they want.
Thanks !
>
> The point of this patch is that user space can query this information
> using statvfs(), right?
Yes, which means that a simple df command will show the used and free
space remaining in the EFI variable storage area:
$ df -h /sys/firmware/efi/efivars/
Filesystem Size Used Avail Use% Mounted on
efivarfs 176K 106K 66K 62% /sys/firmware/efi/efivars
Regards,
Anisse
>
>
>
> > Signed-off-by: Anisse Astier <[email protected]>
> > ---
> > Notes:
> > Patch isn't split per subsystem intentionally, for better understanding
> > of intent; split could be trivial in a later version.
> >
> > I'm not sure whether statfs(2) should return an error if the efi request
> > fails; I think it could be ignored with maybe a WARN_ONCE; which would
> > be close to the current behaviour.
> >
> > Regards,
> >
> > Anisse
> >
> > ---
> > arch/x86/platform/efi/quirks.c | 8 ++++++++
> > drivers/firmware/efi/efi.c | 1 +
> > drivers/firmware/efi/vars.c | 12 ++++++++++++
> > fs/efivarfs/super.c | 26 +++++++++++++++++++++++++-
> > include/linux/efi.h | 10 ++++++++++
> > 5 files changed, 56 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> > index b0b848d6933a..587fa51230e2 100644
> > --- a/arch/x86/platform/efi/quirks.c
> > +++ b/arch/x86/platform/efi/quirks.c
> > @@ -114,6 +114,14 @@ void efi_delete_dummy_variable(void)
> > EFI_VARIABLE_RUNTIME_ACCESS, 0, NULL);
> > }
> >
> > +u64 efi_reserved_space(void)
> > +{
> > + if (efi_no_storage_paranoia)
> > + return 0;
> > + return EFI_MIN_RESERVE;
> > +}
> > +EXPORT_SYMBOL_GPL(efi_reserved_space);
> > +
> > /*
> > * In the nonblocking case we do not attempt to perform garbage
> > * collection if we do not have enough free space. Rather, we do the
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index abeff7dc0b58..d0dfa007bffc 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -211,6 +211,7 @@ static int generic_ops_register(void)
> > generic_ops.get_variable = efi.get_variable;
> > generic_ops.get_next_variable = efi.get_next_variable;
> > generic_ops.query_variable_store = efi_query_variable_store;
> > + generic_ops.query_variable_info = efi.query_variable_info;
> >
> > if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) {
> > generic_ops.set_variable = efi.set_variable;
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index bd75b87f5fc1..c5382d5c3073 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -245,3 +245,15 @@ efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> > return status;
> > }
> > EXPORT_SYMBOL_NS_GPL(efivar_set_variable, EFIVAR);
> > +
> > +efi_status_t efivar_query_variable_info(u32 attr,
> > + u64 *storage_space,
> > + u64 *remaining_space,
> > + u64 *max_variable_size)
> > +{
> > + if (!__efivars->ops->query_variable_info)
> > + return EFI_UNSUPPORTED;
> > + return __efivars->ops->query_variable_info(attr, storage_space,
> > + remaining_space, max_variable_size);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(efivar_query_variable_info, EFIVAR);
> > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > index 482d612b716b..064bfc0243c9 100644
> > --- a/fs/efivarfs/super.c
> > +++ b/fs/efivarfs/super.c
> > @@ -13,6 +13,7 @@
> > #include <linux/ucs2_string.h>
> > #include <linux/slab.h>
> > #include <linux/magic.h>
> > +#include <linux/statfs.h>
> >
> > #include "internal.h"
> >
> > @@ -23,8 +24,31 @@ static void efivarfs_evict_inode(struct inode *inode)
> > clear_inode(inode);
> > }
> >
> > +static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf)
> > +{
> > + u64 storage_space, remaining_space, max_variable_size;
> > + efi_status_t status;
> > + const u32 attr = (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS);
> > +
> > + buf->f_type = dentry->d_sb->s_magic;
> > + buf->f_bsize = 1;
> > + buf->f_namelen = NAME_MAX;
> > +
> > + status = efivar_query_variable_info(attr, &storage_space, &remaining_space,
> > + &max_variable_size);
> > + if (status != EFI_SUCCESS)
> > + return efi_status_to_err(status);
> > + buf->f_blocks = storage_space;
> > + buf->f_bfree = remaining_space;
> > + if (remaining_space > efi_reserved_space())
> > + buf->f_bavail = remaining_space - efi_reserved_space();
> > + else
> > + buf->f_bavail = 0;
> > + return 0;
> > +}
> > static const struct super_operations efivarfs_ops = {
> > - .statfs = simple_statfs,
> > + .statfs = efivarfs_statfs,
> > .drop_inode = generic_delete_inode,
> > .evict_inode = efivarfs_evict_inode,
> > };
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 7aa62c92185f..d2b686191870 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -703,6 +703,7 @@ static inline void efi_enter_virtual_mode (void) {}
> > extern efi_status_t efi_query_variable_store(u32 attributes,
> > unsigned long size,
> > bool nonblocking);
> > +extern u64 efi_reserved_space(void);
> > #else
> >
> > static inline efi_status_t efi_query_variable_store(u32 attributes,
> > @@ -711,6 +712,10 @@ static inline efi_status_t efi_query_variable_store(u32 attributes,
> > {
> > return EFI_SUCCESS;
> > }
> > +static inline u64 efi_reserved_space(void)
> > +{
> > + return 0;
> > +}
> > #endif
> > extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
> >
> > @@ -1042,6 +1047,7 @@ struct efivar_operations {
> > efi_set_variable_t *set_variable;
> > efi_set_variable_t *set_variable_nonblocking;
> > efi_query_variable_store_t *query_variable_store;
> > + efi_query_variable_info_t *query_variable_info;
> > };
> >
> > struct efivars {
> > @@ -1087,6 +1093,10 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
> > efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> > u32 attr, unsigned long data_size, void *data);
> >
> > +efi_status_t efivar_query_variable_info(u32 attr, u64 *storage_space,
> > + u64 *remaining_space,
> > + u64 *max_variable_size);
> > +
> > #if IS_ENABLED(CONFIG_EFI_CAPSULE_LOADER)
> > extern bool efi_capsule_pending(int *reset_type);
> >
> > --
> > 2.34.1
> >
On Wed, May 10, 2023 at 05:13:36PM +0200, Ard Biesheuvel wrote:
> On Wed, 26 Apr 2023 at 21:59, Anisse Astier <[email protected]> wrote:
> >
> > From: Anisse Astier <[email protected]>
> >
> > When writing variables, one might get errors with no other message on
> > why it fails.
> >
> > Being able to see how much is used by EFI variables helps analyzing such
> > issues.
> >
> > Since this is not a conventionnal filesystem, block size is
> > intentionnally set to 1 instead of PAGE_SIZE.
> >
> > x86 quirks of reserved size are taken into account and available and
> > free size can be different, further helping debugging space issues.
> >
>
> I have no objections to this, but I'm not much of a user space/ VFS
> person, so adding some other folks that can chime in if they want.
>
> The point of this patch is that user space can query this information
> using statvfs(), right?
Seems ok to me.
>
>
>
> > Signed-off-by: Anisse Astier <[email protected]>
> > ---
> > Notes:
> > Patch isn't split per subsystem intentionally, for better understanding
> > of intent; split could be trivial in a later version.
> >
> > I'm not sure whether statfs(2) should return an error if the efi request
> > fails; I think it could be ignored with maybe a WARN_ONCE; which would
> > be close to the current behaviour.
Not sure. If you're not returning an error you would have to report made
up/magic values that could end up confusing userspace. So better to be
honest and report an error.
> >
> > Regards,
> >
> > Anisse
> >
> > ---
> > arch/x86/platform/efi/quirks.c | 8 ++++++++
> > drivers/firmware/efi/efi.c | 1 +
> > drivers/firmware/efi/vars.c | 12 ++++++++++++
> > fs/efivarfs/super.c | 26 +++++++++++++++++++++++++-
> > include/linux/efi.h | 10 ++++++++++
> > 5 files changed, 56 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> > index b0b848d6933a..587fa51230e2 100644
> > --- a/arch/x86/platform/efi/quirks.c
> > +++ b/arch/x86/platform/efi/quirks.c
> > @@ -114,6 +114,14 @@ void efi_delete_dummy_variable(void)
> > EFI_VARIABLE_RUNTIME_ACCESS, 0, NULL);
> > }
> >
> > +u64 efi_reserved_space(void)
> > +{
> > + if (efi_no_storage_paranoia)
> > + return 0;
> > + return EFI_MIN_RESERVE;
> > +}
> > +EXPORT_SYMBOL_GPL(efi_reserved_space);
> > +
> > /*
> > * In the nonblocking case we do not attempt to perform garbage
> > * collection if we do not have enough free space. Rather, we do the
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index abeff7dc0b58..d0dfa007bffc 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -211,6 +211,7 @@ static int generic_ops_register(void)
> > generic_ops.get_variable = efi.get_variable;
> > generic_ops.get_next_variable = efi.get_next_variable;
> > generic_ops.query_variable_store = efi_query_variable_store;
> > + generic_ops.query_variable_info = efi.query_variable_info;
> >
> > if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) {
> > generic_ops.set_variable = efi.set_variable;
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index bd75b87f5fc1..c5382d5c3073 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -245,3 +245,15 @@ efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> > return status;
> > }
> > EXPORT_SYMBOL_NS_GPL(efivar_set_variable, EFIVAR);
> > +
> > +efi_status_t efivar_query_variable_info(u32 attr,
> > + u64 *storage_space,
> > + u64 *remaining_space,
> > + u64 *max_variable_size)
> > +{
> > + if (!__efivars->ops->query_variable_info)
> > + return EFI_UNSUPPORTED;
> > + return __efivars->ops->query_variable_info(attr, storage_space,
> > + remaining_space, max_variable_size);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(efivar_query_variable_info, EFIVAR);
> > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > index 482d612b716b..064bfc0243c9 100644
> > --- a/fs/efivarfs/super.c
> > +++ b/fs/efivarfs/super.c
> > @@ -13,6 +13,7 @@
> > #include <linux/ucs2_string.h>
> > #include <linux/slab.h>
> > #include <linux/magic.h>
> > +#include <linux/statfs.h>
> >
> > #include "internal.h"
> >
> > @@ -23,8 +24,31 @@ static void efivarfs_evict_inode(struct inode *inode)
> > clear_inode(inode);
> > }
> >
> > +static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf)
> > +{
> > + u64 storage_space, remaining_space, max_variable_size;
> > + efi_status_t status;
> > + const u32 attr = (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS);
> > +
> > + buf->f_type = dentry->d_sb->s_magic;
> > + buf->f_bsize = 1;
> > + buf->f_namelen = NAME_MAX;
> > +
> > + status = efivar_query_variable_info(attr, &storage_space, &remaining_space,
> > + &max_variable_size);
> > + if (status != EFI_SUCCESS)
> > + return efi_status_to_err(status);
> > + buf->f_blocks = storage_space;
I have no idea about efivarfs specifically but I would add comments
why f_bsize is set to what it is and clarify the relationship between
f_bsize and f_blocks. Even if just for the sake of userspace to be able
to interpret this.
> > + buf->f_bfree = remaining_space;
> > + if (remaining_space > efi_reserved_space())
> > + buf->f_bavail = remaining_space - efi_reserved_space();
> > + else
> > + buf->f_bavail = 0;
> > + return 0;
> > +}
> > static const struct super_operations efivarfs_ops = {
> > - .statfs = simple_statfs,
> > + .statfs = efivarfs_statfs,
> > .drop_inode = generic_delete_inode,
> > .evict_inode = efivarfs_evict_inode,
> > };
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 7aa62c92185f..d2b686191870 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -703,6 +703,7 @@ static inline void efi_enter_virtual_mode (void) {}
> > extern efi_status_t efi_query_variable_store(u32 attributes,
> > unsigned long size,
> > bool nonblocking);
> > +extern u64 efi_reserved_space(void);
> > #else
> >
> > static inline efi_status_t efi_query_variable_store(u32 attributes,
> > @@ -711,6 +712,10 @@ static inline efi_status_t efi_query_variable_store(u32 attributes,
> > {
> > return EFI_SUCCESS;
> > }
> > +static inline u64 efi_reserved_space(void)
> > +{
> > + return 0;
> > +}
> > #endif
> > extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
> >
> > @@ -1042,6 +1047,7 @@ struct efivar_operations {
> > efi_set_variable_t *set_variable;
> > efi_set_variable_t *set_variable_nonblocking;
> > efi_query_variable_store_t *query_variable_store;
> > + efi_query_variable_info_t *query_variable_info;
> > };
> >
> > struct efivars {
> > @@ -1087,6 +1093,10 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
> > efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> > u32 attr, unsigned long data_size, void *data);
> >
> > +efi_status_t efivar_query_variable_info(u32 attr, u64 *storage_space,
> > + u64 *remaining_space,
> > + u64 *max_variable_size);
> > +
> > #if IS_ENABLED(CONFIG_EFI_CAPSULE_LOADER)
> > extern bool efi_capsule_pending(int *reset_type);
> >
> > --
> > 2.34.1
> >
Hi Christian,
On Mon, May 15, 2023 at 10:59:49AM +0200, Christian Brauner wrote:
> On Wed, May 10, 2023 at 05:13:36PM +0200, Ard Biesheuvel wrote:
> > On Wed, 26 Apr 2023 at 21:59, Anisse Astier <[email protected]> wrote:
> > >
> > > From: Anisse Astier <[email protected]>
> > >
> > > When writing variables, one might get errors with no other message on
> > > why it fails.
> > >
> > > Being able to see how much is used by EFI variables helps analyzing such
> > > issues.
> > >
> > > Since this is not a conventionnal filesystem, block size is
> > > intentionnally set to 1 instead of PAGE_SIZE.
> > >
> > > x86 quirks of reserved size are taken into account and available and
> > > free size can be different, further helping debugging space issues.
> > >
> >
> > I have no objections to this, but I'm not much of a user space/ VFS
> > person, so adding some other folks that can chime in if they want.
> >
> > The point of this patch is that user space can query this information
> > using statvfs(), right?
>
> Seems ok to me.
>
> >
> >
> >
> > > Signed-off-by: Anisse Astier <[email protected]>
> > > ---
> > > Notes:
> > > Patch isn't split per subsystem intentionally, for better understanding
> > > of intent; split could be trivial in a later version.
> > >
> > > I'm not sure whether statfs(2) should return an error if the efi request
> > > fails; I think it could be ignored with maybe a WARN_ONCE; which would
> > > be close to the current behaviour.
>
> Not sure. If you're not returning an error you would have to report made
> up/magic values that could end up confusing userspace. So better to be
> honest and report an error.
Ack.
>
> > >
> > > Regards,
> > >
> > > Anisse
> > >
> > > ---
> > > arch/x86/platform/efi/quirks.c | 8 ++++++++
> > > drivers/firmware/efi/efi.c | 1 +
> > > drivers/firmware/efi/vars.c | 12 ++++++++++++
> > > fs/efivarfs/super.c | 26 +++++++++++++++++++++++++-
> > > include/linux/efi.h | 10 ++++++++++
> > > 5 files changed, 56 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> > > index b0b848d6933a..587fa51230e2 100644
> > > --- a/arch/x86/platform/efi/quirks.c
> > > +++ b/arch/x86/platform/efi/quirks.c
> > > @@ -114,6 +114,14 @@ void efi_delete_dummy_variable(void)
> > > EFI_VARIABLE_RUNTIME_ACCESS, 0, NULL);
> > > }
> > >
> > > +u64 efi_reserved_space(void)
> > > +{
> > > + if (efi_no_storage_paranoia)
> > > + return 0;
> > > + return EFI_MIN_RESERVE;
> > > +}
> > > +EXPORT_SYMBOL_GPL(efi_reserved_space);
> > > +
> > > /*
> > > * In the nonblocking case we do not attempt to perform garbage
> > > * collection if we do not have enough free space. Rather, we do the
> > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > index abeff7dc0b58..d0dfa007bffc 100644
> > > --- a/drivers/firmware/efi/efi.c
> > > +++ b/drivers/firmware/efi/efi.c
> > > @@ -211,6 +211,7 @@ static int generic_ops_register(void)
> > > generic_ops.get_variable = efi.get_variable;
> > > generic_ops.get_next_variable = efi.get_next_variable;
> > > generic_ops.query_variable_store = efi_query_variable_store;
> > > + generic_ops.query_variable_info = efi.query_variable_info;
> > >
> > > if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) {
> > > generic_ops.set_variable = efi.set_variable;
> > > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > > index bd75b87f5fc1..c5382d5c3073 100644
> > > --- a/drivers/firmware/efi/vars.c
> > > +++ b/drivers/firmware/efi/vars.c
> > > @@ -245,3 +245,15 @@ efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> > > return status;
> > > }
> > > EXPORT_SYMBOL_NS_GPL(efivar_set_variable, EFIVAR);
> > > +
> > > +efi_status_t efivar_query_variable_info(u32 attr,
> > > + u64 *storage_space,
> > > + u64 *remaining_space,
> > > + u64 *max_variable_size)
> > > +{
> > > + if (!__efivars->ops->query_variable_info)
> > > + return EFI_UNSUPPORTED;
> > > + return __efivars->ops->query_variable_info(attr, storage_space,
> > > + remaining_space, max_variable_size);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(efivar_query_variable_info, EFIVAR);
> > > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > > index 482d612b716b..064bfc0243c9 100644
> > > --- a/fs/efivarfs/super.c
> > > +++ b/fs/efivarfs/super.c
> > > @@ -13,6 +13,7 @@
> > > #include <linux/ucs2_string.h>
> > > #include <linux/slab.h>
> > > #include <linux/magic.h>
> > > +#include <linux/statfs.h>
> > >
> > > #include "internal.h"
> > >
> > > @@ -23,8 +24,31 @@ static void efivarfs_evict_inode(struct inode *inode)
> > > clear_inode(inode);
> > > }
> > >
> > > +static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf)
> > > +{
> > > + u64 storage_space, remaining_space, max_variable_size;
> > > + efi_status_t status;
> > > + const u32 attr = (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > + EFI_VARIABLE_RUNTIME_ACCESS);
> > > +
> > > + buf->f_type = dentry->d_sb->s_magic;
> > > + buf->f_bsize = 1;
> > > + buf->f_namelen = NAME_MAX;
> > > +
> > > + status = efivar_query_variable_info(attr, &storage_space, &remaining_space,
> > > + &max_variable_size);
> > > + if (status != EFI_SUCCESS)
> > > + return efi_status_to_err(status);
> > > + buf->f_blocks = storage_space;
>
> I have no idea about efivarfs specifically but I would add comments
> why f_bsize is set to what it is and clarify the relationship between
> f_bsize and f_blocks. Even if just for the sake of userspace to be able
> to interpret this.
Ack.
Thanks for taking the time to review this patch, I'll send a new version
soon.
Regards,
Anisse