| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20241213000023.jkrxbogcws4azh4w@skbuf>
Date: Fri, 13 Dec 2024 02:00:23 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Tim Harvey <tharvey@...eworks.com>
Cc: Woojung Huh <woojung.huh@...rochip.com>, UNGLinuxDriver@...rochip.com,
Andrew Lunn <andrew@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Arun Ramadoss <arun.ramadoss@...rochip.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: microchip: ksz9477: fix multicast filtering
On Thu, Dec 12, 2024 at 01:51:32PM -0800, Tim Harvey wrote:
> commit 331d64f752bb ("net: dsa: microchip: add the enable_stp_addr
> pointer in ksz_dev_ops") introduced enabling of the reserved multicast
> address table function to filter packets based on multicast MAC address
> but only configured one MAC address group, group 0 for
> (01-80-C2-00)-00-00 for bridge group data.
>
> This causes other multicast groups to fail to be received such as LLDP
> which uses a MAC address of 01-80-c2-00-00-0e (group 6).
>
> Enabling the reserved multicast address table requires configuring the
> port forward mask for all eight address groups as the mask depends on
> the port configuration.
Personal experience reading your commit message: it took me a long while
to realize that the reason why the 8 pre-configured Reserved Multicast
table entries don't work is written here: "the mask depends on the port
configuration." It is absolutely understated IMO.
> The table determines the forwarding ports for
> 48 specific multicast addresses and is addressed by the least
> significant 6 bits of the multicast address. Changing a forwarding
> port mask for one address also makes the same change for all other
> addresses in the same group.
>
> Add configuration of the groups as such:
> - leave these as default:
> group 1 (01-80-C2-00)-00-01 (MAC Control Frame) (drop)
> group 3 (01-80-C2-00)-00-10) (Bridge Management) (all ports)
> - forward to cpu port:
> group 0 (01-80-C2-00)-00-00 (Bridge Group Data)
> group 2 (01-80-C2-00)-00-03 (802.1X access control)
> group 6 (01-80-C2-00)-00-02, (01-80-C2-00)-00-04 – (01-80-C2-00)-00-0F
> - forward to all but cpu port:
Why would you not forward packets to the CPU port as a hardcoded configuration?
What if the KSZ ports are bridged together with a foreign interface
(different NIC, WLAN, tunnel etc), how should the packets reach that?
> group 4 (01-80-C2-00)-00-20 (GMRP)
> group 5 (01-80-C2-00)-00-21 (GVRP)
> group 7 (01-80-C2-00)-00-11 - (01-80-C2-00)-00-1F,
> (01-80-C2-00)-00-22 - (01-80-C2-00)-00-2F
Don't you want to forgo the (odd) hardware defaults for the Reserved Multicast
table, and instead follow what the Linux bridge does in br_handle_frame()?
Which is to trap all is_link_local_ether_addr() addresses to the CPU, do
_not_ call dsa_default_offload_fwd_mark() for those packets (aka let the
bridge know that they haven't been forwarded in hardware, and if they
should reach other bridge ports, this must be done in software), and let the
user choose, via the bridge group_fwd_mask, if they should be forwarded
to other bridge ports or not?
>
> Datasheets:
> [1] https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9897S-Data-Sheet-DS00002394C.pdf
> [2] https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9896C-Data-Sheet-DS00002390C.pdf
> [3] https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9893R-Data-Sheet-DS00002420D.pdf
> [4] https://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf
> [5] https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9563R-Data-Sheet-DS00002419D.pdf
> [6] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ9567R-Data-Sheet-DS00002329.pdf
> [7] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ9567R-Data-Sheet-DS00002329.pdf
[6] and [7] are the same.
Also, you'd better specify in the commit message what's with these datasheet
links, which to me and I suppose all other non-expert readers, are pasted here
out of the blue, with no context.
Like for example: "KSZ9897, ..., have arbitrary CPU port assignments, as
can be seen in the driver's ksz_chip_data :: cpu_ports entries for these
families, and the CPU port selection on a certain board rarely coincides
with the default host port selection in the Reserved Multicast address
table".
>
> Fixes: 331d64f752bb ("net: dsa: microchip: add the enable_stp_addr pointer in ksz_dev_ops")
> Signed-off-by: Tim Harvey <tharvey@...eworks.com>
> ---
> drivers/net/dsa/microchip/ksz9477.c | 84 +++++++++++++++++++++++++----
> 1 file changed, 75 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index d16817e0476f..d8fe809dd461 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -1138,25 +1138,24 @@ void ksz9477_config_cpu_port(struct dsa_switch *ds)
> }
> }
>
> -int ksz9477_enable_stp_addr(struct ksz_device *dev)
> +static int ksz9477_reserved_muticast_group(struct ksz_device *dev, int index, int mask)
> {
> + const u8 *shifts;
> const u32 *masks;
> u32 data;
> int ret;
>
> + shifts = dev->info->shifts;
> masks = dev->info->masks;
>
> - /* Enable Reserved multicast table */
> - ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_RESV_MCAST_ENABLE, true);
> -
> - /* Set the Override bit for forwarding BPDU packet to CPU */
> - ret = ksz_write32(dev, REG_SW_ALU_VAL_B,
> - ALU_V_OVERRIDE | BIT(dev->cpu_port));
> + /* write the PORT_FORWARD value to the Reserved Multicast Address Table Entry 2 Register */
In netdev the coding style limits the line length to 80 characters where
that is easy, like here.
> + ret = ksz_write32(dev, REG_SW_ALU_VAL_B, mask);
> if (ret < 0)
> return ret;
>
> - data = ALU_STAT_START | ALU_RESV_MCAST_ADDR | masks[ALU_STAT_WRITE];
> -
> + /* write to the Static Address and Reserved Multicast Table Control Register */
> + data = (index << shifts[ALU_STAT_INDEX]) |
> + ALU_STAT_START | ALU_RESV_MCAST_ADDR | masks[ALU_STAT_WRITE];
> ret = ksz_write32(dev, REG_SW_ALU_STAT_CTRL__4, data);
> if (ret < 0)
> return ret;
> @@ -1167,8 +1166,75 @@ int ksz9477_enable_stp_addr(struct ksz_device *dev)
> dev_err(dev->dev, "Failed to update Reserved Multicast table\n");
> return ret;
> }
> + return ksz9477_wait_alu_sta_ready(dev);
> +}
> +
> +int ksz9477_enable_stp_addr(struct ksz_device *dev)
> +{
> + int ret;
> + int cpu_mask = dsa_cpu_ports(dev->ds);
> + int user_mask = dsa_user_ports(dev->ds);
Also, in netdev, the coding style is to sort lines with variable
declarations in the reverse order of their length (so-called reverse
Christmas tree).
> + /* array of indexes into table:
> + * The table is indexed by the low 6 bits of the MAC address.
> + * Changing the PORT_FORWARD value for any single address affects
> + * all others in group
> + */
> + u16 addr_groups[8] = {
Array can be static const. Also, since all elements are initialized,
specifying its size explicitly is not necessary ("[8]" can be "[]").
> + /* group 0: (01-80-C2-00)-00-00 (Bridge Group Data) */
> + 0x000,
> + /* group 1: (01-80-C2-00)-00-01 (MAC Control Frame) */
> + 0x001,
> + /* group 2: (01-80-C2-00)-00-03 (802.1X access control) */
> + 0x003,
> + /* group 3: (01-80-C2-00)-00-10) (Bridge Management) */
> + 0x010,
> + /* group 4: (01-80-C2-00)-00-20 (GMRP) */
> + 0x020,
> + /* group 5: (01-80-C2-00)-00-21 (GVRP) */
> + 0x021,
> + /* group 6: (01-80-C2-00)-00-02, (01-80-C2-00)-00-04 – (01-80-C2-00)-00-0F */
> + 0x002,
> + /* group 7: (01-80-C2-00)-00-11 - (01-80-C2-00)-00-1F,
> + * (01-80-C2-00)-00-22 - (01-80-C2-00)-00-2F
> + */
> + 0x011,
> + };
> +
> + /* Enable Reserved multicast table */
> + ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_RESV_MCAST_ENABLE, true);
> +
> + /* update reserved multicast address table:
> + * leave as default:
> + * - group 1 (01-80-C2-00)-00-01 (MAC Control Frame) (drop)
> + * - group 3 (01-80-C2-00)-00-10) (Bridge Management) (all ports)
> + * forward to cpu port:
> + * - group 0 (01-80-C2-00)-00-00 (Bridge Group Data)
> + * - group 2 (01-80-C2-00)-00-03 (802.1X access control)
> + * - group 6 (01-80-C2-00)-00-02, (01-80-C2-00)-00-04 – (01-80-C2-00)-00-0F
> + * forward to all but cpu port:
> + * - group 4 (01-80-C2-00)-00-20 (GMRP)
> + * - group 5 (01-80-C2-00)-00-21 (GVRP)
> + * - group 7 (01-80-C2-00)-00-11 - (01-80-C2-00)-00-1F,
> + * (01-80-C2-00)-00-22 - (01-80-C2-00)-00-2F
> + */
> + if (ksz9477_reserved_muticast_group(dev, addr_groups[0], cpu_mask))
> + goto exit;
err = (function return code), and print it with %pe, ERR_PTR(err) please.
We want to distinguish between -ETIMEDOUT in ksz9477_wait_alu_sta_ready()
vs whatever ksz_write32() may return.
> + if (ksz9477_reserved_muticast_group(dev, addr_groups[2], cpu_mask))
> + goto exit;
> + if (ksz9477_reserved_muticast_group(dev, addr_groups[6], cpu_mask))
> + goto exit;
> + if (ksz9477_reserved_muticast_group(dev, addr_groups[4], user_mask))
> + goto exit;
> + if (ksz9477_reserved_muticast_group(dev, addr_groups[5], user_mask))
> + goto exit;
> + if (ksz9477_reserved_muticast_group(dev, addr_groups[7], user_mask))
> + goto exit;
>
> return 0;
> +
> +exit:
> + dev_err(dev->dev, "Failed to update Reserved Multicast table\n");
> + return ret;
> }
>
> int ksz9477_setup(struct dsa_switch *ds)
> --
> 2.34.1
>
Powered by blists - more mailing lists