Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 1
- [Low] The new/modified worst-case span parameters ("1p5x-split" line
390, "2p2x-large-split" line 396, "3p3x-large-split" line 400) are…
--
net: mctp: usblib: Add to parameterized kunit tests.
Adds three worst-case entries to the mctp_usblib_test_pkt_spans[] table: a
single packet split over five transfers on and between the headers, and two
large-packet cases that split at the worst case for buffer allocation. The
existing "2p2x-large-split" entry is also adjusted so the trailing span is a
single byte.
> diff --git a/drivers/net/mctp/mctp-usblib-test.c
> b/drivers/net/mctp/mctp-usblib-test.c
> index 9df401a914ff..4c27307900fb 100644
> --- a/drivers/net/mctp/mctp-usblib-test.c
> +++ b/drivers/net/mctp/mctp-usblib-test.c
> @@ -386,12 +386,18 @@ static const struct mctp_usblib_test_pkt_span
> mctp_usblib_test_pkt_spans[] = {
> { "1p2x-split-mctphdr", 1, { 16 }, 2, { 6, 10 } },
> /* Single packet split over 3 transfers, middle entirely continuation */
> { "1p3x-split", 1, { 12 }, 3, { 4, 4, 4 } },
> + /* A packet split over 5 transfers, splitting on and between each
> header. */
> + { "1p5x-split", 1, { 12 }, 5, { 3, 1, 1, 3, 4}},
> /* Max-sized single transfer */
> { "1p1x-large", 1, { 8191 }, 1, { 8191 } },
> /* Two large packets, split at the worst-case for allocation, with a
> * single byte continuing the span
> */
> - { "2p2x-large-split", 2, { 8190, 8190 }, 2, { 8191, 8189 } },
> + { "2p2x-large-split", 2, { 8190, 8191 }, 2, { 8191, 8190 } },
> + /* Three large packets, split at the worst-case for allocation,
> + * with a single byte continuing each span
> + */
> + { "3p3x-large-split", 3, { 8190, 8191, 8191 }, 3, { 8191, 8191, 8190 }
> },
> };
[Severity: Low]
Can these new worst-case vectors actually detect a misplaced payload?
The consumer, mctp_usblib_test_rx_pkt_span(), only makes two checks after
feeding all the transfers:
/* check received packets */
KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, pkt_span->n_pkts);
for (i = 0; ; i++) {
...
if (i < pkt_span->n_pkts)
KUNIT_EXPECT_EQ(test, skb->len, pkt_span->pkts[i] - 4);
The bytes of each reassembled skb are never compared against pktbuf.
The payload is also indistinguishable, since mctp_usblib_test_init_pkt()
zero-fills everything after the 8-byte header:
memcpy(data, &hdr, min(len, sizeof(hdr)));
if (len > sizeof(hdr))
memset(data + sizeof(hdr), 0, len - sizeof(hdr));
So a reassembly bug that keeps the USB/MCTP headers where the library expects
them but places payload bytes at the wrong offset would still yield the
expected qlen and skb->len, and these cases would pass.
That matters most for the two "large-split" entries, which exist to exercise
the compaction path in mctp_usblib_rx_prepare():
skb2 = skb_copy_expand(skb, 0, len, gfp);
if (!skb2)
return -ENOMEM;
dev_kfree_skb_any(skb);
skb = skb2;
...
*bufp = skb_tail_pointer(skb);
A wrong copy or wrong returned write offset there is precisely a payload
placement error the current assertions cannot see.
Would it be worth giving each packet a distinguishable byte pattern in
mctp_usblib_test_init_pkt() and comparing the dequeued skbs against pktbuf
with skb_copy_bits() and KUNIT_EXPECT_MEMEQ, so these vectors validate
reassembly rather than only span accounting?
For reference, this gap remains at the end of the series (as of 7b7c42135cec
the rx span test still asserts only qlen and skb->len; only the newly added
tx tests use KUNIT_EXPECT_MEMEQ).
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-dev-mctp-usb-lib-test-v2-0-313cf15e0b56%40codeconstruct.com.au
