> On 10. Apr 2022, at 13:05, Vladimir Oltean <olte...@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-jakobkosc...@gmail.com/
>>>
>>> Jakob
>>
>> Thanks,
>> Jakob
Thanks,
Jakob
[1] https://www.vusec.net/projects/kasper/
[2] https://spectreattack.com/spectre.pdf
