Removed local var change.

@glebius

So instead of a lockless check for IFF_DYING

I think we eventually do not need IFF_DYING.

In D49447#1127724, @glebius wrote:

So instead of a lockless check for IFF_DYING you use lockless check for presence in STAILQ. I don't see a principal change here, but it could make the races less probable.

This is still WIP, but I want it open for discussing. ifnet_byindex() is renamed to ifnet_byindex_ori() only to show clearly all its usage. Will not be in the final revision.

In D49359#1126431, @zlei wrote:

Well I though that it is the best result that the system panics when hitting this bug, but I was wrong.

The test shows it is even possible to write freed memory. That is,
thread A,

			(*dp->dom_ifdetach)(ifp,
			    ifp->if_afdata[dp->dom_family]);
			ifp->if_afdata[dp->dom_family] = NULL;

but thread B see stall reference, i.e. ifp->if_afdata[dp->dom_family] != NULL.

In D49359#1127318, @franco_opnsense.org wrote:

In D49359#1127313, @zlei wrote:

In D49359#1126467, @franco_opnsense.org wrote:

For 285129 this still crashes in the same place: https://github.com/opnsense/src/issues/207#issuecomment-2733080313

I tested the patch for days, it appears quite stable as far as now ( surely, for the input path only ).

Can you share the steps to repeat the Github issue 207 ? I do not understand the original reporter's steps.

I assume you are not testing with netgraph/ppppoe device? The condition doesn't appear to trigger otherwise.

In D49359#1126467, @franco_opnsense.org wrote:

For 285129 this still crashes in the same place: https://github.com/opnsense/src/issues/207#issuecomment-2733080313

This may conflict with your local work ~

In D49412#1126775, @mjg wrote:

Looking at if_unlink_ifnet:

IFNET_WLOCK();
CK_STAILQ_FOREACH(iter, &V_ifnet, if_link)
        if (iter == ifp) {
                CK_STAILQ_REMOVE(&V_ifnet, ifp, ifnet, if_link);
                if (!vmove)
                        ifp->if_flags |= IFF_DYING;
                found = 1;
                break;
        }

it very much *is* possible to race it and spot the flag.

cpu0                                                                                            cpu1
CK_STAILQ_FOREACH(ifp, &V_ifnet, if_link) {
       /* ifp is now loaded */
                                                                                                     CK_STAILQ_REMOVE(&V_ifnet, ifp, ifnet, if_link);
                                                                                                     ifp->if_flags |= IFF_DYING;
/* the flag *can* be visible by now */
if (strncmp(name, ifp->if_xname, IFNAMSIZ) == 0 &&
!(ifp->if_flags & IFF_DYING))

If anything the bug is that the flag can show up immediately after it was tested for not being there. It may be this happens to be harmless.

However, the claim that flag is impossible to spot here does not hold.

In D49359#1126497, @franco_opnsense.org wrote:

In D49359#1126495, @zlei wrote:

In D49359#1126467, @franco_opnsense.org wrote:

For 285129 this still crashes in the same place: https://github.com/opnsense/src/issues/207#issuecomment-2733080313

Is that based on stable/14 ?

Correct. Am I missing patches for this to make more sense?

In D15865#1126562, @editor_callfortesting.org wrote:

From the Jails Production User call: This work is still of interest, particularly in the context of OCI jail progress.
DCH: "This needs serious rebasing." Do any developers have interest in this feature?

In D49359#1126467, @franco_opnsense.org wrote:

For 285129 this still crashes in the same place: https://github.com/opnsense/src/issues/207#issuecomment-2733080313

Well I though that it is the best result that the system panics when hitting this bug, but I was wrong.

In D49356#1125613, @glebius wrote:

Thanks!

I have only light concerns with aggressive MFC plan on refactoring changes. Sometimes bugs sneak into refactorings that seems to be innocent.

Yeah, I think I have checked that carefully. For this case, I checked the usage of if_unroute() from -current to stable/8.

In D49359#1126171, @markj wrote:

Why exactly does this fix the problem? As I understand, the specific bug is (probably) that NET_EPOCH_WAIT() is used too early.

Yes. This is only part of the fix. Still working on it yet. Found other paths to crash to kernel.

In D49359#1125635, @glebius wrote:

Do you have a stress test suite you use for testing these changes?

Looks good to me.

In D49348#1125484, @markj wrote:

It's probably ok to return EINVAL. I was a bit worried that doing so would result in errors from sysctl -a (which is how I found these bugs in the first place), but I think it's ok. Will update.

This depends on D49357 .

This is only the partial fix. I'm cleaning up the NET_EPOCH_ENTER / NET_EPOCH_EXIT and trying to find out all possible the races.

Those sysctl handlers copy in a pair of sockaddr_in[6] ( pointed by req->newptr) and copy out the xucred, so what's the point of a NULL req->newptr ? Maybe an error EINVAL is better than 0 ?

Looks good to me.

In D49226#1123621, @glebius wrote:

Zhenlei, please go forward with Alexander's review, don't wait on me. Thanks!

In D49212#1123230, @zlei wrote:

In D49212#1123224, @markj wrote:

In D49212#1122671, @franco_opnsense.org wrote:

ifp dump:

In if_flags I see that IFF_DYING is set, which suggests that my theory explains the crash. I think this should be reproducible with a test setup that 1) creates and tears down a PPPoE interface in a loop, 2) tries to send traffic over some route assigned to the interface. Would you be willing to try that?

