netdev - Re: [RFC PATCH net-next] net: dsa: tag_qca: Check for upstream VLAN tag

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]

Date:   Sat, 5 Jun 2021 20:34:06 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        Matthew Hagan <mnhagan88@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next] net: dsa: tag_qca: Check for upstream VLAN
 tag



On 6/5/2021 5:53 PM, Vladimir Oltean wrote:
> Hi Matthew,
> 
> On Sat, Jun 05, 2021 at 11:39:24PM +0100, Matthew Hagan wrote:
>> On 05/06/2021 21:35, Andrew Lunn wrote:
>>
>>>> The tested case is a Meraki MX65 which features two QCA8337 switches with
>>>> their CPU ports attached to a BCM58625 switch ports 4 and 5 respectively.
>>> Hi Matthew
>>>
>>> The BCM58625 switch is also running DSA? What does you device tree
>>> look like? I know Florian has used two broadcom switches in cascade
>>> and did not have problems.
>>>
>>>     Andrew
>>
>> Hi Andrew
>>
>> I did discuss this with Florian, who recommended I submit the changes. Can
>> confirm the b53 DSA driver is being used. The issue here is that tagging
>> must occur on all ports. We can't selectively disable for ports 4 and 5
>> where the QCA switches are attached, thus this patch is required to get
>> things working.
>>
>> Setup is like this:
>>                        sw0p2     sw0p4            sw1p2     sw1p4 
>>     wan1    wan2  sw0p1  +  sw0p3  +  sw0p5  sw1p1  +  sw1p3  +  sw1p5
>>      +       +      +    |    +    |    +      +    |    +    |    +
>>      |       |      |    |    |    |    |      |    |    |    |    |
>>      |       |    +--+----+----+----+----+-+ +--+----+----+----+----+-+
>>      |       |    |         QCA8337        | |        QCA8337         |
>>      |       |    +------------+-----------+ +-----------+------------+
>>      |       |             sw0 |                     sw1 |
>> +----+-------+-----------------+-------------------------+------------+
>> |    0       1    BCM58625     4                         5            |
>> +----+-------+-----------------+-------------------------+------------+
> 
> It is a bit unconventional for the upstream Broadcom switch, which is a
> DSA master of its own, to insert a VLAN ID of zero out of the blue,
> especially if it operates in standalone mode. Supposedly sw0 and sw1 are
> not under a bridge net device, are they?

This is because of the need (or desire) to always tag the CPU port
regardless of the untagged VLAN that one of its downstream port is being
added to. Despite talking with Matthew about this before, I had not
realized that dsa_port_is_cpu() will return true for ports 4 and 5 when
a VLAN is added to one of the two QCA8337 switches because from the
perspective of that switch, those ports have been set as DSA_PORT_TYPE_CPU.

This may also mean that b53_setup() needs fixing as well while it
iterates over the ports of the switch though I am not sure how we could
fix that yet.

> 
> If I'm not mistaken, this patch should solve your problem?

How about this:

diff --git a/drivers/net/dsa/b53/b53_common.c
b/drivers/net/dsa/b53/b53_common.c
index 3ca6b394dd5f..6dfcff9018fd 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1455,6 +1455,22 @@ static int b53_vlan_prepare(struct dsa_switch
*ds, int port,
        return 0;
 }

+static inline bool b53_vlan_can_untag(struct dsa_switch *ds, int port)
+{
+       /* If this switch port is a CPU port */
+       if (dsa_is_cpu_port(ds, port)) {
+               /* We permit untagging to be configured if it is the DSA
+                * master of another switch (cascading).
+                */
+               if (dsa_slave_dev_check(dsa_to_port(ds, port)->master))
+                       return true;
+
+               return false;
+       }
+
+       return true;
+}
+
 int b53_vlan_add(struct dsa_switch *ds, int port,
                 const struct switchdev_obj_port_vlan *vlan,
                 struct netlink_ext_ack *extack)
@@ -1477,7 +1493,7 @@ int b53_vlan_add(struct dsa_switch *ds, int port,
                untagged = true;

        vl->members |= BIT(port);
-       if (untagged && !dsa_is_cpu_port(ds, port))
+       if (untagged && b53_vlan_can_untag(ds, port))
                vl->untag |= BIT(port);
        else
                vl->untag &= ~BIT(port);
@@ -1514,7 +1530,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
        if (pvid == vlan->vid)
                pvid = b53_default_pvid(dev);

-       if (untagged && !dsa_is_cpu_port(ds, port))
+       if (untagged && b53_vlan_can_untag(ds, port))
                vl->untag &= ~(BIT(port));

        b53_set_vlan_entry(dev, vlan->vid, vl);
-- 
Florian