netdev - Re: [PATCH net-next] bridge: Set the pvid for untaged packet before prerouting

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]

Message-ID: <55b99afa-7ce5-f9e3-89fb-6f3d17985519@cumulusnetworks.com>
Date:   Mon, 10 Jun 2019 18:21:32 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     wenxu@...oud.cn, roopa@...ulusnetworks.com
Cc:     bridge@...ts.linux-foundation.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] bridge: Set the pvid for untaged packet before
 prerouting

On 10/06/2019 12:44, wenxu@...oud.cn wrote:
> From: wenxu <wenxu@...oud.cn>
> 
> bridge vlan add dev veth1 vid 200 pvid untagged
> bridge vlan add dev veth2 vid 200 pvid untagged
> 
> nft add table bridge firewall
> nft add chain bridge firewall zones { type filter hook prerouting priority - 300 \; }
> nft add rule bridge firewall zones counter ct zone set vlan id map { 100 : 1, 200 : 2 }
> 
> As above set the bridge port with pvid, the received packet don't contain
> the vlan tag which means the packet should belong to vlan 200 through pvid.
> User can do conntrack base base on vlan id and map the vlan id to zone id
> in the prerouting hook.
> 
> Signed-off-by: wenxu <wenxu@...oud.cn>
> ---
>  net/bridge/br_input.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

Nacked-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>

Hi,
I don't think this is a good idea for a few reasons:
- duplicating code (pvid insertion by __allowed_ingress)
- adding 2 new tests in the fast path (even 3 in the vlan filtering case)
- issue can be solved with current state by using different config

Why do you need the vid to be set when you can assume that all the traffic from
that port belongs to the pvid vlan ? In this case you can match the port ifindex
for example and associate the zones based on that. Another approach - you can
insert the vlan by tc's vlan action on ingress, you'll get the same effect.

Overall this looks like an issue solvable by different config instead of adding new tests
in the fast path and increasing the complexity of the bridge input code for little value.

Cheers,
 Nik