git: 40ad87e958e8 - main - usb: if_ure: stop touching the mbuf accounting on rxq insertion
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Sun, 20 Apr 2025 18:28:20 UTC
The branch main has been updated by kevans:
URL: https://cgit.FreeBSD.org/src/commit/?id=40ad87e958e8a9cd5d5c3380e709c7503dd12210
commit 40ad87e958e8a9cd5d5c3380e709c7503dd12210
Author: Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2025-04-20 18:28:12 +0000
Commit: Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2025-04-20 18:28:12 +0000
usb: if_ure: stop touching the mbuf accounting on rxq insertion
uether_rxmbuf() assumes the uether_newbuf() model where the caller has
just set m_len to the entire size of the mbuf, and passed the final
size into uether_rxmbuf() for finalization.
In if_ure we're creating our own mbuf chains, and the accounting is all
already accurate. uether_rxmbuf's logic won't work at all for a chain,
so let's add an assertion to that effect (but I think the other callers
were all OK).
This fixes a panic on my Windows DevKit when undergoing any kind of
network stress that surfaces after the bogus mbuf is injected into the
network stack (usually observed in `m_dup`).
markj and I had spent some time investigating it and decided there's
some kind of buffer underrun happening; the rx packet descriptor reports
a length longer than what's actually available. We discard the rest of
the transfer, but then we end up fetching it in a subsequent transfer
and end up casting packet data to a `struct ure_rxpkt` incorrectly. It
would be better to fix the underlying root cause, but this is a
reasonable mitigation in the interim.
Reviewed by: markj
Fixes: 7d5522e16a ("A major update to the ure driver.")
Differential Revision: https://reviews.freebsd.org/D43465
---
sys/dev/usb/net/if_ure.c | 8 +++++++-
sys/dev/usb/net/usb_ethernet.c | 9 ++++++++-
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/sys/dev/usb/net/if_ure.c b/sys/dev/usb/net/if_ure.c
index 7c9b74334b58..c3f7b622d687 100644
--- a/sys/dev/usb/net/if_ure.c
+++ b/sys/dev/usb/net/if_ure.c
@@ -707,7 +707,13 @@ ure_bulk_read_callback(struct usb_xfer *xfer, usb_error_t error)
/* set the necessary flags for rx checksum */
ure_rxcsum(caps, &pkt, m);
- uether_rxmbuf(ue, m, len - ETHER_CRC_LEN);
+ /*
+ * len has been known to be bogus at times,
+ * which leads to problems when passed to
+ * uether_rxmbuf(). Better understanding why we
+ * can get there make for good future work.
+ */
+ uether_rxmbuf(ue, m, 0);
}
off += roundup(len, URE_RXPKT_ALIGN);
diff --git a/sys/dev/usb/net/usb_ethernet.c b/sys/dev/usb/net/usb_ethernet.c
index 977805cefe66..692ea64128b9 100644
--- a/sys/dev/usb/net/usb_ethernet.c
+++ b/sys/dev/usb/net/usb_ethernet.c
@@ -593,7 +593,14 @@ uether_rxmbuf(struct usb_ether *ue, struct mbuf *m,
/* finalize mbuf */
if_inc_counter(ifp, IFCOUNTER_IPACKETS, 1);
m->m_pkthdr.rcvif = ifp;
- m->m_pkthdr.len = m->m_len = len;
+ if (len != 0) {
+ /*
+ * This is going to get it wrong for an mbuf chain, so let's
+ * make sure we're not doing that.
+ */
+ MPASS(m->m_next == NULL);
+ m->m_pkthdr.len = m->m_len = len;
+ }
/* enqueue for later when the lock can be released */
(void)mbufq_enqueue(&ue->ue_rxq, m);