bridge: distinguish no vlan and vlan 1
ClosedPublic
Actions

Authored by kp on Apr 10 2023, 9:33 AM.

Details

Reviewers

zlei
philip

Group Reviewers

network

Commits

rGb0e38a1373c0: bridge: distinguish no vlan and vlan 1

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 <zlei@FreeBSD.org>

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 50833
Build 47724: arc lint + arc unit

Event Timeline

kp created this revision.Apr 10 2023, 9:33 AM

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald TranscriptApr 10 2023, 9:33 AM

kp requested review of this revision.Apr 10 2023, 9:33 AM

Harbormaster completed remote builds in B50814: Diff 120067.Apr 10 2023, 9:33 AM

zlei added inline comments.Apr 11 2023, 1:37 AM

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.

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

This revision now requires changes to proceed.Apr 11 2023, 2:19 AM

kp added inline comments.Apr 11 2023, 6:31 AM

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.

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

Harbormaster completed remote builds in B50833: Diff 120137.Apr 11 2023, 7:40 AM

zlei added inline comments.Apr 11 2023, 8:19 AM

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

Review remarks.

Harbormaster completed remote builds in B50906: Diff 120297.Apr 14 2023, 9:20 AM

kp added inline comments.Apr 14 2023, 9:20 AM

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.

I think this is good to go. Thank you!

philip accepted this revision.Apr 14 2023, 9:26 AM

This revision is now accepted and ready to land.Apr 14 2023, 9:26 AM

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 added inline comments.Apr 14 2023, 9:58 AM

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.