[PATCH net-next 00/15] net: Remove use of list iterator after loop body

[PATCH net-next 00/15] net: Remove use of list iterator after loop body

[Jakob Koschel]

When the list iterator loop does not exit early the list iterator variable
contains a type-confused pointer to a 'bogus' list element computed based
on the head [1].

Often a 'found' variable is used to ensure the list iterator
variable is only accessed after the loop body if the loop did exit early
(using a break or goto).

In other cases that list iterator variable is used in
combination to access the list member which reverses the invocation of
container_of() and brings back a "safe" pointer to the head of the list.

Since, due to this code patten, there were quite a few bugs discovered [2],
Linus concluded that the rule should be to never use the list iterator
after the loop and introduce a dedicated pointer for that [3].

With the new gnu11 standard, it will now be possible to limit the scope
of the list iterator variable to the traversal loop itself by defining
the variable within the for loop.
This, however, requires to remove all uses of the list iterator after
the loop.

Based on input from Paolo Abeni [4], Vinicius Costa Gomes [5], and
Jakub Kicinski [6], I've splitted all the net-next related changes into
two patch sets, where this is part 1.

Link: https://lwn.net/Articles/887097/ [1]
Link: https://lore.kernel.org/linux-kernel/20220217184829.1991035-4-jakobkoschel@gmail.com/ [2]
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [3]
Link: https://lore.kernel.org/linux-kernel/7393b673c626fd75f2b4f8509faa5459254fb87c.camel@redhat.com/ [4]
Link: https://lore.kernel.org/linux-kernel/877d8a3sww.fsf@intel.com/ [5]
Link: https://lore.kernel.org/linux-kernel/20220403205502.1b34415d@kernel.org/ [6]

[PATCH net-next 01/15] connector: Replace usage of found with dedicated list iterator variable

[Jakob Koschel]

To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.

To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].

This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/connector/cn_queue.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/connector/cn_queue.c b/drivers/connector/cn_queue.c
index 996f025eb63c..ed77599b0b25 100644
--- a/drivers/connector/cn_queue.c
+++ b/drivers/connector/cn_queue.c
@@ -92,20 +92,19 @@ int cn_queue_add_callback(struct cn_queue_dev *dev, const char *name,
 
 void cn_queue_del_callback(struct cn_queue_dev *dev, const struct cb_id *id)
 {
-	struct cn_callback_entry *cbq, *n;
-	int found = 0;
+	struct cn_callback_entry *cbq = NULL, *iter, *n;
 
 	spin_lock_bh(&dev->queue_lock);
-	list_for_each_entry_safe(cbq, n, &dev->queue_list, callback_entry) {
-		if (cn_cb_equal(&cbq->id.id, id)) {
-			list_del(&cbq->callback_entry);
-			found = 1;
+	list_for_each_entry_safe(iter, n, &dev->queue_list, callback_entry) {
+		if (cn_cb_equal(&iter->id.id, id)) {
+			list_del(&iter->callback_entry);
+			cbq = iter;
 			break;
 		}
 	}
 	spin_unlock_bh(&dev->queue_lock);
 
-	if (found)
+	if (cbq)
 		cn_queue_release_callback(cbq);
 }
 
-- 
2.25.1

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

[Jakob Koschel]

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@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 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

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

[Jakub Kicinski]

On Thu,  7 Apr 2022 12:28:47 +0200 Jakob Koschel wrote:
> 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);

This turns a pretty slick piece of code into something ugly :(
I'd rather you open coded the iteration here than make it more 
complex to satisfy "safe coding guidelines".

Also the list_add() could be converted to list_add_tail().

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

[Jakob Koschel]

Hello Jakub,

> On 8. Apr 2022, at 05:54, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> On Thu,  7 Apr 2022 12:28:47 +0200 Jakob Koschel wrote:
>> 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);
> 
> This turns a pretty slick piece of code into something ugly :(
> I'd rather you open coded the iteration here than make it more 
> complex to satisfy "safe coding guidelines".

I'm not entirely sure I understand what you mean with 
"open coded the iteration". But maybe the solution proposed by Vladimir [1]
works for you? I'm planning to rewrite the cases in that way for the relevant
ones.

> 
> Also the list_add() could be converted to list_add_tail().

Good point, I wasn't sure if that's considered as something that should be
done as a separate change. I'm happy to include it in v2.

Thanks for the input.

	Jakob

[1] https://lore.kernel.org/linux-kernel/20220408114120.tvf2lxvhfqbnrlml@skbuf/

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

[Jakub Kicinski]

On Sat, 9 Apr 2022 01:58:29 +0200 Jakob Koschel wrote:
> > This turns a pretty slick piece of code into something ugly :(
> > I'd rather you open coded the iteration here than make it more 
> > complex to satisfy "safe coding guidelines".  
> 
> I'm not entirely sure I understand what you mean with 
> "open coded the iteration". But maybe the solution proposed by Vladimir [1]
> works for you?

Yup, that's what I meant!

> I'm planning to rewrite the cases in that way for the relevant ones.
>
> > Also the list_add() could be converted to list_add_tail().  
> 
> Good point, I wasn't sure if that's considered as something that should be
> done as a separate change. I'm happy to include it in v2.

Ack, separate patch would be better for that. I guess Vladimir may have
used .prev on purpose, since _tail() doesn't intuitively scream _after()
Anyway, not important.

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

[Vladimir Oltean]

On Sat, Apr 09, 2022 at 01:58:29AM +0200, Jakob Koschel wrote:
> Hello Jakub,
> > Also the list_add() could be converted to list_add_tail().
> 
> Good point, I wasn't sure if that's considered as something that should be
> done as a separate change. I'm happy to include it in v2.

By now you probably studied more list access patterns than I did,
but I wrote that deliberately using list_add(..., pos->prev) rather than
list_add_tail(), because even though the code is the same, I tend to
think of the "head" argument of list_add_tail() as being the actual head
of the list, and therefore the head->prev being the tail of the list
(hence the name), something which doesn't hold true here where we're
inserting in the middle of the list. Anyway it's just a name and that's
what felt natural to me at the time, I won't oppose the change, but do
make it a separate change and not clump it together with the unrelated
list_for_each_entry() -> list_for_each() change.

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

[Christophe Leroy]

Le 07/04/2022 à 12:28, Jakob Koschel a écrit :
> 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@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> ---
>   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++;

This change looks ugly, why duplicating the list_add() to do the same ? 
At the end of the loop the pointer contains gating_cfg->entries, so it 
was cleaner before.

If you don't want to use the loop index outside the loop, fair enough, 
all you have to do is:

		struct sja1105_gate_entry *p, *iter;

		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;
			}
			p = iter;

			if (e->interval < iter->interval)
				break;
		}
		list_add(&e->list, p->list.prev);



Christophe

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

[Jakob Koschel]

Hey Christophe,

> On 8. Apr 2022, at 09:47, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
> 
> 
> Le 07/04/2022 à 12:28, Jakob Koschel a écrit :
>> 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@mail.gmail.com/ [1]
>> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
>> ---
>> 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++;
> 
> This change looks ugly, why duplicating the list_add() to do the same ? 
> At the end of the loop the pointer contains gating_cfg->entries, so it 
> was cleaner before.
> 
> If you don't want to use the loop index outside the loop, fair enough, 
> all you have to do is:
> 
> 		struct sja1105_gate_entry *p, *iter;
> 
> 		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;
> 			}
> 			p = iter;
> 
> 			if (e->interval < iter->interval)
> 				break;
> 		}
> 		list_add(&e->list, p->list.prev);

Thanks for the review and input.

The code you are showing here would have an uninitialized access to 'p'
if the list is empty.

Also 'p->list.prev' will be the second last entry if the list iterator
ran through completely, whereas the original code was pointing to the last
entry of the list.

IMO Vladimir Oltean posted a nice alternative way to solve it, see [1].
That way it keeps the semantics of the code the same and doesn't duplicate
the list_add.

> 
> 
> 
> Christophe

[1] https://lore.kernel.org/linux-kernel/20220408114120.tvf2lxvhfqbnrlml@skbuf/

Thanks,
Jakob

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

[Vladimir Oltean]

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@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> ---
>  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 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@nxp.com>
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@nxp.com>
---
 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);
 	}
 
 	gating_cfg->num_entries++;
-----------------------------[ cut here ]-----------------------------

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

[Jakob Koschel]

Hello Vladimir,

> On 8. Apr 2022, at 13:41, Vladimir Oltean <olteanv@gmail.com> 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@mail.gmail.com/ [1]
>> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
>> ---
>> 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@nxp.com>
> 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@nxp.com>
> ---
> 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.

Thanks for the suggestion.

> 	}
> 
> 	gating_cfg->num_entries++;
> -----------------------------[ cut here ]-----------------------------

[1] https://lore.kernel.org/linux-kernel/20220407102900.3086255-12-jakobkoschel@gmail.com/

	Jakob

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@gmail.com> wrote:
> 
> Hello Vladimir,
> 
>> On 8. Apr 2022, at 13:41, Vladimir Oltean <olteanv@gmail.com> 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@mail.gmail.com/ [1]
>>> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
>>> ---
>>> 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@nxp.com>
>> 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@nxp.com>
>> ---
>> 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@gmail.com/
> 
> 	Jakob

Thanks,
Jakob

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

[Vladimir Oltean]

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.

> 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

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

[Jakob Koschel]

> 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

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

[Jakob Koschel]

> On 10. Apr 2022, at 14:39, Jakob Koschel <jakobkoschel@gmail.com> wrote:
> 
> 
> 
>> 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


