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

In D19960#1253517, @thj wrote:

I don't think there are any links with an mtu to carry 65,575, but if there
ever are I would expect to find some bugs.

I'm not sure this is sufficient. It is still possible for tcpdump to have started, but not gotten to the point of actually opening the pflog device.