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