Do you have a pointer to the syzcaller reproducer for this issue? I'd like to add a test.

In D27279#610046, @rstone wrote:

Try putting entries in the witness_order_list_entry in kern/subr_witness.c specifying the ifnet_sx -> in_multi_sx ordering as correct. That should get better information. Likely the issue is a cycle of length larger than 2 (e.g. in_multi_sx before mystery lock, mystery lock before ifnet_sx, ifnet_sx before in_multi_sx).

The more I look at this the more it looks like it's either flat out a witness bug, or witness not correctly expressing what's going on.

In D27279#609879, @marieheleneka_gmail.com wrote:

Perhaps time to make sure entire kernel & all modules are built from scratch, without reusing old obj files, and without ccache -- just to make sure it's a clean slate?

In D27279#609753, @kevans wrote:

It's not clear to me where the other order is (i.e. where we take the multicast lock first, and then the ifnet lock).

Same... and WITNESS didn't record where the initial order happened, so it's not giving anything useful here.

In D27279#609749, @kevans wrote:

kldload pf; for i in $(seq 1 25); do kyua test -k /usr/tests/sys/netpfil/pf/Kyuafile names; done -- I consistently hit it within 5 iterations of this loop on a mostly idle system, usually within two or three. Normally it's after we've scribbled over something within the now-dead interface and I've triggered the panic in mtrash_ctor() w/ INVARIANTS on, but in the above backtrace it actually trapped instead since I had a little more activity going on.

In D27279#609744, @kevans wrote:

This is the classic race of ioctl handler vs. interface detachment.

Remove now pointless do {} while(0) wrappers. While here also remove a stale comment.

In D27279#609734, @kevans wrote:

I'm still trying to track down what seems to be scribbling on freed memory when I run the pf:names test, but I note that it's also complaining about LOR ifnet_sx (vnet_if_return, though it points to the last line that dropped it?) -> in_multi_sx (in_ifdetach)

Fix comment style(9)

I'm happy with it as-is.

I'd like to be able to MFC D27279. I didn't expect it to rely on this patch, and I suspect this one can't be safely merged to stable/12 because it might break kernel ABI compatibility.

Clarify comment

In D27279#609327, @kevans wrote:

I'm not sure that this is actually safe, unfortunately... we're now holding an rwlock and may need to sleep to acquire an sx in, e.g., me_reassign() (via if_vmove -> ifp->if_reassign -> me_reassign) if someone created a cloner and moved it into the now-dying vnet.

if_vmove() probably actually needs to assert that it won't be recursing on the ifnet locks here.

In D27018#604557, @yannis.planus_alstomgroup.com wrote:

In D27018#604255, @kp wrote:

Also, it appears to panic:
...
It looks like this happens because we enqueue an mbuf and free it later. ip_input() has an mbuf pointer to a freed mbuf here.

Oh, I missed it... I'll have a look at it, how do you produce this panic? Do you know if the mbuf pointer of ip_input() corresponds to the real one or the dupliacted one?

Also, it appears to panic:

I can reproduce the problem at least. I'll see if I can get a sane test case out of it as well.

In D27023#602993, @mhorne wrote:

In D27023#602923, @kp wrote:

What else in the kernel wants to have access to this information?

Nothing yet. NetApp has a module that aggregates stats from various protocols, so that's where this patch is coming from. It would seem that a read-only copy is enough for this use-case, if that's your concern.

I was mostly just wondering. If I'd noticed the inverse (i.e. V_igmpstat is in the header but used nowhere else) I'd be inclined to reverse that. Generally you want as little visibility for your data as possible.
It may be worth adding a comment here to explain why it's in the header.

What else in the kernel wants to have access to this information?

Can you describe a setup that demonstrates the problem this fixes?
(Ideally in the form of a test case, but if you can describe a setup I can probably write the test case.)

Do we see measurable performance improvements with this?

Other than the incorrect comparison in the assertion I think this is good to go.

Thank you for making the pun.

I love to see more ifconfig code move into libifconfig.

Other than the file name nit I think this can go in.

Yeah, the errno made the difference. The tests pass now (and quite a lot faster than before).

Simple /usr/tests/sbin/pfctl % sudo kyua test (So I start kyua in the /usr/tests/sbin/pfctl directory)

I've not yet looked into the why, but this seems to fail for me:

In D26537#591054, @bz wrote:

We'll probably want to add more of these in the future for vnets, so happy we start to lay the grounds.
Will you work on jail/jail.conf to also pick the right set for devfs depending on whether the vnet option is given? If not you should given @jamie a ping6.

You missed the opportunity for a disarming pun.

bridge: Call member interface ioctl() without NET_EPOCH

In D26544#592591, @jrtc27 wrote:

Should we be doing something for user load/store/fetch faults? Probably a SIGBUS?

Hopefully correct patch

Update panic message

Those would also be unrecoverable errors, right?

In D26537#590825, @bz wrote:

Did we ever fix this one?

https://www.openbsd.org/errata48.html
005: SECURITY FIX: December 17, 2010 All architectures
Insufficent initialization of the pf rule structure in the ioctl handler may allow userland to modify kernel memory. By default root privileges are needed to add or modify pf rules.

https://ftp.openbsd.org/pub/OpenBSD/patches/4.8/common/005_pf.patch

It looks like that call to icmp6_error() is no longer present: https://reviews.freebsd.org/rS364982

• Return a1 as well as it can sometimes be used for return values.
• Set rtval to 0 because it's never used. Remove it as an argument for dtrace_invop().

In D26389#586875, @kp wrote:

An alternative might be to pass a1 (as opposed to the 0 we pass now) in the return dtrace_probe(). That'll let scripts access it as arg2. That'd be different from all other platforms, but arg2 is not defined for the return probe anyway.

So this:

Use trapframe rather than rval