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

[Jakob Koschel]

Hey Vladimir,

> On 9. Apr 2022, at 01:54, Jakob Koschel <jakobkoschel@xxxxxxxxx> wrote:
> 
> Hello Vladimir,
> 
>> On 8. Apr 2022, at 13:41, Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
>> 
>> Hello Jakob,
>> 
>> On Thu, Apr 07, 2022 at 12:28:47PM +0200, Jakob Koschel wrote:
>>> In preparation to limit the scope of a list iterator to the list
>>> traversal loop, use a dedicated pointer to point to the found element [1].
>>> 
>>> Before, the code implicitly used the head when no element was found
>>> when using &pos->list. Since the new variable is only set if an
>>> element was found, the list_add() is performed within the loop
>>> and only done after the loop if it is done on the list head directly.
>>> 
>>> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@xxxxxxxxxxxxxx/ [1]
>>> Signed-off-by: Jakob Koschel <jakobkoschel@xxxxxxxxx>
>>> ---
>>> drivers/net/dsa/sja1105/sja1105_vl.c | 14 +++++++++-----
>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
>>> index b7e95d60a6e4..cfcae4d19eef 100644
>>> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
>>> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
>>> @@ -27,20 +27,24 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
>>> 	if (list_empty(&gating_cfg->entries)) {
>>> 		list_add(&e->list, &gating_cfg->entries);
>>> 	} else {
>>> -		struct sja1105_gate_entry *p;
>>> +		struct sja1105_gate_entry *p = NULL, *iter;
>>> 
>>> -		list_for_each_entry(p, &gating_cfg->entries, list) {
>>> -			if (p->interval == e->interval) {
>>> +		list_for_each_entry(iter, &gating_cfg->entries, list) {
>>> +			if (iter->interval == e->interval) {
>>> 				NL_SET_ERR_MSG_MOD(extack,
>>> 						 "Gate conflict");
>>> 				rc = -EBUSY;
>>> 				goto err;
>>> 			}
>>> 
>>> -			if (e->interval < p->interval)
>>> +			if (e->interval < iter->interval) {
>>> +				p = iter;
>>> +				list_add(&e->list, iter->list.prev);
>>> 				break;
>>> +			}
>>> 		}
>>> -		list_add(&e->list, p->list.prev);
>>> +		if (!p)
>>> +			list_add(&e->list, gating_cfg->entries.prev);
>>> 	}
>>> 
>>> 	gating_cfg->num_entries++;
>>> -- 
>>> 2.25.1
>>> 
>> 
>> I apologize in advance if I've misinterpreted the end goal of your patch.
>> I do have a vague suspicion I understand what you're trying to achieve,
>> and in that case, would you mind using this patch instead of yours?
> 
> I think you are very much spot on!
> 
>> I think it still preserves the intention of the code in a clean manner.
>> 
>> -----------------------------[ cut here ]-----------------------------
>> From 7aed740750d1bc3bff6e85fd33298f5905bb4e01 Mon Sep 17 00:00:00 2001
>> From: Vladimir Oltean <vladimir.oltean@xxxxxxx>
>> Date: Fri, 8 Apr 2022 13:55:14 +0300
>> Subject: [PATCH] net: dsa: sja1105: avoid use of type-confused pointer in
>> sja1105_insert_gate_entry()
>> 
>> It appears that list_for_each_entry() leaks a type-confused pointer when
>> the iteration loop ends with no early break, since "*p" will no longer
>> point to a "struct sja1105_gate_entry", but rather to some memory in
>> front of "gating_cfg->entries".
>> 
>> This isn't actually a problem here, because if the element we insert has
>> the highest interval, therefore we never exit the loop early, "p->list"
>> (which is all that we use outside the loop) will in fact point to
>> "gating_cfg->entries" even though "p" itself is invalid.
>> 
>> Nonetheless, there are preparations to increase the safety of
>> list_for_each_entry() by making it impossible to use the encapsulating
>> structure of the iterator element outside the loop. So something needs
>> to change here before those preparations go in, even though this
>> constitutes legitimate use.
>> 
>> Make it clear that we are not dereferencing members of the encapsulating
>> "struct sja1105_gate_entry" outside the loop, by using the regular
>> list_for_each() iterator, and dereferencing the struct sja1105_gate_entry
>> only within the loop.
>> 
>> With list_for_each(), the iterator element at the end of the loop does
>> have a sane value in all cases, and we can just use that as the "head"
>> argument of list_add().
>> 
>> Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
>> ---
>> drivers/net/dsa/sja1105/sja1105_vl.c | 12 +++++++++---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
>> index c0e45b393fde..fe93c80fe5ef 100644
>> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
>> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
>> @@ -27,9 +27,15 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
>> 	if (list_empty(&gating_cfg->entries)) {
>> 		list_add(&e->list, &gating_cfg->entries);
>> 	} else {
>> -		struct sja1105_gate_entry *p;
>> +		struct list_head *pos;
>> +
>> +		/* We cannot safely use list_for_each_entry()
>> +		 * because we dereference "pos" after the loop
>> +		 */
>> +		list_for_each(pos, &gating_cfg->entries) {
>> +			struct sja1105_gate_entry *p;
>> 
>> -		list_for_each_entry(p, &gating_cfg->entries, list) {
>> +			p = list_entry(pos, struct sja1105_gate_entry, list);
>> 			if (p->interval == e->interval) {
>> 				NL_SET_ERR_MSG_MOD(extack,
>> 						 "Gate conflict");
>> @@ -40,7 +46,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
>> 			if (e->interval < p->interval)
>> 				break;
>> 		}
>> -		list_add(&e->list, p->list.prev);
>> +		list_add(&e->list, pos->prev);
> 
> I was actually considering doing it this way before but wasn't sure if this would be preferred.
> I've done something like this in [1] and it does turn out quite well.
> 
> I'll integrate this in the v2 series.

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().

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@xxxxxxxxx/
> 
> 	Jakob

Thanks,
Jakob