netdev - Re: [PATCH net-next 4/6] net: dsa: Don't program the VLAN as pvid on the upstream port

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: <39e6fc4a-5aba-03e4-d7a9-09d2f2a27f7e@gmail.com>
Date:   Tue, 20 Aug 2019 15:43:22 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>, Ido Schimmel <idosch@...sch.org>,
        Roopa Prabhu <roopa@...ulusnetworks.com>,
        nikolay@...ulusnetworks.com,
        "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 4/6] net: dsa: Don't program the VLAN as pvid on
 the upstream port

On 8/20/19 5:09 AM, Vladimir Oltean wrote:
> Hi Florian,
> 
> On Tue, 20 Aug 2019 at 06:15, Florian Fainelli <f.fainelli@...il.com> wrote:
>>
>>
>>
>> On 8/19/2019 5:00 PM, Vladimir Oltean wrote:
>>> Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
>>> programs the VLAN from the bridge into the specified port as well as the
>>> upstream port, with the same set of flags.
>>>
>>> Consider the typical case of installing pvid 1 on user port 1, pvid 2 on
>>> user port 2, etc. The upstream port would end up having a pvid equal to
>>> the last user port whose pvid was programmed from the bridge. Less than
>>> useful.
>>>
>>> So just don't change the pvid of the upstream port and let it be
>>> whatever the driver set it internally to be.
>>
>> This patch should allow removing the !dsa_is_cpu_port() checks from
>> b53_common.c:b53_vlan_add, about time :)
>>
>> It seems to me that the fundamental issue here is that because we do not
>> have a user visible network device that 1:1 maps with the CPU (or DSA)
>> ports for that matter (and for valid reasons, they would represent two
>> ends of the same pipe), we do not have a good way to control the CPU
>> port VLAN attributes.
>>
>> There was a prior attempt at allowing using the bridge master device to
>> program the CPU port's VLAN attributes, see [1], but I did not follow up
>> with that until [2] and then life caught me. If you can/want, that would
>> be great (not asking for TPS reports).
>>
>> [1]:
>> https://lists.linuxfoundation.org/pipermail/bridge/2016-November/010112.html
>> [2]:
>> https://lore.kernel.org/lkml/20180624153339.13572-1-f.fainelli@gmail.com/T/
>>
> 
> So what was the conclusion of that discussion? Should you or should
> you not add the check for vlan->flags & BRIDGE_VLAN_INFO_BRENTRY?

I was not able to test that change, and got distracted for months
(years?) doing "other stuff" that is not DSA related.

> I don't exactly handle the meaning of 'master' and 'self' options from
> a user perspective.
> Right now (no patches applied) I get the following behavior in DSA
> (swp2 is already member of br0):
> 
> $ echo 1 | sudo tee /sys/class/net/br0/bridge/vlan_filtering
> $ sudo bridge vlan add vid 100 dev swp2
> $ sudo bridge vlan add vid 101 dev swp2 self
> RTNETLINK answers: Operation not supported
> $ sudo bridge vlan add vid 102 dev swp2 master
> $ sudo bridge vlan add vid 103 dev br0
> RTNETLINK answers: Operation not supported
> $ sudo bridge vlan add vid 104 dev br0 self
> $ sudo bridge vlan add vid 105 dev br0 master
> RTNETLINK answers: Operation not supported
> 
> $ bridge vlan
> port    vlan ids
> eth0     1 PVID Egress Untagged
> 
> swp5     1 PVID Egress Untagged
> 
> swp2     1 PVID Egress Untagged
>          100
>          102
> 
> swp3     1 PVID Egress Untagged
> 
> swp4     1 PVID Egress Untagged
> 
> br0      1 PVID Egress Untagged
>          104
> 
> Who returns EOPNOTSUPP for VID 101 and why?
> Why is VID 102 not installed in br0? This part I don't understand from
> your patchset. Does it mean that the CPU port (br0) will have to be
> explicitly configured from now on, even if I run the commands on swp2
> with 'master'?

This does not really answer your questions, but maybe let's agree on the
user visible behavior. My expectations would be the following should be
happening with this patch applied:

- when the VLAN is created for the first and is configured on either the
bridge master device (br0) or an user port (swp2), it gets programmed
into the switch for the CPU port and respectively CPU port and swp2 port

- when you change the bridge master device VLAN attributes, or
add/delete a new one, the programming targets only the CPU port with the
proper operation

That way, there would be no change in how the initial VLAN programming
is done, in that the CPU port still gets programmed, but later on, if
you want e.g.: your CPU port not to be tagged, but untagged into a
particular VLAN.

My upcoming weeks don't look great in terms of resuming active or semi
active DSA work, but working with the DSA mock-up driver might be an
option to avoid spending too much time testing on real HW.
-- 
Florian