Split out a more generic debugnet(4) from netdump(4)

[cem]

Summary

Debugnet is a simplistic and specialized panic- or debug-time reliable
datagram transport. It can drive a single connection at a time and is
currently unidirectional (debug/panic machine transmit to remote server
only).

It is mostly a verbatim code lift from netdump(4). Netdump(4) remains
the only consumer for now.

The INET-specific logic has been extracted somewhat more thoroughly than
previously in netdump(4), into debugnet_inet.c. UDP-layer logic and up, as
much as possible as is protocol-independent, remains in debugnet.c.

Much of the diff is gratuitous rename from 'netdump_' or 'nd_' to
'debugnet_' or 'dn_' -- sorry. I thought keeping the netdump name on the
generic module would be more confusing than the refactoring.

The only functional change here is the mbuf allocation / tracking.
Instead of functioning solely on netdump-configured interface at
dumpon(8) configuration time, we watch for any debugnet-enabled NIC to
linkup and query it for mbuf parameters at that time. If they exceed
the existing allocation, we re-allocate and track the high watermark.
Otherwise, we leave the pre-panic mbuf allocation alone.

No other functional change intended.

Test Plan

I was able to successfully netdump from my test VM to my workstation running
vanilla netdumpd from binary pkg(8) (n=1).

TODO before commit:

• Only bump __FreeBSD_version a little bit.

[jhb]

Is the intention to support remote GDB over this? If so, that seems super nifty. My only question is on the name. If this is truly restricted to only be used during panics, then panicnet is fine. OTOH, if you could use this even on a non-panicked machine (e.g. if you could configure a dedicated interface that you could attach remote GDB to or that you could send netdumps over from DDB via 'dump' before doing a continue), then maybe something like 'debugnet' is more apt?

[cem]

In D21421#466125, @jhb wrote:

Is the intention to support remote GDB over this?

Yes.

If so, that seems super nifty. My only question is on the name. If this is truly restricted to only be used during panics, then panicnet is fine.

Yes, it's for panics and debug.kdb.enter=1; N-1 cores stopped, interrupts disabled.

OTOH, if you could use this even on a non-panicked machine (e.g. if you could configure a dedicated interface that you could attach remote GDB to or that you could send netdumps over from DDB via 'dump' before doing a continue), then maybe something like 'debugnet' is more apt?

The goal is to support GDB over the net during debug.kdb.enter=1, as well as during actual panic state. I have no objection to the name 'debugnet'. (I am not particularly happy with 'panicnet'; it was just the least bad option I came up with.) If there is consensus (i.e., MarkJ agrees), I'm happy to change it.

I think netdump itself (maybe dump in general?) may still require an actual panic (vs 'dump' from debug.kdb.enter=1 ddb), although I have not researched that (and if so, it can be enhanced independent of this change).

[emaste]

I agree debugnet is reasonable

[emaste]

sys/dev/alc/if_alc.c

67–68

should sort with other net/?
presumably because it's basically an independent stack the IPv4/IPv6 bits ought not be split out into netinet/ or netinet6/?

[cem]

sys/dev/alc/if_alc.c

67–68

Agree re: sort. Driver changes like this were done with simplistic mechanical sed so missed subtleties like header sorting.

Re netinet/6, yeah long term we should support both. Having worked on netdump a little in the past, I’d prefer to keep the level of subdirectory nesting to the minimum acceptable level. Net/ seemed appropriate although admittedly it is outside of my general domain expertise.

[emaste]

Net/ seemed appropriate although admittedly it is outside of my general domain expertise.

I think net/ is justifiable from the perspective that netinet/netinet6 are files for the production IPv4/IPv6 stacks, while netdump/panicnet/debugnet is really a stripped-down special-purpose stack.

[cem]

In D21421#466143, @emaste wrote:

I think net/ is justifiable from the perspective that netinet/netinet6 are files for the production IPv4/IPv6 stacks, while netdump/panicnet/debugnet is really a stripped-down special-purpose stack.

