Re: [PATCH net-next 02/15] net: dsa: sja1105: Remove usage of iterator for list_add() after loop

[Jakob Koschel <jakobkoschel@gmail.com>]

> On 10. Apr 2022, at 13:05, Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> On Sun, Apr 10, 2022 at 12:51:56PM +0200, Jakob Koschel wrote:
>> I've just looked at this again in a bit more detail while integrating it into the patch series.
>> 
>> I realized that this just shifts the 'problem' to using the 'pos' iterator variable after the loop.
>> If the scope of the list iterator would be lowered to the list traversal loop it would also make sense
>> to also do it for list_for_each().
> 
> Yes, but list_for_each() was never formulated as being problematic in
> the same way as list_for_each_entry(), was it? I guess I'm starting to
> not understand what is the true purpose of the changes.

Sorry for having caused the confusion. Let me elaborate a bit to give more context.

There are two main benefits of this entire effort.

1) fix the architectural bugs and avoid any missuse of the list iterator after the loop
by construction. This only concerns the list_for_each_entry_*() macros and your change
will allow lowering the scope for all of those. It might be debatable that it would be
more consistent to lower the scope for list_for_each() as well, but it wouldn't be
strictly necessary.

2) fix *possible* speculative bugs. In our project, Kasper [1], we were able to show
that this can be an issue for the list traversal macros (and this is how the entire
effort started).
The reason is that the processor might run an additional loop iteration in speculative
execution with the iterator variable computed based on the head element. This can
(and we have verified this) happen if the CPU incorrectly 
assumes !list_entry_is_head(pos, head, member).

If this happens, all memory accesses based on the iterator variable *potentially* open
the chance for spectre [2] gadgets. The proposed mitigation was setting the iterator variable
to NULL when the terminating condition is reached (in speculative safe way). Then,
the additional speculative list iteration would still execute but won't access any
potential secret data.

And this would also be required for list_for_each() since combined with the list_entry()
within the loop it basically is semantically identical to list_for_each_entry()
for the additional speculative iteration.

Now, I have no strong opinion on going all the way and since 2) is not the main motivation
for this I'm also fine with sticking to your proposed solution, but it would mean that implementing
a "speculative safe" list_for_each() will be more difficult in the future since it is using
the iterator of list_for_each() past the loop.

I hope this explains the background a bit better.

> 
>> What do you think about doing it this way:
>> 
>> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
>> index b7e95d60a6e4..f5b0502c1098 100644
>> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
>> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
>> @@ -28,6 +28,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
>>                list_add(&e->list, &gating_cfg->entries);
>>        } else {
>>                struct sja1105_gate_entry *p;
>> +               struct list_head *pos = NULL;
>> 
>>                list_for_each_entry(p, &gating_cfg->entries, list) {
>>                        if (p->interval == e->interval) {
>> @@ -37,10 +38,14 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
>>                                goto err;
>>                        }
>> 
>> -                       if (e->interval < p->interval)
>> +                       if (e->interval < p->interval) {
>> +                               pos = &p->list;
>>                                break;
>> +                       }
>>                }
>> -               list_add(&e->list, p->list.prev);
>> +               if (!pos)
>> +                       pos = &gating_cfg->entries;
>> +               list_add(&e->list, pos->prev);
>>        }
>> 
>>        gating_cfg->num_entries++;
>> --
>> 
>>> 
>>> Thanks for the suggestion.
>>> 
>>>> 	}
>>>> 
>>>> 	gating_cfg->num_entries++;
>>>> -----------------------------[ cut here ]-----------------------------
>>> 
>>> [1] https://lore.kernel.org/linux-kernel/20220407102900.3086255-12-jakobkoschel@gmail.com/
>>> 
>>> 	Jakob
>> 
>> Thanks,
>> Jakob

Thanks,
Jakob

[1] https://www.vusec.net/projects/kasper/
[2] https://spectreattack.com/spectre.pdf