netdev - RE: [PATCH] net: dsa: ksz: Fix port membership

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: <SN1PR11MB044652B4B259DCE1EAF0F8B5ECAB0@SN1PR11MB0446.namprd11.prod.outlook.com>
Date:   Sat, 8 Dec 2018 00:01:09 +0000
From:   <Tristram.Ha@...rochip.com>
To:     <f.fainelli@...il.com>, <marex@...x.de>
CC:     <vivien.didelot@...oirfairelinux.com>, <andrew@...n.ch>,
        <Woojung.Huh@...rochip.com>, <UNGLinuxDriver@...rochip.com>,
        <davem@...emloft.net>, <netdev@...r.kernel.org>
Subject: RE: [PATCH] net: dsa: ksz: Fix port membership

> >> If two ports are in the same bridge and in forwarding state, the packets
> >> must be able to pass between them in both directions. The current code
> >> only configures this bridge membership for a port newly added to the
> >> bridge, but does not update all the other ports. Thus, ingress packets
> >> on the new port will be forwarded, but ingress packets on other ports
> >> destined for the new port (eg. a reply) will not be forwarded back to
> >> the new port, because they are not configured to do so. This patch fixes
> >> that by updating the membership registers of all ports.
> >>
> >> Signed-off-by: Marek Vasut <marex@...x.de>
> >> Cc: Vivien Didelot <vivien.didelot@...oirfairelinux.com>
> >> Cc: Woojung Huh <woojung.huh@...rochip.com>
> >> Cc: David S. Miller <davem@...emloft.net>
> >> Cc: Tristram Ha <Tristram.Ha@...rochip.com>
> >> ---
> >>  drivers/net/dsa/microchip/ksz9477.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/dsa/microchip/ksz9477.c
> >> b/drivers/net/dsa/microchip/ksz9477.c
> >> index 0684657fbf9a9..e24dd14ccde77 100644
> >> --- a/drivers/net/dsa/microchip/ksz9477.c
> >> +++ b/drivers/net/dsa/microchip/ksz9477.c
> >> @@ -396,7 +396,7 @@ static void ksz9477_port_stp_state_set(struct
> >> dsa_switch *ds, int port,
> >>  	struct ksz_device *dev = ds->priv;
> >>  	struct ksz_port *p = &dev->ports[port];
> >>  	u8 data;
> >> -	int member = -1;
> >> +	int i, member = -1;
> >>
> >>  	ksz_pread8(dev, port, P_STP_CTRL, &data);
> >>  	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE |
> >> PORT_LEARN_DISABLE);
> >> @@ -454,8 +454,8 @@ static void ksz9477_port_stp_state_set(struct
> >> dsa_switch *ds, int port,
> >>  		dev->tx_ports &= ~(1 << port);
> >>
> >>  	/* Port membership may share register with STP state. */
> >> -	if (member >= 0 && member != p->member)
> >> -		ksz9477_cfg_port_member(dev, port, (u8)member);
> >> +	for (i = 0; i < SWITCH_PORT_NUM; i++)
> >> +		ksz9477_cfg_port_member(dev, i, (u8)member);
> >>
> >>  	/* Check if forwarding needs to be updated. */
> >>  	if (state != BR_STATE_FORWARDING) {
> >
> > The original DSA model did not have a way to tell the bridge device not to
> > forward the frame, so the switch driver always setup the membership to
> > disable forwarding between ports.
> >
> > When lan devices are setup they act like individual devices.  A bridge device
> > adding them under it will forward the frames.
> >
> > The new switchdev model adds the offload_fwd_mark bit to tell the bridge
> not to
> > forward frame.
> >
> > The ksz_update_port_member function in ksz_common.c is doing this
> membership
> > setup for all forwarding ports.  It was finally enabled in one of the RFC
> patches I
> > submitted recently (Add switch forward offloading support).
> >
> > I think if you do this without setting offload_fwd_mark you will receive
> duplicate
> > frame.
> >
> 
> Either I am misreading Marek's patch, or I don't quite understand your
> response, but what is happening when you enslave a switch port into a
> bridge is that you need to make sure that:
> 
> - the switch port being enslaved will be part of the same forwarding
> group as any other switch port already in the bridge
> - any existing switch port already enslaved in the bridge must now also
> be allowed to forward to the port that is being enslaved
> 
> That is to me, exactly what Marek's patch is fixing, your response is
> about something slightly orthogonal here.

I got confused here as the code is obviously wrong and should not work,
so I found out why it works in the bridge device situation.  There is actually
a bug in the driver that enables this behavior.  The port_vlan_filtering function
turns off the port membership enforcement.  Fixing this problem should be
easy, but this port_vlan_filtering function is also not implemented right.  It treats the
operation as a simple VLAN on/off, but it is more complex than that.

Anyway it seems to work in the bridge device situation, but it does not work
in the default situation:

Assume there are two port devices lan1 and lan2.  The device lan1 is assigned
an IP address and can talk to outside.  Enabling and disabling lan2 by doing
"ifconfig lan2 up" and "ifconfig lan2 down."  The device lan1 is no longer working.

Create a bridge device and add a child device by doing "brctl addbr br0" and
"brctl addif br0 lan1."  This will call port_vlan_filtering and then the feature
UNICAST_VLAN_BOUNDARY is disabled.  This causes port membership to have no
effect on unicast packets and so it does not matter what member value is used.
The device lan1 can start working again.

The fix is to avoid disabling UNICAST_VLAN_BOUNDARY and it should be set all
the time.  In this switch the default is on.