[PATCH net-next v2 0/7] net: mctp: usblib: Increase coverage of kunit testing

[PATCH net-next v2 0/7] net: mctp: usblib: Increase coverage of kunit testing

[James Lee]

Current usblib tests only check receipt of a limited combination of 
packets lengths.

Added tests of receiving packets with invalid fields, and more complex
packet spanning. Also created tests for transmitting packets, covering
most code, except for mctp_usblib_tx_cancel() and allocation failures.

Signed-off-by: James Lee <james@codeconstruct.com.au>
---
Changes in v2:
- Address concerns identified by Sashiko
- Add missing net-next prefix
- Link to v1: https://lore.kernel.org/r/20260831-dev-mctp-usb-lib-test-v1-0-79699bf67a18@codeconstruct.com.au

---
James Lee (7):
      net: mctp: usblib: Add to parameterized kunit tests.
      net: mctp: usblib: Add test for splits inside headers
      net: mctp: usblib: Add tests of invalid headers
      net: mctp: usblib: Complete rx tests
      net: mctp: usblib: Simplify allocation logic in mctp_usblib_test_rx_init
      net: mctp: usblib: Add initial kunit tx tests
      net: mctp: usblib: Add test for failing append

 drivers/net/mctp/mctp-usblib-test.c | 542 +++++++++++++++++++++++++++++++++++-
 1 file changed, 531 insertions(+), 11 deletions(-)
---
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
change-id: 20260617-dev-mctp-usb-lib-test-151728774e95

Best regards,
-- 
James Lee <james@codeconstruct.com.au>

[PATCH net-next v2 1/7] net: mctp: usblib: Add to parameterized kunit tests.

[James Lee]

Add parameterized tests to cover worst case scenarios for packet
splitting and length.

Signed-off-by: James Lee <james@codeconstruct.com.au>
---
 drivers/net/mctp/mctp-usblib-test.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
