linux-kernel - Re: [PATCH v2] dpaa2-switch: fix flooding domain among multiple vlans

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240902105714.GH23170@kernel.org>
Date: Mon, 2 Sep 2024 11:57:14 +0100
From: Simon Horman <horms@...nel.org>
To: Wan Junjie <junjie.wan@...eptio.ai>
Cc: Ioana Ciornei <ioana.ciornei@....com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] dpaa2-switch: fix flooding domain among multiple vlans

On Mon, Sep 02, 2024 at 09:50:51AM +0800, Wan Junjie wrote:
> Currently, dpaa2 switch only cares dst mac and egress interface
> in FDB. And all ports with different vlans share the same FDB.
> 
> This will make things messed up when one device connected to
> dpaa2 switch via two interfaces. Ports get two different vlans
> assigned. These two ports will race for a same dst mac entry
> since multiple vlans share one FDB.
> 
> FDB below may not show up at the same time.
> 02:00:77:88:99:aa dev swp0 self
> 02:00:77:88:99:aa dev swp1 self
> But in fact, for rules on the bridge, they should be:
> 02:00:77:88:99:aa dev swp0 vlan 10 master br0
> 02:00:77:88:99:aa dev swp1 vlan 20 master br0
> 
> This patch address this by borrowing unused form ports' FDB
> when ports join bridge. And append offload flag to hardware
> offloaded rules so we can tell them from those on bridges.
> 
> Signed-off-by: Wan Junjie <junjie.wan@...eptio.ai>

Hi Wan Junjie,

Some minor feedback from my side.

...

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> index a293b08f36d4..217c68bb0faa 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> @@ -25,8 +25,17 @@
>  
>  #define DEFAULT_VLAN_ID			1
>  
> -static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv)
> +static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv, u16 vid)

This, and several other lines in this patch, could be trivially
line wrapped in order for them to be <= 80 columns wide, as is
still preferred in Networking code.

This and a number of other minor problems are flagged by:
./scripts/checkpatch.pl --strict --codespell --max-line-length=80

>  {
> +	struct ethsw_core *ethsw = port_priv->ethsw_data;
> +	int i;
> +
> +	if (port_priv->fdb->bridge_dev) {
> +		for (i = 0; i < ethsw->sw_attr.max_fdbs; i++)
> +			if (ethsw->fdbs[i].vid == vid)
> +				return ethsw->fdbs[i].fdb_id;
> +	}
> +	/* Default vlan, use port's fdb id directly */
>  	return port_priv->fdb->fdb_id;
>  }
>  

...

> @@ -191,10 +212,38 @@ static void *dpaa2_iova_to_virt(struct iommu_domain *domain,
>  static int dpaa2_switch_add_vlan(struct ethsw_port_priv *port_priv, u16 vid)
>  {
>  	struct ethsw_core *ethsw = port_priv->ethsw_data;
> +	struct net_device *netdev = port_priv->netdev;
> +	struct dpsw_fdb_cfg fdb_cfg = {0};
>  	struct dpsw_vlan_cfg vcfg = {0};
> +	struct dpaa2_switch_fdb *fdb;
> +	u16 fdb_id;
>  	int err;
>  
> -	vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv);
> +	/* If ports are under a bridge, then
> +	 * every VLAN domain should use a different fdb.
> +	 * If ports are standalone, and
> +	 * vid is 1 this should reuse the allocated port fdb.
> +	 */
> +	if (port_priv->fdb->bridge_dev) {
> +		fdb = dpaa2_switch_fdb_get_unused(ethsw);
> +		if (!fdb) {
> +			/* if not available, create a new fdb */
> +			err = dpsw_fdb_add(ethsw->mc_io, 0, ethsw->dpsw_handle,
> +					   &fdb_id, &fdb_cfg);
> +			if (err) {
> +				netdev_err(netdev, "dpsw_fdb_add err %d\n", err);
> +				return err;
> +			}

fdb is still NULL here. Based on my reading of dpaa2_switch_port_init()
I think you need the following. Possibly also with an error check.

			fdb = dpaa2_switch_fdb_get_unused(ethsw);

Flagged by Smatch.

> +			fdb->fdb_id = fdb_id;
> +		}
> +		fdb->vid = vid;
> +		fdb->in_use = true;
> +		fdb->bridge_dev = NULL;
> +		vcfg.fdb_id = fdb->fdb_id;
> +	} else {
> +		/* Standalone, port's private fdb shared */
> +		vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid);
> +	}
>  	err = dpsw_vlan_add(ethsw->mc_io, 0,
>  			    ethsw->dpsw_handle, vid, &vcfg);
>  	if (err) {

...

-- 
pw-bot: cr