Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.80.82.1.1\))
Subject: Re: [PATCH 1/2] efi: remove use of list iterator variable after loop
From:   Jakob Koschel <jakobkoschel@gmail.com>
In-Reply-To: <CAMj1kXE2r4xrtFc+=OJfzutZzTtaUoFtW=f7y9+us9h+xGVEnA@mail.gmail.com>
Date:   Wed, 13 Apr 2022 20:08:09 +0200
Cc:     Matthew Garrett <mjg59@srcf.ucam.org>,
        Peter Jones <pjones@redhat.com>,
        linux-efi <linux-efi@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Mike Rapoport <rppt@kernel.org>,
        Brian Johannesmeyer <bjohannesmeyer@gmail.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>
Content-Transfer-Encoding: quoted-printable
Message-Id: <9142505E-720F-401E-AD48-BA9D0880EDD1@gmail.com>
References: <20220331221030.889718-1-jakobkoschel@gmail.com>
 <CAMj1kXE2r4xrtFc+=OJfzutZzTtaUoFtW=f7y9+us9h+xGVEnA@mail.gmail.com>
To:     Ard Biesheuvel <ardb@kernel.org>
Precedence: bulk


> On 13. Apr 2022, at 19:05, Ard Biesheuvel <ardb@kernel.org> wrote:
>=20
> On Fri, 1 Apr 2022 at 00:11, Jakob Koschel <jakobkoschel@gmail.com> =
wrote:
>>=20
>> 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].
>>=20
>> 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.
>>=20
>> 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 =3D NULL (as it was before).
>>=20
>=20
> 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.

>=20
> 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 !=3D 0
which would mean err !=3D 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.

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 =3D 0;

		/* If prev is set and *prev !=3D NULL start iterating =
from there */
		if (prev)
			entry =3D list_prepare_entry(*prev, head, list);
		/* Otherwise start at the beginning */
		else
			entry =3D list_entry(head, typeof(*entry), =
list);
		list_for_each_entry_safe_continue(entry, n, head, list) =
{
			err =3D func(entry, data);
			if (err && prev)
				*prev =3D entry;
			if (err)
				return err;
		}

		return 0;
	}