Date:   Mon, 30 Aug 2021 11:12:54 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>
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>, netdev@...r.kernel.org,
        DENG Qingfang <dqfext@...il.com>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Mauri Sandberg <sandberg@...lfence.com>
Subject: Re: [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge
 offloading

On Sun, Aug 29, 2021 at 02:26:00AM +0200, Linus Walleij wrote:
> From: DENG Qingfang <dqfext@...il.com>
> 
> Use port isolation registers to configure bridge offloading.
> 
> Tested on the D-Link DIR-685, switching between ports and
> sniffing ports to make sure no packets leak.
> 
> Cc: Vladimir Oltean <olteanv@...il.com>
> Cc: Alvin Šipraga <alsi@...g-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@...lfence.com>
> Signed-off-by: DENG Qingfang <dqfext@...il.com>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
>  drivers/net/dsa/rtl8366rb.c | 84 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index a89093bc6c6a..14939188c108 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -300,6 +300,12 @@
>  #define RTL8366RB_INTERRUPT_STATUS_REG	0x0442
>  #define RTL8366RB_NUM_INTERRUPT		14 /* 0..13 */
>  
> +/* Port isolation registers */
> +#define RTL8366RB_PORT_ISO_BASE		0x0F08
> +#define RTL8366RB_PORT_ISO(pnum)	(RTL8366RB_PORT_ISO_BASE + (pnum))
> +#define RTL8366RB_PORT_ISO_EN		BIT(0)
> +#define RTL8366RB_PORT_ISO_PORTS_MASK	GENMASK(7, 1)

If RTL8366RB_NUM_PORTS is 6, then why is RTL8366RB_PORT_ISO_PORTS_MASK a
7-bit field?

> +
>  /* bits 0..5 enable force when cleared */
>  #define RTL8366RB_MAC_FORCE_CTRL_REG	0x0F11
>  
> @@ -835,6 +841,21 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> +	/* Isolate all user ports so only the CPU port can access them */
> +	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
> +		ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(i),
> +				   RTL8366RB_PORT_ISO_EN |
> +				   BIT(RTL8366RB_PORT_NUM_CPU + 1));

The shifting due to RTL8366RB_PORT_ISO_EN looks weird, I can see it
being mishandled in the future, with code moved around, copied and
pasted between realtek drivers and such. How about making a macro

#define RTL8366RB_PORT_ISO_PORTS(x)	((x) << 1)

> +		if (ret)
> +			return ret;
> +	}
> +	/* CPU port can access all ports */

Except itself maybe? RTL8366RB_PORT_NUM_CPU is 5, so maybe use something
like

RTL8366RB_PORT_ISO_PORTS(dsa_user_ports(ds))

