netgdb: Fix netgdb double ack, print proxy address

- Fixes netgdb's double ack
- Moving ack responsibility to debugnet, decision to ack made by netdump/netgdb.
- Finish responsibility moved to debugnet, new finish handler.
- netgdb now prints the address to connect to in case the user doesn't have
  access to the proxy machine.

Sponsored by:	Dell EMC
Reviewed By:	markj, bdrewery (earlier version)
Differential Revision:	https://reviews.freebsd.org/D40064

sys/gdb/netgdb.c

@@ -57,6 +57,7 @@ __FBSDID("$FreeBSD$");
 #error "NetGDB cannot be used without DDB at this time"
 #endif
 
+#include <sys/errno.h>
 #include <sys/param.h>
 #include <sys/kdb.h>
 #include <sys/sbuf.h>
@@ -110,64 +111,27 @@ static int *netgdb_prev_kdb_inactive;
 /* TODO(CEM) disable ack mode */
 
 /*
- * Receive non-TX ACK packets on the client port.
+ * Attempt to accept the incoming packet. If we run into ENOBUFS or another
+ * error, return it.
  *
- * The mbuf chain will have all non-debugnet framing headers removed
- * (ethernet, inet, udp).  It will start with a debugnet_msg_hdr, of
- * which the header is guaranteed to be contiguous.  If m_pullup is
- * used, the supplied in-out mbuf pointer should be updated
- * appropriately.
- *
- * If the handler frees the mbuf chain, it should set the mbuf pointer
- * to NULL.  Otherwise, the debugnet input framework will free the
- * chain.
+ * The mbuf chain will have all framing headers removed (ethernet, inet, udp,
+ * debugnet).
  */
-static void
-netgdb_rx(struct debugnet_pcb *pcb, struct mbuf **mb)
+static int
+netgdb_rx(struct mbuf *m)
 {
-	const struct debugnet_msg_hdr *dnh;
-	struct mbuf *m;
 	uint32_t rlen, count;
-	int error;
 
-	m = *mb;
-	dnh = mtod(m, const void *);
-
-	if (ntohl(dnh->mh_type) == DEBUGNET_FINISHED) {
-		sbuf_putc(&netgdb_rxsb, CTRL('C'));
-		return;
-	}
-
-	if (ntohl(dnh->mh_type) != DEBUGNET_DATA) {
-		printf("%s: Got unexpected debugnet message %u\n",
-		    __func__, ntohl(dnh->mh_type));
-		return;
-	}
-
-	rlen = ntohl(dnh->mh_len);
+	rlen = m->m_pkthdr.len;
 #define	_SBUF_FREESPACE(s)	((s)->s_size - ((s)->s_len + 1))
 	if (_SBUF_FREESPACE(&netgdb_rxsb) < rlen) {
 		NETGDB_DEBUG("Backpressure: Not ACKing RX of packet that "
 		    "would overflow our buffer (%zd/%zd used).\n",
 		    netgdb_rxsb.s_len, netgdb_rxsb.s_size);
-		return;
+		return (ENOBUFS);
 	}
 #undef _SBUF_FREESPACE
 
-	error = debugnet_ack_output(pcb, dnh->mh_seqno);
-	if (error != 0) {
-		printf("%s: Couldn't ACK rx packet %u; %d\n", __func__,
-		    ntohl(dnh->mh_seqno), error);
-		/*
-		 * Sender will re-xmit, and assuming the condition is
-		 * transient, we'll process the packet's contentss later.
-		 */
-		return;
-	}
-
-	m_adj(m, sizeof(*dnh));
-	dnh = NULL;
-
 	/*
 	 * Inlined m_apply -- why isn't there a macro or inline function
 	 * version?
@@ -181,6 +145,13 @@ netgdb_rx(struct debugnet_pcb *pcb, struct mbuf **mb)
 		rlen -= count;
 		m = m->m_next;
 	}
+	return (0);
+}
+
+static void
+netgdb_finish(void)
+{
+	sbuf_putc(&netgdb_rxsb, CTRL('C'));
 }
 
 /*
@@ -338,6 +309,7 @@ DB_COMMAND_FLAGS(netgdb, db_netgdb_cmd, CS_OWN)
 	struct debugnet_ddb_config params;
 	struct debugnet_conn_params dcp;
 	struct debugnet_pcb *pcb;
+	char proxy_buf[INET_ADDRSTRLEN];
 	int error;
 
 	if (!KERNEL_PANICKED()) {
@@ -374,6 +346,7 @@ DB_COMMAND_FLAGS(netgdb, db_netgdb_cmd, CS_OWN)
 		.dc_client_port = NETGDB_CLIENTPORT,
 		.dc_herald_aux2 = NETGDB_PROTO_V1,
 		.dc_rx_handler = netgdb_rx,
+		.dc_finish_handler = netgdb_finish,
 	};
 
 	error = debugnet_connect(&dcp, &pcb);
@@ -398,6 +371,19 @@ DB_COMMAND_FLAGS(netgdb, db_netgdb_cmd, CS_OWN)
 	db_cmd_loop_done = 1;
 	gdb_return_to_ddb = true;
 	db_printf("(detaching GDB will return control to DDB)\n");
+
+	const in_addr_t *proxy_addr = debugnet_get_server_addr(netgdb_conn);
+	const uint16_t proxy_port = debugnet_get_server_port(netgdb_conn) + 1;
+	inet_ntop(AF_INET, proxy_addr, proxy_buf, sizeof(proxy_buf));
+	if (inet_ntop(AF_INET, proxy_addr, proxy_buf, sizeof(proxy_buf)) == NULL) {
+		db_printf("Connected to proxy. "
+		    "Use target remote <proxy address>:%hu to begin debugging.\n",
+		    proxy_port);
+	} else {
+		db_printf("Connected to proxy. "
+		    "Use target remote %s:%hu to begin debugging.\n",
+		    proxy_buf, proxy_port);
+	}
 #if 0
 	/* Aspirational, but does not work reliably. */
 	db_printf("(ctrl-c will return control to ddb)\n");

sys/net/debugnet.c

@@ -115,6 +115,22 @@ debugnet_get_gw_mac(const struct debugnet_pcb *pcb)
 	return (pcb->dp_gw_mac.octet);
 }
 
