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

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.

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

[ ... ]

[ ... ]

+	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