Page MenuHomeFreeBSD
Differential D40442

wpa_supplicant: Enable receiving priority tagged (VID 0) frames
ClosedPublic
Actions

Authored by rcm on Jun 6 2023, 7:29 PM.
Tags
Referenced Files
Unknown Object (File)
Sun, Nov 24, 11:34 PM
Unknown Object (File)
Sun, Nov 10, 12:55 AM
Unknown Object (File)
Sat, Nov 9, 9:13 PM
Unknown Object (File)
Sat, Nov 9, 7:35 PM
Unknown Object (File)
Sat, Nov 9, 7:17 PM
Unknown Object (File)
Sat, Nov 9, 7:01 PM
Unknown Object (File)
Sat, Nov 9, 6:01 PM
Unknown Object (File)
Sat, Nov 9, 5:56 PM
View All 87 Files
Subscribers
franco_opnsense.org
hayden.james.hill_gmail.com
imp
taylor_taylorjasko.com

Details

Reviewers
kp
cy
bz
Commits
rG1b33afb7e88b: wpa: Enable receiving priority tagged (VID 0) frames
rGcc6d0b4bd2c9: wpa: Enable receiving priority tagged (VID 0) frames
rGbb5d6d14d81b: wpa: Enable receiving priority tagged (VID 0) frames
Summary

Certain internet service providers transmit VLAN 0 priority tagged
EAPOL frames from the ONT towards the residential gateway. VID 0
should be ignored, and the frame processed according to the priority
set in the 802.1P bits and the encapsulated EtherType (i.e. EAPOL).

The pcap filter utilized by l2_packet_* is inadquate for this use case.

Here we modify the pcap filter on FreeBSD to accept both unencapsulated
and encapsulated (with VLAN 0) EAPOL EtherTypes. This preserves the
original filter behavior while also matching on encapsulated EAPOL.

Additional work is required to support this handling on other platforms.

We also modify the rx_receive handler to offset the packet buffer
and length when handling dot1q encapsualted frames so the existing
packet parsing code works as-is.

Sponsored by: Rubicon Communications, LLC ("Netgate")

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

rcm created this revision.Jun 6 2023, 7:29 PM
Herald added a subscriber: imp. · View Herald TranscriptJun 6 2023, 7:29 PM
rcm requested review of this revision.Jun 6 2023, 7:29 PM
lwhsu added a reviewer: cy.Jun 7 2023, 2:07 AM
rcm retitled this revision from wpa_supplicant: Enable receiving priority tagged (VID 0) frames to wip: wpa_supplicant: Enable receiving priority tagged (VID 0) frames.Jun 7 2023, 1:31 PM
rcm updated this revision to Diff 122948.EditedJun 7 2023, 4:31 PM

This revision narrows the scope of change to only focus on FreeBSD related L2 packet handling in wpa_supplicant. The previous revision updated all occurrences of the filter program, however failed to account for packet and length offsets when handling encapsulated frames. Someone else can pick up the torch and provide patches for other platforms upstream as-needed.

rcm retitled this revision from wip: wpa_supplicant: Enable receiving priority tagged (VID 0) frames to wpa_supplicant: Enable receiving priority tagged (VID 0) frames.Jun 7 2023, 4:38 PM
taylor_taylorjasko.com added a subscriber: taylor_taylorjasko.com.Jun 7 2023, 5:57 PM
cy added a reviewer: bz.Jun 8 2023, 12:06 AM
cy added a comment.Jun 8 2023, 12:14 AM

Can you also please send this to our upstream, hostap@lists.infradead.org. I prefer not having our source diverge from upstream too much.

Other than that, looks good. I'll do some regression testing here.

cy added a comment.Jun 8 2023, 12:16 AM

slippy$ git apply /tmp/D40442.diff
error: patch failed: contrib/wpa/src/l2_packet/l2_packet_freebsd.c:99
error: contrib/wpa/src/l2_packet/l2_packet_freebsd.c: patch does not apply
slippy$ patch -C -p1 < /tmp/D40442.diff
Hmm... Looks like a unified diff to me...

The text leading up to this was:

diff --git a/contrib/wpa/src/l2_packet/l2_packet_freebsd.c b/contrib/wpa/src/l2_packet/l2_packet_freebsd.c
--- a/contrib/wpa/src/l2_packet/l2_packet_freebsd.c
+++ b/contrib/wpa/src/l2_packet/l2_packet_freebsd.c

Patching file contrib/wpa/src/l2_packet/l2_packet_freebsd.c using Plan A...
Hunk #1 succeeded at 21.
Hunk #2 failed at 100.
Hunk #3 failed at 131.
2 out of 3 hunks failed while patching contrib/wpa/src/l2_packet/l2_packet_freebsd.c
Hmm... Ignoring the trailing garbage.
done
slippy$

cy requested changes to this revision.Jun 8 2023, 12:17 AM

Patch does not apply.

