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

[PATCH 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>
---
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 | 546 +++++++++++++++++++++++++++++++++++-
 1 file changed, 535 insertions(+), 11 deletions(-)
---
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
change-id: 20260617-dev-mctp-usb-lib-test-151728774e95

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

[PATCH 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

[PATCH 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>
---
 drivers/net/mctp/mctp-usblib-test.c | 69 +++++++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 6 deletions(-)

diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
index 4c27307900fb6116572bc9d89f3def6f06c30397..1a6dd6d10daab314f5e082477ecd3b9518f6b50e 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,71 @@ 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_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_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);
+}
+
+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

[PATCH 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 1a6dd6d10daab314f5e082477ecd3b9518f6b50e..4f499de9a6ce4452dcc71ac38fa26b6595977f77 100644
--- a/drivers/net/mctp/mctp-usblib-test.c
+++ b/drivers/net/mctp/mctp-usblib-test.c
@@ -440,7 +440,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);
@@ -459,11 +458,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

[PATCH 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 4f499de9a6ce4452dcc71ac38fa26b6595977f77..ab323a31161a415d93eafe4314e0e52cedfcd42e 100644
--- a/drivers/net/mctp/mctp-usblib-test.c
+++ b/drivers/net/mctp/mctp-usblib-test.c
@@ -524,6 +524,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,
@@ -531,6 +591,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

[PATCH 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 ab323a31161a415d93eafe4314e0e52cedfcd42e..772484df32a7d7b4d30e51808e06853febe272b6 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 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>
---
 drivers/net/mctp/mctp-usblib-test.c | 292 ++++++++++++++++++++++++++++++++++++
 1 file changed, 292 insertions(+)

diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
index 772484df32a7d7b4d30e51808e06853febe272b6..7dc382caffc17644dd616f78df332201d412f709 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,110 @@ 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;
+	int rc;
+
+	ctx = mctp_usblib_tx_ctx_priv(tx_ctx);
+	ndev = ctx->dev->ndev;
+	rc = 0;
+
+	new_node = kzalloc_flex(*new_node, data, len, GFP_KERNEL);
+	if (!new_node) {
+		rc = -ENOMEM;
+		goto exit;
+	}
+
+	new_node->length = len;
+	memcpy(&new_node->data, data, len);
+	list_add_tail(&new_node->list, &ctx->tx_xfers);
+
+exit:
+	mctp_usblib_tx_send_complete(tx_ctx, ndev, rc == 0);
+	return rc;
+}
+
+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->len = length;
+	memcpy(skb->data, data, length);
+	return skb;
+}
+
 static netdev_tx_t mctp_usblib_dev_tx(struct sk_buff *skb,
 				      struct net_device *ndev)
 {
@@ -119,6 +228,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 +236,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 +253,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 +340,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, true);
+
+	return tx;
+}
+
 /* Single packet, starting on a transfer boundary, contained entirely within
  * the transfer
  */
@@ -582,6 +721,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,
@@ -591,6 +880,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

