Thanks!

I suppose we could spell the example rules like this too:
block out quick on $wan from any to { 255.255.255.255, ($wan:broadcast), 224.0.0.0/4, ff00::/8 } received-on any
but they're fine as they are. They result in the same rules in the kernel anyway.

Okay, I think I've got this sorted now. The patch now leaves pf.c
untouched and instead:

In D56559#1295233, @glebius wrote:

In D56559#1294810, @rcm wrote:

So I think the question is really: are we okay with pf_route
forwarding broadcasts when ip_forward (nominally) doesn't? If yes, then I do agree at least a few lines in the man page be introduced that calls out this difference, and maybe prescribes example rules that can be used to plug any undesirable leaks.

I think we are okay. Packet filters are really designed to make network stacks to malfunction.

A good documentation change would be better than a policy.

In D56559#1294810, @rcm wrote:

So I think the question is really: are we okay with pf_route
forwarding broadcasts when ip_forward (nominally) doesn't? If yes, then I do agree at least a few lines in the man page be introduced that calls out this difference, and maybe prescribes example rules that can be used to plug any undesirable leaks.

In D56559#1295079, @kp wrote:

I do also see test failures in sys/netpfil/pf/pfsync:rt_af, sys/netpfil/pf/src_track:sn_types_compat and sys/netpfil/pf/src_track:sn_types_pass with this patch. I've not debugged these, but this change does appear to be responsible.

There are good arguments for both blocking and allowing this I believe.
I'm not entirely sure where I fall. On the one hand, yes, users should be allowed to shoot themselves in the foot if they really want to, but on the other hand, it's non-obvious that this will happen. There are going to be a lot more users in the "I didn't want this to happen but it did" camp than there'd be in the "I want to do this dumb thing and pf won't let me." camp.

In D56559#1294669, @eugen_grosbein.net wrote:

In D56559#1294664, @rcm wrote:

In D56559#1294663, @glebius wrote:

I always assumed "policy routing" by packet filters a tool that allows to shoot into ones leg. I can imagine some weird scenarios where people would use pf to actually inject packets where it won't be routed by the normal stack.

That's fair. Though, currently, an operator who uses pf route-to without realizing it can emit L2 broadcasts across broadcast domains is foot-shooting themselves unknowingly. :)

We do not prevent a superuser from foot-shooting and we do not assume that superuser is stupid. A warning should be enough.

In D56559#1294664, @rcm wrote:

In D56559#1294663, @glebius wrote:

I always assumed "policy routing" by packet filters a tool that allows to shoot into ones leg. I can imagine some weird scenarios where people would use pf to actually inject packets where it won't be routed by the normal stack.

That's fair. Though, currently, an operator who uses pf route-to without realizing it can emit L2 broadcasts across broadcast domains is foot-shooting themselves unknowingly. :)

In D56559#1294663, @glebius wrote:

I always assumed "policy routing" by packet filters a tool that allows to shoot into ones leg. I can imagine some weird scenarios where people would use pf to actually inject packets where it won't be routed by the normal stack.

I always assumed "policy routing" by packet filters a tool that allows to shoot into ones leg. I can imagine some weird scenarios where people would use pf to actually inject packets where it won't be routed by the normal stack.

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.

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).

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).

LGTM. I will commit this on Monday for you.

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.

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.

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

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.

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

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:

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$

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

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.