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

[netdev-bot+sashiko@kernel.org]

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