Page MenuHomeFreeBSD

dhclient: support VID 0 (no vlan) decapsulation
Needs ReviewPublic

Authored by franco_opnsense.org on Aug 12 2021, 8:14 AM.

Details

Reviewers
kevans
markj
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)

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 41522
Build 38411: arc lint + arc unit

Event Timeline

sbin/dhclient/bpf.c
199

EVL_VLID_MASK?

sbin/dhclient/bpf.c
199

nice catch, thanks

sbin/dhclient/bpf.c
200

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.

  • skip to end on vlanid != 0 and add comments
sbin/dhclient/bpf.c
200

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.

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.

This revision is now accepted and ready to land.Tue, Aug 24, 2:17 PM

Should we ideally handle VLAN=0 in kernel?

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?

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?

add comment about need to test for VID 0 presence

This revision now requires review to proceed.Wed, Sep 15, 8:07 AM

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!