Hi Vladimir,
> On 8. Apr 2022, at 14:31, Vladimir Oltean <olte...@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=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
>> [1]
>> Signed-off-by: Jakob Koschel <jakobkosc...@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.olt...@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.olt...@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.olt...@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.olt...@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
