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.

Thanks, I don't know how I messed that up, but mess that up I did.

I'm not familiar enough with the build system to have opinions on how it got fixed.

Ah, yes, thanks!

I'm seeing panics with this patch ("panic: lock "pf_keyhash" 0xfffffe00e8dffff8 already initialized").
I believe the problem is that hashalloc() allocates unzero'd memory, and which leads to incorrect assertions on the lock, if LO_INITIALIZED happens to be set in lo_flags.

I've had a quick look at making pf use this, and I have a minor annoyance.

In D55558#1271287, @olivier wrote:

In D55558#1271271, @kp wrote:

I haven't debugged it any depth (and won't be able to before early next week), but the new test case fails for me:

(kp@nut)  /usr/tests/sys/netinet % sudo kyua debug carp:vrrp_preempt                                                                                                               [14:00]
net.inet.carp.preempt: 0 -> 1
net.inet.carp.preempt: 0 -> 1
	vrrp: MASTER vrid 2 prio 10 interval 100
Files left in work directory after failure: created_interfaces.lst, created_jails.lst
ifconfig: interface epair0b does not exist
ifconfig: interface epair1b does not exist
carp:vrrp_preempt  ->  failed: preemption did not affect the second interface

But it works on both my side, and on my VM lab:

olivier@workstation:/usr/tests/sys/netinet $ sudo kyua debug carp:vrrp_preempt
net.inet.carp.preempt: 0 -> 1
net.inet.carp.preempt: 0 -> 1
        vrrp: MASTER vrid 2 prio 10 interval 100
ifconfig: interface epair0b does not exist
ifconfig: interface epair1b does not exist
carp:vrrp_preempt  ->  passed

I haven't debugged it any depth (and won't be able to before early next week), but the new test case fails for me:

One small issue is that this patch claims to move these files. That's clearly unintentional. Perhaps an artefact of how it was uploaded?

This is already in the tree, as cd0169c9379c400ec75b77e87ca770e37f964276. I managed to forget to add the 'differential revision' tag, so Phabricator didn't notice.

(Not tested, but that just seems sensible.)