NetBSD Problem Report #59943

From www@netbsd.org  Mon Jan 26 02:56:50 2026
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.netbsd.org", Issuer "R13" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 1A00B1A923D
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 26 Jan 2026 02:56:50 +0000 (UTC)
Message-Id: <20260126025648.EC1CC1A923E@mollari.NetBSD.org>
Date: Mon, 26 Jan 2026 02:56:48 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: usbnet(9) keeps failed packet in queue to retry indefinitely
X-Send-Pr-Version: www-1.0
X-From4GNATS: "campbell+netbsd@mumble.net via gnats" <gnats-admin@NetBSD.org>

>Number:         59943
>Category:       kern
>Synopsis:       usbnet(9) keeps failed packet in queue to retry indefinitely
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Jan 26 03:00:00 +0000 2026
>Last-Modified:  Sun Feb 01 03:35:04 +0000 2026
>Originator:     Taylor R Campbell
>Release:        current, 11, 10, 9, ...
>Organization:
ugroundhogday(4)
>Environment:
>Description:

	In usbnet_start_locked, whose job is to kick off USB transfers
	when there are packets queued for tx on the interface, if
	either:

	(a) the driver rejects the packet, or
	(b) the usbnet chain has no transfer allocated, or
	(c) usbd_transfer fails,

	the packet is left in the interface's tx queue and we break out
	of the loop.

523	while (cd->uncd_tx_cnt < un->un_tx_list_cnt) {
524		IFQ_POLL(&ifp->if_snd, m);
...
533		length = uno_tx_prepare(un, m, c);
534		if (length == 0) {
535			DPRINTF("uno_tx_prepare gave zero length", 0, 0, 0, 0);
536			if_statinc(ifp, if_oerrors);
537			break;
538		}
...
540		if (__predict_false(c->unc_xfer == NULL)) {
541			DPRINTF("unc_xfer is NULL", 0, 0, 0, 0);
542			if_statinc(ifp, if_oerrors);
543			break;
544		}
...
550		usbd_status err = usbd_transfer(c->unc_xfer);
551		if (err != USBD_IN_PROGRESS) {
552			DPRINTF("usbd_transfer on %#jx for %ju bytes: %jd",
553			    (uintptr_t)c->unc_buf, length, err, 0);
554			if_statinc(ifp, if_oerrors);
555			break;
556		}

	https://nxr.netbsd.org/xref/src/sys/dev/usb/usbnet.c?r=1.121#523

	These all strike me as wrong or bonkers.

	(a) If the driver rejects the packet, which in all the drivers
	    I reviewed happens only when the packet does not physically
	    fit in the buffer, surely it makes no sense to retry the
	    packet again because it will almost certainly fail again in
	    the same way.

	(b) The usbnet chain _cannot_ fail to have a transfer allocated
	    because usbnet_start_locked is gated on the success of
	    usbnet_init_tx_rx, which fails if it can't allocate a
	    transfer for every usbnet chain.

	(c) If the hardware fails to actually transfer the packet,
	    surely we should drop it rather than keep retrying, no?
	    That's what we do on usbnet_txeof if the transfer has
	    failed, for example.  The only case in which we should
	    retry, I think, is temporary memory shortage.

	I have seen some USB ethernet interfaces get stuck unable to
	transmit packets until I bring them down and back up again.
	Haven't investigated in a while but this might explain some of
	that.

>How-To-Repeat:

	cod inspection

>Fix:

	Patch in progress!

>Audit-Trail:
From: Taylor R Campbell <riastradh@NetBSD.org>
To: netbsd-bugs@NetBSD.org, gnats-bugs@NetBSD.org
Cc: mrg@NetBSD.org, skrll@NetBSD.org, jakllsch@NetBSD.org
Subject: Re: kern/59943: usbnet(9) keeps failed packet in queue to retry indefinitely
Date: Mon, 26 Jan 2026 03:53:26 +0000

 This is a multi-part message in MIME format.
 --=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG

 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?

 --=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr59940-pr59943-usbnettxfixes"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr59940-pr59943-usbnettxfixes.patch"

 # HG changeset patch
 # User Taylor R Campbell <riastradh@NetBSD.org>
 # 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=3Dusbnet_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 =3D cd->uncd_tx_prod;
  	count =3D 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 =3D=3D 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 <=3D un->un_tx_bufsz);
 =20
  		struct usbnet_chain *c =3D &cd->uncd_tx_chain[idx];
 +		KASSERT(c->unc_xfer !=3D NULL);
 =20
  		length =3D uno_tx_prepare(un, m, c);
  		if (length =3D=3D 0) {
  			DPRINTF("uno_tx_prepare gave zero length", 0, 0, 0, 0);
  			if_statinc(ifp, if_oerrors);
 -			break;
 -		}
 -
 -		if (__predict_false(c->unc_xfer =3D=3D NULL)) {
 -			DPRINTF("unc_xfer is NULL", 0, 0, 0, 0);
 -			if_statinc(ifp, if_oerrors);
 -			break;
 +			m_freem(m);
 +			continue;
  		}
 =20
  		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 =3D true;
 =20
 -		IFQ_DEQUEUE(&ifp->if_snd, m);
 -
  		/*
  		 * If there's a BPF listener, bounce a copy of this frame
  		 * to him.

 --=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG--

From: Nick Hudson <nick.hudson@gmx.co.uk>
To: Taylor R Campbell <riastradh@NetBSD.org>
Cc: "netbsd-bugs@netbsd.org" <netbsd-bugs@NetBSD.org>,
 "gnats-bugs@netbsd.org" <gnats-bugs@NetBSD.org>,
 "mrg@netbsd.org" <mrg@NetBSD.org>,
 "skrll@netbsd.org" <skrll@NetBSD.org>,
 "jakllsch@netbsd.org" <jakllsch@NetBSD.org>
Subject: Re: kern/59943: usbnet(9) keeps failed packet in queue to retry
 indefinitely
Date: Mon, 26 Jan 2026 07:12:16 +0000

 > On 26 Jan 2026, at 03:53, Taylor R Campbell <riastradh@NetBSD.org> wrote=
 :
 >=20
 > 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?
 > <pr59940-pr59943-usbnettxfixes.patch>

 LGTM

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59943 CVS commit: src/sys/dev/usb
Date: Sun, 1 Feb 2026 03:33:19 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sun Feb  1 03:33:19 UTC 2026

 Modified Files:
 	src/sys/dev/usb: usbnet.c

 Log Message:
 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


 To generate a diff of this commit:
 cvs rdiff -u -r1.121 -r1.122 src/sys/dev/usb/usbnet.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Home	PR Database Search