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

[netdev-bot+sashiko@kernel.org]

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