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: <20240902124318.263883-1-junjie.wan@inceptio.ai>
Date: Mon,  2 Sep 2024 20:43:18 +0800
From: Wan Junjie <junjie.wan@...eptio.ai>
To: horms@...nel.org
Cc: davem@...emloft.net,
	edumazet@...gle.com,
	ioana.ciornei@....com,
	junjie.wan@...eptio.ai,
	kuba@...nel.org,
	linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org,
	pabeni@...hat.com
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.
>
> ...

Hi Simon Horman,

Thanks for your commnets.

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

It is hard to keep all lines limited to the length of 80 since some function
names are really long. I have tried to make the line length to 85 without
breaking some if conditions into multiple lines. You will see in v3.

> >  {
> > +	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.

Nice catch. The fdb is not assigned anyway. And num of fdbs is limited by HW.
So here I can do a judge instead of creating a new one.
	if (port_priv->fdb->bridge_dev) {
		fdb = dpaa2_switch_fdb_get_unused(ethsw);
		if (!fdb) {
			netdev_err(netdev, "vlan nums reach max_fdbs limits\n");
			return -ENOENT;
		}

Thank you.

> > +			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) {
> 
> ...