Btw, I just realized that the if (!pos) is not necessary. This should simply do it:

diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index b7e95d60a6e4..2d59e75a9e3d 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 list_head *pos = &gating_cfg->entries;
 		struct sja1105_gate_entry *p;
 
 		list_for_each_entry(p, &gating_cfg->entries, list) {
 			if (p->interval == e->interval) {
@@ -37,10 +38,12 @@ 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);
+		list_add(&e->list, pos->prev);
 	}
 
 	gating_cfg->num_entries++;
-- 
2.25.1

Thanks,
Jakob

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

[Vladimir Oltean]

On Sun, Apr 10, 2022 at 08:24:37PM +0200, Jakob Koschel wrote:
> Btw, I just realized that the if (!pos) is not necessary. This should simply do it:
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
> index b7e95d60a6e4..2d59e75a9e3d 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 list_head *pos = &gating_cfg->entries;
>  		struct sja1105_gate_entry *p;
>  
>  		list_for_each_entry(p, &gating_cfg->entries, list) {
>  			if (p->interval == e->interval) {
> @@ -37,10 +38,12 @@ 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);
> +		list_add(&e->list, pos->prev);
>  	}
>  
>  	gating_cfg->num_entries++;
> -- 
> 2.25.1

I think we can give this a turn that is actually beneficial for the driver.
I've prepared and tested 3 patches on this function, see below.
Concrete improvements:
- that thing with list_for_each_entry() and list_for_each()
- no more special-casing of an empty list
- simplifying the error path
- that thing with list_add_tail()

What do you think about the changes below?

From 5b952b75e239cbe96729cf78c17e8d9725c9617c Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Sun, 10 Apr 2022 22:21:41 +0300
Subject: [PATCH 1/3] net: dsa: sja1105: remove use of iterator after
 list_for_each_entry() loop

Jakob Koschel explains in the link below that there is a desire to
syntactically change list_for_each_entry() and list_for_each() such that
it becomes impossible to use the iterator variable outside the scope of
the loop.

Although sja1105_insert_gate_entry() makes legitimate use of the
iterator pointer when it breaks out, the pattern it uses may become
illegal, so it needs to change.

It is deemed acceptable to use a copy of the loop iterator, and
sja1105_insert_gate_entry() only needs to know the list_head element
before which the list insertion should be made. So let's profit from the
occasion and refactor the list iteration to a dedicated function.

An additional benefit is given by the fact that with the helper function
in place, we no longer need to special-case the empty list, since it is
equivalent to not having found any gating entry larger than the
specified interval in the list. We just need to insert at the tail of
that list (list_add vs list_add_tail on an empty list does the same
thing).

Link: https://patchwork.kernel.org/project/netdevbpf/patch/20220407102900.3086255-3-jakobkoschel@gmail.com/#24810127
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_vl.c | 46 ++++++++++++++++++----------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index b7e95d60a6e4..369be2ac3587 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -7,6 +7,27 @@
 
 #define SJA1105_SIZE_VL_STATUS			8
 
+static struct list_head *
+sja1105_first_entry_longer_than(struct list_head *entries,
+				s64 interval,
+				struct netlink_ext_ack *extack)
+{
+	struct sja1105_gate_entry *p;
+
+	list_for_each_entry(p, entries, list) {
+		if (p->interval == interval) {
+			NL_SET_ERR_MSG_MOD(extack, "Gate conflict");
+			return ERR_PTR(-EBUSY);
+		}
+
+		if (interval < p->interval)
+			return &p->list;
+	}
+
+	/* Empty list, or specified interval is largest within the list */
+	return entries;
+}
+
 /* Insert into the global gate list, sorted by gate action time. */
 static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
 				     struct sja1105_rule *rule,
@@ -14,6 +35,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
 				     struct netlink_ext_ack *extack)
 {
 	struct sja1105_gate_entry *e;
+	struct list_head *pos;
 	int rc;
 
 	e = kzalloc(sizeof(*e), GFP_KERNEL);
@@ -24,25 +46,15 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
 	e->gate_state = gate_state;
 	e->interval = entry_time;
 
-	if (list_empty(&gating_cfg->entries)) {
-		list_add(&e->list, &gating_cfg->entries);
-	} else {
-		struct sja1105_gate_entry *p;
-
-		list_for_each_entry(p, &gating_cfg->entries, list) {
-			if (p->interval == e->interval) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "Gate conflict");
-				rc = -EBUSY;
-				goto err;
-			}
-
-			if (e->interval < p->interval)
-				break;
-		}
-		list_add(&e->list, p->list.prev);
+	pos = sja1105_first_entry_longer_than(&gating_cfg->entries,
+					      e->interval, extack);
+	if (IS_ERR(pos)) {
+		rc = PTR_ERR(pos);
+		goto err;
 	}
 
+	list_add(&e->list, pos->prev);
+
 	gating_cfg->num_entries++;
 
 	return 0;
-- 
2.25.1

From b6385f62d730b69007ea218e885461542ca4c44c Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Sun, 10 Apr 2022 22:34:35 +0300
Subject: [PATCH 2/3] net: dsa: sja1105: reorder
 sja1105_first_entry_longer_than with memory allocation

sja1105_first_entry_longer_than() does not make use of the full struct
sja1105_gate_entry *e, just of e->interval which is set from the passed
entry_time.

