ifnet: Fix the teardown process of an interface

[zlei]

Summary

The interface should be brought down before it been detached, but lots
of drivers do not. Ideally this should be done on the driver side, but
that requires lots of modification so let's do the job in if_detach_internal().

PR: 279653
PR: 285129
MFC after: 2 weeks

[zlei]

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.

Typically the input / output paths should have

NET_EPOCH_ENTER
...
NET_EPOCH_EXIT
...

NET_EPOCH_ENTER
if ((ifp->if_flags & IFF_UP) == 0)
    abort processing

NET_EPOCH_EXIT

[glebius]

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

[franco_opnsense.org]

I'll do a CFT on this change with OPNsense users next week. A few can trigger the if_afdata panics with PPPoE.

[markj]

Why exactly does this fix the problem? As I understand, the specific bug is (probably) that NET_EPOCH_WAIT() is used too early. At that point, the ifnet is still visible from several global data structures, and in particular the routing tables, since rt_flushifroutes() is not called yet. From what I can see, clearing IFF_UP does not seem to be sufficient. Maybe some drivers set their link state to down when stopped, but is it guaranteed?

sys/net/if.c

1137

I'd explain a bit further that this ensures that the driver sees that IFF_UP is clear.

1139

Should we zero the structure here, just in case drivers peek at other fields besides ifr.ifr_flags and ifr.ifr_flagshigh?

[zlei]

In D49359#1125635, @glebius wrote:

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

I have one Dtrace script and a script for the input path, ether_input_internal() / ip6_input() and can reliable crash the kernel.

There're other crashing paths. Still trying to figure out them all.

[zlei]

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.

At that point, the ifnet is still visible from several global data structures, and in particular the routing tables, since rt_flushifroutes() is not called yet. From what I can see, clearing IFF_UP does not seem to be sufficient. Maybe some drivers set their link state to down when stopped, but is it guaranteed?

Yes, you're right. While reviewing the logic teardown and the netisr part, I can conclude that merely clearing IFF_UP is not sufficient. This only fix one path, that is ether_input_internal() / ip6_input().

[zlei]

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.

[franco_opnsense.org]

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

[zlei]

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 ?

[franco_opnsense.org]

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?

[zlei]

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?

No, this is not complete, but your feedback is useful.

The logic of if_vmove() of stable/13 is a little different from stable/14, and I'm not testing on stable/13 yet. Just asked to confirm.