Re: [PATCH v2] efi: vars: commonize the 512-byte name buffer quirk

[David Laight <david.laight.linux@gmail.com>]

On Thu, 03 Sep 2026 22:03:18 +0200
"Ard Biesheuvel" <ardb@kernel.org> wrote:

> Thanks for respinning this.
> 
> Some additional thoughts below.
> 
> On Thu, 3 Sep 2026, at 13:55, Jonggeun Park wrote:
> > Some old UEFI implementations reject GetNextVariableName() calls with
> > a name buffer size larger than 512 bytes. Both efivar_init() and
> > efi_pstore_read() open-code the same workaround of resetting the size
> > to 512 on every iteration.
> >
> > Move this workaround into efivar_get_next_variable(), which keeps this
> > quirk in one place, resolving the TODO in efi-pstore.c.
> >
> > No functional change intended.
> >
> > Signed-off-by: Jonggeun Park <jakejgpark@gmail.com>
> > ---
> > v2:
> > - Move the workaround into the existing efivar_get_next_variable().
> > ---
> >  drivers/firmware/efi/efi-pstore.c | 10 ----------
> >  drivers/firmware/efi/vars.c       |  7 +++++++
> >  fs/efivarfs/vars.c                | 12 +-----------
> >  3 files changed, 8 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efi-pstore.c 
> > b/drivers/firmware/efi/efi-pstore.c
> > index a5db3534f..4f2594d08 100644
> > --- a/drivers/firmware/efi/efi-pstore.c
> > +++ b/drivers/firmware/efi/efi-pstore.c
> > @@ -164,16 +164,6 @@ static ssize_t efi_pstore_read(struct 
> > pstore_record *record)
> >  	efi_status_t status;
> > 
> >  	for (;;) {
> > -		/*
> > -		 * A small set of old UEFI implementations reject sizes
> > -		 * above a certain threshold, the lowest seen in the wild
> > -		 * is 512.
> > -		 *
> > -		 * TODO: Commonize with the iteration implementation in
> > -		 *       fs/efivarfs to keep all the quirks in one place.
> > -		 */
> > -		varname_size = 512;
> > -
> >  		/*
> >  		 * If this is the first read() call in the pstore enumeration,
> >  		 * varname will be the empty string, and the GetNextVariable()
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index 3700e9869..0f95fc60d 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -191,11 +191,18 @@ EXPORT_SYMBOL_NS_GPL(efivar_get_variable, 
> > "EFIVAR");
> >  /*
> >   * efivar_get_next_variable() - enumerate the next name/vendor pair
> >   *
> > + * A small set of old UEFI implementations reject sizes above a certain
> > + * threshold, the lowest seen in the wild is 512. Set the name buffer 
> > size
> > + * to 512 on each call.
> > + *
> >   * Must be called with efivars_lock held.
> >   */
> >  efi_status_t efivar_get_next_variable(unsigned long *name_size,
> >  				      efi_char16_t *name, efi_guid_t *vendor)
> >  {
> > +	BUILD_BUG_ON(EFI_VAR_NAME_LEN < 512);
> > +	*name_size = 512;
> > +  
> 
> This should really be
> 
>   *name_size = min(*name_size, 512UL);

You don't need the UL suffix.

> 
> so that a smaller buffer size provided by the caller is respected.
> 
> That also removes the need for the BUILD_BUG_ON().
> 
> No need to send a v3, I can fix that up when applying.
> 
> >  	return __efivars->ops->get_next_variable(name_size, name, vendor);
> >  }
> >  EXPORT_SYMBOL_NS_GPL(efivar_get_next_variable, "EFIVAR");
> > diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
> > index 6833c3d24..5a01833a6 100644
> > --- a/fs/efivarfs/vars.c
> > +++ b/fs/efivarfs/vars.c
> > @@ -391,18 +391,8 @@ int efivar_init(int (*func)(efi_char16_t *, 
> > efi_guid_t, unsigned long, void *),
> >  	if (err)
> >  		goto free;
> > 
> > -	/*
> > -	 * A small set of old UEFI implementations reject sizes
> > -	 * above a certain threshold, the lowest seen in the wild
> > -	 * is 512.
> > -	 */
> > -
> >  	do {
> > -		variable_name_size = 512;
> > -		BUILD_BUG_ON(EFI_VAR_NAME_LEN < 512);
> > -
> > -		status = efivar_get_next_variable(&variable_name_size,
> > -						  variable_name,
> > +		status = efivar_get_next_variable(&variable_name_size, variable_name,
> >  						  &vendor_guid);
> >  		switch (status) {
> >  		case EFI_SUCCESS:
> > -- 
> > 2.43.0  
>