netdev - Re: mv88e6xxx broken on 6176 with "Disentangle STU from VTU"

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 19 Mar 2022 03:27:20 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     Marek Behún <kabel@...nel.org>,
        Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org
Subject: Re: mv88e6xxx broken on 6176 with "Disentangle STU from VTU"

On Sat, Mar 19, 2022 at 02:15:58AM +0100, Tobias Waldekranz wrote:
> >> This still doesn't explain why we're able to load
> >> MV88E6XXX_VID_STANDALONE though...
> >
> > Why doesn't it explain it? MV88E6XXX_VID_STANDALONE is 0, we have this
> > code so it falls in the branch that doesn't call mv88e6xxx_vtu_get():
> >
> > 	if (vid == 0) {
> > 		fid = MV88E6XXX_FID_BRIDGED;
> > 	} else {
> > 		err = mv88e6xxx_vtu_get(chip, vid, &vlan);
> > 		if (err)
> > 			return err;
> >
> > 		/* switchdev expects -EOPNOTSUPP to honor software VLANs */
> > 		if (!vlan.valid)
> > 			return -EOPNOTSUPP;
> >
> > 		fid = vlan.fid;
> > 	}
> 
> Ah, yes, of course. We should really change that to
> 
>     if (vid == MV88E6XXX_VID_BRIDGED)
> 
> I guess we can also add an exception for MV88E6XXX_VID_STANDALONE now so
> that we save a roundtrip to the VTU in those cases too. Anyway...

Hmmm, no. It's a bit late to think straight right now, but I'm pretty
sure that was not the intention, and that the code was at least at some
point correct.

The 0 from "vid == 0" is not the 0 from MV88E6XXX_VID_STANDALONE.
Instead, the "vid == 0" is supposed to match on the bridge's VID 0, aka
the VLAN-unaware database. Hence FID_BRIDGED. Perhaps confusingly, 'vid 0'
bridge FDB entries are classified to VTU entry 4095.

The fact that we have code that's loading ATU entries in FID_BRIDGED
from code paths that were triggered by VID_STANDALONE is what is
concerning. If I was the one who added those code paths, it was
certainly not intended. But I think it's since you've added the
mv88e6xxx_port_vlan_join(VID_STANDALONE) call in mv88e6xxx_setup_port().

> >> Vladimir, any advise on how to proceed here? I took a very conservative
> >> approach to filling in the STU ops, only enabling it on HW that I could
> >> test. I could study some datasheets and make an educated guess about the
> >> full range of chips that we could enable this on, and which version of
> >> the ops to use. Does that sound reasonable?
> >
> > Before, MV88E6XXX_G1_VTU_OP_STU_LOAD_PURGE was done from 2 places:
> >
> > mv88e6352_g1_vtu_loadpurge()
> > mv88e6085_ops, mv88e6097_ops, mv88e6123_ops, mv88e6141_ops, mv88e6161_ops,
> > mv88e6165_ops, mv88e6171_ops, mv88e6172_ops, mv88e6175_ops, mv88e6176_ops,
> > mv88e6240_ops, mv88e6341_ops, mv88e6350_ops, mv88e6351_ops, mv88e6352_ops
> >
> > mv88e6390_g1_vtu_loadpurge()
> > mv88e6190_ops, mv88e6190x_ops, mv88e6191_ops, mv88e6290_ops, mv88e6390_ops,
> > mv88e6390x_ops, mv88e6393x_ops
> >
> > After the change, MV88E6XXX_G1_VTU_OP_STU_LOAD_PURGE is done only from the ops
> > that have stu_loadpurge:
> >
> > mv88e6352_g1_stu_loadpurge()
> > mv88e6097_ops, mv88e6352_ops
> >
> > mv88e6390_g1_stu_loadpurge()
> > mv88e6390_ops, mv88e6390x_ops, mv88e6393x_ops
> >
> > So if I understand correctly, we have this regression for all families that are
> > in the first group but not in the second group. I.e. a lot of families.
> 
> I don't have the hardware to test it, but I have now gone through the
> the functional spec for each of these devices.
> 
> I have confirmed that all those using mv88e6352_g1_vtu_loadpurge support
> the same STU operations and SID pool size (63).
> 
> Ditto for those using mv88e6390_g1_vtu_loadpurge.
> 
> > There's nothing wrong with being conservative, as long as you're a
> > correct conservative. In this case, I believe that the switch families
> > where you couldn't test MSTP should at least have a max_sid of 1, to
> > allow SID 0 to be loaded. So you don't have to claim untested MSTP
> > support. But then you may need to refine the guarding that allows for
> > MSTP support, to check for > 1 instead of > 0.
> 
> Having now done the research, which I should have just done from the
> beginning, I think the best approach is to just enable the regular MST
> offloading for all supported chips, i.e. all chips with separated
> VTU/STU.
> 
> Fair?

Fair.