As for a solution, we can call NET_EPOCH_WAIT() after purging routes in if_detach_internal(), though that's something of a band-aid. The general pattern in if_detach_internal() should be to first remove the ifnet from globally visible data structures, then block and drain readers, then tear down the structure.

Does NET_EPOCH_WAIT() and NET_EPOCH_ENTER() establish a happens-before order ?

I had a look at the implementation of epoch_wait_preempt() and epoch_enter_preempt().

Previous work D45690 by @takahiro.kurosawa_gmail.com . That is IMHO brute force and may relief the issue, but the net stack will have to check the unstable state of ifp a lot.

I think the root cause is, thread A NET_EPOCH_WAIT() does not block thread B's NET_EPOCH_ENTER(). Well the net epoch does protect the liveness of ifp, but it does not guard ifp's state from been changed. For example the two threads execute in the following sequence,

In D49212#1123224, @markj wrote:

In D49212#1122671, @franco_opnsense.org wrote:

ifp dump:

In if_flags I see that IFF_DYING is set, which suggests that my theory explains the crash. I think this should be reproducible with a test setup that 1) creates and tears down a PPPoE interface in a loop, 2) tries to send traffic over some route assigned to the interface. Would you be willing to try that?

As for a solution, we can call NET_EPOCH_WAIT() after purging routes in if_detach_internal(), though that's something of a band-aid. The general pattern in if_detach_internal() should be to first remove the ifnet from globally visible data structures, then block and drain readers, then tear down the structure.

In D49227#1122827, @glebius wrote:

Didn't do a full examination of all stacks, the change by itself should be correct. If any code path triggers the new assertion, that path needs to be fixed.

In D49053#1118518, @zlei wrote:

I personally do not like this ntohl sprinkle everywhere. Some usages in sys/netlink/route/iface.c are also suspicious.

...

That may want more attention from the network .

Looks good to me.
Just a reminder, the commit message should be revised as well.

This appears to be the same with https://bugs.freebsd.org/279653 .

In D49172#1122546, @kevans wrote:

In D49172#1122101, @zlei wrote:

IPv6 packets can be routed via an IPv4 nexthop

IIRC that has not been implemented / supported yet, as IPv6 has much richer addresses, e.g., IPv6 via IPv6 LL nexthop, we do not need IPv6 via IPv4 nexthop.

I noticed that when I went to write the test by copying the wg_basic test and setting up IPv6 on the other side. :-)

That is a perfect valid usage.

D47174 is a better fix.

IPv6 packets can be routed via an IPv4 nexthop

IIRC that has not been implemented / supported yet, as IPv6 has much richer addresses, e.g., IPv6 via IPv6 LL nexthop, we do not need IPv6 via IPv4 nexthop.

Landed as 90953d2f829a .

Landed as 449496eb28da .

panic: bpfread: lost bd_hbuf