Linux Ethernet Bridging - Jun 2019 - [Bridge] [PATCH net-next] bridge: Set the pvid for untaged packet before prerouting

From: wenxu <wenxu at ucloud.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 at ucloud.cn>
---
 net/bridge/br_input.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 21b74e7..31b44bc 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -341,6 +341,13 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 	}
 
 forward:
+	if (br_opt_get(p->br, BROPT_VLAN_ENABLED) &&
!skb_vlan_tag_present(skb)) {
+		u16 pvid = br_get_pvid(nbp_vlan_group_rcu(p));
+
+		if (pvid)
+			__vlan_hwaccel_put_tag(skb, p->br->vlan_proto, pvid);
+	}
+
 	switch (p->state) {
 	case BR_STATE_FORWARDING:
 	case BR_STATE_LEARNING:
-- 
1.8.3.1

On 10/06/2019 12:44, wenxu at ucloud.cn wrote:
> From: wenxu <wenxu at ucloud.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 at ucloud.cn>
> ---
>  net/bridge/br_input.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
Nacked-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.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