Re: [PATCH v5 6/8] efi: load SSTDs from EFI variables

[Matt Fleming]

(Sorry, didn't get chance to reply before you sent this version)

On Fri, 01 Jul, at 11:19:10PM, Octavian Purdila wrote:
> +
> +static __init int efivar_ssdt_load(void)
> +{
> +	struct efivar_entry *entry;
> +	/* We need a temporary empty list to be able to set duplicates
> +	 * to true so that the efivar lock is dropped to allow us to
> +	 * call efivar_entry_get from the iterator function.
> +	 */
> +	LIST_HEAD(tmp);
> +	int ret;

I suspect you actually want to enable duplicate detection though,
because that bug (GetNextVariable() returning the same variable over
and over) does exist in the wild and it isn't that much more work to
enable the check.

Yes, the stack-local list looks like the right approach, because you
don't need access to these entries once this function returns.

Just be sure to free all those entries after efivar_init(), which
doesn't need to be done under efivar_entry_iter_{begin,end}() because
no one can concurrently access the list anyway.

Also...

	/*
	 * Multi-line comments should look like this, with the empty
	 * first line.
	 */

> +
> +	/* efivar_entry is too big to allocate it on stack */
> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry)
> +		return -ENOMEM;
> +	ret = efivar_init(efivar_ssdt_iter, entry, true, &tmp);
> +	kfree(entry);
> +	return ret;
> +}
> +#else
> +static inline int efivar_ssdt_load(void) { return 0; }
> +#endif

My suggestion is,

  - Push the 'entry' allocation into efivar_ssdt_iter()
  - Add 'entry' to the stack-local tmp list via efivar_entry_add()
  - Iterate 'tmp' and free entries before returning from efivar_ssdt_load()

Make sense?