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: <660463ff-9e87-0221-11d9-6d04e57d901f@vyatta.att-mail.com>
Date:   Wed, 3 Apr 2019 18:43:54 +0100
From:   Mike Manning <mmanning@...tta.att-mail.com>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.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 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.

> 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.