Nouveau - Feb 2022 - [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

On Mon, 2022-02-28 at 21:07 +0100, Christian K?nig
wrote:
> Am 28.02.22 um 20:56 schrieb Linus Torvalds:
> > On Mon, Feb 28, 2022 at 4:19 AM Christian K?nig
> > <christian.koenig at amd.com> wrote:
> > > I don't think that using the extra variable makes the code in
any
> > > way
> > > more reliable or easier to read.
> > So I think the next step is to do the attached patch (which
> > requires
> > that "-std=gnu11" that was discussed in the original
thread).
> > 
> > That will guarantee that the 'pos' parameter of
> > list_for_each_entry()
> > is only updated INSIDE the for_each_list_entry() loop, and can
> > never
> > point to the (wrongly typed) head entry.
> > 
> > And I would actually hope that it should actually cause compiler
> > warnings about possibly uninitialized variables if people then use
> > the
> > 'pos' pointer outside the loop. Except
> > 
> >   (a) that code in sgx/encl.c currently initializes 'tmp' to
NULL
> > for
> > inexplicable reasons - possibly because it already expected this
> > behavior
> > 
> >   (b) when I remove that NULL initializer, I still don't get a
> > warning,
> > because we've disabled -Wno-maybe-uninitialized since it results
in
> > so
> > many false positives.
> > 
> > Oh well.
> > 
> > Anyway, give this patch a look, and at least if it's expanded to
do
> > "(pos) = NULL" in the entry statement for the for-loop, it
will
> > avoid the HEAD type confusion that Jakob is working on. And I think
> > in a cleaner way than the horrid games he plays.
> > 
> > (But it won't avoid possible CPU speculation of such type
> > confusion. That, in my opinion, is a completely different issue)
> 
> Yes, completely agree.
> 
> > I do wish we could actually poison the 'pos' value after the
loop
> > somehow - but clearly the "might be uninitialized" I was
hoping for
> > isn't the way to do it.
> > 
> > Anybody have any ideas?
> 
> I think we should look at the use cases why code is touching (pos)
> after the loop.
> 
> Just from skimming over the patches to change this and experience
> with the drivers/subsystems I help to maintain I think the primary
> pattern looks something like this:
> 
> list_for_each_entry(entry, head, member) {
>      if (some_condition_checking(entry))
>          break;
> }
> do_something_with(entry);

Actually, we usually have a check to see if the loop found anything,
but in that case it should something like

if (list_entry_is_head(entry, head, member)) {
    return with error;
}
do_somethin_with(entry);

Suffice?  The list_entry_is_head() macro is designed to cope with the
bogus entry on head problem.

James

Am 28.02.22 um 21:42 schrieb James Bottomley:
> On Mon, 2022-02-28 at 21:07 +0100, Christian K?nig wrote:
>> Am 28.02.22 um 20:56 schrieb Linus Torvalds:
>>> On Mon, Feb 28, 2022 at 4:19 AM Christian K?nig
>>> <christian.koenig at amd.com> wrote:
>>> [SNIP]
>>> Anybody have any ideas?
>> I think we should look at the use cases why code is touching (pos)
>> after the loop.
>>
>> Just from skimming over the patches to change this and experience
>> with the drivers/subsystems I help to maintain I think the primary
>> pattern looks something like this:
>>
>> list_for_each_entry(entry, head, member) {
>>       if (some_condition_checking(entry))
>>           break;
>> }
>> do_something_with(entry);
>
> Actually, we usually have a check to see if the loop found anything,
> but in that case it should something like
>
> if (list_entry_is_head(entry, head, member)) {
>      return with error;
> }
> do_somethin_with(entry);
>
> Suffice?  The list_entry_is_head() macro is designed to cope with the
> bogus entry on head problem.
That will work and is also what people already do.

The key problem is that we let people do the same thing over and over 
again with slightly different implementations.

Out in the wild I've seen at least using a separate variable, using a 
bool to indicate that something was found and just assuming that the 
list has an entry.

The last case is bogus and basically what can break badly.

If we would have an unified macro which search for an entry combined 
with automated reporting on patches to use that macro I think the 
potential to introduce such issues will already go down massively 
without auditing tons of existing code.

Regards,
Christian.
>
> James
>
>

On Mon, 2022-02-28 at 21:56 +0100, Christian K?nig
wrote:
> 
> Am 28.02.22 um 21:42 schrieb James Bottomley:
> > On Mon, 2022-02-28 at 21:07 +0100, Christian K?nig wrote:
> > > Am 28.02.22 um 20:56 schrieb Linus Torvalds:
> > > > On Mon, Feb 28, 2022 at 4:19 AM Christian K?nig
> > > > <christian.koenig at amd.com> wrote:
> > > > [SNIP]
> > > > Anybody have any ideas?
> > > I think we should look at the use cases why code is touching
> > > (pos)
> > > after the loop.
> > > 
> > > Just from skimming over the patches to change this and experience
> > > with the drivers/subsystems I help to maintain I think the
> > > primary pattern looks something like this:
> > > 
> > > list_for_each_entry(entry, head, member) {
> > >       if (some_condition_checking(entry))
> > >           break;
> > > }
> > > do_something_with(entry);
> > 
> > Actually, we usually have a check to see if the loop found
> > anything, but in that case it should something like
> > 
> > if (list_entry_is_head(entry, head, member)) {
> >      return with error;
> > }
> > do_somethin_with(entry);
> > 
> > Suffice?  The list_entry_is_head() macro is designed to cope with
> > the bogus entry on head problem.
> 
> That will work and is also what people already do.
> 
> The key problem is that we let people do the same thing over and
> over again with slightly different implementations.
> 
> Out in the wild I've seen at least using a separate variable, using
> a bool to indicate that something was found and just assuming that
> the list has an entry.
> 
> The last case is bogus and basically what can break badly.
Yes, I understand that.  I'm saying we should replace that bogus checks
of entry->something against some_value loop termination condition with
the list_entry_is_head() macro.  That should be a one line and fairly
mechanical change rather than the explosion of code changes we seem to
have in the patch series.

James

On Mon, Feb 28, 2022 at 3:45 PM James Bottomley
<James.Bottomley at hansenpartnership.com> wrote:
> ...
> > Just from skimming over the patches to change this and experience
> > with the drivers/subsystems I help to maintain I think the primary
> > pattern looks something like this:
> >
> > list_for_each_entry(entry, head, member) {
> >      if (some_condition_checking(entry))
> >          break;
> > }
> > do_something_with(entry);
>
>
> Actually, we usually have a check to see if the loop found anything,
> but in that case it should something like
>
> if (list_entry_is_head(entry, head, member)) {
>     return with error;
> }
> do_somethin_with(entry);
Borrowing from c++, perhaps an explicit end should be used:

  if (list_entry_not_end(entry))
    do_somethin_with(entry)

It is modelled after c++ and the 'while(begin != end) {}' pattern.

Jeff

On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley <James.Bottomley
at HansenPartnership.com> wrote:
>On Mon, 2022-02-28 at 21:07 +0100, Christian K?nig wrote:
>> Am 28.02.22 um 20:56 schrieb Linus Torvalds:
>> > On Mon, Feb 28, 2022 at 4:19 AM Christian K?nig
>> > <christian.koenig at amd.com> wrote:
>> > > I don't think that using the extra variable makes the
code in any
>> > > way
>> > > more reliable or easier to read.
>> > So I think the next step is to do the attached patch (which
>> > requires
>> > that "-std=gnu11" that was discussed in the original
thread).
>> > 
>> > That will guarantee that the 'pos' parameter of
>> > list_for_each_entry()
>> > is only updated INSIDE the for_each_list_entry() loop, and can
>> > never
>> > point to the (wrongly typed) head entry.
>> > 
>> > And I would actually hope that it should actually cause compiler
>> > warnings about possibly uninitialized variables if people then use
>> > the
>> > 'pos' pointer outside the loop. Except
>> > 
>> >   (a) that code in sgx/encl.c currently initializes 'tmp'
to NULL
>> > for
>> > inexplicable reasons - possibly because it already expected this
>> > behavior
>> > 
>> >   (b) when I remove that NULL initializer, I still don't get a
>> > warning,
>> > because we've disabled -Wno-maybe-uninitialized since it
results in
>> > so
>> > many false positives.
>> > 
>> > Oh well.
>> > 
>> > Anyway, give this patch a look, and at least if it's expanded
to do
>> > "(pos) = NULL" in the entry statement for the for-loop,
it will
>> > avoid the HEAD type confusion that Jakob is working on. And I
think
>> > in a cleaner way than the horrid games he plays.
>> > 
>> > (But it won't avoid possible CPU speculation of such type
>> > confusion. That, in my opinion, is a completely different issue)
>> 
>> Yes, completely agree.
>> 
>> > I do wish we could actually poison the 'pos' value after
the loop
>> > somehow - but clearly the "might be uninitialized" I was
hoping for
>> > isn't the way to do it.
>> > 
>> > Anybody have any ideas?
>> 
>> I think we should look at the use cases why code is touching (pos)
>> after the loop.
>> 
>> Just from skimming over the patches to change this and experience
>> with the drivers/subsystems I help to maintain I think the primary
>> pattern looks something like this:
>> 
>> list_for_each_entry(entry, head, member) {
>>      if (some_condition_checking(entry))
>>          break;
>> }
>> do_something_with(entry);
>
>
>Actually, we usually have a check to see if the loop found anything,
>but in that case it should something like
>
>if (list_entry_is_head(entry, head, member)) {
>    return with error;
>}
>do_somethin_with(entry);
>
>Suffice?  The list_entry_is_head() macro is designed to cope with the
>bogus entry on head problem.
Won't suffice because the end goal of this work is to limit scope of entry
only to loop. Hence the need for additional variable.

Besides, there are no guarantees that people won't do_something_with(entry)
without the check or won't compare entry to NULL to check if the loop
finished with break or not.
>James

-- 
Sincerely yours,
Mike

> On 28. Feb 2022, at 21:56, Christian K?nig <christian.koenig at
amd.com> wrote:
> 
> 
> 
> Am 28.02.22 um 21:42 schrieb James Bottomley:
>> On Mon, 2022-02-28 at 21:07 +0100, Christian K?nig wrote:
>>> Am 28.02.22 um 20:56 schrieb Linus Torvalds:
>>>> On Mon, Feb 28, 2022 at 4:19 AM Christian K?nig
>>>> <christian.koenig at amd.com> wrote:
>>>> [SNIP]
>>>> Anybody have any ideas?
>>> I think we should look at the use cases why code is touching (pos)
>>> after the loop.
>>> 
>>> Just from skimming over the patches to change this and experience
>>> with the drivers/subsystems I help to maintain I think the primary
>>> pattern looks something like this:
>>> 
>>> list_for_each_entry(entry, head, member) {
>>>      if (some_condition_checking(entry))
>>>          break;
>>> }
>>> do_something_with(entry);
There are other cases where the list iterator variable is used after the loop
Some examples:

- list_for_each_entry_continue() and list_for_each_entry_from().

- (although very rare) the head is actually of the correct struct type.
		(ppc440spe_get_group_entry(): drivers/dma/ppc4xx/adma.c:1436)

- to use pos->list for example for list_add_tail():
		(add_static_vm_early(): arch/arm/mm/ioremap.c:107)

If the scope of the list iterator is limited those still need fixing in a
different way.
>> 
>> Actually, we usually have a check to see if the loop found anything,
>> but in that case it should something like
>> 
>> if (list_entry_is_head(entry, head, member)) {
>>     return with error;
>> }
>> do_somethin_with(entry);
>> 
>> Suffice?  The list_entry_is_head() macro is designed to cope with the
>> bogus entry on head problem.
> 
> That will work and is also what people already do.
> 
> The key problem is that we let people do the same thing over and over again
with slightly different implementations.
> 
> Out in the wild I've seen at least using a separate variable, using a
bool to indicate that something was found and just assuming that the list has an
entry.
> 
> The last case is bogus and basically what can break badly.
> 
> If we would have an unified macro which search for an entry combined with
automated reporting on patches to use that macro I think the potential to
introduce such issues will already go down massively without auditing tons of
existing code.
Having a unified way to do the same thing would indeed be great.
> 
> Regards,
> Christian.
> 
>> 
>> James
>> 
>> 
> 
- Jakob