lkml.org 
[lkml]   [2013]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space
From
Date
On Mon, 2013-04-01 at 11:14 -0400, Matthew Garrett wrote:
> EFI implementations distinguish between space that is actively used by a
> variable and space that merely hasn't been garbage collected yet. Space
> that hasn't yet been garbage collected isn't available for use and so isn't
> counted in the remaining_space field returned by QueryVariableInfo().
>
> Combined with commit 68d9298 this can cause problems. Some implementations
> don't garbage collect until the remaining space is smaller than the maximum
> variable size, and as a result check_var_size() will always fail once more
> than 50% of the variable store has been used even if most of that space is
> marked as available for garbage collection. The user is unable to create
> new variables, and deleting variables doesn't increase the remaining space.
>
> The problem that 68d9298 was attempting to avoid was one where certain
> platforms fail if the actively used space is greater than 50% of the
> available storage space. We should be able to calculate that by simply
> summing the size of each available variable and subtracting that from
> the total storage space. With luck this will fix the problem described in
> https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting
> damage to occur to the machines 68d9298 was attempting to fix.
>
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
> drivers/firmware/efivars.c | 41 ++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 60e7d8f..a6d8a58 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
[...]
> @@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes,
> if (status != EFI_SUCCESS)
> return status;
>
> - if (!storage_size || size > remaining_size || size > max_size ||
> - (remaining_size - size) < (storage_size / 2))
> + list_for_each_entry(entry, &efivars->list, list) {
> + var = &entry->var;
> + status = get_var_data_locked(efivars, var);
> +
> + if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE))
> + continue;
> +
> + active_size += var->DataSize;
> + active_size += utf16_strsize(var->VariableName, 1024);
> + /*
> + * There's some additional metadata associated with each
> + * variable. Intel's reference implementation is 60 bytes -
> + * bump that to 128 to account for vendor additions and
> + * potential alignment constraints
> + */
> + active_size += 128;
> + }
> +
> + /*
> + * Add on the size of any boot services-only variables
> + */
> + active_size += boot_var_size;
> +
> + active_available = storage_size - active_size;
> +
> + if (!storage_size || size > remaining_size ||
> + (active_available - size) < (storage_size / 2))
> return EFI_OUT_OF_RESOURCES;

Both substractions here could quite possibly underflow due to
over-estimation of active_size.

We can rearrange the condition to be:

active_size + size > storage_size / 2

and delete the active_available variable. But I think metadata for the
new variable should be accounted as well:

active_size + size + 128 > storage_size / 2

Since we'll now be using that magic number twice, it needs a name.

Ben.

> return status;

--
Ben Hutchings
DNRC Motto: I can please only one person per day.
Today is not your day. Tomorrow isn't looking good either.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2013-04-01 18:01    [W:0.895 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site