That seems like a reasonable argument to me.

[markj]

+1 to renaming it to debugnet. I have no objection to putting it in net/.

sys/net/panicnet.c

357 ↗

(On Diff #61301)

This function looks like it belongs in panicnet_inet.c.

[markj]

sys/net/panicnet.c

195 ↗

(On Diff #61301)

This code is also IPv4-specific.

[cem]

I'll do the big name change as a single step first, then work on addressing the other good feedback (to make it easier to use Differential's revision comparison feature). Please continue to submit feedback.

[cem]

Just rename to debugnet, etc. No other changes included in this revision.

[emaste]

sys/dev/alc/if_alc.c

67–68

ought to sort while here

sys/kern/kern_mbuf.c

437–445

not part of the renaming?

[cem]

One missed boring name change. No functional change.

[cem]

Address review feedback:

• Sort net/debugnet.h includes in drivers
• More thoroughly extract ipv4 recv/xmit logic into debugnet_inet.c. It's not totally !INET clean but I'll worry about that more when we have a working debugnet_inet6.c.

Tested after these changes and n=1 it seems to work fine.

[cem]

sys/dev/alc/if_alc.c

67–68

well, ok

sys/kern/kern_mbuf.c

437–445

Is the comment that this change was introduced as part of the rename patch? It wasn't, or shouldn't have been. I thought it was part of the original panicnet_ named version uploaded as v0. Or is the question that these haven't been renamed? They have, I think. Please clarify :-)

[marius]

Could you please consistently use if_t instead of struct ifnet * in debugnet(4) components and its support code in e. g. drivers?
IIRC, there's already quite some mixing and matching in that regard in netdump(4) counterparts, for example:
typedef void netdump_init_t(struct ifnet *, int *nrxr, int *ncl, int *clsize);
but:
bge_netdump_init(if_t ifp, int *nrxr, int *ncl, int *clsize)

[cem]

In D21421#466900, @marius wrote:

Could you please consistently use if_t instead of struct ifnet * in debugnet(4) components and its support code in e. g. drivers?

I think this concern is based in a factual misunderstanding. debugnet(4) does use struct ifnet * consistently, and as a project we have a preference to avoid this kind of typedef. style(9):

Avoid using typedefs for structure types. Typedefs are problematic because they do not properly hide their underlying type; for example you need to know if the typedef is the structure itself or a pointer to the structure.

In D21421#466900, @marius wrote:

IIRC, there's already quite some mixing and matching in that regard in netdump(4) counterparts,

This is exaggerated — bge(4) is the only case.

for example:
typedef void netdump_init_t(struct ifnet *, int *nrxr, int *ncl, int *clsize);
but:
bge_netdump_init(if_t ifp, int *nrxr, int *ncl, int *clsize)

Yes, bge is sort of inconsistent with the rest of debugnet/netdump by using if_t is used in these definitions. But there is a good reason for it — if_t is the prevailing style in the driver.

Generally our style preference when making small additions to existing files (e.g., debugnet hooks) is to follow the file's latent style, and I don't think there's a great reason to make an exception here.

[marius]

I generally agree with the style(9) argument, however, if_t explicitly was introduced in r266974 as part of the - back then - new API:
"This commit introduces the 'if_t' type to replace 'struct ifnet *'as the type of a network interface."
So I'd say that new code no longer should use struct ifnet * (netdump(4) was added in r333283, so should have used if_t, too).

[cem]

In D21421#466989, @marius wrote:

I generally agree with the style(9) argument, however, if_t explicitly was introduced in r266974 as part of the - back then - new API:
"This commit introduces the 'if_t' type to replace 'struct ifnet *'as the type of a network interface."

Shrug. Mistakes were made.

So I'd say that new code no longer should use struct ifnet * (netdump(4) was added in r333283, so should have used if_t, too).

I'd take the opposite position on unification — someone should coccinelle replace if_t with struct ifnet* instead. Either way, it is definitely out of scope here.

[markj]

I'm not crazy about the fact that debugnet subsumes most the netdump protocol itself:

• netdump defines some message types that are not in the main debugnet header, so if you define a new debugnet message type, you have to be careful not to collide with netdump.
• netdump is always client-initiated, but it's not clear to me that that's true of netgdb as well.
• netdump assumes that client messages are always ACKs, but presumably that's not going to be true with netgdb.
• The netdump protocol doesn't have a version number, which might not be so great for netgdb.

Isn't it straightforward for debugnet to simply provide an unreliable UDP transport and to define a separate protocol for netgdb? It looks like the code which polls for pending ACKs is the main thing you might have to implement twice. If over time the protocols really end up being more or less identical, we can try and merge them, but without having seen any of netgdb, I feel uneasy about lifting too much of netdump into this generic layer.

[cem]

In D21421#467347, @markj wrote:

I'm not crazy about the fact that debugnet subsumes most the netdump protocol itself:

• netdump defines some message types that are not in the main debugnet header, so if you define a new debugnet message type, you have to be careful not to collide with netdump.

Yes, if you want to listen for netdump and the other protocol on the same port. In other L4 protocols, this is uncommon. I considered adding a "reserved" mh_type table to debugnet.h and ultimately decided against it. But that could be restored.

• netdump is always client-initiated, but it's not clear to me that that's true of netgdb as well.

It is.

• netdump assumes that client messages are always ACKs, but presumably that's not going to be true with netgdb.

Yes, optional full-duplex connections will be added to debugnet in a later revision.

• The netdump protocol doesn't have a version number, which might not be so great for netgdb.

We could subsume the pad field in the HERALD as a version number (currently always zero); or netgdb could go ahead and just add a version number packet as a post-HERALD message type. It doesn't matter too much for this revision, IMO.

Isn't it straightforward for debugnet to simply provide an unreliable UDP transport and to define a separate protocol for netgdb?

Not really; netgdb wants a reliable connection and has similar reasons as netdump to perform the dance with the HERALD packet. The v1 netgdb equivalent of netdumpd proxies an ordinary GDB TCP "target remote foo:1234" into our reliable netgdb UDP transport (netdump/debugnet).

It looks like the code which polls for pending ACKs is the main thing you might have to implement twice.

I guess I don't have a clear picture of which parts of netdump you'd prefer to pull back out of debugnet, so it's hard for me to get a sense of where duplication would occur.

If over time the protocols really end up being more or less identical, we can try and merge them, but without having seen any of netgdb, I feel uneasy about lifting too much of netdump into this generic layer.

They are pretty much identical. The PoC is largely just some extra conditionals in netdump_client.c and a sys/gdb debugport:

...
 sys/gdb/netgdb.c                     | 288 ++++++++++++++++++++++++++++++
 sys/gdb/netgdb.h                     |  14 ++
 sys/netinet/netdump/netdump.h        |   4 +
 sys/netinet/netdump/netdump_client.c | 112 ++++++++----
...

I'm trying for a cleaner separation than that.

If it helps, I don't intend to commit this until the full netgdb stack is available up for your review, anyway.

[markj]

In D21421#467368, @cem wrote:

In D21421#467347, @markj wrote:

I'm not crazy about the fact that debugnet subsumes most the netdump protocol itself:

• netdump defines some message types that are not in the main debugnet header, so if you define a new debugnet message type, you have to be careful not to collide with netdump.

Yes, if you want to listen for netdump and the other protocol on the same port. In other L4 protocols, this is uncommon. I considered adding a "reserved" mh_type table to debugnet.h and ultimately decided against it. But that could be restored.

I mean that debugnet itself partially implements the protocol, so if we want to extend the "base" protocol we have to consider all of its consumers.

• netdump assumes that client messages are always ACKs, but presumably that's not going to be true with netgdb.

Yes, optional full-duplex connections will be added to debugnet in a later revision.

• The netdump protocol doesn't have a version number, which might not be so great for netgdb.

We could subsume the pad field in the HERALD as a version number (currently always zero); or netgdb could go ahead and just add a version number packet as a post-HERALD message type. It doesn't matter too much for this revision, IMO.

I'm sure there is a way to extend the protocol in a backwards-compatible way so that everything still works. I'm just surprised that debugnet subsumes some of the netdump protocol. That could be perfectly fine in the long run, or it might create extra work were we to add a third debugnet consumer or extend netgdb or netdump somehow.

Isn't it straightforward for debugnet to simply provide an unreliable UDP transport and to define a separate protocol for netgdb?

Not really; netgdb wants a reliable connection and has similar reasons as netdump to perform the dance with the HERALD packet. The v1 netgdb equivalent of netdumpd proxies an ordinary GDB TCP "target remote foo:1234" into our reliable netgdb UDP transport (netdump/debugnet).

I see. That dance really isn't very complicated though.

It looks like the code which polls for pending ACKs is the main thing you might have to implement twice.

I guess I don't have a clear picture of which parts of netdump you'd prefer to pull back out of debugnet, so it's hard for me to get a sense of where duplication would occur.

Basically, debugnet_handle_ack() and the loop in debugnet_send(). In my head, for debugnet_send() the lower layer would provide a header (pointer to a netdump message header, or a netgdb message header, or whatever) and a pointer to data. The header can be copied into an mbuf, and follows the UDP header which is constructed by debugnet, and the data would be attached to an mbuf to get an ext cluster.

I understand that making debugnet's consumer interface might make things more brittle elsewhere, so maybe it is not the right approach. For instance, debugnet_any_ifnet_update() needs to have an idea of how many mbufs we need per transmission.

If over time the protocols really end up being more or less identical, we can try and merge them, but without having seen any of netgdb, I feel uneasy about lifting too much of netdump into this generic layer.

They are pretty much identical. The PoC is largely just some extra conditionals in netdump_client.c and a sys/gdb debugport:

...
 sys/gdb/netgdb.c                     | 288 ++++++++++++++++++++++++++++++
 sys/gdb/netgdb.h                     |  14 ++
 sys/netinet/netdump/netdump.h        |   4 +
 sys/netinet/netdump/netdump_client.c | 112 ++++++++----
...

I'm trying for a cleaner separation than that.

If it helps, I don't intend to commit this until the full netgdb stack is available up for your review, anyway.

Sounds good, thank you.

[cem]

The full stack is up now, with a more or less working PoC Python proxy. If you would like to try it out, usage is something like:

Term 1:

$ python3 netgdb_proxy.py -p 8000
Waiting for connection from NetGDB client on :20025.
Use 'netgdb -s <this machine's ip>' at the 'db>' prompt to connect.
...

Term 2, a VM or whatever running this patch stack:

$ mount -uf -o current,ro / && sysctl debug.kdb.panic=1
...
db> netgdb -s <ip_of_proxy>
debugnet: overwriting mbuf zone pointers
debugnet_connect: Destination address is on link.
debugnet_connect: Connecting to ...:20025 from ...:20026 on vtnet0
debugnet_connect: searching for server MAC...
...
debugnet_handle_arp: got server MAC address ...
(detaching GDB will return control to DDB)
Switching to gdb back-end
...

Back in Term 1:

Connection from NetGDB at ... received.
Waiting for connection from GDB client on port 8000.
Use 'target remote <ip>:8000' from gdb to connect.

Term 3, somewhere with debug symbols for the kernel running in Term 2:

$ ( cd /obj/usr/home/conrad/src/freebsd/amd64.amd64/sys/GENERIC/ ; gdb -ex "target remote localhost:8000" ./kernel.full )
...
Reading symbols from ./kernel.full...
Remote debugging using localhost:8000
...

Back in Term 1:

Connection from GDB client received.
Starting proxy. Press ctrl-C to stop.

Term 3:

Reading symbols from /boot/test.GENERIC/virtio_random.ko...
Reading symbols from /usr/lib/debug//boot/test.GENERIC/virtio_random.ko.debug...
Reading symbols from /boot/test.GENERIC/mac_ntpd.ko...
Reading symbols from /usr/lib/debug//boot/test.GENERIC/mac_ntpd.ko.debug...
kdb_enter (why=why@entry=0xffffffff815fd7f1 "panic", msg=msg@entry=0xffffffff815fd7f1 "panic") at /usr/home/conrad/src/freebsd/sys/kern/subr_kdb.c:478
478                     kdb_why = KDB_WHY_UNSET;
(gdb) bt
#0  kdb_enter (why=why@entry=0xffffffff815fd7f1 "panic", msg=msg@entry=0xffffffff815fd7f1 "panic") at /usr/home/conrad/src/freebsd/sys/kern/subr_kdb.c:478
#1  0xffffffff80beca6d in kdb_enter (why=why@entry=0xffffffff815fd7f1 "panic", msg=msg@entry=0xffffffff815fd7f1 "panic") at /usr/home/conrad/src/freebsd/sys/kern/subr_kdb.c:480
#2  0xffffffff80b9d592 in vpanic (fmt=<optimized out>, ap=ap@entry=0xfffffe0016e316c8) at /usr/home/conrad/src/freebsd/sys/kern/kern_shutdown.c:897
#3  0xffffffff80b9d6ac in panic (fmt=<optimized out>) at /usr/home/conrad/src/freebsd/sys/kern/kern_shutdown.c:835
#4  0xffffffff80bec4af in kdb_sysctl_panic (oidp=0xffffffff81b97fc0 <sysctl___debug_kdb_panic>, arg1=<optimized out>, arg2=<optimized out>, req=0xfffffe0016e31830) at /usr/home/conrad/src/freebsd/sys/kern/subr_kdb.c:195
#5  0xffffffff80bac1e8 in sysctl_root_handler_locked (oid=0xffffffff81b97fc0 <sysctl___debug_kdb_panic>, arg1=0x0, arg2=0, req=0xfffffe0016e31830, tracker=0xfffffe0016e317b0) at /usr/home/conrad/src/freebsd/sys/kern/kern_sysctl.c:174
#6  0xffffffff80badd4f in sysctl_root (arg1=0x0, arg1@entry=0xfffffe0016e31900, arg2=0, arg2@entry=3, req=req@entry=0xfffffe0016e31830, oidp=0x0) at /usr/home/conrad/src/freebsd/sys/kern/kern_sysctl.c:2105
#7  0xffffffff80bae816 in userland_sysctl (td=td@entry=0xfffff800074d35a0, name=name@entry=0xfffffe0016e31900, namelen=3, old=<optimized out>, oldlenp=<optimized out>, inkernel=inkernel@entry=0, new=0x800649008, newlen=4, retval=0xfffffe0016e318f8, flags=0)
    at /usr/home/conrad/src/freebsd/sys/kern/kern_sysctl.c:2262
#8  0xffffffff80baea8b in sys___sysctl (td=0xfffff800074d35a0, uap=0xfffff800074d3968) at /usr/home/conrad/src/freebsd/sys/kern/kern_sysctl.c:2135
#9  0xffffffff8116f387 in syscallenter (td=0xfffff800074d35a0) at /usr/home/conrad/src/freebsd/sys/amd64/amd64/../../kern/subr_syscall.c:144
#10 amd64_syscall (td=0xfffff800074d35a0, traced=0) at /usr/home/conrad/src/freebsd/sys/amd64/amd64/trap.c:1180
#11 <signal handler called>
#12 0x000000080098f40a in ?? ()
#13 0x000000080098e9ad in ?? ()
#14 0x00000000ffffff9f in ?? ()
#15 0x0000000000000003 in ?? ()
#16 0x00007fffffffdd5f in ?? ()
#17 0x0000000000000004 in ?? ()
#18 0x0000000800649008 in ?? ()
#19 0x0000000000000000 in ?? ()

[markj]

This seems fine to me. I still don't love the tight coupling between netdump and debugnet - the aux1_add_sent hack is a case in point. That said, it's a step in the right direction to get netgdb, and we can always further refactor debugnet in the future if it proves to be worth the effort.

sys/amd64/conf/GENERIC

115

I wish we had a way to automatically configure DEBUGNET if NETDUMP (or NETGDB) is configured - it's unfortunate that anyone with a custom configuration containing options NETDUMP is going to have to update. Oh well.

One alternative would be to not add a new option, and instead conditionally compile the debugnet .c files depending on whether NETDUMP or NETGDB is configured. In debugnet.h we could then have

#if defined(NETDUMP) || defined(NETGDB)
#define DEBUGNET 1
#endif

It's hacky, but I think we do similar things elsewhere for subsystems that don't have a lot of consumers as in this case. I don't strongly prefer one approach over the other though.

sys/net/debugnet.c

197

Unhandled XXX?

sys/net/debugnet.h

145

The indentation here is off.

[cem]

In D21421#477006, @markj wrote:

This seems fine to me. I still don't love the tight coupling between netdump and debugnet - the aux1_add_sent hack is a case in point.

I went ahead and used aux1 for the same purpose (essentially, fragment offset tracking) in netgdb, so maybe it's just more appropriate to rename aux1 to "fragment_offset" and remove the optionality? Both protocols now use it in the same way.

That said, it's a step in the right direction to get netgdb, and we can always further refactor debugnet in the future if it proves to be worth the effort.

Right. Nothing here is written in stone. :-)

