That looks pretty good to me, but I'd like to give Gleb some time to look as well. He's a lot more familiar with unmapped buffers than I am.

In D57851#1327103, @glebius wrote:

In D57851#1327098, @kp wrote:

Do you happen to know of a way to get an interface (struct ifnet) without AF_INET6? I used to use pflog0 for this, and now clearly can't any more. (The test for PR 288263 does that.)
Ideally I'd like to not remove the test, but without such an interface there's no point to it.

The whole point of the change, that now (after pfsync0 de-ifnetting) you can assert that any interface has IPv6 context. Lots of checks for that in netinet6 go away.

Remove epoch cleanup.
Update failing test case to use pfsync0 iso. pflog0 (for now).

mtx -> sx

Do you happen to know of a way to get an interface (struct ifnet) without AF_INET6? I used to use pflog0 for this, and now clearly can't any more. (The test for PR 288263 does that.)
Ideally I'd like to not remove the test, but without such an interface there's no point to it.

• locking fixes
• man page improvement
• fix pflogd to not look for a network interface
• have /etc/rc.d/pflogd create extra log devices if required

In D57851#1326392, @glebius wrote:

My only concern is racyness of sysctl vs sysctl. The node and proc are marked CTLFLAG_MPSAFE, so they can execute in parallel. bpf_attach() doesn't have protection against duplicates, so we will leak one.

Ah, yeah, that's an issue.
I could remove the MPSAFE, but we probably don't want to rely on Giant, so that's not great either.

In D57737#1323743, @oshogbo wrote:

Oh, thank you somehow I have missed it.

Thanks!

Yes. that's indeed a bug, but I fixed it two weeks ago: https://cgit.freebsd.org/src/commit/?id=fcb31b57112425a4eb64241651a0206108105298

In D57537#1321192, @gallatin wrote:

@gallatin Do you want to commit this, or should I? (Reid doesn't have a bit, so someone else needs to.)

If you could push it, that would be great.

Will do.

In D57537#1319082, @gallatin wrote:

It took me a while, but I think your patch is correct.

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.

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.

The pf change looks fine to me.

In D54817#1294095, @lwhsu wrote:

Maybe we should rename ai-policy to ai-note. During the many and length meetings, discussions, a "policy" cannot please everyone.

The current situation and the truth is that people are already using it, and we can't stop them. A strong "NO" will just let some people lie and pretend not using it, or worse, we lost some good contributions.

If we tweak it slightly I guess we can express everything we need.
So here's a version where we deny the call from the indicated securelevel on up,
and don't do anything if the value is 0

I did see a warning about that recently but didn't investigate too much. This (with Gleb's remark) makes sense.

In D56390#1291460, @glebius wrote:

The previous version (modulo the mistake) looked better. What's the point in the additional bool? All existing declarations rely on sparse initialization, so would have .cmd_securelevel = 0 always. If you add cmd_securelevel_set, it would be .cmd_securelevel_set = false. Thus, checking .cmd_securelevel_set for being true has no difference to checking .cmd_securelevel to be positive. I'd suggest to just do the securelevel_gt() check unconditionally.

P.S. Of course the inverted logic of securelevel_gt() really blows one's mind.