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

[Jakob Koschel <jakobkoschel@gmail.com>]

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