dhclient: support VID 0 (no vlan) decapsulation

[franco_opnsense.org]

Summary

VLAN ID 0 is supposed to be interpreted as having no VLAN with
a bit of priority on the side, but the kernel is not able to
decapsulate this on the fly so dhclient needs to take care of it.

Test Plan

In production in OPNsense since 21.7 (July 2021)

[markj]

sbin/dhclient/bpf.c

200

EVL_VLID_MASK?

[franco_opnsense.org]

Use EVL_VLID_MASK as suggested

[franco_opnsense.org]

sbin/dhclient/bpf.c

200

nice catch, thanks

[markj]

sbin/dhclient/bpf.c

201

Shouldn't we just drop the packet if the comparison fails?

224–233

I think some more detailed comment is worth having here. It's not obvious to me what this block is doing.

[franco_opnsense.org]

• skip to end on vlanid != 0 and add comments

[franco_opnsense.org]

sbin/dhclient/bpf.c

201

Yes, saves time.

224–233

I did this first back in 2018 so I had to take a closer look. Comments may not be well-rounded.

The biggest issue was that BPF_MSH wasn't going to the index based on variable offset through IP or VLAN situation requiring a static comparison between both cases and calculating the result moving it back.

[markj]

I think I'm ok with this change. I would suggest adding an additional comment to the beginning of the filter explaining why we look for VLAN tags at all.

[emaste]

Should we ideally handle VLAN=0 in kernel?

[markj]

In D31515#714094, @emaste wrote:

Should we ideally handle VLAN=0 in kernel?

Hide as in hide the vlan header from the BPF filter? Or hide it from all kernel packet processing?

[franco_opnsense.org]

There is an older discussion about it here https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=224961

Handling it transparently in BPF would avoid the need to do this particular dance, fix all potential consumers and is probably faster. Handling it in the network stack on the other hand is probably a lot of work but would fix non-BPF use cases as well.

FWIW, so far our only report about this in OPNsense was dhclient, but I could look into the BPF approach instead?

[franco_opnsense.org]

add comment about need to test for VID 0 presence

[franco_opnsense.org]

Added motivation for checking for untagged priority to the filter program comments.

After scanning the code I don't feel comfortable bringing the support to bpf(4) in general so this dhclient change is all I can offer at this point.

In any case thanks for review and questions!

[markj]

Sorry for the delay. I have no objections to the change and the implementation looks fine. I am a little wary of committing it without some wider approval: could I ask you to post a short note to freebsd-net@ linking this diff and soliciting opinions? If there are no objections after a week or so I will commit this.

[franco_opnsense.org]

In D31515#724915, @markj wrote:

Sorry for the delay. I have no objections to the change and the implementation looks fine. I am a little wary of committing it without some wider approval: could I ask you to post a short note to freebsd-net@ linking this diff and soliciting opinions? If there are no objections after a week or so I will commit this.

Dispatched a message on Feb 3 but no additional feedback until now, see https://lists.freebsd.org/archives/freebsd-net/2022-February/001248.html

[franco_opnsense.org]

thanks a lot :)