[PATCH 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>
---
 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 7dc382caffc17644dd616f78df332201d412f709..b3a2cc5cbd99defde308cf23a1b559dbdc73ca22 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/percpu-defs.h"
 #include <linux/array_size.h>
 #include <uapi/linux/netdevice.h>
 #include <linux/netdevice.h>
@@ -871,6 +872,46 @@ 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);
+
+	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);
+	}
+
+	dstats = get_cpu_ptr(ndev->dstats);
+	do {
+		start = u64_stats_fetch_begin(&dstats->syncp);
+		stats = u64_stats_read(&dstats->tx_drops);
+	} while (u64_stats_fetch_retry(&dstats->syncp, start));
+
+	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,
@@ -883,6 +924,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: [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: 0 · Medium: 2 · Low: 1

- [Medium] SKB leak in the new helper
  `mctp_usblib_test_rx_split_header()` (drivers/net/mctp/mctp-usblib-…
- [Medium] `mctp_usblib_test_rx_split_header()` calls the driver's
  completion entry point `mctp_usblib_rx_complete()` directly…
- [Low] Unused local variable `size_t i;` in the newly added
  `mctp_usblib_test_rx_header_splits()` (drivers/net/mctp/mctp-usblib-…

--

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

Adds a new KUnit case, mctp_usblib_test_rx_header_splits(), which walks a
helper across every byte offset where a 16-byte packet's headers can be
split between two transfers, and drops the three parameterized span entries
that covered a subset of those cases.

A few questions about the new test below.

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

[ ... ]

> @@ -403,10 +399,71 @@ 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_rx_complete(dev->ndev, rx, offset);

[Severity: Medium]
Should these two calls go through the mctp_usblib_test_rx_complete()
wrapper rather than calling mctp_usblib_rx_complete() directly?

The wrapper in this same file exists specifically to provide the calling
context the driver expects:

  /* Wrappers for usblib's rx_complete callback, which is intended to be called
   * from atomic context
   */
  static int mctp_usblib_test_rx_complete(struct net_device *netdev,
                                          struct mctp_usblib_rx *rx, size_t len)
  {
          local_bh_disable();
          rc = mctp_usblib_rx_complete(netdev, rx, len);
          local_bh_enable();
          return rc;
  }

mctp_usblib_test_rx_single() and mctp_usblib_test_rx_pkt_span() both use it,
but mctp_usblib_test_rx_split_header() calls the raw entry point from
preemptible kunit process context.

The delivery path starts with a per-CPU pointer fetch in
drivers/net/mctp/mctp-usblib.c:

  static void mctp_usblib_rx(struct net_device *netdev, struct sk_buff *skb)
  {
          struct pcpu_dstats *dstats = this_cpu_ptr(netdev->dstats);
          ...
          flags = u64_stats_update_begin_irqsave(&dstats->syncp);

Can this produce a "BUG: using smp_processor_id() in preemptible code"
splat on a CONFIG_DEBUG_PREEMPT build, given this_cpu_ptr() resolves
through my_cpu_offset/smp_processor_id() there? The same this_cpu_ptr()
pattern is in the drop path used when a header is rejected.

Separately, with preemption enabled between the this_cpu_ptr() and the
u64_stats_update_begin_irqsave(), can the task migrate and leave two
contexts writing the same per-CPU dstats syncp?

Also, both of these calls discard the return code, unlike every other
mctp_usblib_rx_complete() call site in the file which does
KUNIT_ASSERT_EQ(test, rc, 0). Was dropping the check intentional?

> +
> +	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_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);
> +}

[Severity: Medium]
Does this helper leak the dequeued skb?

The skb is unlinked from dev->rx_pkts by __skb_dequeue() and the function
returns without a kfree_skb(). The teardown action only purges the queue:

  static void mctp_usblib_test_dev_action(void *data)
  {
          struct mctp_usblib_test_dev *dev = data;

          skb_queue_purge(&dev->rx_pkts);

so once the skb has been dequeued it is no longer reachable from there.

The two existing tests free it after dequeue:

  skb = __skb_dequeue(&dev->rx_pkts);
  ...
  kfree_skb(skb);

Since mctp_usblib_test_rx_header_splits() calls this helper eight times,
does that leak eight skbs plus their linear data areas per run of the
suite? This looks like it is still missing 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 referenced in the unrolled body below, so
this should warn with -Wunused-variable when the KUnit module is built at
this commit.

The declaration does get removed by the following patch in the series,
"net: mctp: usblib: Add tests of invalid headers"
(49a493a0ed5d5a9d6a79967c53761c7a58c76c67), and it is absent at the tip of
the series, so this is only visible when bisecting through this commit.
Could it be dropped here instead?

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-dev-mctp-usb-lib-test-v1-0-79699bf67a18%40codeconstruct.com.au

Re: [3/7] net: mctp: usblib: Add tests of invalid headers

[netdev-bot+sashiko]

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

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

- [Low] Undisclosed fixup hunk: besides adding the two new test cases
  described in the changelog, this patch silently deletes the unused…
- [Low] The new negative test `mctp_usblib_test_rx_short_packet()`
  documents and exercises the wrong minimum-length boundary.

--

net: mctp: usblib: Add tests of invalid headers

Adds two kunit cases to the mctp-usblib tests: one submitting a packet
whose USB transport header length field is below the minimum, and one
submitting a packet with a corrupted DMTF ID. Both expect -EPROTO from
mctp_usblib_rx_complete(), a NULL rx->skb, and an empty rx queue.

> diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
> index 1a6dd6d10daa..4f499de9a6ce 100644
> --- a/drivers/net/mctp/mctp-usblib-test.c
> +++ b/drivers/net/mctp/mctp-usblib-test.c
> @@ -440,7 +440,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);

[Severity: Low]
Should this hunk be squashed into the previous patch instead?  The commit
message only mentions adding invalid-header tests, but this removes an
unused declaration from mctp_usblib_test_rx_header_splits(), which is a
different function and unrelated to the new cases.

That declaration arrives with the preceding patch "net: mctp: usblib: Add
test for splits inside headers", where the body only contains the unrolled
calls and never uses i:

	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;

	mctp_usblib_test_rx_split_header(test, 1, dev, rx);
	...
	mctp_usblib_test_rx_split_header(test, 8, dev, rx);

So the intermediate commit builds with a -Wunused-variable warning, which
shows up during a bisect.  Would folding the removal into that patch (or at
least mentioning it in this changelog) be preferable?

> @@ -459,11 +458,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)
> +{

[Severity: Low]
Is "Values less than HDR_LEN are invalid" the right threshold here?
HDR_LEN is sizeof(struct mctp_usb_hdr), i.e. 4, but the check in
mctp_usblib_rx_complete() rejects anything below the USB header plus the
MCTP header, i.e. 8:

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

> +	len = sizeof(pktbuf);
> +	mctp_usblib_test_init_pkt(pktbuf, len, HDR_LEN - 1);

Following on from the comment above, does this pin the boundary the test
name suggests?  With hdr.usb.len set to 3, the case still passes if the
driver check were weakened to pkt_len < sizeof(*hdr), so lengths of 4..7
stay uncovered.

Those are the values that would otherwise reach the pkt_len == skb->len /
skb_clone path and hand mctp_usblib_rx() an skb whose
skb_pull(skb, sizeof(struct mctp_usb_hdr)) leaves no struct mctp_hdr behind
the USB header.

Would HDR_LEN + sizeof(struct mctp_hdr) - 1, or a parameterised sweep over
0..7, be a closer match to the validation being tested?  The comment and
the value are unchanged through the end of the series (0782a305), so this
applies to the final state of the file too.

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-dev-mctp-usb-lib-test-v1-0-79699bf67a18%40codeconstruct.com.au

Re: [6/7] net: mctp: usblib: Add initial kunit tx tests

[netdev-bot+sashiko]

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

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

- [High] Use-after-free / double-free: the model ->send() hook
  mctp_usblib_test_tx_send() calls mctp_usblib_tx_send_complete()…
- [Medium] mctp_usblib_test_tx_init() accepts a `bool span` parameter but
  discards it, passing the literal `true` to mctp_usblib_tx_init()…
- [Medium] static_assert(!sends[ARRAY_SIZE(sends) - 1].more, ...) at
  drivers/net/mctp/mctp-usblib-test.c:822 reads an element of `sends[]`,…
- [Medium] The test comment on mctp_usblib_test_tx_multi_push() claims it
  tests "multiple packets in the same transfer, followed by one that…
- [Low] mctp_usblib_test_init_skb() (drivers/net/mctp/mctp-usblib-
  test.c:134-135) assigns skb->len = length and memcpy()s into skb->data…

--

Thanks for the patch. A few questions about the new test code below.

net: mctp: usblib: Add initial kunit tx tests

Adds three KUnit cases for the mctp-usblib transmit path: a spanning
transfer that should succeed, the behaviour when ops.send() fails, and a
sequence of pushes ending in a large packet. The tests drive
mctp_usblib_tx_push() with a model ops.send() that captures each outgoing
transfer into a list for later comparison.

> diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
> index 772484df32a7..7dc382caffc1 100644
> --- a/drivers/net/mctp/mctp-usblib-test.c
> +++ b/drivers/net/mctp/mctp-usblib-test.c

[ ... ]

> @@ -24,9 +32,110 @@ 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;
> +	int rc;
> +
> +	ctx = mctp_usblib_tx_ctx_priv(tx_ctx);
> +	ndev = ctx->dev->ndev;
> +	rc = 0;
> +
> +	new_node = kzalloc_flex(*new_node, data, len, GFP_KERNEL);
> +	if (!new_node) {
> +		rc = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	new_node->length = len;
> +	memcpy(&new_node->data, data, len);
> +	list_add_tail(&new_node->list, &ctx->tx_xfers);
> +
> +exit:
> +	mctp_usblib_tx_send_complete(tx_ctx, ndev, rc == 0);
> +	return rc;
> +}

[Severity: High]
Can the kzalloc_flex() failure path here free the tx context twice?

On allocation failure this goes to exit:, which still calls
mctp_usblib_tx_send_complete(), and then returns -ENOMEM.
mctp_usblib_tx_send_complete() drops the context:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_send_complete() {
	mctp_usblib_tx_stats_update(tx_ctx, dev, ok);
	mctp_usblib_tx_ctx_free(tx_ctx, reason);
}

and mctp_usblib_tx_ctx_free() dequeues/frees every skb and kfree()s ctx.

But the non-zero return makes mctp_usblib_tx_push() believe it still owns
the context, at both of its send sites:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_push() {
	...
	rc = mctp_usblib_tx_send(ctx);
	if (rc) {
		mctp_usblib_tx_stats_update(ctx, dev, false);
		mctp_usblib_tx_ctx_free(ctx, reason);
	}
	...
}

That reads ctx->skbs.qlen and ctx->len after the free, then runs
__skb_dequeue() on the freed list head and kfree()s ctx a second time.

The header documents the contract as completion being owed only for a
successful send:

include/linux/usb/mctp-usb.h:
	/* 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).
	 */

and the real driver mctp_usb_tx_send() follows it by returning rc from
usb_submit_urb() without calling send_complete(). Should the exit: path
only call mctp_usblib_tx_send_complete() when rc == 0?

The trigger is a NULL return from the kzalloc_flex(), which becomes
deterministic under slab fault injection (CONFIG_FAILSLAB), a config often
combined with KUnit runs.

> +
> +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 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->len = length;
> +	memcpy(skb->data, data, length);
> +	return skb;
> +}

[Severity: Low]
Would skb_put_data(skb, data, length) be preferable here?

After __netdev_alloc_skb() the tail pointer equals skb->data and skb->len
is 0. Setting skb->len directly leaves skb->len != skb_tail_pointer(skb) -
skb->data, so skb_tailroom() over-reports by length and the skb_put() tail
check is bypassed for every skb the three new tx tests hand to
mctp_usblib_tx_push().

I traced the current path and it happens to survive: the 4 bytes of header
room needed by mctp_usblib_tx_skb_prepare() are already covered by
NET_SKB_PAD so pskb_expand_head() is never reached, and
mctp_usblib_tx_send() copies using skb_headlen()/skb->data rather than the
tail pointer. So there is no out-of-bounds access today, but the skbs do
not model what the networking core hands to ndo_start_xmit, and anything
later added on this path that uses skb_tail_pointer(), skb_tailroom() or
pskb_expand_head() would silently see zero-length payload.

[ ... ]

> @@ -227,6 +340,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, true);
> +
> +	return tx;
> +}

