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@xxxxxxxxxx> > --- > 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.
Attachment:
signature.asc
Description: This is a digitally signed message part