slippy$ git apply /tmp/D40442.diff 
error: patch failed: contrib/wpa/src/l2_packet/l2_packet_freebsd.c:99
error: contrib/wpa/src/l2_packet/l2_packet_freebsd.c: patch does not apply
slippy$ patch -C -p1 < /tmp/D40442.diff 
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/contrib/wpa/src/l2_packet/l2_packet_freebsd.c b/contrib/wpa/src/l2_packet/l2_packet_freebsd.c
|--- a/contrib/wpa/src/l2_packet/l2_packet_freebsd.c
|+++ b/contrib/wpa/src/l2_packet/l2_packet_freebsd.c
--------------------------
Patching file contrib/wpa/src/l2_packet/l2_packet_freebsd.c using Plan A...
Hunk #1 succeeded at 21.
Hunk #2 failed at 100.
Hunk #3 failed at 131.
2 out of 3 hunks failed while patching contrib/wpa/src/l2_packet/l2_packet_freebsd.c
Hmm...  Ignoring the trailing garbage.
done
slippy$
This revision now requires changes to proceed.Jun 8 2023, 12:17 AM
rcm updated this revision to Diff 122967.Jun 8 2023, 2:54 AM

Not sure why the last diff did not apply. I recreated it. Hopefully this one works.

rcm added a comment.Jun 8 2023, 1:23 PM
In D40442#921284, @cy wrote:

Can you also please send this to our upstream, hostap@lists.infradead.org. I prefer not having our source diverge from upstream too much.

Other than that, looks good. I'll do some regression testing here.

Absolutely. I sent a subscribe email to hostap-join a few days ago actually but I guess it hasn't been manually verified yet (rcm <at> rcm dot sh)

hayden.james.hill_gmail.com added a subscriber: hayden.james.hill_gmail.com.Jun 8 2023, 5:20 PM
rcm updated this revision to Diff 122985.Jun 8 2023, 7:13 PM

There was a regression in the latest diff. That has been fixed now.

rcm added a comment.Jun 9 2023, 4:33 PM

http://lists.infradead.org/pipermail/hostap/2023-June/041627.html

I managed to get subscribed to the hostap mailing list. I also brought along another patch from FreeBSD so the l2 receive code should now be consistent between the two trees.

cy added a comment.Jun 11 2023, 2:25 AM
In D40442#921594, @rcm_rcm.sh wrote:

http://lists.infradead.org/pipermail/hostap/2023-June/041627.html

I managed to get subscribed to the hostap mailing list. I also brought along another patch from FreeBSD so the l2 receive code should now be consistent between the two trees.

Excellent! Thanks.

They take their time. If after a couple of weeks there is no reply or no commit, just email j@w1.fi. Whatever you submit to them will be the commit message.

cy accepted this revision.Jun 11 2023, 3:45 PM

LGTM. I will commit this on Monday for you.

This revision is now accepted and ready to land.Jun 11 2023, 3:45 PM
franco_opnsense.org added a subscriber: franco_opnsense.org.Jun 12 2023, 11:40 AM
franco_opnsense.org added inline comments.
contrib/wpa/src/l2_packet/l2_packet_freebsd.c
104

Is there already a place that handles the explicit VLAN "0" check, because this patch does not? Or do we just assume it has to be "0" because we got the package with a VLAN header?

139

Why print the same value twice?

rcm marked an inline comment as done.Jun 12 2023, 1:21 PM
rcm added inline comments.
contrib/wpa/src/l2_packet/l2_packet_freebsd.c
104

The pcap filter program already performs this assertion implicitly by virtue of the filter itself. It isn't really necessary to test the VID again. I mean, we certainly can but that would require casting to an extended Ethernet header struct that includes dot1q bits just for the purpose of asserting that VID=0.

139

Because we use the value of protocol again in the new ethertype filter here ( vlan 0 and ether proto 0x%x ) )

We need to test for the ether proto twice, both for unencap'd frames and encap'd frames. Without the second test we would match on any frame with a VID=0 regardless of encap'd ethertype, which is not correct.

rcm marked 2 inline comments as done.Jun 12 2023, 1:22 PM
rcm edited the summary of this revision. (Show Details)Jun 12 2023, 1:31 PM
rcm edited the summary of this revision. (Show Details)
Closed by commit rGbb5d6d14d81b: wpa: Enable receiving priority tagged (VID 0) frames (authored by rcm, committed by cy). · Explain WhyJun 12 2023, 2:30 PM
This revision was automatically updated to reflect the committed changes.
cy added a commit: rGbb5d6d14d81b: wpa: Enable receiving priority tagged (VID 0) frames.
franco_opnsense.org added a comment.Jun 12 2023, 7:09 PM

For the print alone it's beneficial to read the VLAN ID and show it. The way it is now it just pushes the maintenance cost to a future point/individual if the PCAP implementation doesn't do what is assumed here (and not even correctly documented as a comment).

rcm added a comment.Jun 12 2023, 7:12 PM
In D40442#922144, @franco_opnsense.org wrote:

For the print alone it's beneficial to read the VLAN ID and show it. The way it is now it just pushes the maintenance cost to a future point/individual if the PCAP implementation doesn't do what is assumed here (and not even correctly documented as a comment).

This isn't printed, it's an snprintf to compose the filter program string.

franco_opnsense.org added a comment.Jun 12 2023, 7:16 PM

For emphasis: I said for clarity it's beneficial to read the VLAN ID and at least show it. Doing it here assuming it's zero but giving no way to verify is simply risky.

cy added a commit: rGcc6d0b4bd2c9: wpa: Enable receiving priority tagged (VID 0) frames.Jun 25 2023, 4:09 AM
cy added a commit: rG1b33afb7e88b: wpa: Enable receiving priority tagged (VID 0) frames.

Revision Contents
Changeset List

PathSize
contrib/
wpa/
src/
l2_packet/
10 lines

Diff 122985

View Options

contrib/wpa/src/l2_packet/l2_packet_freebsd.c

Loading...