netdev - Re: [PATCH net-next 3/4] bridge: support binding vlan dev link state to vlan member bridge ports

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: <cb4a0a34-bd68-9178-38f1-b0cc136cafcb@cumulusnetworks.com>
Date:   Wed, 3 Apr 2019 21:23:15 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     mmanning@...tta.att-mail.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 3/4] bridge: support binding vlan dev link state
 to vlan member bridge ports

On 03/04/2019 21:17, Nikolay Aleksandrov wrote:
> On 03/04/2019 20:53, Nikolay Aleksandrov wrote:
>> On 03/04/2019 20:43, Mike Manning wrote:
>>> On 02/04/2019 20:22, Nikolay Aleksandrov wrote:
>>>> On 02/04/2019 18:35, Mike Manning wrote:
>>>>> In the case of vlan filtering on bridges, the bridge may also have the
>>>>> corresponding vlan devices as upper devices. A vlan bridge binding mode
>>>>> is added to allow the link state of the vlan device to track only the
>>>>> state of the subset of bridge ports that are also members of the vlan,
>>>>> rather than that of all bridge ports. This mode is set with a vlan flag
>>>>> rather than a bridge sysfs so that the 8021q module is aware that it
>>>>> should not set the link state for the vlan device.
>>>>>
>>>>> If bridge vlan is configured, the bridge device event handling results
>>>>> in the link state for an upper device being set, if it is a vlan device
>>>>> with the vlan bridge binding mode enabled. This also sets a
>>>>> vlan_bridge_binding flag so that subsequent UP/DOWN/CHANGE events for
>>>>> the ports in that bridge result in a link state update of the vlan
>>>>> device if required.
>>>>>
>>>>> The link state of the vlan device is up if there is at least one bridge
>>>>> port that is a vlan member that is admin & oper up, otherwise its oper
>>>>> state is IF_OPER_LOWERLAYERDOWN.
>>>>>
>>>>> Signed-off-by: Mike Manning <mmanning@...tta.att-mail.com>
>>>>> ---
>>>>>  net/bridge/br.c         |  23 ++++++--
>>>>>  net/bridge/br_private.h |  17 ++++++
>>>>>  net/bridge/br_vlan.c    | 143 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 179 insertions(+), 4 deletions(-)
>>>>>
>>>> Hi,
>>>> Please CC bridge maintainers when sending bridge patches.
>>> Thank you very much for the review, I will CC you and Roopa when I have
>>> the v1 series ready.
>>>> One question/thought - can't we add a ports_up counter in the vlan's master
>>>> struct and keep how many ports are up for that vlan ?
>>>
>>> This would have been my preferred choice, but for this one would need to
>>> know the old link state for a port so as to determine if/what link state
>>> transition has occurred for a NETDEV_CHANGE notification. This is if
>>> only a single counter is kept for the vlan for all ports (also it might
>>> be difficult to recover from an error in the counter). I could see it
>>> working if one kept track of the operational state for each port in the
>>> vlan in a data structure specific to this purpose i.e. that is more
>>> efficient than the existing walk. However, speed in processing these
>>> state changes is not that important, also the link state is quickly
>>> determined when it might matter more, i.e. on link up of a port.
>>>
>>
>> Indeed, the NETDEV_CHANGE is harder, but we can keep the last known carrier state
>> in the per-port structure and make a decision based on that and the new state.
>> That wouldn't require any additional structures. Speed is important to us when
>> we deploy the bridge at scale, we have tests with thousands of vlans and devices
>> where this walk would become expensive on link flaps.
>>
> 
> In fact we already have a similar tracking field used for the port state, maybe
> it can be used as an indicator. That state needs to be taken into account anyway
> or the carrier state would be wrong.
> 

Nevermind the last sentence, spoke too quickly. An additional structure may be needed
after all, this will need some investigating.

>>>> The important part would be to keep it correct, i.e. vlan_add/del should inc/dec
>>>> as well as port up/down. Then we can directly update its carrier on port event
>>>> without doing a possible O(n^2) walk, we just need to walk over the port vlans
>>>> and adjust counters which is always O(n) based on num of that port's vlans.
>>>>
>>>> Some more comments below.
>>> I will make all the other changes you have requested.
>>>
>>
>> Thanks!
>>
>