netdev - Re: [net-next PATCH 14/19] net: dsa: qca8k: add support for mdb

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]
Date:   Fri, 19 Nov 2021 03:19:30 +0100
From:   Ansuel Smith <ansuelsmth@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [net-next PATCH 14/19] net: dsa: qca8k: add support for
 mdb_add/del

On Fri, Nov 19, 2021 at 04:06:57AM +0200, Vladimir Oltean wrote:
> On Wed, Nov 17, 2021 at 10:04:46PM +0100, Ansuel Smith wrote:
> > Add support for mdb add/del function. The ARL table is used to insert
> > the rule. A new search function is introduced to search the rule and add
> > additional port to it. If every port is removed from the rule, it's
> > removed. It's set STATIC in the ARL table (aka it doesn't age) to not be
> > flushed by fast age function.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@...il.com>
> > ---
> >  drivers/net/dsa/qca8k.c | 82 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 82 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index dda99263fe8c..a217c842ab45 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -417,6 +417,23 @@ qca8k_fdb_flush(struct qca8k_priv *priv)
> >  	mutex_unlock(&priv->reg_mutex);
> >  }
> >  
> > +static int
> > +qca8k_fdb_search(struct qca8k_priv *priv, struct qca8k_fdb *fdb, const u8 *mac, u16 vid)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&priv->reg_mutex);
> 
> If I were you, I'd create a locking scheme where the entire FDB entry is
> updated under the same critical section. Right now you're relying on the
> rtnl_mutex serializing calls to ->port_mdb_add()/->port_mdb_del(). But
> that might change. Don't leave that task to someone that has non-expert
> knowledge of the driver.
>

Ok will change the logic and do the search and add/del operation in one
lock.

> > +	qca8k_fdb_write(priv, vid, 0, mac, 0);
> > +	ret = qca8k_fdb_access(priv, QCA8K_FDB_SEARCH, -1);
> > +	if (ret < 0)
> > +		goto exit;
> > +
> > +	ret = qca8k_fdb_read(priv, fdb);
> > +exit:
> > +	mutex_unlock(&priv->reg_mutex);
> > +	return ret;
> > +}
> > +
> >  static int
> >  qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
> >  {
> > @@ -1959,6 +1976,69 @@ qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
> >  	return 0;
> >  }
> >  
> > +static int
> > +qca8k_port_mdb_add(struct dsa_switch *ds, int port,
> > +		   const struct switchdev_obj_port_mdb *mdb)
> > +{
> > +	struct qca8k_priv *priv = ds->priv;
> > +	struct qca8k_fdb fdb = { 0 };
> > +	const u8 *addr = mdb->addr;
> > +	u8 port_mask = BIT(port);
> 
> This doesn't really need to be kept in a temporary variable as it is
> only used once.
> 
> > +	u16 vid = mdb->vid;
> > +	int ret;
> > +
> > +	/* Check if entry already exist */
> > +	ret = qca8k_fdb_search(priv, &fdb, addr, vid);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Rule exist. Delete first */
> > +	if (!fdb.aging) {
> > +		ret = qca8k_fdb_del(priv, addr, fdb.port_mask, vid);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	/* Add port to fdb portmask */
> > +	fdb.port_mask |= port_mask;
> > +
> > +	return qca8k_port_fdb_insert(priv, addr, fdb.port_mask, vid);
> > +}
> > +
> > +static int
> > +qca8k_port_mdb_del(struct dsa_switch *ds, int port,
> > +		   const struct switchdev_obj_port_mdb *mdb)
> > +{
> > +	struct qca8k_priv *priv = ds->priv;
> > +	struct qca8k_fdb fdb = { 0 };
> > +	const u8 *addr = mdb->addr;
> > +	u8 port_mask = BIT(port);
> > +	u16 vid = mdb->vid;
> > +	int ret;
> > +
> > +	/* Check if entry already exist */
> > +	ret = qca8k_fdb_search(priv, &fdb, addr, vid);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Rule doesn't exist. Why delete? */
> 
> Because refcounting is hard. In fact with VLANs it is quite possible to
> delete an absent entry. For MDBs and FDBs, the bridge should now error
> out before it even reaches to you.
> 

So in this specific case I should simply return 0 to correctly decrement
the ref, correct? 

> > +	if (!fdb.aging)
> > +		return -EINVAL;
> > +
> > +	ret = qca8k_fdb_del(priv, addr, port_mask, vid);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Only port in the rule is this port. Don't re insert */
> > +	if (fdb.port_mask == port_mask)
> > +		return 0;
> > +
> > +	/* Remove port from port mask */
> > +	fdb.port_mask &= ~port_mask;
> > +
> > +	return qca8k_port_fdb_insert(priv, addr, fdb.port_mask, vid);
> > +}
> > +
> >  static int
> >  qca8k_port_mirror_add(struct dsa_switch *ds, int port,
> >  		      struct dsa_mall_mirror_tc_entry *mirror,
> > @@ -2160,6 +2240,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> >  	.port_fdb_add		= qca8k_port_fdb_add,
> >  	.port_fdb_del		= qca8k_port_fdb_del,
> >  	.port_fdb_dump		= qca8k_port_fdb_dump,
> > +	.port_mdb_add		= qca8k_port_mdb_add,
> > +	.port_mdb_del		= qca8k_port_mdb_del,
> >  	.port_mirror_add	= qca8k_port_mirror_add,
> >  	.port_mirror_del	= qca8k_port_mirror_del,
> >  	.port_vlan_filtering	= qca8k_port_vlan_filtering,
> > -- 
> > 2.32.0
> > 
> 

-- 
	Ansuel