> +	ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(RTL8366RB_PORT_NUM_CPU),
> +			   RTL8366RB_PORT_ISO_PORTS_MASK |
> +			   RTL8366RB_PORT_ISO_EN);
> +	if (ret)
> +		return ret;
> +
>  	/* Set up the "green ethernet" feature */
>  	ret = rtl8366rb_jam_table(rtl8366rb_green_jam,
>  				  ARRAY_SIZE(rtl8366rb_green_jam), smi, false);
> @@ -1127,6 +1148,67 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port)
>  	rb8366rb_set_port_led(smi, port, false);
>  }
>  
> +static int
> +rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
> +			   struct net_device *bridge)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	unsigned int port_bitmap = 0;
> +	int ret, i;
> +
> +	/* Loop over all other ports than this one */
> +	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
> +		/* Handled last */
> +		if (i == port)
> +			continue;
> +		/* Not on this bridge */
> +		if (dsa_to_port(ds, i)->bridge_dev != bridge)
> +			continue;
> +		/* Join this port to each other port on the bridge */
> +		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
> +					 BIT(port + 1), BIT(port + 1));
> +		if (ret)
> +			return ret;
> +
> +		port_bitmap |= BIT(i);
> +	}
> +
> +	/* Set the bits for the ports we can access */
> +	return regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
> +				  RTL8366RB_PORT_ISO_PORTS_MASK,
> +				  port_bitmap << 1);
> +}
> +
> +static void
> +rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
> +			    struct net_device *bridge)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	unsigned int port_bitmap = 0;
> +	int ret, i;
> +
> +	/* Loop over all other ports than this one */
> +	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
> +		/* Handled last */
> +		if (i == port)
> +			continue;
> +		/* Not on this bridge */
> +		if (dsa_to_port(ds, i)->bridge_dev != bridge)
> +			continue;
> +		/* Remove this port from any other port on the bridge */
> +		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
> +					 BIT(port + 1), 0);
> +		if (ret)
> +			return;
> +
> +		port_bitmap |= BIT(i);
> +	}
> +
> +	/* Clear the bits for the ports we can access */
> +	regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
> +			   port_bitmap << 1, 0);
> +}
> +
>  static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  {
>  	struct realtek_smi *smi = ds->priv;
> @@ -1510,6 +1592,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
>  	.get_strings = rtl8366_get_strings,
>  	.get_ethtool_stats = rtl8366_get_ethtool_stats,
>  	.get_sset_count = rtl8366_get_sset_count,
> +	.port_bridge_join = rtl8366rb_port_bridge_join,
> +	.port_bridge_leave = rtl8366rb_port_bridge_leave,
>  	.port_vlan_filtering = rtl8366_vlan_filtering,
>  	.port_vlan_add = rtl8366_vlan_add,
>  	.port_vlan_del = rtl8366_vlan_del,
> -- 
> 2.31.1
> 

Looks okay for the most part. It is to be expected for a new driver that
introduces bridging offload to also handle .port_pre_bridge_flags,
.port_bridge_flags and .port_fast_age, for two reasons:
(a) it is expected that a port which does not offload the bridge, and
    performs forwarding in software, to not perform address learning in
    hardware
(b) it is expected that the addresses learned while the port was under a
    bridge are not carried over into its life as a standalone port, when
    it leaves that bridge

Also, it would be nice if you could do some minimal isolation at the
level of the FDB lookup. Currently, if I am not mistaken, a port will
perform FDB lookup even if it is standalone, and it might find an FDB
entry for a given {MAC DA, VLAN ID} pair that belongs to a port outside
of its isolation mask, so forwarding will be blocked and that packet
will be dropped (instead of the expected behavior which is for that
packet to be forwarded to the CPU).

Normally the expectation is that this FDB-level isolation can be achieved
by configuring the VLANs of one bridge to use a filter ID that is
different from the VLANs of another bridge, and the port-based default
VLAN of standalone ports to use yet another filter ID. This is yet
another reason to disable learning on standalone ports, so that their
filter ID never contains any FDB entry, and packets are always flooded
to their only possible destination, the CPU port.

Currently in DSA we do not offer a streamlined way for you to determine
what filter ID to use for a certain VLAN belonging to a certain bridge,
but at the very least you can test FDB isolation between standalone
ports and bridged ports. The simplest way to do that, assuming you
already have a forwarding setup with 2 switch ports swp0 and swp1, is to
enable CONFIG_BONDING=y, and then:

ip link add br0 type bridge
ip link set bond0 master br0
ip link set swp1 master bond0
ip link set swp0 master br0

Then ping between station A attached to swp0 and station B attached to
swp1.

Because swp1 cannot offload bond0, it will fall back to software
forwarding and act as standalone, i.e. what you had up till now.
With hardware address learning enabled on swp0 (a port that offloads
br0), it will learn station A's source MAC address. Then when swp1 needs
to send a packet to station A's destination MAC address, it would be
tempted to look up the FDB, find that address, and forward to swp0. But
swp0 is isolated from swp1. If you use a filter ID for standalone ports
and another filter ID for bridged ports you will avoid that problem, and
you will also lay the groundwork for the full FDB isolation even between
bridges that will be coming during the next development cycle.

If you feel that the second part is too much for now, you can just add
the extra callbacks for address learning and flushing (although I do
have some genuine concerns about how reliable was the software forwarding
with this driver, seeing that right now it enables hardware learning
unconditionally). Is there something that isolates FDB lookups already?

Powered by blists - more mailing lists