netdev - Re: [PATCH net-next] net: dsa: mv88e6xxx: 6352: parse VTU data before loading STU data

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: <C70U4J6VK2TJ.208R8QGGUAN47@wkz-x280>
Date:   Thu, 12 Nov 2020 00:49:03 +0100
From:   "Tobias Waldekranz" <tobias@...dekranz.com>
To:     "Jakub Kicinski" <kuba@...nel.org>
Cc:     <davem@...emloft.net>, <andrew@...n.ch>,
        <vivien.didelot@...il.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: dsa: mv88e6xxx: 6352: parse VTU data
 before loading STU data

On Wed Nov 11, 2020 at 4:27 PM CET, Jakub Kicinski wrote:
> On Sun, 8 Nov 2020 23:38:10 +0100 Tobias Waldekranz wrote:
> > On the 6352, doing a VTU GetNext op, followed by an STU GetNext op
> > will leave you with both the member- and state- data in the VTU/STU
> > data registers. But on the 6097 (which uses the same implementation),
> > the STU GetNext will override the information gathered from the VTU
> > GetNext.
> > 
> > Separate the two stages, parsing the result of the VTU GetNext before
> > doing the STU GetNext.
> > 
> > We opt to update the existing implementation for all applicable chips,
> > as opposed to creating a separate callback for 6097. Though the
> > previous implementation did work for (at least) 6352, the datasheet
> > does not mention the masking behavior.
> > 
> > Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> > ---
> > 
> > I was not sure if I should have created a separate callback, but I
> > have not found any documentation that suggests that you can expect the
> > STU GetNext op to mask the bits that are used to store VTU membership
> > information in the way that 6352 does. So depending on undocumented
> > behavior felt like something we would want to get rid of anyway.
> > 
> > Tested on 6097F and 6352.
>
> I'm unclear what this fixes. What functionality is broken on 6097?

VLAN configuration. As soon as you add the second port to a VLAN, all
other port membership configuration is overwritten with zeroes. The HW
interprets this as all ports being "unmodified members" of the VLAN.

I suspect that is why it has not been discovered. In the simple case
when all ports belong to the same VLAN, switching will still work. But
using multiple VLANs or trying to set multiple ports as tagged members
will not work.

At the lowest level, the current implementation assumes that it can
perform two consecutive operations where each op will load half of a
register, and then read out the union of the information. This is true
for some devices (6352), but not for others (6097).

6352 pseudo-hdl-in-c:

stu_get_next()
{
	*data |= stu_data & 0xf0f0;
}

vtu_get_next()
{
	*data |= vtu_data & 0x0f0f;
}

6097 pseudo-hdl-in-c:

stu_get_next()
{
	*data = stu_data;
}

vtu_get_next()
{
	*data = vtu_data;
}

> Can we identify the commit for a fixes tag?

I will try to pinpoint it tomorrow. I suppose I should also rebase it
against "net" since it is a bug.