[Severity: Medium]
Should this pass span rather than the literal true?

As written the span parameter is unused, so mctp_usblib_test_tx_failing_send()
below, which calls mctp_usblib_test_tx_init(test, &ops, ctx, false), still
gets a spanning tx engine.

tx->span is the only selector for the tx geometry:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_skb_prepare() {
	max_len = span ? MCTP_USB_1_1_PKTLEN_MAX : MCTP_USB_1_0_PKTLEN_MAX;

drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_ctx_create() {
		sz = tx->span ? TX_SPAN_MAX : MCTP_USB_1_0_XFER_SIZE;

So the v1.0 non-spanning geometry (the 255-byte packet ceiling with
-EMSGSIZE / SKB_DROP_REASON_PKT_TOO_BIG, and the 512-byte aggregation
buffer that drives mctp_usblib_tx_append()) ends up with no coverage at
all.

This also affects the later mctp_usblib_test_tx_overflow() in the series:
with span forced true, three 204-byte transfers fit in the ~4032-byte span
buffer, so mctp_usblib_tx_append() never returns -ENOBUFS and the
append-overflow path the test is named for is not executed, though
tx_drops still reaches 3 through the send failure and the test passes.

[ ... ]

> @@ -582,6 +721,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_failing_send(struct kunit *test)
> +{

[ ... ]

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

[ ... ]

> +/* 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");

[Severity: Medium]
Does this static_assert() compile?

sends[] is a function-local array with automatic storage duration, and an
array subscript plus member access on such an object is not an integer
constant expression in C even when the array is const-qualified. Clang
reports "static assertion expression is not an integral constant
expression" and GCC reports "expression in static assertion is not
constant".

Since mctp-usblib-test.c is textually included:

drivers/net/mctp/mctp-usblib.c:
	#if IS_ENABLED(CONFIG_MCTP_TRANSPORT_USBLIB_TEST)
	#include "mctp-usblib-test.c"
	#endif

this would break the whole mctp-usblib.c translation unit whenever
CONFIG_MCTP_TRANSPORT_USBLIB_TEST=y, which is the default under
KUNIT_ALL_TESTS. Marking sends static const does not make the expression
an integer constant expression either; would a runtime
KUNIT_ASSERT_FALSE() on the last entry work instead?

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

[Severity: Medium]
Do the assertions in this test actually check the transfer boundaries the
comment describes?

mctp_usblib_test_flatten_tx_buff() concatenates every captured ops.send()
call into one contiguous buffer, and the checks below only walk that byte
stream. The number of tx_buff nodes and their individual lengths are never
examined, so a change that regroups packets into different transfers while
preserving byte order still passes.

Also, the last sends[] entry has more == false, and mctp_usblib_tx_push()
inverts that:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_push() {
	ctx = mctp_usblib_tx_ctx_create(tx, skb, !more);

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

Tracing the three pushes: the 1000-byte push creates a TX_FLAT context and
queues, the 500-byte push appends and sends one 1508-byte transfer, and
the 5000-byte push takes TX_SINGLE and sends one 5004-byte transfer. That
is two ops.send() calls, both single transfers, so nothing "spans multiple
subsequent transfers".

The commit message says "Testing a spanning transfer that is expected to
succeed" - is that the same mismatch, or is a different meaning of
spanning intended there?

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-dev-mctp-usb-lib-test-v1-0-79699bf67a18%40codeconstruct.com.au

Re: [7/7] net: mctp: usblib: Add test for failing append

[netdev-bot+sashiko]

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

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

- [Medium] The new KUnit test `mctp_usblib_test_tx_overflow()`
  (drivers/net/mctp/mctp-usblib-test.c) does not exercise the append-…
- [Medium] Preempt-count leak in the new test:
  `mctp_usblib_test_tx_overflow()` calls `dstats =…
- [Medium] `mctp_usblib_test_tx_overflow()` verifies the drop accounting
  by reading `tx_drops` from only one CPU's per-CPU `pcpu_dstats` — the…
- [Low] The patch adds `#include "linux/percpu-defs.h"` at
  drivers/net/mctp/mctp-usblib-test.c:9 — a quoted (local-path) include…

--

net: mctp: usblib: Add test for failing append

Adds a KUnit case, mctp_usblib_test_tx_overflow(), which pushes three
200-byte packets through mctp_usblib_tx_push() with a failing send op and
then checks that tx_drops was incremented three times.  A few questions
about the new test below.

> diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
> index 7dc382caffc1..b3a2cc5cbd99 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/percpu-defs.h"

[Severity: Low]
This isn't a bug, but is this include intentional?  It uses the quoted
form for a kernel header and sits ahead of the sorted <linux/...> block,
and it appears redundant: the file already includes <linux/netdevice.h>,
which pulls in <linux/percpu.h> and therefore percpu-defs.h, and which is
where struct pcpu_dstats comes from.  Could it be dropped, or at least
switched to the angle-bracket form in the sorted position?  The commit
message doesn't mention this hunk.

>  #include <linux/array_size.h>
>  #include <uapi/linux/netdevice.h>
>  #include <linux/netdevice.h>

[ ... ]

> @@ -871,6 +872,46 @@ 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)
> +{

[ ... ]

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

[Severity: Medium]
Does this test actually reach the append-overflow path it is named for?
The span = false argument is discarded by the helper, which hardcodes
true:

drivers/net/mctp/mctp-usblib-test.c:mctp_usblib_test_tx_init() {
	...
	mctp_usblib_tx_init(tx, ops, priv, true);
	...
}

With span forced on, mctp_usblib_tx_ctx_create() picks the large buffer:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_ctx_create() {
	...
	sz = tx->span ? TX_SPAN_MAX : MCTP_USB_1_0_XFER_SIZE;
	...
}

Each skb here is 200 bytes plus the 4-byte struct mctp_usb_hdr pushed by
mctp_usblib_tx_skb_prepare(), so 3 x 204 bytes fit in the ~4040-byte
spanning buffer and this check never fires:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_append() {
	...
	if (mctp_usblib_tx_avail(ctx) < skb->len)
		return -ENOBUFS;
	...
}

So only the single !more send at i == 2 fails, and
mctp_usblib_tx_stats_update() adds ctx->skbs.qlen == 3 in one go.

> +	buf = mctp_usblib_test_init_buf(test, len);
> +
> +	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);
> +	}
> +
> +	dstats = get_cpu_ptr(ndev->dstats);

[Severity: Medium]
Is the missing put_cpu_ptr() here a problem?  get_cpu_ptr() expands to
preempt_disable() plus this_cpu_ptr(), and mctp_usblib_test_tx_overflow()
returns without re-enabling preemption, so the case leaves preempt_count
elevated by one.

Two effects follow.  The KUNIT_EXPECT_EQ() below now runs in atomic
context, and on failure it reaches a sleeping allocation:

lib/kunit/test.c:kunit_fail() {
	...
	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
	...
}

which gives "BUG: sleeping function called from invalid context" with
CONFIG_DEBUG_ATOMIC_SLEEP.  On the passing path the kunit try-thread exits
with preempt_count 1, so do_exit() complains and force-resets the count.

The driver pairs these calls in both of its stats helpers, e.g.:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_stats_update() {
	struct pcpu_dstats *dstats = get_cpu_ptr(dev->dstats);
	...
	put_cpu_ptr(dev->dstats);
}

Could this read use per_cpu_ptr() instead, given it does not need
preemption disabled at all?

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

[Severity: Medium]
Can this expectation fail spuriously on an SMP preemptible kernel?

The three mctp_usblib_tx_push() calls above run in the kunit try-thread
with preemption enabled and no migrate_disable(), and each writer picks
the current CPU inside its own short window:

drivers/net/mctp/mctp-usblib.c:mctp_usblib_tx_stats_single_drop() {
	struct pcpu_dstats *dstats = get_cpu_ptr(dev->dstats);
	...
	put_cpu_ptr(dev->dstats);
}

If the thread migrates between pushes, the increments are spread over
several CPUs while this code samples only one, so stats comes back below
3 and the failure looks like a driver accounting bug.  Would aggregating
over for_each_possible_cpu() (each with its own fetch_begin/retry loop),
or calling dev_get_stats(), be more robust here?

[Severity: Medium]
A related question on this assertion: both control flows produce
tx_drops == 3.  In the intended non-spanning case the third append fails
with -ENOBUFS (avail 512 - 408 = 104 < 204), the pending 2-skb context
fails to send (+2), and the new single-packet context fails to send (+1).
In the spanning case actually exercised, one context of 3 skbs fails to
send (+3).

Since the aggregate is identical, would this test still pass if the
append-failure accounting were broken?  As written it looks like it
duplicates mctp_usblib_test_tx_failing_send() rather than adding new
coverage.

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-dev-mctp-usb-lib-test-v1-0-79699bf67a18%40codeconstruct.com.au