netdev - Re: [PATCH net-next 5/5] net: dsa: mv88e6xxx: Offload bridge broadcast flooding flag

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: <20210316093948.zbhouadshgedktcb@skbuf>
Date:   Tue, 16 Mar 2021 11:39:48 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 5/5] net: dsa: mv88e6xxx: Offload bridge
 broadcast flooding flag

On Mon, Mar 15, 2021 at 10:14:00PM +0100, Tobias Waldekranz wrote:
> These switches have two modes of classifying broadcast:
> 
> 1. Broadcast is multicast.
> 2. Broadcast is its own unique thing that is always flooded
>    everywhere.
> 
> This driver uses the first option, making sure to load the broadcast
> address into all active databases. Because of this, we can support
> per-port broadcast flooding by (1) making sure to only set the subset
> of ports that have it enabled whenever joining a new bridge or VLAN,
> and (2) by updating all active databases whenever the setting is
> changed on a port.
> 
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 68 +++++++++++++++++++++++++++++++-
>  1 file changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 48e65f22641e..e6987c501fb7 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1950,6 +1950,18 @@ static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
>  	int err;
>  
>  	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
> +		struct dsa_port *dp = dsa_to_port(chip->ds, port);
> +
> +		if (dsa_is_unused_port(chip->ds, port))
> +			continue;
> +
> +		if (dsa_is_user_port(chip->ds, port) && dp->bridge_dev &&
> +		    !br_port_flag_is_set(dp->slave, BR_BCAST_FLOOD))

What if dp->slave is not the bridge port, but a LAG? br_port_flag_is_set
will return false.

Speaking of, shouldn't mv88e6xxx_port_vlan_join also be called from
mv88e6xxx_port_bridge_join somehow, or are we waiting for the bridge
facility to replay VLANs added to the LAG when we emit the offload
notification for it?

> +			/* Skip bridged user ports where broadcast
> +			 * flooding is disabled.
> +			 */
> +			continue;
> +
>  		err = mv88e6xxx_port_add_broadcast(chip, port, vid);
>  		if (err)
>  			return err;
> @@ -1958,6 +1970,51 @@ static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
>  	return 0;
>  }
>  
> +struct mv88e6xxx_port_broadcast_sync_ctx {
> +	int port;
> +	bool flood;
> +};
> +
> +static int
> +mv88e6xxx_port_broadcast_sync_vlan(struct mv88e6xxx_chip *chip,
> +				   const struct mv88e6xxx_vtu_entry *vlan,
> +				   void *_ctx)
> +{
> +	const char broadcast[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };

MAC addresses are usually defined as unsigned char[ETH_ALEN]. You can
also use eth_broadcast_addr(broadcast) for initialization.

> +	struct mv88e6xxx_port_broadcast_sync_ctx *ctx = _ctx;
> +	u8 state;
> +
> +	if (ctx->flood)
> +		state = MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC;
> +	else
> +		state = MV88E6XXX_G1_ATU_DATA_STATE_MC_UNUSED;
> +
> +	return mv88e6xxx_port_db_load_purge(chip, ctx->port, broadcast,
> +					    vlan->vid, state);
> +}
> +
> +static int mv88e6xxx_port_broadcast_sync(struct mv88e6xxx_chip *chip, int port,
> +					 bool flood)
> +{
> +	struct mv88e6xxx_port_broadcast_sync_ctx ctx = {
> +		.port = port,
> +		.flood = flood,
> +	};
> +	struct mv88e6xxx_vtu_entry vid0 = {
> +		.vid = 0,
> +	};
> +	int err;
> +
> +	/* Update the port's private database... */
> +	err = mv88e6xxx_port_broadcast_sync_vlan(chip, &vid0, &ctx);
> +	if (err)
> +		return err;
> +
> +	/* ...and the database for all VLANs. */
> +	return mv88e6xxx_vtu_walk(chip, mv88e6xxx_port_broadcast_sync_vlan,
> +				  &ctx);
> +}
> +
>  static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port,
>  				    u16 vid, u8 member, bool warn)
>  {
> @@ -5431,7 +5488,8 @@ static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  	const struct mv88e6xxx_ops *ops;
>  
> -	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD))
> +	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
> +			   BR_BCAST_FLOOD))
>  		return -EINVAL;
>  
>  	ops = chip->info->ops;
> @@ -5480,6 +5538,14 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
>  			goto out;
>  	}
>  
> +	if (flags.mask & BR_BCAST_FLOOD) {
> +		bool broadcast = !!(flags.val & BR_BCAST_FLOOD);
> +
> +		err = mv88e6xxx_port_broadcast_sync(chip, port, broadcast);
> +		if (err)
> +			goto out;
> +	}
> +
>  out:
>  	mv88e6xxx_reg_unlock(chip);
>  
> -- 
> 2.25.1
>