index 9df401a914ff537207911f03bd08f32b5a5e20d1..4c27307900fb6116572bc9d89f3def6f06c30397 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 } },
 };
 
 KUNIT_ARRAY_PARAM(mctp_usblib_test_rx_pkt_span, mctp_usblib_test_pkt_spans,

-- 
2.47.3

Re: [PATCH net-next v2 1/7] net: mctp: usblib: Add to parameterized kunit tests.

[netdev-bot+sashiko]

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

[PATCH net-next v2 2/7] net: mctp: usblib: Add test for splits inside headers

[James Lee]

Add test covering every possible split of a packet's headers between
transfers and remove parameterized tests that are now covered.

Signed-off-by: James Lee <james@codeconstruct.com.au>

---
v2:
- Replace direct calls to mctp_usblib_rx_complete()
- Fix sk_buff leak
---
 drivers/net/mctp/mctp-usblib-test.c | 71 +++++++++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
index 4c27307900fb6116572bc9d89f3def6f06c30397..a2ca79f61910af129af956a6f38ad69d5dae8c10 100644
--- a/drivers/net/mctp/mctp-usblib-test.c
+++ b/drivers/net/mctp/mctp-usblib-test.c
@@ -14,6 +14,8 @@
 #include <net/mctpdevice.h>
 #include <linux/usb/mctp-usb.h>
 
+#define HDR_LEN sizeof(struct mctp_usb_hdr)
+
 struct mctp_usblib_test_dev {
 	struct net_device *ndev;
 	struct mctp_dev *mdev;
@@ -378,12 +380,6 @@ static const struct mctp_usblib_test_pkt_span mctp_usblib_test_pkt_spans[] = {
 	{ "1p1x-complete", 1, { 8 }, 1, { 8 } },
 	/* Two small packets combined within one transfer */
 	{ "2p1x-combined", 2, { 8, 8 }, 1, { 16 } },
-	/* A packet split over two transfers, at the MCTP payload */
-	{ "1p2x-split-payload", 1, { 16 }, 2, { 8, 8 } },
-	/* A packet split over two transfers, at the USB transport header */
-	{ "1p2x-split-usbhdr", 1, { 16 }, 2, { 2, 14 } },
-	/* A packet split over two transfers, at the MCTP header */
-	{ "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. */
@@ -403,10 +399,73 @@ static const struct mctp_usblib_test_pkt_span mctp_usblib_test_pkt_spans[] = {
 KUNIT_ARRAY_PARAM(mctp_usblib_test_rx_pkt_span, mctp_usblib_test_pkt_spans,
 		  mctp_usblib_test_pkt_span_to_desc);
 
+static void mctp_usblib_test_rx_split_header(struct kunit *test, size_t offset,
+					     struct mctp_usblib_test_dev *dev,
+					     struct mctp_usblib_rx *rx)
+{
+	struct sk_buff *skb;
+	size_t buflen, len;
+	u8 packet[16];
+	void *buf;
+	int rc;
+
+	len = sizeof(packet);
+	mctp_usblib_test_init_pkt(packet, len, len);
+
+	rc = mctp_usblib_rx_prepare(dev->ndev, rx, &buf, &buflen, GFP_KERNEL);
+	KUNIT_ASSERT_EQ(test, rc, 0);
+	KUNIT_ASSERT_GE(test, buflen, len);
+
+	memcpy(buf, packet, offset);
+	mctp_usblib_test_rx_complete(dev->ndev, rx, offset);
+
+	rc = mctp_usblib_rx_prepare(dev->ndev, rx, &buf, &buflen,
+				    GFP_KERNEL);
+	KUNIT_ASSERT_EQ(test, rc, 0);
+	KUNIT_ASSERT_GE(test, buflen, len);
+	KUNIT_ASSERT_EQ(test, dev->rx_pkts.qlen, 0);
+
+	memcpy(buf, packet + offset, len - offset);
+	mctp_usblib_test_rx_complete(dev->ndev, rx, len - offset);
+	KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, 1);
+
+	skb = __skb_dequeue(&dev->rx_pkts);
+	KUNIT_EXPECT_NOT_NULL(test, skb);
+	if (skb) {
+		KUNIT_EXPECT_EQ(test, skb->len, len - HDR_LEN);
+		kfree(skb);
+	}
+}
+
+static void mctp_usblib_test_rx_header_splits(struct kunit *test)
+{
+	struct mctp_usblib_test_dev *dev;
+	struct mctp_usblib_test_ctx *ctx;
+	struct mctp_usblib_rx *rx;
+	size_t i;
+
+	ctx = mctp_usblib_test_init(test);
+	rx = mctp_usblib_test_rx_init(test, true);
+	dev = ctx->dev;
+
+	/* Unrolling here so stack traces point to the invocation with the
+	 * failing length.
+	 */
+	mctp_usblib_test_rx_split_header(test, 1, dev, rx);
+	mctp_usblib_test_rx_split_header(test, 2, dev, rx);
+	mctp_usblib_test_rx_split_header(test, 3, dev, rx);
+	mctp_usblib_test_rx_split_header(test, 4, dev, rx);
+	mctp_usblib_test_rx_split_header(test, 5, dev, rx);
+	mctp_usblib_test_rx_split_header(test, 6, dev, rx);
+	mctp_usblib_test_rx_split_header(test, 7, dev, rx);
+	mctp_usblib_test_rx_split_header(test, 8, dev, rx);
+}
+
 static struct kunit_case mctp_usblib_test_cases[] = {
 	KUNIT_CASE(mctp_usblib_test_rx_single),
 	KUNIT_CASE_PARAM(mctp_usblib_test_rx_pkt_span,
 			 mctp_usblib_test_rx_pkt_span_gen_params),
+	KUNIT_CASE(mctp_usblib_test_rx_header_splits),
 	{}
 };
 

-- 
2.47.3

Re: [PATCH net-next v2 2/7] net: mctp: usblib: Add test for splits inside headers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 2

- [High] mctp_usblib_test_rx_split_header() releases the received sk_buff
  with kfree(skb) instead of kfree_skb()/consume_skb()…
- [Low] mctp_usblib_test_rx_header_splits() declares `size_t i;`
  (drivers/net/mctp/mctp-usblib-test.c:445) but never reads or writes it…
- [Low] Both mctp_usblib_test_rx_complete() calls in the new helper
  discard the int return value (drivers/net/mctp/mctp-usblib-test.c:420…

Pre-existing issues:
- [Medium] The new helper reads dev->rx_pkts.qlen directly (lines 426 and
  430) and removes entries with the lock-free __skb_dequeue() (line…

--

net: mctp: usblib: Add test for splits inside headers

This adds a KUnit case, mctp_usblib_test_rx_header_splits(), that feeds a
16-byte MCTP-over-USB packet in two transfers for every split offset from 1
to 8, and drops the three parameterized span cases that covered a subset of
those offsets.

> diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
> index 4c27307900fb..a2ca79f61910 100644
> --- a/drivers/net/mctp/mctp-usblib-test.c
> +++ b/drivers/net/mctp/mctp-usblib-test.c

[ ... ]

> @@ -378,12 +380,6 @@ static const struct mctp_usblib_test_pkt_span mctp_usblib_test_pkt_spans[] = {
>  	{ "1p1x-complete", 1, { 8 }, 1, { 8 } },
>  	/* Two small packets combined within one transfer */
>  	{ "2p1x-combined", 2, { 8, 8 }, 1, { 16 } },
> -	/* A packet split over two transfers, at the MCTP payload */
> -	{ "1p2x-split-payload", 1, { 16 }, 2, { 8, 8 } },
> -	/* A packet split over two transfers, at the USB transport header */
> -	{ "1p2x-split-usbhdr", 1, { 16 }, 2, { 2, 14 } },
> -	/* A packet split over two transfers, at the MCTP header */
> -	{ "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. */

[ ... ]

> @@ -403,10 +399,73 @@ static const struct mctp_usblib_test_pkt_span mctp_usblib_test_pkt_spans[] = {
>  KUNIT_ARRAY_PARAM(mctp_usblib_test_rx_pkt_span, mctp_usblib_test_pkt_spans,
>  		  mctp_usblib_test_pkt_span_to_desc);
>  
> +static void mctp_usblib_test_rx_split_header(struct kunit *test, size_t offset,
> +					     struct mctp_usblib_test_dev *dev,
> +					     struct mctp_usblib_rx *rx)
> +{
> +	struct sk_buff *skb;
> +	size_t buflen, len;
> +	u8 packet[16];
> +	void *buf;
> +	int rc;
> +
> +	len = sizeof(packet);
> +	mctp_usblib_test_init_pkt(packet, len, len);
> +
> +	rc = mctp_usblib_rx_prepare(dev->ndev, rx, &buf, &buflen, GFP_KERNEL);
> +	KUNIT_ASSERT_EQ(test, rc, 0);
> +	KUNIT_ASSERT_GE(test, buflen, len);
> +
> +	memcpy(buf, packet, offset);
> +	mctp_usblib_test_rx_complete(dev->ndev, rx, offset);

[Severity: Low]

Should the return value of mctp_usblib_test_rx_complete() be checked here,
as the other call sites in this file do?

mctp_usblib_test_rx_single() and mctp_usblib_test_rx_pkt_span() both follow
each completion with:

	rc = mctp_usblib_test_rx_complete(dev->ndev, rx, len);
	KUNIT_ASSERT_EQ(test, rc, 0);

mctp_usblib_rx_complete() has err_reset paths that return -ENOMSG/-EPROTO
and also tear down the RX context:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_rx_complete() {
	...
err_reset:
	dev_kfree_skb_any(rx->skb);
	rx->skb = NULL;
	return rc;
}

If the first completion takes that path, the helper carries on: the second
mctp_usblib_rx_prepare() allocates a fresh skb, the mid-sequence
KUNIT_ASSERT_EQ(test, dev->rx_pkts.qlen, 0) still passes, the remainder of
the packet is fed at the wrong offset, and the failure only surfaces at the
later qlen check.  Doesn't that work against the stated reason for
unrolling, "so stack traces point to the invocation with the failing
length"?

Related to that, the removed cases 1p2x-split-payload, 1p2x-split-usbhdr
and 1p2x-split-mctphdr did assert rc == 0 on every transfer, so is the
commit message wording "remove parameterized tests that are now covered"
fully accurate?

The new checks also only compare skb->len against len - HDR_LEN, and never
compare the delivered bytes against packet + sizeof(struct mctp_usb_hdr).
Would a regression that corrupts bytes across the split (say the MCTP
src/flags fields around offsets 6 to 8) while leaving dest routable and the
length unchanged still pass all of the new assertions?

> +
> +	rc = mctp_usblib_rx_prepare(dev->ndev, rx, &buf, &buflen,
> +				    GFP_KERNEL);
> +	KUNIT_ASSERT_EQ(test, rc, 0);
> +	KUNIT_ASSERT_GE(test, buflen, len);
> +	KUNIT_ASSERT_EQ(test, dev->rx_pkts.qlen, 0);
> +
> +	memcpy(buf, packet + offset, len - offset);
> +	mctp_usblib_test_rx_complete(dev->ndev, rx, len - offset);
> +	KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, 1);
> +
> +	skb = __skb_dequeue(&dev->rx_pkts);

[Severity: Medium]

This isn't something introduced by this patch, but the reads of
dev->rx_pkts.qlen and the __skb_dequeue() here run with no lock held, while
the only producer, mctp_usblib_test_dst_output(), inserts with the locked
variant from the netif_rx/backlog delivery context:

	skb_queue_tail(&dev->rx_pkts, skb);

The existing mctp_usblib_test_rx_single() and
mctp_usblib_test_rx_pkt_span() already do the same unlocked reads, so this
is pre-existing rather than new, and delivery does appear to be synchronous
inside the local_bh_disable()/local_bh_enable() window of
mctp_usblib_test_rx_complete() in both default and PREEMPT_RT
configurations.  Would skb_dequeue() and skb_queue_len() be preferable here
anyway, so the helper does not depend on that assumption?

> +	KUNIT_EXPECT_NOT_NULL(test, skb);
> +	if (skb) {
> +		KUNIT_EXPECT_EQ(test, skb->len, len - HDR_LEN);
> +		kfree(skb);
                ^^^^^

[Severity: High]

Does this leak the skb data buffer?  kfree() releases only the struct
sk_buff itself and skips skb_release_all()/skb_free_head(), so skb->head is
never freed.

The skb reaching this queue is the one allocated in
mctp_usblib_rx_prepare():

drivers/net/mctp/mctp-usblib.c:mctp_usblib_rx_prepare() {
	...
	len = rx->span ? roundup(MCTP_USB_1_1_PKTLEN_MAX, rx->ep_pktlen)
		: MCTP_USB_1_0_XFER_SIZE;

	if (!skb) {
		skb = __netdev_alloc_skb(netdev, len, gfp);
	...
}

and with span mode that is an 8 KiB head.  On the second transfer
pkt_len == skb->len, so mctp_usblib_rx_complete() hands that same skb
onward:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_rx_complete() {
	...
		/* we have (exactly) a complete packet, RX it directly */
		if (pkt_len == skb->len) {
			mctp_usblib_rx(netdev, skb);
			rx->skb = NULL;
			break;
		}
	...
}

That gives one leaked head buffer per call, so eight per run of
mctp_usblib_test_rx_header_splits().  Every other release site in this file
uses kfree_skb() - mctp_usblib_test_rx_single(),
mctp_usblib_test_rx_pkt_span() and mctp_usblib_dev_tx() - and kfree_skb() is
NULL safe, so the "if (skb)" guard around it could be dropped as well.

The v2 changelog says "Fix sk_buff leak"; with kfree() here, is the leak
actually closed?  The same kfree() is still present at the end of the
series.

> +	}
> +}
> +
> +static void mctp_usblib_test_rx_header_splits(struct kunit *test)
> +{
> +	struct mctp_usblib_test_dev *dev;
> +	struct mctp_usblib_test_ctx *ctx;
> +	struct mctp_usblib_rx *rx;
> +	size_t i;
             ^

[Severity: Low]

This isn't a bug, but i is never used once the invocations are unrolled, so
this emits a -Wunused-variable warning and fails the build under
CONFIG_WERROR with CONFIG_MCTP_TRANSPORT_USBLIB_TEST enabled.  The
declaration is removed again by the next patch in the series, "net: mctp:
usblib: Add tests of invalid headers", so the final state of the series is
clean; could it be dropped in this patch instead to keep the series
bisectable?

> +
> +	ctx = mctp_usblib_test_init(test);
> +	rx = mctp_usblib_test_rx_init(test, true);
> +	dev = ctx->dev;
> +
> +	/* Unrolling here so stack traces point to the invocation with the
> +	 * failing length.
> +	 */
> +	mctp_usblib_test_rx_split_header(test, 1, dev, rx);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-dev-mctp-usb-lib-test-v2-0-313cf15e0b56%40codeconstruct.com.au

Re: [PATCH net-next v2 2/7] net: mctp: usblib: Add test for splits inside headers

[Simon Horman]

On Thu, Sep 03, 2026 at 10:42:19AM +0800, James Lee wrote:

...

> +static void mctp_usblib_test_rx_header_splits(struct kunit *test)
> +{
> +	struct mctp_usblib_test_dev *dev;
> +	struct mctp_usblib_test_ctx *ctx;
> +	struct mctp_usblib_rx *rx;
> +	size_t i;

Hi James,

This is a minor problem and it is addressed in the following patch,
by removing the line above, but i is declared yet unused  and should
not be added here.

Flagged by W=1 builds with GCC 16.2.0 and Clang 22.1.8.

> +
> +	ctx = mctp_usblib_test_init(test);
> +	rx = mctp_usblib_test_rx_init(test, true);
> +	dev = ctx->dev;
> +
> +	/* Unrolling here so stack traces point to the invocation with the
> +	 * failing length.
> +	 */
> +	mctp_usblib_test_rx_split_header(test, 1, dev, rx);
> +	mctp_usblib_test_rx_split_header(test, 2, dev, rx);
> +	mctp_usblib_test_rx_split_header(test, 3, dev, rx);
> +	mctp_usblib_test_rx_split_header(test, 4, dev, rx);
> +	mctp_usblib_test_rx_split_header(test, 5, dev, rx);
> +	mctp_usblib_test_rx_split_header(test, 6, dev, rx);
> +	mctp_usblib_test_rx_split_header(test, 7, dev, rx);
> +	mctp_usblib_test_rx_split_header(test, 8, dev, rx);
> +}

...

[PATCH net-next v2 3/7] net: mctp: usblib: Add tests of invalid headers

[James Lee]

Add tests where the length field of the USB transport header is below
the minimum value, and where the DMTF ID is invalid.

Signed-off-by: James Lee <james@codeconstruct.com.au>
---
 drivers/net/mctp/mctp-usblib-test.c | 69 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
index a2ca79f61910af129af956a6f38ad69d5dae8c10..b0266be79e94c5adb1cf4286d5aeef9768d28b34 100644
--- a/drivers/net/mctp/mctp-usblib-test.c
+++ b/drivers/net/mctp/mctp-usblib-test.c
@@ -442,7 +442,6 @@ static void mctp_usblib_test_rx_header_splits(struct kunit *test)
 	struct mctp_usblib_test_dev *dev;
 	struct mctp_usblib_test_ctx *ctx;
 	struct mctp_usblib_rx *rx;
-	size_t i;
 
 	ctx = mctp_usblib_test_init(test);
 	rx = mctp_usblib_test_rx_init(test, true);
@@ -461,11 +460,79 @@ static void mctp_usblib_test_rx_header_splits(struct kunit *test)
 	mctp_usblib_test_rx_split_header(test, 8, dev, rx);
 }
 
+/* Test the submission of a packet with an impossibly small value in the
+ * header's length field. Values less than HDR_LEN are invalid.
+ */
+static void mctp_usblib_test_rx_short_packet(struct kunit *test)
+{
+	struct mctp_usblib_test_dev *dev;
+	struct mctp_usblib_test_ctx *ctx;
+	struct mctp_usblib_rx *rx;
+	size_t len, buflen;
+	u8 pktbuf[12];
+	void *buf;
+	int rc;
+
+	ctx = mctp_usblib_test_init(test);
+	rx = mctp_usblib_test_rx_init(test, true);
+	dev = ctx->dev;
+
+	len = sizeof(pktbuf);
+	mctp_usblib_test_init_pkt(pktbuf, len, HDR_LEN - 1);
+
+	buflen = 0;
+	rc = mctp_usblib_rx_prepare(dev->ndev, rx, &buf, &buflen, GFP_KERNEL);
+	KUNIT_ASSERT_EQ(test, rc, 0);
+	KUNIT_ASSERT_GE(test, buflen, len);
+
+	memcpy(buf, pktbuf, len);
+
+	rc = mctp_usblib_rx_complete(dev->ndev, rx, len);
+	KUNIT_EXPECT_EQ(test, rc, -EPROTO);
+	KUNIT_EXPECT_NULL(test, rx->skb);
+	KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, 0);
+}
+
+static void mctp_usblib_test_rx_invalid_dmtf_id(struct kunit *test)
+{
+	struct mctp_usblib_test_dev *dev;
+	struct mctp_usblib_test_ctx *ctx;
+	struct mctp_usblib_rx *rx;
+	size_t len, buflen;
+	u8 pktbuf[12];
+	void *buf;
+	int rc;
+
+	ctx = mctp_usblib_test_init(test);
+	rx = mctp_usblib_test_rx_init(test, true);
+	dev = ctx->dev;
+
+	len = sizeof(pktbuf);
+	mctp_usblib_test_init_pkt(pktbuf, len, len);
+
+	// Make packet DMTF ID invalid
+	pktbuf[1] = ~pktbuf[1];
+
+	buflen = 0;
+	rc = mctp_usblib_rx_prepare(dev->ndev, rx, &buf, &buflen, GFP_KERNEL);
+	KUNIT_ASSERT_EQ(test, rc, 0);
+	KUNIT_ASSERT_GE(test, buflen, len);
+
+	memcpy(buf, pktbuf, len);
+
+	rc = mctp_usblib_rx_complete(dev->ndev, rx, len);
+	KUNIT_EXPECT_EQ(test, rc, -EPROTO);
+	KUNIT_EXPECT_NULL(test, rx->skb);
+	KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, 0);
+}
+
 static struct kunit_case mctp_usblib_test_cases[] = {
 	KUNIT_CASE(mctp_usblib_test_rx_single),
 	KUNIT_CASE_PARAM(mctp_usblib_test_rx_pkt_span,
 			 mctp_usblib_test_rx_pkt_span_gen_params),
 	KUNIT_CASE(mctp_usblib_test_rx_header_splits),
+	KUNIT_CASE(mctp_usblib_test_rx_short_packet),
+	KUNIT_CASE(mctp_usblib_test_rx_invalid_dmtf_id),
 	{}
 };
 

-- 
2.47.3

Re: [PATCH net-next v2 3/7] net: mctp: usblib: Add tests of invalid headers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 3

- [Medium] Kernel memory leak in the kunit test helper
  mctp_usblib_test_rx_split_header(): the sk_buff dequeued from…
- [Low] Undisclosed unrelated fixup: this patch silently removes the
  unused `size_t i;` declaration from…
- [Low] Incorrect comment and a test that does not pin the real length
  boundary.
- [Low] mctp_usblib_test_rx_invalid_dmtf_id() introduces a C99 `//`
  comment ("// Make packet DMTF ID invalid"), which checkpatch.pl…

--

net: mctp: usblib: Add tests of invalid headers

Two new kunit cases are added to the mctp-usblib test module: one feeds a
USB transport header whose length field is below the minimum, the other
corrupts the DMTF ID byte.  Both expect mctp_usblib_rx_complete() to
return -EPROTO and to drop the pending skb.

A few questions below.

> diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
> index a2ca79f61910..b0266be79e94 100644
> --- a/drivers/net/mctp/mctp-usblib-test.c
> +++ b/drivers/net/mctp/mctp-usblib-test.c
> @@ -442,7 +442,6 @@ static void mctp_usblib_test_rx_header_splits(struct kunit *test)
>  	struct mctp_usblib_test_dev *dev;
>  	struct mctp_usblib_test_ctx *ctx;
>  	struct mctp_usblib_rx *rx;
> -	size_t i;

[Severity: Low]
Should this removal be squashed into the previous patch of the series?  The
declaration of i in mctp_usblib_test_rx_header_splits() was added by "net:
mctp: usblib: Add test for splits inside headers", and at that commit the
function never references i because the loop is written out as eight
mctp_usblib_test_rx_split_header() calls.  Building that intermediate commit
with CONFIG_MCTP_TRANSPORT_USBLIB_TEST=y should therefore warn with
-Wunused-variable, and the commit message here only mentions the two new
invalid-header tests, not this fixup.

>  
>  	ctx = mctp_usblib_test_init(test);
>  	rx = mctp_usblib_test_rx_init(test, true);

[Severity: Medium]
While looking at this function, does the helper it calls leak the skb data
buffer?  mctp_usblib_test_rx_split_header() ends with:

	skb = __skb_dequeue(&dev->rx_pkts);
	KUNIT_EXPECT_NOT_NULL(test, skb);
	if (skb) {
		KUNIT_EXPECT_EQ(test, skb->len, len - HDR_LEN);
		kfree(skb);
	}

kfree() returns only the struct sk_buff to the slab; skb_release_all() never
runs, so skb->head and the shinfo dataref are never freed.  The buffer comes
from mctp_usblib_rx_prepare():

	len = rx->span ? roundup(MCTP_USB_1_1_PKTLEN_MAX, rx->ep_pktlen)
		: MCTP_USB_1_0_XFER_SIZE;
	...
		skb = __netdev_alloc_skb(netdev, len, gfp);

which is roundup(8191, 512) = 8192 bytes per prepared transfer.
mctp_usblib_test_rx_header_splits() calls the helper eight times, so each run
of the case would leak around 64 KiB, and kmemleak/KASAN would attribute it
to the MCTP driver.  mctp_usblib_test_rx_single() and
mctp_usblib_test_rx_pkt_span() both use kfree_skb() here.  The helper came in
with the earlier "net: mctp: usblib: Add test for splits inside headers"
patch and is unchanged at the end of the series, so would a kfree_skb() fixup
there be appropriate?

> @@ -461,11 +460,79 @@ static void mctp_usblib_test_rx_header_splits(struct kunit *test)
>  	mctp_usblib_test_rx_split_header(test, 8, dev, rx);
>  }
>  
> +/* Test the submission of a packet with an impossibly small value in the
> + * header's length field. Values less than HDR_LEN are invalid.
> + */

[Severity: Low]
Is "Values less than HDR_LEN are invalid" accurate?  HDR_LEN is
sizeof(struct mctp_usb_hdr), which is 4:

include/linux/usb/mctp-usb.h:
	struct mctp_usb_hdr {
		__be16	id;
		__be16	len;
	} __packed;

but the check being exercised in mctp_usblib_rx_complete() is:

	if (pkt_len < sizeof(*hdr) + sizeof(struct mctp_hdr)) {
		netdev_dbg(netdev, "rx: invalid len %d\n", pkt_len);
		rc = -EPROTO;
		goto err_reset;
	}

so the real minimum is 8, not 4.

> +static void mctp_usblib_test_rx_short_packet(struct kunit *test)
> +{
> +	struct mctp_usblib_test_dev *dev;
> +	struct mctp_usblib_test_ctx *ctx;
> +	struct mctp_usblib_rx *rx;
> +	size_t len, buflen;
> +	u8 pktbuf[12];
> +	void *buf;
> +	int rc;
> +
> +	ctx = mctp_usblib_test_init(test);
> +	rx = mctp_usblib_test_rx_init(test, true);
> +	dev = ctx->dev;
> +
> +	len = sizeof(pktbuf);
> +	mctp_usblib_test_init_pkt(pktbuf, len, HDR_LEN - 1);

[Severity: Low]
Following on from the comment above, would pinning the actual boundary give
more coverage here?  HDR_LEN - 1 is 3, which is below both bounds, so the
4..7 range stays untested — including pkt_len == 4, where a header-only
packet would leave a zero-length MCTP packet after
skb_pull(sizeof(struct mctp_usb_hdr)) in mctp_usblib_rx().  If the driver
check were ever relaxed to the weaker bound the comment describes, this case
would still see -EPROTO and pass.  Would rejecting 7 and accepting 8 be a
better test, along with naming HDR_LEN + sizeof(struct mctp_hdr) in the
comment?

> +
> +	buflen = 0;
> +	rc = mctp_usblib_rx_prepare(dev->ndev, rx, &buf, &buflen, GFP_KERNEL);
> +	KUNIT_ASSERT_EQ(test, rc, 0);
> +	KUNIT_ASSERT_GE(test, buflen, len);
> +
> +	memcpy(buf, pktbuf, len);
> +
> +	rc = mctp_usblib_rx_complete(dev->ndev, rx, len);
> +	KUNIT_EXPECT_EQ(test, rc, -EPROTO);
> +	KUNIT_EXPECT_NULL(test, rx->skb);
> +	KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, 0);
> +}

[ ... ]

> +	len = sizeof(pktbuf);
> +	mctp_usblib_test_init_pkt(pktbuf, len, len);
> +
> +	// Make packet DMTF ID invalid

[Severity: Low]
This isn't a bug, but checkpatch.pl reports "ERROR: do not use C99 //
comments" for this line in mctp_usblib_test_rx_invalid_dmtf_id().  It is the
only // comment in drivers/net/mctp/mctp-usblib-test.c, and it is still
there at the end of the series; could it be converted to /* ... */?

> +	pktbuf[1] = ~pktbuf[1];

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-dev-mctp-usb-lib-test-v2-0-313cf15e0b56%40codeconstruct.com.au

[PATCH net-next v2 4/7] net: mctp: usblib: Complete rx tests

[James Lee]

Add rx tests without spanning, covering cases where a packet is shorter
than allowed without spanning, isn't completed within one submission.

Fully cover rx functions except for memory allocation failures and
trivial functions.

Signed-off-by: James Lee <james@codeconstruct.com.au>
---
 drivers/net/mctp/mctp-usblib-test.c | 62 +++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
index b0266be79e94c5adb1cf4286d5aeef9768d28b34..22aaa4795cbdd13432a3c1e7cc85ecbb3a30fe88 100644
--- a/drivers/net/mctp/mctp-usblib-test.c
+++ b/drivers/net/mctp/mctp-usblib-test.c
@@ -526,6 +526,66 @@ static void mctp_usblib_test_rx_invalid_dmtf_id(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, 0);
 }
 
+static void mctp_usblib_test_rx_nonspanning_tiny(struct kunit *test)
+{
+	struct mctp_usblib_test_dev *dev;
+	struct mctp_usblib_test_ctx *ctx;
+	struct mctp_usblib_rx *rx;
+	size_t len, buflen;
+	u8 pktbuf[3];
+	void *buf;
+	int rc;
+
+	ctx = mctp_usblib_test_init(test);
+	rx = mctp_usblib_test_rx_init(test, false);
+	dev = ctx->dev;
+
+	len = sizeof(pktbuf);
+	mctp_usblib_test_init_pkt(pktbuf, len, len);
+
+	buflen = 0;
+	rc = mctp_usblib_rx_prepare(dev->ndev, rx, &buf, &buflen, GFP_KERNEL);
+	KUNIT_ASSERT_EQ(test, rc, 0);
+	KUNIT_ASSERT_GE(test, buflen, len);
+
+	memcpy(buf, pktbuf, len);
+
+	rc = mctp_usblib_rx_complete(dev->ndev, rx, len);
+	KUNIT_EXPECT_EQ(test, rc, -ENOMSG);
+	KUNIT_EXPECT_NULL(test, rx->skb);
+	KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, 0);
+}
+
+static void mctp_usblib_test_rx_nonspanning_partial(struct kunit *test)
+{
+	struct mctp_usblib_test_dev *dev;
+	struct mctp_usblib_test_ctx *ctx;
+	struct mctp_usblib_rx *rx;
+	size_t len, buflen;
+	u8 pktbuf[20];
+	void *buf;
+	int rc;
+
+	ctx = mctp_usblib_test_init(test);
+	rx = mctp_usblib_test_rx_init(test, false);
+	dev = ctx->dev;
+
+	len = sizeof(pktbuf);
+	mctp_usblib_test_init_pkt(pktbuf, len, len + 1);
+
+	buflen = 0;
+	rc = mctp_usblib_rx_prepare(dev->ndev, rx, &buf, &buflen, GFP_KERNEL);
+	KUNIT_ASSERT_EQ(test, rc, 0);
+	KUNIT_ASSERT_GE(test, buflen, len);
+
+	memcpy(buf, pktbuf, len);
+
+	rc = mctp_usblib_rx_complete(dev->ndev, rx, len);
+	KUNIT_EXPECT_EQ(test, rc, -EPROTO);
+	KUNIT_EXPECT_NULL(test, rx->skb);
+	KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, 0);
+}
+
 static struct kunit_case mctp_usblib_test_cases[] = {
 	KUNIT_CASE(mctp_usblib_test_rx_single),
 	KUNIT_CASE_PARAM(mctp_usblib_test_rx_pkt_span,
@@ -533,6 +593,8 @@ static struct kunit_case mctp_usblib_test_cases[] = {
 	KUNIT_CASE(mctp_usblib_test_rx_header_splits),
 	KUNIT_CASE(mctp_usblib_test_rx_short_packet),
 	KUNIT_CASE(mctp_usblib_test_rx_invalid_dmtf_id),
+	KUNIT_CASE(mctp_usblib_test_rx_nonspanning_tiny),
+	KUNIT_CASE(mctp_usblib_test_rx_nonspanning_partial),
 	{}
 };
 

-- 
2.47.3

Re: [PATCH net-next v2 4/7] net: mctp: usblib: Complete rx tests

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] In mctp_usblib_test_rx_split_header() (drivers/net/mctp/mctp-
  usblib-test.c:436) an skb dequeued from dev->rx_pkts is released with…

--

net: mctp: usblib: Complete rx tests

This adds two KUnit cases to the mctp-usblib test suite exercising the
non-spanning receive path: a packet shorter than the minimum header length,
and a packet whose declared payload length is not satisfied by a single
submission. Both new cases are registered in mctp_usblib_test_cases[].

> diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
> index b0266be79e94..22aaa4795cbd 100644
> --- a/drivers/net/mctp/mctp-usblib-test.c
> +++ b/drivers/net/mctp/mctp-usblib-test.c

[ ... ]

> @@ -533,6 +593,8 @@ static struct kunit_case mctp_usblib_test_cases[] = {
>  	KUNIT_CASE(mctp_usblib_test_rx_header_splits),
              ^^^^

[Severity: Medium]
While looking at the suite that these two new cases join, is the skb
release in mctp_usblib_test_rx_split_header() correct?  That helper is not
touched by this patch (it comes from the earlier "Add test for splits
inside headers" change in the same series), but it is the only skb release
site in this file that does not use the skbuff API:

drivers/net/mctp/mctp-usblib-test.c:mctp_usblib_test_rx_split_header() {
	...
	skb = __skb_dequeue(&dev->rx_pkts);
	KUNIT_EXPECT_NOT_NULL(test, skb);
	if (skb) {
		KUNIT_EXPECT_EQ(test, skb->len, len - HDR_LEN);
		kfree(skb);
	}
}

Does this leak skb->head?  kfree() returns only the sk_buff head object to
its cache and never runs skb_release_all()/skb_release_data(), so the
separately allocated data buffer is never freed.

In span mode mctp_usblib_rx_prepare() sizes that buffer as
roundup(MCTP_USB_1_1_PKTLEN_MAX, ep_pktlen), i.e. 8192 bytes, and
mctp_usblib_test_rx_header_splits() calls the helper eight times, so a
single run of the suite would leak roughly 64 KB and likely trip kmemleak
in CI.  The other release sites in the same file use kfree_skb(), and
mctp_usblib_test_dev_action() uses skb_queue_purge().

Would kfree_skb(skb) be the right call here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-dev-mctp-usb-lib-test-v2-0-313cf15e0b56%40codeconstruct.com.au

[PATCH net-next v2 5/7] net: mctp: usblib: Simplify allocation logic in mctp_usblib_test_rx_init

[James Lee]

The if statement testing rx's allocation is unneeded.

Signed-off-by: James Lee <james@codeconstruct.com.au>
---
 drivers/net/mctp/mctp-usblib-test.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
index 22aaa4795cbdd13432a3c1e7cc85ecbb3a30fe88..64c758013854c697767d3cc80d46ebaa310188a4 100644
--- a/drivers/net/mctp/mctp-usblib-test.c
+++ b/drivers/net/mctp/mctp-usblib-test.c
@@ -202,11 +202,9 @@ mctp_usblib_test_rx_init(struct kunit *test, bool span)
 	int rc;
 
 	rx = kzalloc_obj(*rx);
-	if (rx) {
-		rc = kunit_add_action_or_reset(test, action_rx_fini, rx);
-		KUNIT_ASSERT_EQ(test, rc, 0);
-	}
 	KUNIT_ASSERT_NOT_NULL(test, rx);
+	rc = kunit_add_action_or_reset(test, action_rx_fini, rx);
+	KUNIT_ASSERT_EQ(test, rc, 0);
 
 	rc = mctp_usblib_rx_init(rx, ep_maxpacket, span);
 	KUNIT_ASSERT_EQ(test, rc, 0);

-- 
2.47.3

[PATCH net-next v2 6/7] net: mctp: usblib: Add initial kunit tx tests

[James Lee]

Add tests for the transmit path, where MCTP packets are handed to
outgoing USB transfer data. Testing a spanning transfer that is expected
to succeed, the failure paths when sends fail, and large sequential
sends.

Signed-off-by: James Lee <james@codeconstruct.com.au>

---
v2:
- Correct flex array reference
- Simply return when mctp_usblib_test_tx_send() fails
- Use skb_put_data() instead of manual assignment
- Remember span parameter exists
---
 drivers/net/mctp/mctp-usblib-test.c | 286 ++++++++++++++++++++++++++++++++++++
 1 file changed, 286 insertions(+)

diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
index 64c758013854c697767d3cc80d46ebaa310188a4..73bda4745433f689504e65685b73da2117f1d3db 100644
--- a/drivers/net/mctp/mctp-usblib-test.c
+++ b/drivers/net/mctp/mctp-usblib-test.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2026 Code Construct Pty Ltd
  */
 
+#include <linux/array_size.h>
 #include <uapi/linux/netdevice.h>
 #include <linux/netdevice.h>
 #include <kunit/test.h>
@@ -16,6 +17,13 @@
 
 #define HDR_LEN sizeof(struct mctp_usb_hdr)
 
+struct tx_buff {
+	struct list_head list;
+
+	size_t length;
+	u8 data[] __counted_by(length);
+};
+
 struct mctp_usblib_test_dev {
 	struct net_device *ndev;
 	struct mctp_dev *mdev;
@@ -24,9 +32,104 @@ struct mctp_usblib_test_dev {
 
 struct mctp_usblib_test_ctx {
 	struct mctp_usblib_test_dev *dev;
+	struct list_head tx_xfers;
 	struct mctp_route rt;
 };
 
+static int mctp_usblib_test_tx_send(struct mctp_usblib_tx_ctx *tx_ctx,
+				    void *data, size_t len)
+{
+	struct mctp_usblib_test_ctx *ctx;
+	struct tx_buff *new_node;
+	struct net_device *ndev;
+
+	ctx = mctp_usblib_tx_ctx_priv(tx_ctx);
+	ndev = ctx->dev->ndev;
+
+	new_node = kzalloc_flex(*new_node, data, len, GFP_KERNEL);
+	if (!new_node)
+		return -ENOMEM;
+
+	new_node->length = len;
+	memcpy(new_node->data, data, len);
+	list_add_tail(&new_node->list, &ctx->tx_xfers);
+
+	mctp_usblib_tx_send_complete(tx_ctx, ndev, true);
+	return 0;
+}
+
+static int mctp_usblib_test_tx_send_fail(struct mctp_usblib_tx_ctx *tx_ctx,
+					 void *data, size_t len)
+{
+	return -ENOMEM;
+}
+
+static u8 *mctp_usblib_test_flatten_tx_buff(struct kunit *test,
+					    struct list_head *in,
+					    size_t *length_out)
+{
+	struct tx_buff *pos;
+	size_t length;
+	u8 *buf, *tail;
+
+	KUNIT_ASSERT_TRUE(test, length_out);
+	KUNIT_ASSERT_TRUE(test, in);
+
+	length = 0;
+	list_for_each_entry(pos, in, list)
+		length = size_add(length, pos->length);
+
+	KUNIT_ASSERT_NE(test, length, 0);
+	KUNIT_ASSERT_NE(test, length, SIZE_MAX);
+
+	buf = kunit_kzalloc(test, length, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, buf);
+
+	tail = buf;
+	list_for_each_entry(pos, in, list) {
+		memcpy(tail, pos->data, pos->length);
+		tail += pos->length;
+	}
+
+	*length_out = length;
+	return buf;
+}
+
+static u8 *mctp_usblib_test_init_buf(struct kunit *test, size_t length)
+{
+	u8 *buffer;
+	size_t i;
+
+	buffer = kunit_kzalloc(test, length, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, buffer);
+
+	for (i = 0; i < length; i++)
+		buffer[i] = i % 256;
+
+	return buffer;
+}
+
+static void mctp_usblib_test_fill_head(struct mctp_usb_hdr *head, size_t len)
+{
+	len += HDR_LEN;
+	head->id = cpu_to_be16(MCTP_USB_DMTF_ID);
+	head->len = cpu_to_be16(len & MCTP_USB_1_1_PKTLEN_MAX);
+}
+
+static struct sk_buff *mctp_usblib_test_init_skb(struct kunit *test,
+						 unsigned int length,
+						 struct net_device *ndev,
+						 void *data)
+{
+	struct sk_buff *skb;
+
+	skb = __netdev_alloc_skb(ndev, length, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, skb);
+
+	skb_put_data(skb, data, length);
+	return skb;
+}
+
 static netdev_tx_t mctp_usblib_dev_tx(struct sk_buff *skb,
 				      struct net_device *ndev)
 {
@@ -119,6 +222,7 @@ static int mctp_usblib_test_dst_output(struct mctp_dst *dst,
 static void mctp_usblib_test_fini_action(void *data)
 {
 	struct mctp_usblib_test_ctx *ctx = data;
+	struct tx_buff *curr, *temp;
 
 	/* The device will have been destroyed, so ->rt will be unlinked.
 	 * Just ensure that the refcount is as expected.
@@ -126,6 +230,8 @@ static void mctp_usblib_test_fini_action(void *data)
 	KUNIT_EXPECT_TRUE(current->kunit_test,
 			  refcount_dec_and_test(&ctx->rt.refs));
 
+	list_for_each_entry_safe(curr, temp, &ctx->tx_xfers, list)
+		kfree(curr);
 	kfree(ctx);
 }
 
@@ -141,6 +247,7 @@ static struct mctp_usblib_test_ctx *mctp_usblib_test_init(struct kunit *test)
 	INIT_LIST_HEAD(&ctx->rt.list);
 	rt = &ctx->rt;
 	refcount_set(&rt->refs, 1);
+	INIT_LIST_HEAD(&ctx->tx_xfers);
 
 	rc = kunit_add_action_or_reset(test, mctp_usblib_test_fini_action, ctx);
 	KUNIT_ASSERT_EQ(test, rc, 0);
@@ -227,6 +334,32 @@ static int mctp_usblib_test_rx_complete(struct net_device *netdev,
 	return rc;
 }
 
+static void action_tx_fini(void *data)
+{
+	struct mctp_usblib_tx *tx = data;
+
+	mctp_usblib_tx_fini(tx);
+	kfree(tx);
+}
+
+static struct mctp_usblib_tx *
+mctp_usblib_test_tx_init(struct kunit *test,
+			 const struct mctp_usblib_tx_ops *ops,
+			 void *priv, bool span)
+{
+	struct mctp_usblib_tx *tx;
+	int rc;
+
+	tx = kzalloc_obj(*tx);
+	KUNIT_ASSERT_NOT_NULL(test, tx);
+	rc = kunit_add_action_or_reset(test, action_tx_fini, tx);
+	KUNIT_ASSERT_EQ(test, rc, 0);
+
+	mctp_usblib_tx_init(tx, ops, priv, span);
+
+	return tx;
+}
+
 /* Single packet, starting on a transfer boundary, contained entirely within
  * the transfer
  */
@@ -584,6 +717,156 @@ static void mctp_usblib_test_rx_nonspanning_partial(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, 0);
 }
 
+static void mctp_usblib_test_tx_pkt_span(struct kunit *test)
+{
+	struct mctp_usblib_test_ctx *ctx;
+	struct mctp_usblib_tx_ops ops;
+	struct mctp_usblib_tx *tx;
+	struct mctp_usb_hdr head;
+	struct net_device *ndev;
+	struct sk_buff *skb;
+	size_t len, tx_len;
+	u8 *buf, *flat_tx;
+	int rc;
+
+	len = 1000;
+
+	ctx = mctp_usblib_test_init(test);
+	ndev = ctx->dev->ndev;
+
+	ops.send = mctp_usblib_test_tx_send;
+
+	tx = mctp_usblib_test_tx_init(test, &ops, ctx, true);
+
+	buf = mctp_usblib_test_init_buf(test, len);
+	mctp_usblib_test_fill_head(&head, len);
+
+	skb = mctp_usblib_test_init_skb(test, len, ndev, buf);
+
+	rc = mctp_usblib_tx_push(ndev, tx, skb, false);
+	KUNIT_ASSERT_EQ(test, rc, 0);
+	KUNIT_ASSERT_FALSE(test, list_empty(&ctx->tx_xfers));
+
+	flat_tx = mctp_usblib_test_flatten_tx_buff(test, &ctx->tx_xfers,
+						   &tx_len);
+	KUNIT_ASSERT_NOT_NULL(test, flat_tx);
+
+	KUNIT_EXPECT_EQ(test, tx_len, len + HDR_LEN);
+	KUNIT_EXPECT_MEMEQ(test, flat_tx, &head, HDR_LEN);
+	KUNIT_EXPECT_MEMEQ(test, flat_tx + HDR_LEN, buf, len);
+}
+
+static void mctp_usblib_test_tx_failing_send(struct kunit *test)
+{
+	struct mctp_usblib_test_ctx *ctx;
+	struct mctp_usblib_tx_ops ops;
+	struct mctp_usblib_tx *tx;
+	struct net_device *ndev;
+	struct sk_buff *skb;
+	size_t len;
+	u8 *buf;
+	int rc;
+
+	len = 100;
+
+	ctx = mctp_usblib_test_init(test);
+	ndev = ctx->dev->ndev;
+
+	ops.send = mctp_usblib_test_tx_send_fail;
+
+	tx = mctp_usblib_test_tx_init(test, &ops, ctx, false);
+	buf = mctp_usblib_test_init_buf(test, len);
+	skb = mctp_usblib_test_init_skb(test, len, ndev, buf);
+
+	/* Doesn't call ops.send as more packets are expected,
+	 * so the push shouldn't fail.
+	 */
+	rc = mctp_usblib_tx_push(ndev, tx, skb, true);
+	KUNIT_ASSERT_EQ(test, rc, 0);
+
+	skb = mctp_usblib_test_init_skb(test, len, ndev, buf);
+
+	/* Calls ops.send as no further packets are expected. */
+	rc = mctp_usblib_tx_push(ndev, tx, skb, false);
+	KUNIT_EXPECT_EQ(test, rc, 0);
+	KUNIT_EXPECT_NULL(test, tx->cur_ctx);
+	KUNIT_EXPECT_TRUE(test, list_empty(&ctx->tx_xfers));
+}
+
+/* Test sending multiple packets in the same transfer, followed by one that
+ * spans multiple subsequent transfers.
+ */
+static void mctp_usblib_test_tx_multi_push(struct kunit *test)
+{
+	struct mctp_usblib_test_ctx *ctx;
+	size_t i, max_length, tx_length;
+	struct mctp_usblib_tx_ops ops;
+	u8 *buf, *flat_tx, *index;
+	struct mctp_usblib_tx *tx;
+	struct net_device *ndev;
+	struct sk_buff *skb;
+	const struct {
+		size_t len;
+		bool more;
+	} sends[] = {
+		{ 1000, true  },
+		{  500, false },
+		{ 5000, false },
+	};
+	int rc;
+
+	static_assert(!sends[ARRAY_SIZE(sends) - 1].more,
+		      "The last push must claim there will be no more");
+
+	max_length = 0;
+	for (i = 0; i < ARRAY_SIZE(sends); i++) {
+		if (sends[i].len > max_length)
+			max_length = sends[i].len;
+	}
+
+	ctx = mctp_usblib_test_init(test);
+	ndev = ctx->dev->ndev;
+
+	ops.send = mctp_usblib_test_tx_send;
+
+	tx = mctp_usblib_test_tx_init(test, &ops, ctx, true);
+	buf = mctp_usblib_test_init_buf(test, max_length);
+
+	for (i = 0; i < ARRAY_SIZE(sends); i++) {
+		skb = mctp_usblib_test_init_skb(test, sends[i].len, ndev, buf);
+
+		rc = mctp_usblib_tx_push(ndev, tx, skb, sends[i].more);
+		KUNIT_ASSERT_EQ(test, rc, 0);
+	}
+	KUNIT_ASSERT_FALSE(test, list_empty(&ctx->tx_xfers));
+
+	flat_tx = mctp_usblib_test_flatten_tx_buff(test, &ctx->tx_xfers,
+						   &tx_length);
+
+	for (i = 0, index = flat_tx; i < ARRAY_SIZE(sends); i++) {
+		size_t length_to_check, remaining_bytes;
+		struct mctp_usb_hdr head;
+
+		if (index - flat_tx >= tx_length - HDR_LEN)
+			break;
+
+		mctp_usblib_test_fill_head(&head, sends[i].len);
+		KUNIT_EXPECT_MEMEQ(test, index, &head, HDR_LEN);
+		index += HDR_LEN;
+		remaining_bytes = tx_length - (index - flat_tx);
+
+		length_to_check = sends[i].len;
+		KUNIT_EXPECT_GE(test, remaining_bytes, length_to_check);
+		length_to_check = min(remaining_bytes, length_to_check);
+
+		KUNIT_EXPECT_MEMEQ(test, index,
+				   buf, length_to_check);
+
+		index += length_to_check;
+	}
+	KUNIT_EXPECT_EQ(test, i, ARRAY_SIZE(sends));
+}
+
 static struct kunit_case mctp_usblib_test_cases[] = {
 	KUNIT_CASE(mctp_usblib_test_rx_single),
 	KUNIT_CASE_PARAM(mctp_usblib_test_rx_pkt_span,
@@ -593,6 +876,9 @@ static struct kunit_case mctp_usblib_test_cases[] = {
 	KUNIT_CASE(mctp_usblib_test_rx_invalid_dmtf_id),
 	KUNIT_CASE(mctp_usblib_test_rx_nonspanning_tiny),
 	KUNIT_CASE(mctp_usblib_test_rx_nonspanning_partial),
+	KUNIT_CASE(mctp_usblib_test_tx_pkt_span),
+	KUNIT_CASE(mctp_usblib_test_tx_multi_push),
+	KUNIT_CASE(mctp_usblib_test_tx_failing_send),
 	{}
 };
 

-- 
2.47.3

Re: [PATCH net-next v2 6/7] net: mctp: usblib: Add initial kunit tx tests

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 2

- [High] Hard compile error: `static_assert(!sends[ARRAY_SIZE(sends) -
  1].more, ...)` in mctp_usblib_test_tx_multi_push()…
- [Low] mctp_usblib_test_tx_multi_push() (drivers/net/mctp/mctp-usblib-
  test.c:796-869) is documented as "Test sending multiple packets in the…
- [Low] mctp_usblib_test_tx_send() (drivers/net/mctp/mctp-usblib-
  test.c:39-59) calls mctp_usblib_tx_send_complete(tx_ctx, ndev, true)…

--

net: mctp: usblib: Add initial kunit tx tests

This adds KUnit coverage for the mctp-usblib transmit path: a spanning
transfer that is expected to succeed, the failure paths when ops->send
returns an error, and a sequence of larger pushes. The tests record each
->send() call in a list and compare the resulting bytes against the
expected MCTP-over-USB headers and payload.

> diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
> index 64c758013854..73bda4745433 100644
> --- a/drivers/net/mctp/mctp-usblib-test.c
> +++ b/drivers/net/mctp/mctp-usblib-test.c

[ ... ]

> @@ -24,9 +32,104 @@ struct mctp_usblib_test_dev {
>  
>  struct mctp_usblib_test_ctx {
>  	struct mctp_usblib_test_dev *dev;
> +	struct list_head tx_xfers;
>  	struct mctp_route rt;
>  };
>  
> +static int mctp_usblib_test_tx_send(struct mctp_usblib_tx_ctx *tx_ctx,
> +				    void *data, size_t len)
> +{
> +	struct mctp_usblib_test_ctx *ctx;
> +	struct tx_buff *new_node;
> +	struct net_device *ndev;
> +
> +	ctx = mctp_usblib_tx_ctx_priv(tx_ctx);
> +	ndev = ctx->dev->ndev;
> +
> +	new_node = kzalloc_flex(*new_node, data, len, GFP_KERNEL);
> +	if (!new_node)
> +		return -ENOMEM;
> +
> +	new_node->length = len;
> +	memcpy(new_node->data, data, len);
> +	list_add_tail(&new_node->list, &ctx->tx_xfers);
> +
> +	mctp_usblib_tx_send_complete(tx_ctx, ndev, true);
> +	return 0;
> +}

[Severity: Low]

Is completing the transfer inline here consistent with the documented
ops->send contract?

include/linux/usb/mctp-usb.h says, for struct mctp_usblib_tx_ops::send:

	/* Start a USB TX for @data. On returning success, the implementation
	 * must arrange for mctp_usblib_tx_send_complete() to be called at some
	 * later point (eg., on urb completion).
	 */

mctp_usblib_test_tx_send() instead calls mctp_usblib_tx_send_complete()
(which runs mctp_usblib_tx_stats_update() and mctp_usblib_tx_ctx_free())
and then returns 0, so the tx context is already freed at the point
success is reported.  The only in-tree implementer,
mctp_usb_out_complete() in drivers/net/mctp/mctp-usb.c, completes
asynchronously from URB completion.

Nothing breaks today: mctp_usblib_tx_send() reads ctx->tx->ops.send, buf
and ctx->len before the call and only returns the result, and both send
sites in mctp_usblib_tx_push() touch the context afterwards only under
if (rc).  But if the library ever legitimately inspects the context after
a successful ->send(), this harness would report it as a KASAN
use-after-free in the test rather than a driver bug.

Would it be better to either record the context and complete it after the
push returns, or to relax the header comment to explicitly allow
synchronous completion?

[ ... ]

> +static u8 *mctp_usblib_test_flatten_tx_buff(struct kunit *test,
> +					    struct list_head *in,
> +					    size_t *length_out)
> +{
[ ... ]
> +	tail = buf;
> +	list_for_each_entry(pos, in, list) {
> +		memcpy(tail, pos->data, pos->length);
> +		tail += pos->length;
> +	}
> +
> +	*length_out = length;
> +	return buf;
> +}

[ ... ]

> @@ -584,6 +717,156 @@ static void mctp_usblib_test_rx_nonspanning_partial(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, 0);
>  }
>  
> +static void mctp_usblib_test_tx_pkt_span(struct kunit *test)
> +{

[ ... ]

> +/* Test sending multiple packets in the same transfer, followed by one that
> + * spans multiple subsequent transfers.
> + */
> +static void mctp_usblib_test_tx_multi_push(struct kunit *test)
> +{

[Severity: Low]

Do the assertions in this test actually observe either of the two
properties named in the comment?

mctp_usblib_test_tx_send() records every ops->send() call as its own
tx_buff node, but mctp_usblib_test_flatten_tx_buff() concatenates all of
them into one buffer before any comparison, so the transfer count and the
transfer boundaries are discarded.  If mctp_usblib_tx_append() or
mctp_usblib_tx_should_send() regressed and the {1000, true} and
{500, false} packets each went out in their own transfer, the flattened
byte stream would be identical and the test would still pass.

For the second half of the comment, the {5000, false} entry does not span
transfers at the ops layer.  mctp_usblib_tx_push() calls
mctp_usblib_tx_ctx_create(tx, skb, single = !more), and:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_ctx_create() {
	...
	if (single || skb->len > TX_SPAN_MAX) {
		type = TX_SINGLE;
	} else {
	...
}

so with more == false (and skb->len of 5004 exceeding TX_SPAN_MAX anyway)
mctp_usblib_tx_send() issues exactly one ops->send() carrying all 5004
bytes; the library never splits one MCTP packet over several ->send()
calls.

The same naming question applies to mctp_usblib_test_tx_pkt_span(), which
also pushes with more == false and produces a single ->send(), so what it
really covers is the v1.1 13-bit length path in
mctp_usblib_tx_skb_prepare() rather than spanning.

Could the comment and the test names be reworded, and could the
per-transfer boundaries be checked instead of flattened, so the packing
behaviour is actually asserted?

> +	struct mctp_usblib_test_ctx *ctx;
> +	size_t i, max_length, tx_length;
> +	struct mctp_usblib_tx_ops ops;
> +	u8 *buf, *flat_tx, *index;
> +	struct mctp_usblib_tx *tx;
> +	struct net_device *ndev;
> +	struct sk_buff *skb;
> +	const struct {
> +		size_t len;
> +		bool more;
> +	} sends[] = {
> +		{ 1000, true  },
> +		{  500, false },
> +		{ 5000, false },
> +	};
> +	int rc;
> +
> +	static_assert(!sends[ARRAY_SIZE(sends) - 1].more,
> +		      "The last push must claim there will be no more");

[Severity: High]

Does this static_assert() compile?

sends[] is a block-scope object with automatic storage duration; the const
qualifier does not make a read of one of its members an integer constant
expression, which is what _Static_assert() requires.  include/linux/build_bug.h
expands static_assert() straight through:

	#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
	#define __static_assert(expr, msg, ...) _Static_assert(expr, msg)

so gcc reports "expression in static assertion is not constant" and clang
reports "static assertion expression is not an integral constant
expression" here.

Because drivers/net/mctp/mctp-usblib.c textually includes this file:

	#if IS_ENABLED(CONFIG_MCTP_TRANSPORT_USBLIB_TEST)
	#include "mctp-usblib-test.c"
	#endif

the failure takes out the whole translation unit, so
drivers/net/mctp/mctp-usblib.o does not build whenever
CONFIG_MCTP_TRANSPORT_USBLIB_TEST is enabled, which is exactly the config
needed to run these tests.  The same static_assert is still present in
mctp_usblib_test_tx_multi_push() at the end of the series (7b7c42135cec),
so nothing later in the series fixes it.

Would BUILD_BUG_ON(), which relies on optimizer folding, or a run-time
KUNIT_ASSERT_FALSE(test, sends[ARRAY_SIZE(sends) - 1].more) express the
same invariant?

> +
> +	max_length = 0;
> +	for (i = 0; i < ARRAY_SIZE(sends); i++) {
> +		if (sends[i].len > max_length)
> +			max_length = sends[i].len;
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-dev-mctp-usb-lib-test-v2-0-313cf15e0b56%40codeconstruct.com.au

[PATCH net-next v2 7/7] net: mctp: usblib: Add test for failing append

[James Lee]

Add test ensuring that network device stats are updated appropriately
when a previously pushed packet fails to send during a subsequent push.

Signed-off-by: James Lee <james@codeconstruct.com.au>

---
v2:
- Fix preemption management
---
 drivers/net/mctp/mctp-usblib-test.c | 42 +++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
index 73bda4745433f689504e65685b73da2117f1d3db..25eac80db3e2d12aabdabbe234261dc335d653b8 100644
--- a/drivers/net/mctp/mctp-usblib-test.c
+++ b/drivers/net/mctp/mctp-usblib-test.c
@@ -867,6 +867,47 @@ static void mctp_usblib_test_tx_multi_push(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, i, ARRAY_SIZE(sends));
 }
 
+static void mctp_usblib_test_tx_overflow(struct kunit *test)
+{
+	struct mctp_usblib_test_ctx *ctx;
+	struct mctp_usblib_tx_ops ops;
+	struct pcpu_dstats *dstats;
+	struct mctp_usblib_tx *tx;
+	struct net_device *ndev;
+	struct sk_buff *skb;
+	unsigned int start;
+	size_t len, i;
+	u64 stats;
+	u8 *buf;
+	int rc;
+
+	len = 200;
+
+	ctx = mctp_usblib_test_init(test);
+	ndev = ctx->dev->ndev;
+
+	ops.send = mctp_usblib_test_tx_send_fail;
+
+	tx = mctp_usblib_test_tx_init(test, &ops, ctx, false);
+	buf = mctp_usblib_test_init_buf(test, len);
+
+	dstats = get_cpu_ptr(ndev->dstats);
+	for (i = 0; i < 3; i++) {
+		skb = mctp_usblib_test_init_skb(test, len, ndev, buf);
+
+		rc = mctp_usblib_tx_push(ndev, tx, skb, i != 2);
+		KUNIT_EXPECT_EQ(test, rc, 0);
+	}
+
+	do {
+		start = u64_stats_fetch_begin(&dstats->syncp);
+		stats = u64_stats_read(&dstats->tx_drops);
+	} while (u64_stats_fetch_retry(&dstats->syncp, start));
+	put_cpu_ptr(dstats);
+
+	KUNIT_EXPECT_EQ(test, stats, 3);
+}
+
 static struct kunit_case mctp_usblib_test_cases[] = {
 	KUNIT_CASE(mctp_usblib_test_rx_single),
 	KUNIT_CASE_PARAM(mctp_usblib_test_rx_pkt_span,
@@ -879,6 +920,7 @@ static struct kunit_case mctp_usblib_test_cases[] = {
 	KUNIT_CASE(mctp_usblib_test_tx_pkt_span),
 	KUNIT_CASE(mctp_usblib_test_tx_multi_push),
 	KUNIT_CASE(mctp_usblib_test_tx_failing_send),
+	KUNIT_CASE(mctp_usblib_test_tx_overflow),
 	{}
 };
 

-- 
2.47.3

Re: [PATCH net-next v2 7/7] net: mctp: usblib: Add test for failing append

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 1

- [High] drivers/net/mctp/mctp-usblib-
  test.c:mctp_usblib_test_tx_overflow() disables preemption with `dstats…
- [Low] The oracle of the new mctp_usblib_test_tx_overflow()
  (drivers/net/mctp/mctp-usblib-test.c:869-908) is a single aggregate…

--

net: mctp: usblib: Add test for failing append

Adds a KUnit case, mctp_usblib_test_tx_overflow(), to the mctp-usblib
test suite that pushes three 200-byte packets through
mctp_usblib_tx_push() with a failing send op, then checks that the
per-CPU tx_drops counter reached 3.

> diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
> index 73bda4745433..25eac80db3e2 100644
> --- a/drivers/net/mctp/mctp-usblib-test.c
> +++ b/drivers/net/mctp/mctp-usblib-test.c
> @@ -867,6 +867,47 @@ static void mctp_usblib_test_tx_multi_push(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, i, ARRAY_SIZE(sends));
>  }
>  
> +static void mctp_usblib_test_tx_overflow(struct kunit *test)
> +{
> +	struct mctp_usblib_test_ctx *ctx;
> +	struct mctp_usblib_tx_ops ops;
> +	struct pcpu_dstats *dstats;
> +	struct mctp_usblib_tx *tx;
> +	struct net_device *ndev;
> +	struct sk_buff *skb;
> +	unsigned int start;
> +	size_t len, i;
> +	u64 stats;
> +	u8 *buf;
> +	int rc;
> +
> +	len = 200;

[Severity: Low]
This isn't a bug, but len = 200 and the loop count of 3 below silently
encode a dependency on MCTP_USB_1_0_XFER_SIZE (512) and MCTP_USB_BTU,
without referencing either constant.

I traced the intended path and it does run today with span=false: each skb
becomes 204 bytes after mctp_usblib_tx_skb_prepare() pushes the 4-byte
header, push 1 creates the 512-byte ctx, push 2 appends (len 408, avail
104), and push 3 hits mctp_usblib_tx_append():

	if (mctp_usblib_tx_avail(ctx) < skb->len)
		return -ENOBUFS;

with avail 104 < skb->len 204.

Could len and the iteration count be derived from
MCTP_USB_1_0_XFER_SIZE / MCTP_USB_BTU instead, so the case keeps hitting
the append-failure branch if those change?

Also, would tx_overflow be better named after the append failure the
commit subject describes?

> +
> +	ctx = mctp_usblib_test_init(test);
> +	ndev = ctx->dev->ndev;
> +
> +	ops.send = mctp_usblib_test_tx_send_fail;
> +
> +	tx = mctp_usblib_test_tx_init(test, &ops, ctx, false);
> +	buf = mctp_usblib_test_init_buf(test, len);
> +
> +	dstats = get_cpu_ptr(ndev->dstats);
> +	for (i = 0; i < 3; i++) {
> +		skb = mctp_usblib_test_init_skb(test, len, ndev, buf);
> +
> +		rc = mctp_usblib_tx_push(ndev, tx, skb, i != 2);
> +		KUNIT_EXPECT_EQ(test, rc, 0);
> +	}

[Severity: High]
Can this sleep with preemption disabled?  get_cpu_ptr() expands to
preempt_disable() plus this_cpu_ptr(), so the whole loop runs in atomic
context, and mctp_usblib_test_init_skb() allocates with GFP_KERNEL:

drivers/net/mctp/mctp-usblib-test.c:mctp_usblib_test_init_skb() {
	skb = __netdev_alloc_skb(ndev, length, GFP_KERNEL);
	KUNIT_ASSERT_NOT_NULL(test, skb);
	...
}

GFP_KERNEL sets __GFP_DIRECT_RECLAIM, so __netdev_alloc_skb() takes the
blocking slab path:

net/core/skbuff.c:__netdev_alloc_skb() {
	if (len <= SKB_WITH_OVERHEAD(SKB_SMALL_HEAD_CACHE_SIZE) ||
	    len > SKB_WITH_OVERHEAD(PAGE_SIZE) ||
	    (gfp_mask & (__GFP_DIRECT_RECLAIM | GFP_DMA))) {
		skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX, NUMA_NO_NODE);
	...
}

With CONFIG_DEBUG_ATOMIC_SLEEP, wouldn't the might_sleep() in the
allocator produce a "sleeping function called from invalid context" splat
every time this case runs?

Two further consequences of the same region:

The KUNIT_ASSERT_NOT_NULL() in mctp_usblib_test_init_skb() and the KUnit
failure reporting behind KUNIT_EXPECT_EQ() also allocate with GFP_KERNEL,
and the assert variant does not return:

lib/kunit/try-catch.c:kunit_try_catch_throw() {
	try_catch->try_result = -EFAULT;
	kthread_exit(0);
}

so put_cpu_ptr() below is skipped and the thread exits still in atomic
context, which kernel/exit.c reports:

kernel/exit.c:do_exit() {
	if (unlikely(in_atomic())) {
		pr_info("note: %s[%d] exited with preempt_count %d\n",
			current->comm, task_pid_nr(current),
			preempt_count());
		preempt_count_set(PREEMPT_ENABLED);
	}
	...
}

mctp_usblib_tx_push() also takes a spinlock_t here:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_push() {
	spin_lock_irqsave(&tx->lock, flags);
	...
}

On PREEMPT_RT that is a sleeping lock, so is acquiring it from an
explicitly preempt-disabled section valid?

Note the driver itself uses GFP_ATOMIC on this path
(mctp_usblib_tx_ctx_create() uses kzalloc_flex(..., GFP_ATOMIC)), so only
the test's own allocation is out of context.

Since mctp_usblib_tx_stats_update() picks the counter with
get_cpu_ptr(dev->dstats), could the skb allocation and the assertions move
outside the pinned region, leaving only the counter read inside
get_cpu_ptr()/put_cpu_ptr(), or could tx_drops be summed over
for_each_possible_cpu() instead?

> +
> +	do {
> +		start = u64_stats_fetch_begin(&dstats->syncp);
> +		stats = u64_stats_read(&dstats->tx_drops);
> +	} while (u64_stats_fetch_retry(&dstats->syncp, start));
> +	put_cpu_ptr(dstats);
> +
> +	KUNIT_EXPECT_EQ(test, stats, 3);

[Severity: Low]
This is the same finding as the len = 200 comment above: this aggregate
drop count is the only oracle in the case.

The append-failure path yields 2 drops from the detached two-skb context
plus 1 from the single-skb context, but so do other paths: if the ctx
buffer size grew, if the MCTP_USB_BTU margin in
mctp_usblib_tx_should_send() changed so the ctx flushed after push 2, or
if all three skbs fit one ctx, the append branch never runs and stats is
still 3.

Neither the number of ops.send() invocations, nor tx->cur_ctx, nor
ctx->tx_xfers is checked here, while the neighbouring
mctp_usblib_test_tx_failing_send() does:

	KUNIT_EXPECT_EQ(test, rc, 0);
	KUNIT_EXPECT_NULL(test, tx->cur_ctx);
	KUNIT_EXPECT_TRUE(test, list_empty(&ctx->tx_xfers));

Would checking tx_drops == 0 after the first two pushes and counting the
send attempts (2 sends carrying 2 and 1 packets) pin down the branch under
test?

> +}
> +
>  static struct kunit_case mctp_usblib_test_cases[] = {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-dev-mctp-usb-lib-test-v2-0-313cf15e0b56%40codeconstruct.com.au