Re: [PATCH net-next v5 00/12] net: mctp: usb: Add support for MCTP-over-USB v1.1
From: Jeremy Kerr <jk@codeconstruct.com.au>
Date: 2026-07-27 02:18:58
Also in:
linux-usb
Hi all, Some review feedback from sashiko, along with responses:
quoted hunk ↗ jump to hunk
--- Patch [2]: [PATCH net-next v5 02/12] net: mctp: usb: Use packet-length max for maximum packet-size check ---commit df3ce01edadf3abe9a5e04b1500317e960abd98f Author: Jeremy Kerr [off-list ref] net: mctp: usb: Use packet-length max for maximum packet-size check This commit updates the maximum packet size check in mctp-usb to use a new define based on the u8 transport header length field. It also applies this new maximum to the MTU calculation.quoted
diff --git a/drivers/net/mctp/mctp-usb.c b/drivers/net/mctp/mctp-usb.c index 545eff06322ce..c6e36b63e87a0 100644 --- a/drivers/net/mctp/mctp-usb.c +++ b/drivers/net/mctp/mctp-usb.c@@ -76,7 +76,7 @@ static netdev_tx_t mctp_usb_start_xmit(struct sk_buff *skb, plen = skb->len; - if (plen + sizeof(*hdr) > MCTP_USB_1_0_XFER_SIZE) + if (plen + sizeof(*hdr) > MCTP_USB_1_0_PKTLEN_MAX) goto err_drop; rc = skb_cow_head(skb, sizeof(*hdr));This isn't a bug introduced by this patch, but could a transient USB error during TX permanently stall the network device's transmit queue?
This is a shortcoming in the existing tx path, and is addressed by patch 05/12. [...]
This is also a pre-existing issue, but does the RX polling loop stop permanently on transient USB errors?
... and this by patch 04/12.
quoted hunk ↗ jump to hunk
--- Patch [5]: [PATCH net-next v5 05/12] net: mctp: usblib: Move TX transfer processing to mctp-usblib ---commit ee53d6679c1c34404c4b886475dd086d680df184 Author: Jeremy Kerr [off-list ref] net: mctp: usblib: Move TX transfer processing to mctp-usblib This commit moves the TX processing alongside RX processing in mctp-usblib to accommodate packed transfers where a transfer may contain multiple MCTP packets. It splits the transmit API into push, send, and complete stages and fixes an inconsistency between tx and rx stats.quoted
diff --git a/drivers/net/mctp/mctp-usblib.c b/drivers/net/mctp/mctp-usblib.c --- a/drivers/net/mctp/mctp-usblib.c +++ b/drivers/net/mctp/mctp-usblib.c[ ... ]quoted
+static int mctp_usblib_tx_skb_prepare(struct sk_buff *skb, + enum skb_drop_reason *reason) +{ + struct mctp_usb_hdr *hdr; + unsigned long plen; + int rc;[ ... ]quoted
+ hdr = skb_push(skb, sizeof(*hdr)); + if (!hdr) { + *reason = SKB_DROP_REASON_NOMEM; + return -ENOMEM; + } + + hdr->id = cpu_to_be16(MCTP_USB_DMTF_ID); + hdr->rsvd = 0; + hdr->len = plen + sizeof(*hdr);This isn't a bug introduced by this patch, but does this assignment miss a cpu_to_be16() conversion? The len field in struct mctp_usb_hdr is defined as __be16, but it appears to be assigned a native integer value here.
At this point in the series, hdr->len is a u8, not __be16. We change this to a __be16 two patches later in 07/12, and at the same time add the necessary endian conversion to all uses of hdr->len.
quoted hunk ↗ jump to hunk
--- Patch [10]: [PATCH net-next v5 10/12] net: mctp: usblib: Add initial kunit tests ---commit 6e78640ea8bf2b13bb32c513ae1e30a04bebf125 Author: Jeremy Kerr [off-list ref] net: mctp: usblib: Add initial kunit tests This commit adds initial KUnit tests for the MCTP USB library receive path to verify extraction of MCTP packets from incoming USB transfer data.quoted
diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c new file mode 100644 --- /dev/null +++ b/drivers/net/mctp/mctp-usblib-test.c[ ... ]quoted
+static int mctp_usblib_test_dst_output(struct mctp_dst *dst, + struct sk_buff *skb) +{ + struct mctp_usblib_test_dev *dev = netdev_priv(skb->dev); + + skb_queue_tail(&dev->rx_pkts, skb); + + return 0; +}Could this lead to memory corruption if an external packet is routed here?
This is testcase code, so intentionally simplified, and typically running within the kunit harness. We should not be receiving or routing external packets at this point. Cheers, Jeremy