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

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.

[ ... ]

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

[ ... ]

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

[ ... ]

[ ... ]

+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