NetBSD-Bugs archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: kern/59943: usbnet(9) keeps failed packet in queue to retry indefinitely



The attached (untested) patch tries to address these issues (and some
of those in PR kern/59940: usbnet(9): uno_tx_prepare buffer overrun
audit); details in commit message.  Thoughts?
# HG changeset patch
# User Taylor R Campbell <riastradh%NetBSD.org@localhost>
# Date 1769392552 0
#      Mon Jan 26 01:55:52 2026 +0000
# Branch trunk
# Node ID f2a961ab939686586e96a4a42110282d83dcad30
# Parent  ce5666ee3bee99fc43a60b487110d6418fb6dd5c
# EXP-Topic riastradh-pr59940-usbnettxlenaudit
usbnet(9): Tighten tx path.

1. Verify, don't just assert, that the packet length is below the
   buffer size.  Even if all the callers enforce the interface's MTU,
   I can't prove that the usbnet(9) tx buffer size is an upper bound
   enforced on the interface's MTU.

   We can remove the check later if we do enforce that upper bound at
   some point, which would probably be worth doing anyway since the
   MTU is checked earlier in the tx path.

2. Assert, don't check, that c->unc_xfer is nonnull.  We can only
   reach the tx path if we cross if_init=usbnet_init_rx_tx, and that
   (via usbnet_tx_list_init) is guaranteed to fail and back out
   unless all of the usbnet_chain unc_xfers get initialized.

3. If we can't fit the packet into a buffer, drop it -- don't leave
   it in the queue to try again when it still won't fit in the
   buffer.

4. If the transfer for this packet fails, drop it -- don't leave it
   in the queue to try again just in case it might work better the
   next time.

PR kern/59940: usbnet(9): uno_tx_prepare buffer overrun audit
PR kern/59943: usbnet(9) keeps failed packet in queue to retry indefinitely

diff -r ce5666ee3bee -r f2a961ab9396 sys/dev/usb/usbnet.c
--- a/sys/dev/usb/usbnet.c	Sat Jan 24 23:11:52 2026 +0000
+++ b/sys/dev/usb/usbnet.c	Mon Jan 26 01:55:52 2026 +0000
@@ -521,26 +521,29 @@ usbnet_start_locked(struct ifnet *ifp)
 	idx = cd->uncd_tx_prod;
 	count = 0;
 	while (cd->uncd_tx_cnt < un->un_tx_list_cnt) {
-		IFQ_POLL(&ifp->if_snd, m);
+		IFQ_DEQUEUE(&ifp->if_snd, m);
 		if (m == NULL) {
 			DPRINTF("start called, queue empty", 0, 0, 0, 0);
 			break;
 		}
+		if (m->m_pkthdr.len > un->un_tx_bufsz) {
+			DPRINTF("oversize packet, %ju > %ju",
+			    m->m_pkthdr.len, un->un_tx_bufsz, 0, 0);
+			if_statinc(ifp, if_oerrors);
+			m_freem(m);
+			continue;
+		}
 		KASSERT(m->m_pkthdr.len <= un->un_tx_bufsz);
 
 		struct usbnet_chain *c = &cd->uncd_tx_chain[idx];
+		KASSERT(c->unc_xfer != NULL);
 
 		length = uno_tx_prepare(un, m, c);
 		if (length == 0) {
 			DPRINTF("uno_tx_prepare gave zero length", 0, 0, 0, 0);
 			if_statinc(ifp, if_oerrors);
-			break;
-		}
-
-		if (__predict_false(c->unc_xfer == NULL)) {
-			DPRINTF("unc_xfer is NULL", 0, 0, 0, 0);
-			if_statinc(ifp, if_oerrors);
-			break;
+			m_freem(m);
+			continue;
 		}
 
 		usbd_setup_xfer(c->unc_xfer, c, c->unc_buf, length,
@@ -552,12 +555,11 @@ usbnet_start_locked(struct ifnet *ifp)
 			DPRINTF("usbd_transfer on %#jx for %ju bytes: %jd",
 			    (uintptr_t)c->unc_buf, length, err, 0);
 			if_statinc(ifp, if_oerrors);
-			break;
+			m_freem(m);
+			continue;
 		}
 		done_transmit = true;
 
-		IFQ_DEQUEUE(&ifp->if_snd, m);
-
 		/*
 		 * If there's a BPF listener, bounce a copy of this frame
 		 * to him.


Home | Main Index | Thread Index | Old Index