Re: [PATCH 1/2] efi: remove use of list iterator variable after loop

[Ard Biesheuvel]

On Thu, 14 Apr 2022 at 00:09, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> (cc Kees for pstore)
>
> On Wed, 13 Apr 2022 at 20:08, Jakob Koschel <jakobkoschel@xxxxxxxxx> wrote:
> >
> >
> >
> > > On 13. Apr 2022, at 19:05, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > >
> > > On Fri, 1 Apr 2022 at 00:11, Jakob Koschel <jakobkoschel@xxxxxxxxx> wrote:
> > >>
> > >> In preparation to limiting the scope of a list iterator to the list
> > >> traversal loop, use a dedicated pointer to iterate through the list [1].
> > >>
> > >> In the current state the list_for_each_entry() is guaranteed to
> > >> hit a break or goto in order to work properly. If the list iterator
> > >> executes completely or the list is empty the iterator variable contains
> > >> a type confused bogus value infered from the head of the list.
> > >>
> > >> With this patch the variable used past the list iterator is only set
> > >> if the list exists early and is NULL otherwise. It should therefore
> > >> be safe to just set *prev = NULL (as it was before).
> > >>
> > >
> > > This generic boilerplate is fine to include, but it would help if you
> > > could point out why repainting the current logic with your new brush
> > > is appropriate here.
> >
> > This makes sense, I can see that the commit message should be improved here.
> >
> > >
> > > In this particular case, I wonder whether updating *prev makes sense
> > > to begin with if we are returning an error, and if we fix that, the
> > > issue disappears as well.
> >
> > Actually I'm rethinking this now. The only use of 'prev' that I can see is
> > in efi_pstore_erase_name(). It only uses it if found != 0
> > which would mean err != 0 in __efivar_entry_iter().
> >
> > This would allow massively simplifying the entire function.
> > The valid case is updating *prev when there is an "error" as far as I can tell.
> >
>
> OK, so in summary, the only user of that code that bothers to pass a
> value for prev abuses it to implement its own version of
> efivar_entry_find(), and so if we fix that caller, we can drop the
> 'prev' argument from this function altogether.
>
>
> > I've sketched up a rewritten function that should hopefully be more clear and
> > archive the same goal, I'm curious what you think:
> >
> >
> >         int __efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
> >                                 struct list_head *head, void *data,
> >                                 struct efivar_entry **prev)
> >         {
> >                 struct efivar_entry *entry, *n;
> >                 int err = 0;
> >
> >                 /* If prev is set and *prev != NULL start iterating from there */
> >                 if (prev)
> >                         entry = list_prepare_entry(*prev, head, list);
> >                 /* Otherwise start at the beginning */
> >                 else
> >                         entry = list_entry(head, typeof(*entry), list);
> >                 list_for_each_entry_safe_continue(entry, n, head, list) {
> >                         err = func(entry, data);
> >                         if (err && prev)
> >                                 *prev = entry;
> >                         if (err)
> >                                 return err;
> >                 }
> >
> >                 return 0;
> >         }
> >
>
> Thanks for this. I'll have a stab myself at fixing the EFI pstore
> code, and hopefully we can clean up __efivar_entry_iter() as I
> suggested.

This is now queued up in the EFI tree: the pstore driver no longer use
the efivar_entry API at all, and the remaining user of
efivar_entry_iter() does not use the value of the iterator variable in
the same way.