This means that if there is a gate conflict, we have allocated e for
nothing, just to free it later. Reorder the memory allocation and the
function call, to avoid that and simplify the error unwind path.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_vl.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index 369be2ac3587..e5ea8eb9ec4e 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -36,7 +36,11 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
 {
 	struct sja1105_gate_entry *e;
 	struct list_head *pos;
-	int rc;
+
+	pos = sja1105_first_entry_longer_than(&gating_cfg->entries,
+					      entry_time, extack);
+	if (IS_ERR(pos))
+		return PTR_ERR(pos);
 
 	e = kzalloc(sizeof(*e), GFP_KERNEL);
 	if (!e)
@@ -45,22 +49,11 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
 	e->rule = rule;
 	e->gate_state = gate_state;
 	e->interval = entry_time;
-
-	pos = sja1105_first_entry_longer_than(&gating_cfg->entries,
-					      e->interval, extack);
-	if (IS_ERR(pos)) {
-		rc = PTR_ERR(pos);
-		goto err;
-	}
-
 	list_add(&e->list, pos->prev);
 
 	gating_cfg->num_entries++;
 
 	return 0;
-err:
-	kfree(e);
-	return rc;
 }
 
 /* The gate entries contain absolute times in their e->interval field. Convert
-- 
2.25.1

From 8aa272b8a3f53aba7b80f181b275e040b9aaed8d Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Sun, 10 Apr 2022 22:37:14 +0300
Subject: [PATCH 3/3] net: dsa: sja1105: use list_add_tail(pos) instead of
 list_add(pos->prev)

When passed a non-head list element, list_add_tail() actually adds the
new element to its left, which is what we want. Despite the slightly
confusing name, use the dedicated function which does the same thing as
the open-coded list_add(pos->prev).

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_vl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index e5ea8eb9ec4e..7fe9b18f1cbd 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -49,7 +49,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
 	e->rule = rule;
 	e->gate_state = gate_state;
 	e->interval = entry_time;
-	list_add(&e->list, pos->prev);
+	list_add_tail(&e->list, pos);
 
 	gating_cfg->num_entries++;
 
-- 
2.25.1

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

[Jakob Koschel]

> On 10. Apr 2022, at 22:02, Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> On Sun, Apr 10, 2022 at 08:24:37PM +0200, Jakob Koschel wrote:
>> Btw, I just realized that the if (!pos) is not necessary. This should simply do it:
>> 
>> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
>> index b7e95d60a6e4..2d59e75a9e3d 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 list_head *pos = &gating_cfg->entries;
>> 		struct sja1105_gate_entry *p;
>> 
>> 		list_for_each_entry(p, &gating_cfg->entries, list) {
>> 			if (p->interval == e->interval) {
>> @@ -37,10 +38,12 @@ 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);
>> +		list_add(&e->list, pos->prev);
>> 	}
>> 
>> 	gating_cfg->num_entries++;
>> -- 
>> 2.25.1
> 
> I think we can give this a turn that is actually beneficial for the driver.
> I've prepared and tested 3 patches on this function, see below.
> Concrete improvements:
> - that thing with list_for_each_entry() and list_for_each()
> - no more special-casing of an empty list
> - simplifying the error path
> - that thing with list_add_tail()
> 
> What do you think about the changes below?

Thanks for all the good cooperation and help. The changes look great.
I'll include them in v2 unless you want to do that separately, then I'll
just remove them from the patch series.

> 
> From 5b952b75e239cbe96729cf78c17e8d9725c9617c Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Sun, 10 Apr 2022 22:21:41 +0300
> Subject: [PATCH 1/3] net: dsa: sja1105: remove use of iterator after
> list_for_each_entry() loop
> 
> Jakob Koschel explains in the link below that there is a desire to
> syntactically change list_for_each_entry() and list_for_each() such that
> it becomes impossible to use the iterator variable outside the scope of
> the loop.
> 
> Although sja1105_insert_gate_entry() makes legitimate use of the
> iterator pointer when it breaks out, the pattern it uses may become
> illegal, so it needs to change.
> 
> It is deemed acceptable to use a copy of the loop iterator, and
> sja1105_insert_gate_entry() only needs to know the list_head element
> before which the list insertion should be made. So let's profit from the
> occasion and refactor the list iteration to a dedicated function.
> 
> An additional benefit is given by the fact that with the helper function
> in place, we no longer need to special-case the empty list, since it is
> equivalent to not having found any gating entry larger than the
> specified interval in the list. We just need to insert at the tail of
> that list (list_add vs list_add_tail on an empty list does the same
> thing).
> 
> Link: https://patchwork.kernel.org/project/netdevbpf/patch/20220407102900.3086255-3-jakobkoschel@gmail.com/#24810127
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> drivers/net/dsa/sja1105/sja1105_vl.c | 46 ++++++++++++++++++----------
> 1 file changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
> index b7e95d60a6e4..369be2ac3587 100644
> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
> @@ -7,6 +7,27 @@
> 
> #define SJA1105_SIZE_VL_STATUS			8
> 
> +static struct list_head *
> +sja1105_first_entry_longer_than(struct list_head *entries,
> +				s64 interval,
> +				struct netlink_ext_ack *extack)
> +{
> +	struct sja1105_gate_entry *p;
> +
> +	list_for_each_entry(p, entries, list) {
> +		if (p->interval == interval) {
> +			NL_SET_ERR_MSG_MOD(extack, "Gate conflict");
> +			return ERR_PTR(-EBUSY);
> +		}
> +
> +		if (interval < p->interval)
> +			return &p->list;
> +	}
> +
> +	/* Empty list, or specified interval is largest within the list */
> +	return entries;
> +}
> +
> /* Insert into the global gate list, sorted by gate action time. */
> static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
> 				     struct sja1105_rule *rule,
> @@ -14,6 +35,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
> 				     struct netlink_ext_ack *extack)
> {
> 	struct sja1105_gate_entry *e;
> +	struct list_head *pos;
> 	int rc;
> 
> 	e = kzalloc(sizeof(*e), GFP_KERNEL);
> @@ -24,25 +46,15 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
> 	e->gate_state = gate_state;
> 	e->interval = entry_time;
> 
> -	if (list_empty(&gating_cfg->entries)) {
> -		list_add(&e->list, &gating_cfg->entries);
> -	} else {
> -		struct sja1105_gate_entry *p;
> -
> -		list_for_each_entry(p, &gating_cfg->entries, list) {
> -			if (p->interval == e->interval) {
> -				NL_SET_ERR_MSG_MOD(extack,
> -						   "Gate conflict");
> -				rc = -EBUSY;
> -				goto err;
> -			}
> -
> -			if (e->interval < p->interval)
> -				break;
> -		}
> -		list_add(&e->list, p->list.prev);
> +	pos = sja1105_first_entry_longer_than(&gating_cfg->entries,
> +					      e->interval, extack);
> +	if (IS_ERR(pos)) {
> +		rc = PTR_ERR(pos);
> +		goto err;
> 	}
> 
> +	list_add(&e->list, pos->prev);
> +
> 	gating_cfg->num_entries++;
> 
> 	return 0;
> -- 
> 2.25.1
> 
> From b6385f62d730b69007ea218e885461542ca4c44c Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Sun, 10 Apr 2022 22:34:35 +0300
> Subject: [PATCH 2/3] net: dsa: sja1105: reorder
> sja1105_first_entry_longer_than with memory allocation
> 
> sja1105_first_entry_longer_than() does not make use of the full struct
> sja1105_gate_entry *e, just of e->interval which is set from the passed
> entry_time.
> 
> This means that if there is a gate conflict, we have allocated e for
> nothing, just to free it later. Reorder the memory allocation and the
> function call, to avoid that and simplify the error unwind path.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> drivers/net/dsa/sja1105/sja1105_vl.c | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
> index 369be2ac3587..e5ea8eb9ec4e 100644
> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
> @@ -36,7 +36,11 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
> {
> 	struct sja1105_gate_entry *e;
> 	struct list_head *pos;
> -	int rc;
> +
> +	pos = sja1105_first_entry_longer_than(&gating_cfg->entries,
> +					      entry_time, extack);
> +	if (IS_ERR(pos))
> +		return PTR_ERR(pos);
> 
> 	e = kzalloc(sizeof(*e), GFP_KERNEL);
> 	if (!e)
> @@ -45,22 +49,11 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
> 	e->rule = rule;
> 	e->gate_state = gate_state;
> 	e->interval = entry_time;
> -
> -	pos = sja1105_first_entry_longer_than(&gating_cfg->entries,
> -					      e->interval, extack);
> -	if (IS_ERR(pos)) {
> -		rc = PTR_ERR(pos);
> -		goto err;
> -	}
> -
> 	list_add(&e->list, pos->prev);
> 
> 	gating_cfg->num_entries++;
> 
> 	return 0;
> -err:
> -	kfree(e);
> -	return rc;
> }
> 
> /* The gate entries contain absolute times in their e->interval field. Convert
> -- 
> 2.25.1
> 
> From 8aa272b8a3f53aba7b80f181b275e040b9aaed8d Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Sun, 10 Apr 2022 22:37:14 +0300
> Subject: [PATCH 3/3] net: dsa: sja1105: use list_add_tail(pos) instead of
> list_add(pos->prev)
> 
> When passed a non-head list element, list_add_tail() actually adds the
> new element to its left, which is what we want. Despite the slightly
> confusing name, use the dedicated function which does the same thing as
> the open-coded list_add(pos->prev).
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> drivers/net/dsa/sja1105/sja1105_vl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
> index e5ea8eb9ec4e..7fe9b18f1cbd 100644
> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
> @@ -49,7 +49,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
> 	e->rule = rule;
> 	e->gate_state = gate_state;
> 	e->interval = entry_time;
> -	list_add(&e->list, pos->prev);
> +	list_add_tail(&e->list, pos);
> 
> 	gating_cfg->num_entries++;
> 
> -- 
> 2.25.1
> 

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

[Vladimir Oltean]

On Sun, Apr 10, 2022 at 10:30:31PM +0200, Jakob Koschel wrote:
> > On 10. Apr 2022, at 22:02, Vladimir Oltean <olteanv@gmail.com> wrote:
> > 
> > On Sun, Apr 10, 2022 at 08:24:37PM +0200, Jakob Koschel wrote:
> >> Btw, I just realized that the if (!pos) is not necessary. This should simply do it:
> >> 
> >> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
> >> index b7e95d60a6e4..2d59e75a9e3d 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 list_head *pos = &gating_cfg->entries;
> >> 		struct sja1105_gate_entry *p;
> >> 
> >> 		list_for_each_entry(p, &gating_cfg->entries, list) {
> >> 			if (p->interval == e->interval) {
> >> @@ -37,10 +38,12 @@ 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);
> >> +		list_add(&e->list, pos->prev);
> >> 	}
> >> 
> >> 	gating_cfg->num_entries++;
> >> -- 
> >> 2.25.1
> > 
> > I think we can give this a turn that is actually beneficial for the driver.
> > I've prepared and tested 3 patches on this function, see below.
> > Concrete improvements:
> > - that thing with list_for_each_entry() and list_for_each()
> > - no more special-casing of an empty list
> > - simplifying the error path
> > - that thing with list_add_tail()
> > 
> > What do you think about the changes below?
> 
> Thanks for all the good cooperation and help. The changes look great.
> I'll include them in v2 unless you want to do that separately, then I'll
> just remove them from the patch series.

I think it's less of a synchronization hassle if you send them along
with your list of others. Good luck.

[PATCH net-next 03/15] net: dsa: mv88e6xxx: Replace usage of found with dedicated iterator

[Jakob Koschel]

To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.

To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].

This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 64f4fdd02902..f254f537c357 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1381,28 +1381,27 @@ static int mv88e6xxx_set_mac_eee(struct dsa_switch *ds, int port,
 /* Mask of the local ports allowed to receive frames from a given fabric port */
 static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
 {
+	struct dsa_port *dp = NULL, *iter, *other_dp;
 	struct dsa_switch *ds = chip->ds;
 	struct dsa_switch_tree *dst = ds->dst;
-	struct dsa_port *dp, *other_dp;
-	bool found = false;
 	u16 pvlan;
 
 	/* dev is a physical switch */
 	if (dev <= dst->last_switch) {
-		list_for_each_entry(dp, &dst->ports, list) {
-			if (dp->ds->index == dev && dp->index == port) {
-				/* dp might be a DSA link or a user port, so it
+		list_for_each_entry(iter, &dst->ports, list) {
+			if (iter->ds->index == dev && iter->index == port) {
+				/* iter might be a DSA link or a user port, so it
 				 * might or might not have a bridge.
-				 * Use the "found" variable for both cases.
+				 * Set the "dp" variable for both cases.
 				 */
-				found = true;
+				dp = iter;
 				break;
 			}
 		}
 	/* dev is a virtual bridge */
 	} else {
-		list_for_each_entry(dp, &dst->ports, list) {
-			unsigned int bridge_num = dsa_port_bridge_num_get(dp);
+		list_for_each_entry(iter, &dst->ports, list) {
+			unsigned int bridge_num = dsa_port_bridge_num_get(iter);
 
 			if (!bridge_num)
 				continue;
@@ -1410,13 +1409,13 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
 			if (bridge_num + dst->last_switch != dev)
 				continue;
 
-			found = true;
+			dp = iter;
 			break;
 		}
 	}
 
 	/* Prevent frames from unknown switch or virtual bridge */
-	if (!found)
+	if (!dp)
 		return 0;
 
 	/* Frames from DSA links and CPU ports can egress any local port */
-- 
2.25.1

Re: [PATCH net-next 03/15] net: dsa: mv88e6xxx: Replace usage of found with dedicated iterator

[Vladimir Oltean]

Hi Jakob,

On Thu, Apr 07, 2022 at 12:28:48PM +0200, Jakob Koschel wrote:
> To move the list iterator variable into the list_for_each_entry_*()
> macro in the future it should be avoided to use the list iterator
> variable after the loop body.
> 
> To *never* use the list iterator variable after the loop it was
> concluded to use a separate iterator variable instead of a
> found boolean [1].
> 
> This removes the need to use a found variable and simply checking if
> the variable was set, can determine if the break/goto was hit.
> 
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 64f4fdd02902..f254f537c357 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1381,28 +1381,27 @@ static int mv88e6xxx_set_mac_eee(struct dsa_switch *ds, int port,
>  /* Mask of the local ports allowed to receive frames from a given fabric port */
>  static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>  {
> +	struct dsa_port *dp = NULL, *iter, *other_dp;
>  	struct dsa_switch *ds = chip->ds;
>  	struct dsa_switch_tree *dst = ds->dst;
> -	struct dsa_port *dp, *other_dp;
> -	bool found = false;
>  	u16 pvlan;
>  
>  	/* dev is a physical switch */
>  	if (dev <= dst->last_switch) {
> -		list_for_each_entry(dp, &dst->ports, list) {
> -			if (dp->ds->index == dev && dp->index == port) {
> -				/* dp might be a DSA link or a user port, so it
> +		list_for_each_entry(iter, &dst->ports, list) {
> +			if (iter->ds->index == dev && iter->index == port) {
> +				/* iter might be a DSA link or a user port, so it
>  				 * might or might not have a bridge.
> -				 * Use the "found" variable for both cases.
> +				 * Set the "dp" variable for both cases.
>  				 */
> -				found = true;
> +				dp = iter;
>  				break;
>  			}
>  		}
>  	/* dev is a virtual bridge */
>  	} else {
> -		list_for_each_entry(dp, &dst->ports, list) {
> -			unsigned int bridge_num = dsa_port_bridge_num_get(dp);
> +		list_for_each_entry(iter, &dst->ports, list) {
> +			unsigned int bridge_num = dsa_port_bridge_num_get(iter);
>  
>  			if (!bridge_num)
>  				continue;
> @@ -1410,13 +1409,13 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>  			if (bridge_num + dst->last_switch != dev)
>  				continue;
>  
> -			found = true;
> +			dp = iter;
>  			break;
>  		}
>  	}
>  
>  	/* Prevent frames from unknown switch or virtual bridge */
> -	if (!found)
> +	if (!dp)
>  		return 0;
>  
>  	/* Frames from DSA links and CPU ports can egress any local port */
> -- 
> 2.25.1
> 

Let's try to not make convoluted code worse. Do the following 2 patches
achieve what you are looking for? Originally I had a single patch (what
is now 2/2) but I figured it would be cleaner to break out the unrelated
change into what is now 1/2.

If you want I can submit these changes separately.

-----------------------------[ cut here ]-----------------------------
From 2d84ecd87566b1535a04526b4ebb2764e764625f Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 8 Apr 2022 15:15:30 +0300
Subject: [PATCH 1/2] net: dsa: mv88e6xxx: remove redundant check in
 mv88e6xxx_port_vlan()

We know that "dev > dst->last_switch" in the "else" block.
In other words, that "dev - dst->last_switch" is > 0.

dsa_port_bridge_num_get(dp) can be 0, but the check
"if (bridge_num + dst->last_switch != dev) continue", rewritten as
"if (bridge_num != dev - dst->last_switch) continue", aka
"if (bridge_num != something which cannot be 0) continue",
makes it redundant to have the extra "if (!bridge_num) continue" logic,
since a bridge_num of zero would have been skipped anyway.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 64f4fdd02902..b3aa0e5bc842 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1404,9 +1404,6 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
 		list_for_each_entry(dp, &dst->ports, list) {
 			unsigned int bridge_num = dsa_port_bridge_num_get(dp);
 
-			if (!bridge_num)
-				continue;
-
 			if (bridge_num + dst->last_switch != dev)
 				continue;
 
-----------------------------[ cut here ]-----------------------------

-----------------------------[ cut here ]-----------------------------
From dabafdbe38b408f7c563ad91fc6e57791055fed7 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 8 Apr 2022 14:57:45 +0300
Subject: [PATCH 2/2] net: dsa: mv88e6xxx: refactor mv88e6xxx_port_vlan()

To avoid bugs and speculative execution exploits due to type-confused
pointers at the end of a list_for_each_entry() loop, one measure is to
restrict code to not use the iterator variable outside the loop block.

In the case of mv88e6xxx_port_vlan(), this isn't a problem, as we never
let the loops exit through "natural causes" anyway, by using a "found"
variable and then using the last "dp" iterator prior to the break, which
is a safe thing to do.

Nonetheless, with the expected new syntax, this pattern will no longer
be possible.

Profit off of the occasion and break the two port finding methods into
smaller sub-functions. Somehow, returning a copy of the iterator pointer
is still accepted.

This change makes it redundant to have a "bool found", since the "dp"
from mv88e6xxx_port_vlan() now holds NULL if we haven't found what we
were looking for.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 54 ++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index b3aa0e5bc842..1f35e89053e6 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1378,42 +1378,50 @@ static int mv88e6xxx_set_mac_eee(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static struct dsa_port *mv88e6xxx_find_port(struct dsa_switch_tree *dst,
+					    int sw_index, int port)
+{
+	struct dsa_port *dp;
+
+	list_for_each_entry(dp, &dst->ports, list)
+		if (dp->ds->index == sw_index && dp->index == port)
+			return dp;
+
+	return NULL;
+}
+
+static struct dsa_port *
+mv88e6xxx_find_port_by_bridge_num(struct dsa_switch_tree *dst,
+				  unsigned int bridge_num)
+{
+	struct dsa_port *dp;
+
+	list_for_each_entry(dp, &dst->ports, list)
+		if (dsa_port_bridge_num_get(dp) == bridge_num)
+			return dp;
+
+	return NULL;
+}
+
 /* Mask of the local ports allowed to receive frames from a given fabric port */
 static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
 {
 	struct dsa_switch *ds = chip->ds;
 	struct dsa_switch_tree *dst = ds->dst;
 	struct dsa_port *dp, *other_dp;
-	bool found = false;
 	u16 pvlan;
 
-	/* dev is a physical switch */
 	if (dev <= dst->last_switch) {
-		list_for_each_entry(dp, &dst->ports, list) {
-			if (dp->ds->index == dev && dp->index == port) {
-				/* dp might be a DSA link or a user port, so it
-				 * might or might not have a bridge.
-				 * Use the "found" variable for both cases.
-				 */
-				found = true;
-				break;
-			}
-		}
-	/* dev is a virtual bridge */
+		/* dev is a physical switch */
+		dp = mv88e6xxx_find_port(dst, dev, port);
 	} else {
-		list_for_each_entry(dp, &dst->ports, list) {
-			unsigned int bridge_num = dsa_port_bridge_num_get(dp);
-
-			if (bridge_num + dst->last_switch != dev)
-				continue;
-
-			found = true;
-			break;
-		}
+		/* dev is a virtual bridge */
+		dp = mv88e6xxx_find_port_by_bridge_num(dst,
+						       dev - dst->last_switch);
 	}
 
 	/* Prevent frames from unknown switch or virtual bridge */
-	if (!found)
+	if (!dp)
 		return 0;
 
 	/* Frames from DSA links and CPU ports can egress any local port */
-----------------------------[ cut here ]-----------------------------

Re: [PATCH net-next 03/15] net: dsa: mv88e6xxx: Replace usage of found with dedicated iterator

[Jakob Koschel]

Hi Vladimir,

> On 8. Apr 2022, at 14:31, Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> Hi Jakob,
> 
> On Thu, Apr 07, 2022 at 12:28:48PM +0200, Jakob Koschel wrote:
>> To move the list iterator variable into the list_for_each_entry_*()
>> macro in the future it should be avoided to use the list iterator
>> variable after the loop body.
>> 
>> To *never* use the list iterator variable after the loop it was
>> concluded to use a separate iterator variable instead of a
>> found boolean [1].
>> 
>> This removes the need to use a found variable and simply checking if
>> the variable was set, can determine if the break/goto was hit.
>> 
>> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
>> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
>> ---
>> drivers/net/dsa/mv88e6xxx/chip.c | 21 ++++++++++-----------
>> 1 file changed, 10 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 64f4fdd02902..f254f537c357 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -1381,28 +1381,27 @@ static int mv88e6xxx_set_mac_eee(struct dsa_switch *ds, int port,
>> /* Mask of the local ports allowed to receive frames from a given fabric port */
>> static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>> {
>> +	struct dsa_port *dp = NULL, *iter, *other_dp;
>> 	struct dsa_switch *ds = chip->ds;
>> 	struct dsa_switch_tree *dst = ds->dst;
>> -	struct dsa_port *dp, *other_dp;
>> -	bool found = false;
>> 	u16 pvlan;
>> 
>> 	/* dev is a physical switch */
>> 	if (dev <= dst->last_switch) {
>> -		list_for_each_entry(dp, &dst->ports, list) {
>> -			if (dp->ds->index == dev && dp->index == port) {
>> -				/* dp might be a DSA link or a user port, so it
>> +		list_for_each_entry(iter, &dst->ports, list) {
>> +			if (iter->ds->index == dev && iter->index == port) {
>> +				/* iter might be a DSA link or a user port, so it
>> 				 * might or might not have a bridge.
>> -				 * Use the "found" variable for both cases.
>> +				 * Set the "dp" variable for both cases.
>> 				 */
>> -				found = true;
>> +				dp = iter;
>> 				break;
>> 			}
>> 		}
>> 	/* dev is a virtual bridge */
>> 	} else {
>> -		list_for_each_entry(dp, &dst->ports, list) {
>> -			unsigned int bridge_num = dsa_port_bridge_num_get(dp);
>> +		list_for_each_entry(iter, &dst->ports, list) {
>> +			unsigned int bridge_num = dsa_port_bridge_num_get(iter);
>> 
>> 			if (!bridge_num)
>> 				continue;
>> @@ -1410,13 +1409,13 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>> 			if (bridge_num + dst->last_switch != dev)
>> 				continue;
>> 
>> -			found = true;
>> +			dp = iter;
>> 			break;
>> 		}
>> 	}
>> 
>> 	/* Prevent frames from unknown switch or virtual bridge */
>> -	if (!found)
>> +	if (!dp)
>> 		return 0;
>> 
>> 	/* Frames from DSA links and CPU ports can egress any local port */
>> -- 
>> 2.25.1
>> 
> 
> Let's try to not make convoluted code worse. Do the following 2 patches
> achieve what you are looking for? Originally I had a single patch (what
> is now 2/2) but I figured it would be cleaner to break out the unrelated
> change into what is now 1/2.

I do agree with not making convoluted code worse, but I was reluctant with
e.g. introducing new functions for this because others essentially
have the opposite opinion on this.

I however like solving it that way, it makes it a lot cleaner.

> 
> If you want I can submit these changes separately.

Sure if you want to submit them separately, go ahead. Otherwise I can
integrate it into a v2, whatever you prefer essentially.

> 
> -----------------------------[ cut here ]-----------------------------
> From 2d84ecd87566b1535a04526b4ebb2764e764625f Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Fri, 8 Apr 2022 15:15:30 +0300
> Subject: [PATCH 1/2] net: dsa: mv88e6xxx: remove redundant check in
> mv88e6xxx_port_vlan()
> 
> We know that "dev > dst->last_switch" in the "else" block.
> In other words, that "dev - dst->last_switch" is > 0.
> 
> dsa_port_bridge_num_get(dp) can be 0, but the check
> "if (bridge_num + dst->last_switch != dev) continue", rewritten as
> "if (bridge_num != dev - dst->last_switch) continue", aka
> "if (bridge_num != something which cannot be 0) continue",
> makes it redundant to have the extra "if (!bridge_num) continue" logic,
> since a bridge_num of zero would have been skipped anyway.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> drivers/net/dsa/mv88e6xxx/chip.c | 3 ---
> 1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 64f4fdd02902..b3aa0e5bc842 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1404,9 +1404,6 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
> 		list_for_each_entry(dp, &dst->ports, list) {
> 			unsigned int bridge_num = dsa_port_bridge_num_get(dp);
> 
> -			if (!bridge_num)
> -				continue;
> -
> 			if (bridge_num + dst->last_switch != dev)
> 				continue;
> 
> -----------------------------[ cut here ]-----------------------------
> 
> -----------------------------[ cut here ]-----------------------------
> From dabafdbe38b408f7c563ad91fc6e57791055fed7 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Fri, 8 Apr 2022 14:57:45 +0300
> Subject: [PATCH 2/2] net: dsa: mv88e6xxx: refactor mv88e6xxx_port_vlan()
> 
> To avoid bugs and speculative execution exploits due to type-confused
> pointers at the end of a list_for_each_entry() loop, one measure is to
> restrict code to not use the iterator variable outside the loop block.
> 
> In the case of mv88e6xxx_port_vlan(), this isn't a problem, as we never
> let the loops exit through "natural causes" anyway, by using a "found"
> variable and then using the last "dp" iterator prior to the break, which
> is a safe thing to do.
> 
> Nonetheless, with the expected new syntax, this pattern will no longer
> be possible.
> 
> Profit off of the occasion and break the two port finding methods into
> smaller sub-functions. Somehow, returning a copy of the iterator pointer
> is still accepted.
> 
> This change makes it redundant to have a "bool found", since the "dp"
> from mv88e6xxx_port_vlan() now holds NULL if we haven't found what we
> were looking for.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> drivers/net/dsa/mv88e6xxx/chip.c | 54 ++++++++++++++++++--------------
> 1 file changed, 31 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index b3aa0e5bc842..1f35e89053e6 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1378,42 +1378,50 @@ static int mv88e6xxx_set_mac_eee(struct dsa_switch *ds, int port,
> 	return 0;
> }
> 
> +static struct dsa_port *mv88e6xxx_find_port(struct dsa_switch_tree *dst,
> +					    int sw_index, int port)
> +{
> +	struct dsa_port *dp;
> +
> +	list_for_each_entry(dp, &dst->ports, list)
> +		if (dp->ds->index == sw_index && dp->index == port)
> +			return dp;
> +
> +	return NULL;
> +}
> +
> +static struct dsa_port *
> +mv88e6xxx_find_port_by_bridge_num(struct dsa_switch_tree *dst,
> +				  unsigned int bridge_num)
> +{
> +	struct dsa_port *dp;
> +
> +	list_for_each_entry(dp, &dst->ports, list)
> +		if (dsa_port_bridge_num_get(dp) == bridge_num)
> +			return dp;
> +
> +	return NULL;
> +}
> +
> /* Mask of the local ports allowed to receive frames from a given fabric port */
> static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
> {
> 	struct dsa_switch *ds = chip->ds;
> 	struct dsa_switch_tree *dst = ds->dst;
> 	struct dsa_port *dp, *other_dp;
> -	bool found = false;
> 	u16 pvlan;
> 
> -	/* dev is a physical switch */
> 	if (dev <= dst->last_switch) {
> -		list_for_each_entry(dp, &dst->ports, list) {
> -			if (dp->ds->index == dev && dp->index == port) {
> -				/* dp might be a DSA link or a user port, so it
> -				 * might or might not have a bridge.
> -				 * Use the "found" variable for both cases.
> -				 */
> -				found = true;
> -				break;
> -			}
> -		}
> -	/* dev is a virtual bridge */
> +		/* dev is a physical switch */
> +		dp = mv88e6xxx_find_port(dst, dev, port);
> 	} else {
> -		list_for_each_entry(dp, &dst->ports, list) {
> -			unsigned int bridge_num = dsa_port_bridge_num_get(dp);
> -
> -			if (bridge_num + dst->last_switch != dev)
> -				continue;
> -
> -			found = true;
> -			break;
> -		}
> +		/* dev is a virtual bridge */
> +		dp = mv88e6xxx_find_port_by_bridge_num(dst,
> +						       dev - dst->last_switch);
> 	}
> 
> 	/* Prevent frames from unknown switch or virtual bridge */
> -	if (!found)
> +	if (!dp)
> 		return 0;
> 
> 	/* Frames from DSA links and CPU ports can egress any local port */
> -----------------------------[ cut here ]-----------------------------

Thanks,
Jakob

Re: [PATCH net-next 03/15] net: dsa: mv88e6xxx: Replace usage of found with dedicated iterator

[Vladimir Oltean]

On Sat, Apr 09, 2022 at 01:44:00AM +0200, Jakob Koschel wrote:
> > Let's try to not make convoluted code worse. Do the following 2 patches
> > achieve what you are looking for? Originally I had a single patch (what
> > is now 2/2) but I figured it would be cleaner to break out the unrelated
> > change into what is now 1/2.
> 
> I do agree with not making convoluted code worse, but I was reluctant with
> e.g. introducing new functions for this because others essentially
> have the opposite opinion on this.
> 
> I however like solving it that way, it makes it a lot cleaner.

Yeah, I think 'just adapt to the context and style and intentions of the
code you're changing and don't try to push a robotic one-size-fits-all
solution' is sensible enough for an initial guiding principle.

> > If you want I can submit these changes separately.
> 
> Sure if you want to submit them separately, go ahead. Otherwise I can
> integrate it into a v2, whatever you prefer essentially.

If you're moving quickly feel free to pick them up. I have lots of other
things on my backlog so it won't be until late next week until I even
consider submitting these.

Re: [PATCH net-next 03/15] net: dsa: mv88e6xxx: Replace usage of found with dedicated iterator

[Jakob Koschel]

> On 9. Apr 2022, at 01:50, Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> On Sat, Apr 09, 2022 at 01:44:00AM +0200, Jakob Koschel wrote:
>>> Let's try to not make convoluted code worse. Do the following 2 patches
>>> achieve what you are looking for? Originally I had a single patch (what
>>> is now 2/2) but I figured it would be cleaner to break out the unrelated
>>> change into what is now 1/2.
>> 
>> I do agree with not making convoluted code worse, but I was reluctant with
>> e.g. introducing new functions for this because others essentially
>> have the opposite opinion on this.
>> 
>> I however like solving it that way, it makes it a lot cleaner.
> 
> Yeah, I think 'just adapt to the context and style and intentions of the
> code you're changing and don't try to push a robotic one-size-fits-all
> solution' is sensible enough for an initial guiding principle.
> 
>>> If you want I can submit these changes separately.
>> 
>> Sure if you want to submit them separately, go ahead. Otherwise I can
>> integrate it into a v2, whatever you prefer essentially.
> 
> If you're moving quickly feel free to pick them up. I have lots of other
> things on my backlog so it won't be until late next week until I even
> consider submitting these.

I'm planning to send a v2 earlier than that, so I'll just integrate it there.

Thanks,
Jakob

[PATCH net-next 04/15] net: dsa: Replace usage of found with dedicated list iterator variable

[Jakob Koschel]

To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.

To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].

This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 net/dsa/dsa.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 89c6c86e746f..645522c4dd4a 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -112,22 +112,21 @@ const struct dsa_device_ops *dsa_find_tagger_by_name(const char *buf)
 
 const struct dsa_device_ops *dsa_tag_driver_get(int tag_protocol)
 {
-	struct dsa_tag_driver *dsa_tag_driver;
+	struct dsa_tag_driver *dsa_tag_driver = NULL, *iter;
 	const struct dsa_device_ops *ops;
-	bool found = false;
 
 	request_module("%s%d", DSA_TAG_DRIVER_ALIAS, tag_protocol);
 
 	mutex_lock(&dsa_tag_drivers_lock);
-	list_for_each_entry(dsa_tag_driver, &dsa_tag_drivers_list, list) {
-		ops = dsa_tag_driver->ops;
+	list_for_each_entry(iter, &dsa_tag_drivers_list, list) {
+		ops = iter->ops;
 		if (ops->proto == tag_protocol) {
-			found = true;
+			dsa_tag_driver = iter;
 			break;
 		}
 	}
 
-	if (found) {
+	if (dsa_tag_driver) {
 		if (!try_module_get(dsa_tag_driver->owner))
 			ops = ERR_PTR(-ENOPROTOOPT);
 	} else {
-- 
2.25.1

[PATCH net-next 05/15] net: sparx5: Replace usage of found with dedicated list iterator variable

[Jakob Koschel]

To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.

To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].

This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 .../microchip/sparx5/sparx5_mactable.c        | 25 +++++++++----------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
index a5837dbe0c7e..bb8d9ce79ac2 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
@@ -362,8 +362,7 @@ static void sparx5_mact_handle_entry(struct sparx5 *sparx5,
 				     unsigned char mac[ETH_ALEN],
 				     u16 vid, u32 cfg2)
 {
-	struct sparx5_mact_entry *mact_entry;
-	bool found = false;
+	struct sparx5_mact_entry *mact_entry = NULL, *iter;
 	u16 port;
 
 	if (LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_TYPE_GET(cfg2) !=
@@ -378,28 +377,28 @@ static void sparx5_mact_handle_entry(struct sparx5 *sparx5,
 		return;
 
 	mutex_lock(&sparx5->mact_lock);
-	list_for_each_entry(mact_entry, &sparx5->mact_entries, list) {
-		if (mact_entry->vid == vid &&
-		    ether_addr_equal(mac, mact_entry->mac)) {
-			found = true;
-			mact_entry->flags |= MAC_ENT_ALIVE;
-			if (mact_entry->port != port) {
+	list_for_each_entry(iter, &sparx5->mact_entries, list) {
+		if (iter->vid == vid &&
+		    ether_addr_equal(mac, iter->mac)) {
+			iter->flags |= MAC_ENT_ALIVE;
+			if (iter->port != port) {
 				dev_warn(sparx5->dev, "Entry move: %d -> %d\n",
-					 mact_entry->port, port);
-				mact_entry->port = port;
-				mact_entry->flags |= MAC_ENT_MOVED;
+					 iter->port, port);
+				iter->port = port;
+				iter->flags |= MAC_ENT_MOVED;
 			}
 			/* Entry handled */
+			mact_entry = iter;
 			break;
 		}
 	}
 	mutex_unlock(&sparx5->mact_lock);
 
-	if (found && !(mact_entry->flags & MAC_ENT_MOVED))
+	if (mact_entry && !(mact_entry->flags & MAC_ENT_MOVED))
 		/* Present, not moved */
 		return;
 
-	if (!found) {
+	if (!mact_entry) {
 		/* Entry not found - now add */
 		mact_entry = alloc_mact_entry(sparx5, mac, vid, port);
 		if (!mact_entry)
-- 
2.25.1

[PATCH net-next 06/15] qed: Use dedicated list iterator variable

[Jakob Koschel]

To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.

To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable [1].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/ethernet/qlogic/qed/qed_dev.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index 672480c9d195..e920e7dcf66a 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -174,7 +174,7 @@ int qed_db_recovery_add(struct qed_dev *cdev,
 int qed_db_recovery_del(struct qed_dev *cdev,
 			void __iomem *db_addr, void *db_data)
 {
-	struct qed_db_recovery_entry *db_entry = NULL;
+	struct qed_db_recovery_entry *db_entry = NULL, *iter;
 	struct qed_hwfn *p_hwfn;
 	int rc = -EINVAL;
 
@@ -190,12 +190,13 @@ int qed_db_recovery_del(struct qed_dev *cdev,
 
 	/* Protect the list */
 	spin_lock_bh(&p_hwfn->db_recovery_info.lock);
-	list_for_each_entry(db_entry,
+	list_for_each_entry(iter,
 			    &p_hwfn->db_recovery_info.list, list_entry) {
 		/* search according to db_data addr since db_addr is not unique (roce) */
-		if (db_entry->db_data == db_data) {
-			qed_db_recovery_dp_entry(p_hwfn, db_entry, "Deleting");
-			list_del(&db_entry->list_entry);
+		if (iter->db_data == db_data) {
+			qed_db_recovery_dp_entry(p_hwfn, iter, "Deleting");
+			list_del(&iter->list_entry);
+			db_entry = iter;
 			rc = 0;
 			break;
 		}
-- 
2.25.1

[PATCH net-next 07/15] qed: Replace usage of found with dedicated list iterator variable

[Jakob Koschel]

To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.

To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].

This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 26 ++++++++++-----------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
index 1d1d4caad680..198c9321bf51 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
@@ -1630,38 +1630,36 @@ static struct qed_iwarp_listener *
 qed_iwarp_get_listener(struct qed_hwfn *p_hwfn,
 		       struct qed_iwarp_cm_info *cm_info)
 {
-	struct qed_iwarp_listener *listener = NULL;
+	struct qed_iwarp_listener *listener = NULL, *iter;
 	static const u32 ip_zero[4] = { 0, 0, 0, 0 };
-	bool found = false;
 
-	list_for_each_entry(listener,
+	list_for_each_entry(iter,
 			    &p_hwfn->p_rdma_info->iwarp.listen_list,
 			    list_entry) {
-		if (listener->port == cm_info->local_port) {
-			if (!memcmp(listener->ip_addr,
+		if (iter->port == cm_info->local_port) {
+			if (!memcmp(iter->ip_addr,
 				    ip_zero, sizeof(ip_zero))) {
-				found = true;
+				listener = iter;
 				break;
 			}
 
-			if (!memcmp(listener->ip_addr,
+			if (!memcmp(iter->ip_addr,
 				    cm_info->local_ip,
 				    sizeof(cm_info->local_ip)) &&
-			    (listener->vlan == cm_info->vlan)) {
-				found = true;
+			    iter->vlan == cm_info->vlan) {
+				listener = iter;
 				break;
 			}
 		}
 	}
 
-	if (found) {
+	if (listener)
 		DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener found = %p\n",
 			   listener);
-		return listener;
-	}
+	else
+		DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener not found\n");
 
-	DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener not found\n");
-	return NULL;
+	return listener;
 }
 
 static int
-- 
2.25.1

[PATCH net-next 08/15] qed: Remove usage of list iterator variable after the loop

[Jakob Koschel]

To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.

Since "found" and "p_ent" need to be equal, "found" should be used
consistently to limit the scope of "p_ent" to the list traversal in
the future.

Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/ethernet/qlogic/qed/qed_spq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_spq.c b/drivers/net/ethernet/qlogic/qed/qed_spq.c
index d01b9245f811..cbaa2abed660 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_spq.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_spq.c
@@ -934,10 +934,10 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
 		       u8 fw_return_code,
 		       union event_ring_data *p_data)
 {
+	struct qed_spq_entry	*found = NULL;
 	struct qed_spq		*p_spq;
-	struct qed_spq_entry	*p_ent = NULL;
+	struct qed_spq_entry	*p_ent;
 	struct qed_spq_entry	*tmp;
-	struct qed_spq_entry	*found = NULL;
 
 	if (!p_hwfn)
 		return -EINVAL;
@@ -980,7 +980,7 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
 	DP_VERBOSE(p_hwfn, QED_MSG_SPQ,
 		   "Complete EQE [echo %04x]: func %p cookie %p)\n",
 		   le16_to_cpu(echo),
-		   p_ent->comp_cb.function, p_ent->comp_cb.cookie);
+		   found->comp_cb.function, found->comp_cb.cookie);
 	if (found->comp_cb.function)
 		found->comp_cb.function(p_hwfn, found->comp_cb.cookie, p_data,
 					fw_return_code);
-- 
2.25.1

[PATCH net-next 09/15] net: qede: Replace usage of found with dedicated list iterator variable

[Jakob Koschel]

To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.

To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].

This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/ethernet/qlogic/qede/qede_rdma.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_rdma.c b/drivers/net/ethernet/qlogic/qede/qede_rdma.c
index 6304514a6f2c..2eb03ffe2484 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_rdma.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_rdma.c
@@ -246,18 +246,17 @@ static void qede_rdma_change_mtu(struct qede_dev *edev)
 static struct qede_rdma_event_work *
 qede_rdma_get_free_event_node(struct qede_dev *edev)
 {
-	struct qede_rdma_event_work *event_node = NULL;
-	bool found = false;
+	struct qede_rdma_event_work *event_node = NULL, *iter;
 
-	list_for_each_entry(event_node, &edev->rdma_info.rdma_event_list,
+	list_for_each_entry(iter, &edev->rdma_info.rdma_event_list,
 			    list) {
-		if (!work_pending(&event_node->work)) {
-			found = true;
+		if (!work_pending(&iter->work)) {
+			event_node = iter;
 			break;
 		}
 	}
 
-	if (!found) {
+	if (!event_node) {
 		event_node = kzalloc(sizeof(*event_node), GFP_ATOMIC);
 		if (!event_node) {
 			DP_NOTICE(edev,
-- 
2.25.1

[PATCH net-next 10/15] net: qede: Remove check of list iterator against head past the loop body

[Jakob Koschel]

When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.

While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
&pos->member == head, using the iterator variable after the loop should
be avoided.

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].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/ethernet/qlogic/qede/qede_filter.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 3010833ddde3..3d167e37e654 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -829,18 +829,21 @@ int qede_configure_vlan_filters(struct qede_dev *edev)
 int qede_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
 {
 	struct qede_dev *edev = netdev_priv(dev);
-	struct qede_vlan *vlan;
+	struct qede_vlan *vlan = NULL;
+	struct qede_vlan *iter;
 	int rc = 0;
 
 	DP_VERBOSE(edev, NETIF_MSG_IFDOWN, "Removing vlan 0x%04x\n", vid);
 
 	/* Find whether entry exists */
 	__qede_lock(edev);
-	list_for_each_entry(vlan, &edev->vlan_list, list)
-		if (vlan->vid == vid)
+	list_for_each_entry(iter, &edev->vlan_list, list)
+		if (iter->vid == vid) {
+			vlan = iter;
 			break;
+		}
 
-	if (list_entry_is_head(vlan, &edev->vlan_list, list)) {
+	if (!vlan) {
 		DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN),
 			   "Vlan isn't configured\n");
 		goto out;
-- 
2.25.1

[PATCH net-next 11/15] sfc: Remove usage of list iterator for list_add() after the loop body

[Jakob Koschel]

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@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/ethernet/sfc/rx_common.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
index 1b22c7be0088..a8822152ff83 100644
--- a/drivers/net/ethernet/sfc/rx_common.c
+++ b/drivers/net/ethernet/sfc/rx_common.c
@@ -563,8 +563,10 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
 
 	/* Search for first gap in the numbering */
 	list_for_each_entry(ctx, head, list) {
-		if (ctx->user_id != id)
+		if (ctx->user_id != id) {
+			head = &ctx->list;
 			break;
+		}
 		id++;
 		/* Check for wrap.  If this happens, we have nearly 2^32
 		 * allocated RSS contexts, which seems unlikely.
@@ -582,7 +584,7 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
 
 	/* Insert the new entry into the gap */
 	new->user_id = id;
-	list_add_tail(&new->list, &ctx->list);
+	list_add_tail(&new->list, head);
 	return new;
 }
 
-- 
2.25.1

Re: [PATCH net-next 11/15] sfc: Remove usage of list iterator for list_add() after the loop body

[Edward Cree]

On 07/04/2022 11:28, 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@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>

The commit message doesn't accurately describe the patch; it states
 that "the list_add() is performed within the loop", which doesn't
 appear to be the case.
Also it seems a bit subtle to use `head` as both the head of the
 list to iterate over and the found entry/gap to insert before; a
 comment explaining that wouldn't go amiss.
(I'd question whether this change is really an improvement in this
 case, where the iterator really does hold the thing we want at the
 end of the search and so there's no if(found) special-casing —
 we're not even abusing the type system, because efx->rss_context
 is of the same type as all the list entries, so ctx really is a
 valid pointer and there shouldn't be any issues with speculative
 accesses or whatever — but it seems Linus has already pronounced
 in favour of the scope limiting, and far be it from me to gainsay
 him.)

-ed

> ---
>  drivers/net/ethernet/sfc/rx_common.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
> index 1b22c7be0088..a8822152ff83 100644
> --- a/drivers/net/ethernet/sfc/rx_common.c
> +++ b/drivers/net/ethernet/sfc/rx_common.c
> @@ -563,8 +563,10 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
>  
>  	/* Search for first gap in the numbering */
>  	list_for_each_entry(ctx, head, list) {
> -		if (ctx->user_id != id)
> +		if (ctx->user_id != id) {
> +			head = &ctx->list;
>  			break;
> +		}
>  		id++;
>  		/* Check for wrap.  If this happens, we have nearly 2^32
>  		 * allocated RSS contexts, which seems unlikely.
> @@ -582,7 +584,7 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
>  
>  	/* Insert the new entry into the gap */
>  	new->user_id = id;
> -	list_add_tail(&new->list, &ctx->list);
> +	list_add_tail(&new->list, head);
>  	return new;
>  }
>  
> 

Re: [PATCH net-next 11/15] sfc: Remove usage of list iterator for list_add() after the loop body

[Jakob Koschel]

Hello Edward,

> On 7. Apr 2022, at 19:42, Edward Cree <ecree.xilinx@gmail.com> wrote:
> 
> On 07/04/2022 11:28, 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@mail.gmail.com/ [1]
>> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> 
> The commit message doesn't accurately describe the patch; it states
> that "the list_add() is performed within the loop", which doesn't
> appear to be the case.

you're right, I've changed the code last minute. I'll make sure
the changelog reflects the actual behaviour here. Thanks for the
input!

> Also it seems a bit subtle to use `head` as both the head of the
> list to iterate over and the found entry/gap to insert before; a
> comment explaining that wouldn't go amiss.

Also a good point, I'll add a comment as well, or perhaps using
a separate 'struct list_head *pos' variable is even cleaner.

> (I'd question whether this change is really an improvement in this
> case, where the iterator really does hold the thing we want at the
> end of the search and so there's no if(found) special-casing —
> we're not even abusing the type system, because efx->rss_context
> is of the same type as all the list entries, so ctx really is a
> valid pointer and there shouldn't be any issues with speculative
> accesses or whatever — but it seems Linus has already pronounced
> in favour of the scope limiting, and far be it from me to gainsay
> him.)

So, since the head is included in the struct of the same type as
the element, it really doesn't make much of a difference here.
It will always be safe to use.

But this is the very rare exception. There are other benefits of
avoiding the use of list iterator after the loop. One of them
is scope limiting but you also might want to set the iterator
variable to a "safe value" before the processor might execute an additional
iteration in speculative execution on the 'bogus' head element.

If you do these kind of patches on the list macros, you need to make sure
they work for all the uses, including the safe ones (like this one).

> 
> -ed
> 
>> ---
>> drivers/net/ethernet/sfc/rx_common.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
>> index 1b22c7be0088..a8822152ff83 100644
>> --- a/drivers/net/ethernet/sfc/rx_common.c
>> +++ b/drivers/net/ethernet/sfc/rx_common.c
>> @@ -563,8 +563,10 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
>> 
>> 	/* Search for first gap in the numbering */
>> 	list_for_each_entry(ctx, head, list) {
>> -		if (ctx->user_id != id)
>> +		if (ctx->user_id != id) {
>> +			head = &ctx->list;
>> 			break;
>> +		}
>> 		id++;
>> 		/* Check for wrap.  If this happens, we have nearly 2^32
>> 		 * allocated RSS contexts, which seems unlikely.
>> @@ -582,7 +584,7 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
>> 
>> 	/* Insert the new entry into the gap */
>> 	new->user_id = id;
>> -	list_add_tail(&new->list, &ctx->list);
>> +	list_add_tail(&new->list, head);
>> 	return new;
>> }
>> 
>> 
> 

Thanks,
Jakob

[PATCH net-next 12/15] net: netcp: Remove usage of list iterator for list_add() after loop body

[Jakob Koschel]

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@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/ethernet/ti/netcp_core.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 16507bff652a..7f89fd82ecc8 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -471,8 +471,8 @@ struct netcp_hook_list {
 int netcp_register_txhook(struct netcp_intf *netcp_priv, int order,
 			  netcp_hook_rtn *hook_rtn, void *hook_data)
 {
+	struct netcp_hook_list *next = NULL, *iter;
 	struct netcp_hook_list *entry;
-	struct netcp_hook_list *next;
 	unsigned long flags;
 
 	entry = devm_kzalloc(netcp_priv->dev, sizeof(*entry), GFP_KERNEL);
@@ -484,11 +484,15 @@ int netcp_register_txhook(struct netcp_intf *netcp_priv, int order,
 	entry->order     = order;
 
 	spin_lock_irqsave(&netcp_priv->lock, flags);
-	list_for_each_entry(next, &netcp_priv->txhook_list_head, list) {
-		if (next->order > order)
+	list_for_each_entry(iter, &netcp_priv->txhook_list_head, list) {
+		if (iter->order > order) {
+			next = iter;
+			list_add_tail(&entry->list, &iter->list);
 			break;
+		}
 	}
-	__list_add(&entry->list, next->list.prev, &next->list);
+	if (!next)
+		list_add_tail(&entry->list, &netcp_priv->txhook_list_head);
 	spin_unlock_irqrestore(&netcp_priv->lock, flags);
 
 	return 0;
@@ -520,8 +524,8 @@ EXPORT_SYMBOL_GPL(netcp_unregister_txhook);
 int netcp_register_rxhook(struct netcp_intf *netcp_priv, int order,
 			  netcp_hook_rtn *hook_rtn, void *hook_data)
 {
+	struct netcp_hook_list *next = NULL, *iter;
 	struct netcp_hook_list *entry;
-	struct netcp_hook_list *next;
 	unsigned long flags;
 
 	entry = devm_kzalloc(netcp_priv->dev, sizeof(*entry), GFP_KERNEL);
@@ -533,11 +537,15 @@ int netcp_register_rxhook(struct netcp_intf *netcp_priv, int order,
 	entry->order     = order;
 
 	spin_lock_irqsave(&netcp_priv->lock, flags);
-	list_for_each_entry(next, &netcp_priv->rxhook_list_head, list) {
-		if (next->order > order)
+	list_for_each_entry(iter, &netcp_priv->rxhook_list_head, list) {
+		if (iter->order > order) {
+			next = iter;
+			list_add_tail(&entry->list, &iter->list);
 			break;
+		}
 	}
-	__list_add(&entry->list, next->list.prev, &next->list);
+	if (!next)
+		list_add_tail(&entry->list, &netcp_priv->rxhook_list_head);
 	spin_unlock_irqrestore(&netcp_priv->lock, flags);
 
 	return 0;
-- 
2.25.1

[PATCH net-next 13/15] ps3_gelic: Replace usage of found with dedicated list iterator variable

[Jakob Koschel]

To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.

To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].

This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 .../net/ethernet/toshiba/ps3_gelic_wireless.c | 30 +++++++++----------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
index dc14a66583ff..c8a016c902cd 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
@@ -1495,14 +1495,14 @@ static int gelic_wl_start_scan(struct gelic_wl_info *wl, int always_scan,
  */
 static void gelic_wl_scan_complete_event(struct gelic_wl_info *wl)
 {
+	struct gelic_wl_scan_info *target = NULL, *iter, *tmp;
 	struct gelic_eurus_cmd *cmd = NULL;
-	struct gelic_wl_scan_info *target, *tmp;
 	struct gelic_wl_scan_info *oldest = NULL;
 	struct gelic_eurus_scan_info *scan_info;
 	unsigned int scan_info_size;
 	union iwreq_data data;
 	unsigned long this_time = jiffies;
-	unsigned int data_len, i, found, r;
+	unsigned int data_len, i, r;
 	void *buf;
 
 	pr_debug("%s:start\n", __func__);
@@ -1539,14 +1539,14 @@ static void gelic_wl_scan_complete_event(struct gelic_wl_info *wl)
 	wl->scan_stat = GELIC_WL_SCAN_STAT_GOT_LIST;
 
 	/* mark all entries are old */
-	list_for_each_entry_safe(target, tmp, &wl->network_list, list) {
-		target->valid = 0;
+	list_for_each_entry_safe(iter, tmp, &wl->network_list, list) {
+		iter->valid = 0;
 		/* expire too old entries */
-		if (time_before(target->last_scanned + wl->scan_age,
+		if (time_before(iter->last_scanned + wl->scan_age,
 				this_time)) {
-			kfree(target->hwinfo);
-			target->hwinfo = NULL;
-			list_move_tail(&target->list, &wl->network_free_list);
+			kfree(iter->hwinfo);
+			iter->hwinfo = NULL;
+			list_move_tail(&iter->list, &wl->network_free_list);
 		}
 	}
 
@@ -1569,22 +1569,22 @@ static void gelic_wl_scan_complete_event(struct gelic_wl_info *wl)
 			continue;
 		}
 
-		found = 0;
+		target = NULL;
 		oldest = NULL;
-		list_for_each_entry(target, &wl->network_list, list) {
-			if (ether_addr_equal(&target->hwinfo->bssid[2],
+		list_for_each_entry(iter, &wl->network_list, list) {
+			if (ether_addr_equal(&iter->hwinfo->bssid[2],
 					     &scan_info->bssid[2])) {
-				found = 1;
+				target = iter;
 				pr_debug("%s: same BBS found scanned list\n",
 					 __func__);
 				break;
 			}
 			if (!oldest ||
-			    (target->last_scanned < oldest->last_scanned))
-				oldest = target;
+			    (iter->last_scanned < oldest->last_scanned))
+				oldest = iter;
 		}
 
-		if (!found) {
+		if (!target) {
 			/* not found in the list */
 			if (list_empty(&wl->network_free_list)) {
 				/* expire oldest */
-- 
2.25.1

[PATCH net-next 14/15] ipvlan: Remove usage of list iterator variable for the loop body

[Jakob Koschel]

In preparation to limit the scope of the list iterator variable to the
list traversal loop, use a dedicated pointer to iterate through the
list [1].

Since that variable should not be used past the loop iteration, a
separate variable is used to 'remember the current location within the
loop'.
By avoiding the use of the iterator variable after the loop, we can lower
the scope of it to the list traversal macros in the future.

To either continue iterating from that position or start a new
iteration (if the previous iteration was complete) list_prepare_entry()
is used.

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/ipvlan/ipvlan_main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 696e245f6d00..063d7c30e944 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -9,7 +9,7 @@
 static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval,
 				struct netlink_ext_ack *extack)
 {
-	struct ipvl_dev *ipvlan;
+	struct ipvl_dev *ipvlan, *tmp = NULL;
 	unsigned int flags;
 	int err;

@@ -26,8 +26,10 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval,
 						       flags & ~IFF_NOARP,
 						       extack);
 			}
-			if (unlikely(err))
+			if (unlikely(err)) {
+				tmp = ipvlan;
 				goto fail;
+			}
 		}
 		if (nval == IPVLAN_MODE_L3S) {
 			/* New mode is L3S */
@@ -43,6 +45,7 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval,
 	return 0;

 fail:
+	ipvlan = list_prepare_entry(tmp, &port->ipvlans, pnode);
 	/* Undo the flags changes that have been done so far. */
 	list_for_each_entry_continue_reverse(ipvlan, &port->ipvlans, pnode) {
 		flags = ipvlan->dev->flags;
--
2.25.1

[PATCH net-next 15/15] team: Remove use of list iterator variable for list_for_each_entry_from()

[Jakob Koschel]

In preparation to limit the scope of the list iterator variable to the
list traversal loop, use a dedicated pointer to iterate through the
list [1].

Since that variable should not be used past the loop iteration, a
separate variable is used to 'remember the current location within the
loop'.
By avoiding the use of the iterator variable after the loop, we can lower
the scope of it to the list traversal macros in the future.

To either continue iterating from that position or skip the iteration
(if the previous iteration was complete) list_prepare_entry() is used.

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 drivers/net/team/team.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index b07dde6f0abf..688c4393f099 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2425,17 +2425,17 @@ static int team_nl_send_options_get(struct team *team, u32 portid, u32 seq,
 				    int flags, team_nl_send_func_t *send_func,
 				    struct list_head *sel_opt_inst_list)
 {
+	struct team_option_inst *opt_inst, *tmp = NULL;
 	struct nlattr *option_list;
 	struct nlmsghdr *nlh;
 	void *hdr;
-	struct team_option_inst *opt_inst;
 	int err;
 	struct sk_buff *skb = NULL;
 	bool incomplete;
 	int i;

-	opt_inst = list_first_entry(sel_opt_inst_list,
-				    struct team_option_inst, tmp_list);
+	tmp = list_first_entry(sel_opt_inst_list,
+			       struct team_option_inst, tmp_list);

 start_again:
 	err = __send_and_alloc_skb(&skb, team, portid, send_func);
@@ -2456,7 +2456,9 @@ static int team_nl_send_options_get(struct team *team, u32 portid, u32 seq,
 		goto nla_put_failure;

 	i = 0;
+	opt_inst = list_prepare_entry(tmp, sel_opt_inst_list, tmp_list);
 	incomplete = false;
+	tmp = NULL;
 	list_for_each_entry_from(opt_inst, sel_opt_inst_list, tmp_list) {
 		err = team_nl_fill_one_option_get(skb, team, opt_inst);
 		if (err) {
@@ -2464,6 +2466,7 @@ static int team_nl_send_options_get(struct team *team, u32 portid, u32 seq,
 				if (!i)
 					goto errout;
 				incomplete = true;
+				tmp = opt_inst;
 				break;
 			}
 			goto errout;
@@ -2707,14 +2710,14 @@ static int team_nl_send_port_list_get(struct team *team, u32 portid, u32 seq,
 	struct nlattr *port_list;
 	struct nlmsghdr *nlh;
 	void *hdr;
-	struct team_port *port;
+	struct team_port *port, *tmp = NULL;
 	int err;
 	struct sk_buff *skb = NULL;
 	bool incomplete;
 	int i;

-	port = list_first_entry_or_null(&team->port_list,
-					struct team_port, list);
+	tmp = list_first_entry_or_null(&team->port_list,
+				       struct team_port, list);

 start_again:
 	err = __send_and_alloc_skb(&skb, team, portid, send_func);
@@ -2744,7 +2747,9 @@ static int team_nl_send_port_list_get(struct team *team, u32 portid, u32 seq,
 		err = team_nl_fill_one_port_get(skb, one_port);
 		if (err)
 			goto errout;
-	} else if (port) {
+	} else {
+		port = list_prepare_entry(tmp, &team->port_list, list);
+		tmp = NULL;
 		list_for_each_entry_from(port, &team->port_list, list) {
 			err = team_nl_fill_one_port_get(skb, port);
 			if (err) {
@@ -2752,6 +2757,7 @@ static int team_nl_send_port_list_get(struct team *team, u32 portid, u32 seq,
 					if (!i)
 						goto errout;
 					incomplete = true;
+					tmp = port;
 					break;
 				}
 				goto errout;
--
2.25.1