+const in_addr_t *
+debugnet_get_server_addr(const struct debugnet_pcb *pcb)
+{
+	MPASS(g_debugnet_pcb_inuse && pcb == &g_dnet_pcb &&
+	    pcb->dp_state >= DN_STATE_GOT_HERALD_PORT);
+	return (&pcb->dp_server);
+}
+
+const uint16_t
+debugnet_get_server_port(const struct debugnet_pcb *pcb)
+{
+	MPASS(g_debugnet_pcb_inuse && pcb == &g_dnet_pcb &&
+	    pcb->dp_state >= DN_STATE_GOT_HERALD_PORT);
+	return (pcb->dp_server_port);
+}
+
 /*
  * Start of network primitives, beginning with output primitives.
  */
@@ -365,6 +381,8 @@ debugnet_handle_rx_msg(struct debugnet_pcb *pcb, struct mbuf **mb)
 {
 	const struct debugnet_msg_hdr *dnh;
 	struct mbuf *m;
+	uint32_t hdr_type;
+	uint32_t seqno;
 	int error;
 
 	m = *mb;
@@ -383,33 +401,46 @@ debugnet_handle_rx_msg(struct debugnet_pcb *pcb, struct mbuf **mb)
 			return;
 		}
 	}
-	dnh = mtod(m, const void *);
 
+	dnh = mtod(m, const void *);
 	if (ntohl(dnh->mh_len) + sizeof(*dnh) > m->m_pkthdr.len) {
 		DNETDEBUG("Dropping short packet.\n");
 		return;
 	}
 
+	hdr_type = ntohl(dnh->mh_type);
+	if (hdr_type != DEBUGNET_DATA) {
+		if (hdr_type == DEBUGNET_FINISHED) {
+			printf("Remote shut down the connection on us!\n");
+			pcb->dp_state = DN_STATE_REMOTE_CLOSED;
+			if (pcb->dp_finish_handler != NULL) {
+				pcb->dp_finish_handler();
+			}
+		} else {
+			DNETDEBUG("Got unexpected debugnet message %u\n", hdr_type);
+		}
+		return;
+	}
+
 	/*
 	 * If the issue is transient (ENOBUFS), sender should resend.  If
 	 * non-transient (like driver objecting to rx -> tx from the same
 	 * thread), not much else we can do.
 	 */
-	error = debugnet_ack_output(pcb, dnh->mh_seqno);
-	if (error != 0)
+	seqno = dnh->mh_seqno; /* net endian */
+	m_adj(m, sizeof(*dnh));
+	dnh = NULL;
+	error = pcb->dp_rx_handler(m);
+	if (error != 0) {
+		DNETDEBUG("RX handler was not able to accept message, error %d. "
+		    "Skipping ack.\n", error);
 		return;
-
-	if (ntohl(dnh->mh_type) == DEBUGNET_FINISHED) {
-		printf("Remote shut down the connection on us!\n");
-		pcb->dp_state = DN_STATE_REMOTE_CLOSED;
-
-		/*
-		 * Continue through to the user handler so they are signalled
-		 * not to wait for further rx.
-		 */
 	}
 
-	pcb->dp_rx_handler(pcb, mb);
+	error = debugnet_ack_output(pcb, seqno);
+	if (error != 0) {
+		DNETDEBUG("Couldn't ACK rx packet %u; %d\n", ntohl(seqno), error);
+	}
 }
 
 static void

sys/net/debugnet.h

@@ -134,7 +134,10 @@ struct debugnet_conn_params {
 	 *
 	 * The handler should ACK receieved packets with debugnet_ack_output.
 	 */
-	void		(*dc_rx_handler)(struct debugnet_pcb *, struct mbuf **);
+	int			(*dc_rx_handler)(struct mbuf *);
+
+	/* Cleanup signal for bidirectional protocols. */
+	void		(*dc_finish_handler)(void);
 };
 
 /*
@@ -207,6 +210,16 @@ void debugnet_network_poll(struct debugnet_pcb *);
  */
 const unsigned char *debugnet_get_gw_mac(const struct debugnet_pcb *);
 
+/*
+ * Get the connected server address.
+ */
+const in_addr_t *debugnet_get_server_addr(const struct debugnet_pcb *);
+
+/*
+ * Get the connected server port.
+ */
+const uint16_t debugnet_get_server_port(const struct debugnet_pcb *);
+
 /*
  * Callbacks from core mbuf code.
  */

sys/net/debugnet_int.h

@@ -69,8 +69,10 @@ struct debugnet_pcb {
 	void			(*dp_drv_input)(struct ifnet *, struct mbuf *);
 
 	/* RX handler for bidirectional protocols. */
-	void			(*dp_rx_handler)(struct debugnet_pcb *,
-				    struct mbuf **);
+	int			(*dp_rx_handler)(struct mbuf *);
+
+	/* Cleanup signal for bidirectional protocols. */
+	void			(*dp_finish_handler)(void);
 
 	enum dnet_pcb_st	dp_state;
 	uint16_t		dp_client_port;