usb: if_ure: stop touching the mbuf accounting on rxq insertion

[kevans]

Summary

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

Fixes: 7d5522e16a ("A major update to the ure driver.")

[kevans]

sys/dev/usb/net/if_ure.c

614

This also feels a bit off; m_len is initialized to 0, so m_adj will effectively do nothing because every mbuf in the chain should be empty at this point AFAICT. It won't cause any problems, but I guess it could be breaking if_ure on armv7 maybe since we're not actually re-aligning the buffer at all?

[markj]

sys/dev/usb/net/if_ure.c

614

Yes, I'm pretty sure that this is solely for the benefit of platforms which require strict alignment. In particular, uether_newbuf() also initializes m_len first. Probably we should do this adjustment in the first iteration of the loop below.

m_adj() should maybe assert that the mbuf chain is longer than len (when len is positive at least). I kicked off a run of the test suite with that change, just to see if it blows up.

sys/dev/usb/net/usb_ethernet.c

610

Shouldn't this function also check that m->m_len >= len, and if not, adjust the lengths of subsequent mbufs in the chain?

[kevans]

sys/dev/usb/net/usb_ethernet.c

610

Shouldn't this function also check that m->m_len >= len, and if not, adjust the lengths of subsequent mbufs in the chain?

For existing callers this is sufficient (they only ever allocate one cluster and tap that out to the rxq at a time, ure was the exception because jmg improved it). I kind of think long term this finalization should just go away entirely in favor of the callers setting up their mbufs right to begin with

[markj]

Kyle and I looked at the underlying problem for a while yesterday. When running a stress test, it's possible to trigger a buffer underrun in which if_ure's rx packet descriptor reports a packet with length longer than what's available from the USB transfer. In this case, we discard the rest of the transfer. What appears to happen is that the rest of the packet appears in a subsequent transfer, and we end up casting packet data to a struct ure_rxpkt, and in so doing get a bogus packet length. Then, ure_makebuf() returns a multi-mbuf chain which uether_rxmbuf() handles incorrectly.

This patch seems like a reasonable mitigation until the root cause is handled somehow.

sys/dev/usb/net/if_ure.c

708

I'd mention in a comment that len is known to sometimes be bogus.