netdev - Re: [PATCH net-next v2] net: dsa: b53: Do not force CPU to be always tagged

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] [day] [month] [year] [list]

Date:   Wed, 9 Jun 2021 18:05:33 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     netdev@...r.kernel.org, mnhagan88@...il.com,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v2] net: dsa: b53: Do not force CPU to be always
 tagged



On 6/9/2021 3:30 PM, Vladimir Oltean wrote:
> On Tue, Jun 08, 2021 at 02:22:04PM -0700, Florian Fainelli wrote:
>> Commit ca8931948344 ("net: dsa: b53: Keep CPU port as tagged in all
>> VLANs") forced the CPU port to be always tagged in any VLAN membership.
>> This was necessary back then because we did not support Broadcom tags
>> for all configurations so the only way to differentiate tagged and
>> untagged traffic while DSA_TAG_PROTO_NONE was used was to force the CPU
>> port into being always tagged.
>>
>> With most configurations enabling Broadcom tags, especially after
>> 8fab459e69ab ("net: dsa: b53: Enable Broadcom tags for 531x5/539x
>> families") we do not need to apply this unconditional force tagging of
>> the CPU port in all VLANs.
>>
>> A helper function is introduced to faciliate the encapsulation of the
>> specific condition requiring the CPU port to be tagged in all VLANs and
>> the dsa_switch_ops::untag_bridge_pvid boolean is moved to when
>> dsa_switch_ops::setup is called when we have already determined the
>> tagging protocol we will be using.
>>
>> Reported-by: Matthew Hagan <mnhagan88@...il.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>> ---
> 
> Florian, does the hardware behave in the same way if you disable
> CONFIG_VLAN_8021Q?

Unfortunately it does not because there is no more code calling into
b53_vlan_add() with the desired egress untagged attribute:

00:01:23.015477 b8:ac:6f:80:af:7e (oui Unknown) > 00:10:18:cd:c9:c2 (oui
Unknown), BRCM tag OP: EG, CID: 0, RC: exception, TC: 0, port: 0,
ethertype 802.1Q (0x8100), length 106: vlan 0, p 0, ethertype IPv4
(0x0800), (tos 0x0, ttl 64, id 3662, offset 0, flags [none], proto ICMP
(1), length 84)
    192.168.1.254 > 192.168.1.10: ICMP echo reply, id 5127, seq 17,
length 64
        0x0000:  0010 18cd c9c2 b8ac 6f80 af7e 0000 2000  ........o..~....
        0x0010:  8100 0000 0800 4500 0054 0e4e 0000 4001  ......E..T.N..@.
        0x0020:  e802 c0a8 01fe c0a8 010a 0000 bc2c 1407  .............,..
        0x0030:  0011 3db6 f204 0000 0000 0000 0000 0000  ..=.............
        0x0040:  0000 0000 0000 0000 0000 0000 0000 0000  ................
        0x0050:  0000 0000 0000 0000 0000 0000 0000 0000  ................
        0x0060:  0000 0000 0000 0000 0000

Something like this works:

diff --git a/drivers/net/dsa/b53/b53_common.c
b/drivers/net/dsa/b53/b53_common.c
index 6e199454e41d..ac3bb5ef17e2 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -748,9 +748,13 @@ int b53_configure_vlan(struct dsa_switch *ds)

        b53_enable_vlan(dev, -1, dev->vlan_enabled, ds->vlan_filtering);

-       b53_for_each_port(dev, i)
+       b53_for_each_port(dev, i) {
+               v = &dev->vlans[def_vid];
+               v->members |= BIT(i);
+               v->untag = v->members;
                b53_write16(dev, B53_VLAN_PAGE,
                            B53_VLAN_PORT_DEF_TAG(i), def_vid);
+       }

        /* Upon initial call we have not set-up any VLANs, but upon
         * system resume, we need to restore all VLAN entries.
-- 
Florian