netdump: Ref the interface we're attached to

[cem]

Summary

Serialize netdump configuration / deconfiguration, and discard our
configuration when the affiliated interface goes away.

Test Plan

$ dumpon ... vtnet0
$ dumpon -lv
...
vtnet0
...
$ devctl detach vtnet0
$ ifconfig
...
(no vtnet0)
$ dumpon -lv
...
(no vtnet0)

No witness warnings I could see with that workflow on GENERIC.

[cem]

Tag :)

[vangyzen]

sys/netinet/netdump/netdump_client.c

1432 ↗

(On Diff #57203)

Since you're modifying this function, go ahead and fix this copy-pasto.

1436 ↗

(On Diff #57203)

...and this one.

1460 ↗

(On Diff #57203)

Shouldn't you

		if_rele(nd_ifp);
		nd_ifp = NULL;

here, too? (if it's not NULL)

In fact, there is enough duplication between this block and netdump_ifdetach() that you could refactor into a new netdump_unconfigure().

[cem]

sys/netinet/netdump/netdump_client.c

1432 ↗

(On Diff #57203)

I'll do that separately — it's logically distinct from this change.

1460 ↗

(On Diff #57203)

Yes, good catch on both counts, thanks!

I thought nd_enabled <==> (nd_ifp != NULL), but it doesn't really appear to be the case. I think it probably should be; the only added complexity is the net.netdump.enabled sysctl becomes a proc instead.

This also suggests that various places that set nd_enabled = 0 should also release the interface ref — probably using your proposed netdump_unconfigure();

[cem]

• Refactor de-configuration into netdump_unconfigure() routine
• Rebase on D20233

[markj]

sys/netinet/netdump/netdump_client.c

1273 ↗

(On Diff #57289)

Perhaps consider using a bool netdump_enabled(void) predicate which asserts that the conf lock is held.

[cem]

sys/netinet/netdump/netdump_client.c

1273 ↗

(On Diff #57289)

Ok

[cem]

• Add and use netdump_enabled() predicate. We rely on sx_assert neutering

during kdb/panic to obviate the lock assertion during the dump-time code, which
generally does not take locks.

[markj]

In D20206#435984, @cem wrote:

• Add and use netdump_enabled() predicate. We rely on sx_assert neutering

during kdb/panic to obviate the lock assertion during the dump-time code, which
generally does not take locks.

Where are we calling netdump_enabled() after a panic?

[cem]

In D20206#435995, @markj wrote:

Where are we calling netdump_enabled() after a panic?

netdump_send, netdump_network_poll, etc. Via MPASS().

[markj]

sys/netinet/netdump/netdump_client.c

997 ↗

(On Diff #57290)

This seems redundant now.

1193 ↗

(On Diff #57290)

Style: missing newline after '{'.

[cem]

sys/netinet/netdump/netdump_client.c

997 ↗

(On Diff #57290)

Ah, thanks. Will fix before commit.

1193 ↗

(On Diff #57290)

Will fix.