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?

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).

I agree debugnet is reasonable

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.

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.

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

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.

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).

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.

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.

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.

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.

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 ?? ()

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. :-)

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

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

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.)

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"

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.)