bridge: distinguish no vlan and vlan 1

[kp]

Summary

The bridge treated no vlan tag as being equivalent to vlan ID 1, which
causes confusion if the bridge sees both untagged and vlan 1 tagged
traffic.

Instead use 0xfff for no vlan tag, and keep 1 for vlan id 1 (and 2 for ID
2 and so on).

PR: 270559
Suggested by: Zhenlei Huang <[e-mail]>

[zlei]

sys/net/if_bridge.c

394

I'd suggest define VLAN_NONE to 0.

Consider tagged packets with EVL_VLANOFTAG(_m->m_pkthdr.ether_vtag) == 0, they should be treated as untagged ones but with PCP set.

For the VID 0xfff we can refine it as delete all caching forwarding entries for all vlans or find any caching forwarding entries in any vlans. It will be developer friendly.

[philip]

This change looks good in principle, but please adjust the commentary and constant naming to more closely reflect the standard. This makes the code easier to read and understand.

sys/net/if_bridge.c

162

Naming things is hard ... I would suggest naming this constant to align more closely with 802.1Q.

Table 9-2 in the standard calls FFF "Reserved for implementation use". For clarity, perhaps we should define constants for each value in that table, even if we don't use them all. That will make the code a little easier to read for people who are too intimate with the standard (groan).

392–395

Clarify the use of the reserved for implementation constant here.
(I'm not entirely happy with the wording of that comment.)

397

[kp]

sys/net/if_bridge.c

392–395

I'm not sure it's correct either. As Zhenlei points out, if we get a VID_NULL packet (i.e. PCP only) we'd have M_VLANTAG set, but EVL_VLANOFTAG() would give us 0.

We don't want to treat those as different from untagged though, so I'm wondering if we shouldn't use '0' to mean untagged after all.

We do need to be a bit careful in the deletion code, because userspace expects vlan id 0 to mean "delete this address for all vlan ids", and we don't want to break that, while still keeping the code able to distinguish untagged from tagged packets during lookups.

[kp]

• Rename VLAN constants
• use vlan 0 to mean no tag, and DOT1Q_VID_RSVD_IMPL to mean 'any vlan'.

[zlei]

sys/net/if_bridge.c

162

I think it worth to move those defines into sys/net/ethernet.h.
Drivers may also consume them.

2961

So we have vlan 0. That may confuse users.

3104

I'd suggest move this compatible hack into bridge_ioctl_daddr().

It seems ifconfig bridge0 deladdr xx:xx:xx:xx:xx:xx does not allow user to provide a vlan tag, so this compatible hack is transitional.

Anyway this breaks ABI a little.

3107

[kp]

Review remarks.

[kp]

sys/net/if_bridge.c

162

I moved them to if_vlan_var.h, which seems more appropriate than ethernet.h (which in turn is better than keeping it in if_bridge.c.

2961

It may, but I'm not sure it's worth the extra complexity to say 'untagged' or something in what should already be a very rare log message.

3104

We're not changing what userspace sees or has to send us, so I don't see how this breaks the ABI.

[philip]

I think this is good to go. Thank you!

[zlei]

Looks good to me.

And a nit of the comment above #define VLANTAGOF(_m).

sys/net/if_bridge.c

393

This "reserved for implementation" should be updated correspondingly.

[zlei]

sys/net/if_bridge.c

3104

Anyway this breaks ABI a little.

Yes, it does NOT break currently.

Sorry I meant if in the future we want to support deladdr xx:xx:xx:xx:xx:xx vlan 0 then userspace application need to pass DOT1Q_VID_RSVD_IMPL instead of 0, then the behavior changes.