sys/amd64/conf/GENERIC

115

Yeah, I agree it's a gap in our config system. I'm not opposed to making debugnet automatic on enable of either subsystem, but I'd prefer to just leave it as-is since it matches one of the prevailing patterns and is easier than restructuring.

sys/net/debugnet.c

197

Will clean up.

sys/net/debugnet.h

145

It isn't, or rather, this is intentional. It's a nested bullet point.

[cem]

• Clean up stale XXX comment
• Rename mh_aux1 and associated back to original mh_offfset
• Former change obviates indented/nested bullet point comment

[cem]

sys/net/debugnet.c

259

This should probably be htobe64(sent_so_far).

[cem]

• Send fragment offset when auxdata is NULL

[cem]

Committed in r353685, I'm not sure why Phabricator has not closed this yet.

[emaste]

Committed in r353685, I'm not sure why Phabricator has not closed this yet.

For some reason Phabricator is not auto-closing some reviews. One note, if you use rS instead of r Phabricator will make it a link to the commit - e.g. rS353685

[jhb]

In D21421#482078, @emaste wrote:

Committed in r353685, I'm not sure why Phabricator has not closed this yet.

For some reason Phabricator is not auto-closing some reviews. One note, if you use rS instead of r Phabricator will make it a link to the commit - e.g. rS353685

You can also "Edit related revisions" to associate the review with a commit just like a normal auto-close would (the commit is listed at the top as a link, etc.)

[jhb]

In D21421#482081, @jhb wrote:

In D21421#482078, @emaste wrote:

Committed in r353685, I'm not sure why Phabricator has not closed this yet.

For some reason Phabricator is not auto-closing some reviews. One note, if you use rS instead of r Phabricator will make it a link to the commit - e.g. rS353685

You can also "Edit related revisions" to associate the review with a commit just like a normal auto-close would (the commit is listed at the top as a link, etc.)

Sorry, "Edit Related Objects -> Edit Commits"

[cem]

In D21421#482078, @emaste wrote:

For some reason Phabricator is not auto-closing some reviews.

:-(

One note, if you use rS instead of r Phabricator will make it a link to the commit - e.g. rS353685

(I'm not a fan of that markup, since it can't be copy-pasted directly outside of phabricator. I'd really prefer Phabricator just grok rNNNN directly.)

[cem]

(Committed, link/revision above.)