diff mbox

[net-next,v2,9/9] net: dsa: mv88e6xxx: add cross-chip bridging

Message ID 20170330213715.9666-10-vivien.didelot@savoirfairelinux.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Vivien Didelot March 30, 2017, 9:37 p.m. UTC 
Implement the DSA cross-chip bridging operations by remapping the local
ports an external source port can egress frames to, when this cross-chip
port joins or leaves a bridge.

The PVT is no longer configured with all ones allowing any external
frame to egress any local port. Only DSA and CPU ports, as well as
bridge group members, can egress frames on local ports.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

Comments

Andrew Lunn March 31, 2017, 4:39 p.m. UTC | #1 
On Thu, Mar 30, 2017 at 05:37:15PM -0400, Vivien Didelot wrote:
> Implement the DSA cross-chip bridging operations by remapping the local
> ports an external source port can egress frames to, when this cross-chip
> port joins or leaves a bridge.
> 
> The PVT is no longer configured with all ones allowing any external
> frame to egress any local port. Only DSA and CPU ports, as well as
> bridge group members, can egress frames on local ports.

Hi Vivien

With the ZII devel B, we have two switches with PVT, and one
without. What happens in this setup? Can the non-PVT switch leak
frames out user ports which should otherwise be blocked?

Thanks
	Andrew
Vivien Didelot March 31, 2017, 4:55 p.m. UTC | #2 
Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Thu, Mar 30, 2017 at 05:37:15PM -0400, Vivien Didelot wrote:
>> Implement the DSA cross-chip bridging operations by remapping the local
>> ports an external source port can egress frames to, when this cross-chip
>> port joins or leaves a bridge.
>> 
>> The PVT is no longer configured with all ones allowing any external
>> frame to egress any local port. Only DSA and CPU ports, as well as
>> bridge group members, can egress frames on local ports.
>
> With the ZII devel B, we have two switches with PVT, and one
> without. What happens in this setup? Can the non-PVT switch leak
> frames out user ports which should otherwise be blocked?

If CONFIG_BRIDGE_VLAN_FILTERING isn't enabled in the kernel, the non-PVT
switch would indeed have no mean to restrict arbitrary external
frames. So in that setup, yes the switch can theorically leak frames.

With a VLAN-filtering aware system, the VTU policy and 802.1Q Secure
port mode should guard against that.

Thanks,

        Vivien
Andrew Lunn March 31, 2017, 5:09 p.m. UTC | #3 
On Fri, Mar 31, 2017 at 12:55:03PM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > On Thu, Mar 30, 2017 at 05:37:15PM -0400, Vivien Didelot wrote:
> >> Implement the DSA cross-chip bridging operations by remapping the local
> >> ports an external source port can egress frames to, when this cross-chip
> >> port joins or leaves a bridge.
> >> 
> >> The PVT is no longer configured with all ones allowing any external
> >> frame to egress any local port. Only DSA and CPU ports, as well as
> >> bridge group members, can egress frames on local ports.
> >
> > With the ZII devel B, we have two switches with PVT, and one
> > without. What happens in this setup? Can the non-PVT switch leak
> > frames out user ports which should otherwise be blocked?
> 
> If CONFIG_BRIDGE_VLAN_FILTERING isn't enabled in the kernel, the non-PVT
> switch would indeed have no mean to restrict arbitrary external
> frames. So in that setup, yes the switch can theorically leak frames.

I don't like the idea of leaking frames. It has security implications,
and hard to debug weird networking problems, like some other machine
is using my IP address, maybe spanning tree is broken if BPDUs leak,
even broadcast storms?

So we need to consider the complexity of detecting we have a non-PVT
destination switch, and forward it frames via the software bridge.

What about the case the non-PVT switch is in the middle of a chain of
PVT switches?

Maybe to start with, to keep it simple, we check all switches are PVT
capable. If they are not, we refuse to use PVT and all inter-switch
frames need to go via the Linux software bridge?

       Andrew
Vivien Didelot March 31, 2017, 5:27 p.m. UTC | #4 
Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> I don't like the idea of leaking frames.

I don't like it neither. That's why this patch series is out there. It
improves the security on PVT-capable Marvell switches by programming the
tables correctly on bridging events.

A next step can be to warn the user about the software or hardware
limitations (s)he is currently experiencing with something roughly like
this in cross-chip bridging operations:

    if (!mv88e6xxx_has_pvt(chip)) {
    #if IS_ENABLED(BRIDGE_VLAN_FILTERING)
        return 0;
    #else
        pr_err("Cross-chip bridging is forbidden on non-PVT hardware and
               non-VLAN-filtering aware systems\n");
        return -EINVAL;
    #endif
    }

But let's keep it simple for the moment and go baby steps. First program
PVT tables correctly as this patchset does, then figure out how to
handle non-PVT systems. That is a good topic for next week ;-)

Thanks,

        Vivien
diff mbox

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c6f45a2a9335..44ba8cff5631 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1222,7 +1222,7 @@  static int mv88e6xxx_pvt_map(struct mv88e6xxx_chip *chip, int dev, int port)
 
 	/* Skip the local source device, which uses in-chip port VLAN */
 	if (dev != chip->ds->index)
-		pvlan = mv88e6xxx_port_mask(chip);
+		pvlan = mv88e6xxx_port_vlan(chip, dev, port);
 
 	return mv88e6xxx_g2_pvt_write(chip, dev, port, pvlan);
 }
@@ -2203,6 +2203,36 @@  static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port,
 	mutex_unlock(&chip->reg_lock);
 }
 
+static int mv88e6xxx_crosschip_bridge_join(struct dsa_switch *ds, int dev,
+					   int port, struct net_device *br)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int err;
+
+	if (!mv88e6xxx_has_pvt(chip))
+		return 0;
+
+	mutex_lock(&chip->reg_lock);
+	err = mv88e6xxx_pvt_map(chip, dev, port);
+	mutex_unlock(&chip->reg_lock);
+
+	return err;
+}
+
+static void mv88e6xxx_crosschip_bridge_leave(struct dsa_switch *ds, int dev,
+					     int port, struct net_device *br)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+
+	if (!mv88e6xxx_has_pvt(chip))
+		return;
+
+	mutex_lock(&chip->reg_lock);
+	if (mv88e6xxx_pvt_map(chip, dev, port))
+		dev_err(ds->dev, "failed to remap cross-chip Port VLAN\n");
+	mutex_unlock(&chip->reg_lock);
+}
+
 static int mv88e6xxx_software_reset(struct mv88e6xxx_chip *chip)
 {
 	if (chip->info->ops->reset)
@@ -4313,6 +4343,8 @@  static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.port_mdb_add           = mv88e6xxx_port_mdb_add,
 	.port_mdb_del           = mv88e6xxx_port_mdb_del,
 	.port_mdb_dump          = mv88e6xxx_port_mdb_dump,
+	.crosschip_bridge_join	= mv88e6xxx_crosschip_bridge_join,
+	.crosschip_bridge_leave	= mv88e6xxx_crosschip_bridge_leave,
 };
 
 static struct dsa_switch_driver mv88e6xxx_switch_drv = {