Re: [PATCH net-next 10/12] net: mctp: usblib: Add initial kunit tests

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: mctp: usblib: Add initial kunit tests

This commit introduces an initial set of kunit tests covering the
mctp-usblib receive path, exercising packet extraction from incoming
USB transfer data.

> diff --git a/drivers/net/mctp/mctp-usblib-test.c b/drivers/net/mctp/mctp-usblib-test.c
> new file mode 100644
> index 000000000000..ac837cbb4436
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-usblib-test.c
> @@ -0,0 +1,330 @@

[ ... ]

> +static struct mctp_usblib_test_dev *mctp_usblib_test_create_dev(void)
> +{
> +	struct mctp_usblib_test_dev *dev;
> +	struct net_device *ndev;
> +	int rc;
> +
> +	ndev = alloc_netdev(sizeof(*dev), "mctptest%d", NET_NAME_ENUM,
> +			    mctp_usblib_dev_setup);
> +	if (!ndev)
> +		return NULL;
> +
> +	dev = netdev_priv(ndev);
> +	dev->ndev = ndev;
> +	skb_queue_head_init(&dev->rx_pkts);
> +
> +	rc = register_netdev(ndev);
> +	if (rc) {
> +		free_netdev(ndev);
> +		return NULL;
> +	}
> +
> +	rcu_read_lock();
> +	dev->mdev = __mctp_dev_get(ndev);
> +	dev->mdev->net = mctp_default_net(dev_net(ndev));
> +	rcu_read_unlock();

[Low]
Can __mctp_dev_get() return NULL here?  Looking at net/mctp/device.c:

    struct mctp_dev *__mctp_dev_get(const struct net_device *dev)
    {
        struct mctp_dev *mdev = rcu_dereference(dev->mctp_ptr);

        if (mdev)
            if (!refcount_inc_not_zero(&mdev->refs))
                return NULL;
        return mdev;
    }

If mctp_add_dev() fails inside the NETDEV_REGISTER notifier path, mctp_ptr
will not be installed and __mctp_dev_get() returns NULL, making the
subsequent dev->mdev->net = ... a NULL dereference in the test kthread.

> +
> +	rtnl_lock();
> +	dev_open(ndev, NULL);
> +	rtnl_unlock();

[Low]
Is it intentional to ignore the return value of dev_open() here?  If
dev_open() fails the netdev is registered but never brought up, and the
test then proceeds against an inconsistently-initialized device.

> +
> +	return dev;
> +}
> +
> +static int mctp_usblib_test_dst_output(struct mctp_dst *dst,
> +				       struct sk_buff *skb)
> +{
> +	struct mctp_usblib_test_dev *dev = netdev_priv(skb->dev);
> +
> +	skb_queue_tail(&dev->rx_pkts, skb);
> +
> +	return 0;
> +}
> +
> +static void mctp_usblib_test_init(struct kunit *test,
> +				  struct mctp_usblib_test_ctx *ctx)
> +{
> +	struct mctp_route *rt = &ctx->rt;
> +
> +	ctx->dev = mctp_usblib_test_create_dev();
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->dev);
> +
> +	memset(rt, 0, sizeof(*rt));
> +	rt->min = local_eid;
> +	rt->max = local_eid;
> +	rt->dst_type = MCTP_ROUTE_DIRECT;
> +	rt->type = RTN_LOCAL;
> +	rt->dev = ctx->dev->mdev;
> +	rt->output = mctp_usblib_test_dst_output;
> +
> +	list_add_rcu(&ctx->rt.list, &init_net.mctp.routes);
> +}

[High]
What happens to ctx.rt on a KUNIT_ASSERT_* failure later in the test?

The ctx variable is a stack local in mctp_usblib_test_rx_single() and
mctp_usblib_test_rx_pkt_span(), and mctp_usblib_test_init() inserts
&ctx->rt.list into the global RCU-protected list init_net.mctp.routes:

    list_add_rcu(&ctx->rt.list, &init_net.mctp.routes);

KUNIT_ASSERT_* macros invoke __kunit_abort() which calls
kunit_try_catch_throw() and ultimately kthread_exit(0), so control never
returns to the test function:

    lib/kunit/test.c: __kunit_abort()
      -> kunit_try_catch_throw()
        -> kthread_exit(0)

When the test kthread exits, its stack is gone, leaving a list_head in
init_net.mctp.routes that points at freed/reused stack memory.

The next RCU traversal of init_net.mctp.routes (subsequent mctp test,
mctp routing code, another kunit run) would then dereference that
dangling pointer.

The same abort path also bypasses unregister_netdev() and the
mctp_dev_put() done by mctp_usblib_test_fini().

Would using kunit_add_action() / kunit_add_action_or_reset() (or suite
.init/.exit hooks together with kunit_kmalloc()) ensure list_del_rcu(),
synchronize_rcu() and the netdev teardown always run, regardless of
assertion outcome?

> +
[ ... ]

> +static void mctp_usblib_test_rx_single(struct kunit *test)
> +{
> +	struct mctp_usblib_test_dev *dev;
> +	struct mctp_usblib_test_ctx ctx;
> +	struct mctp_usblib_rx rx;
> +	struct sk_buff *skb;
> +	size_t len;
> +	void *buf;
> +	int rc;
> +
> +	mctp_usblib_test_init(test, &ctx);
> +	dev = ctx.dev;
> +
> +	mctp_usblib_rx_init(&rx, true);
> +
> +	rc = mctp_usblib_rx_prepare(dev->ndev, &rx, &buf, &len, GFP_KERNEL);
> +	KUNIT_ASSERT_EQ(test, rc, 0);
> +
> +	mctp_usblib_test_init_pkt(buf, 8, 8);
> +
> +	rc = mctp_usblib_rx_complete(dev->ndev, &rx, 8);
> +	KUNIT_ASSERT_EQ(test, rc, 0);
> +
> +	skb = __skb_dequeue(&dev->rx_pkts);
> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, skb);
> +	KUNIT_EXPECT_EQ(test, skb->len, 4);
> +	kfree_skb(skb);
> +
> +	mctp_usblib_rx_fini(&rx);
> +	mctp_usblib_test_fini(test, &ctx);
> +}
> +
[ ... ]

> +static void mctp_usblib_test_rx_pkt_span(struct kunit *test)
> +{
> +	const struct mctp_usblib_test_pkt_span *pkt_span = test->param_value;
> +	size_t len, xfer_len, off, xfer_off;
> +	struct mctp_usblib_test_dev *dev;
> +	struct mctp_usblib_test_ctx ctx;
> +	struct mctp_usblib_rx rx;
> +	unsigned int i;
> +	u8 *pktbuf;
> +	void *buf;
> +	int rc;
> +
> +	mctp_usblib_test_pkt_span_validate(test, pkt_span, &len);
> +	pktbuf = kmalloc_array(1, len, GFP_KERNEL);
> +
> +	/* lay out packets */
> +	for (off = 0, i = 0; i < pkt_span->n_pkts; i++) {
> +		len = pkt_span->pkts[i];
> +		mctp_usblib_test_init_pkt(pktbuf + off, len, len);
> +		off += len;
> +	}

[Low]
Is the kmalloc_array() return value missing a NULL check?  pktbuf is
dereferenced immediately via mctp_usblib_test_init_pkt(pktbuf + off, ...),
so an allocation failure (including under kunit fault injection) becomes
a NULL deref.  Would kunit_kmalloc_array() or an explicit
KUNIT_ASSERT_NOT_NULL(test, pktbuf) be more appropriate here?

> +
[ ... ]

> +	/* check received packets */
> +	KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, pkt_span->n_pkts);
> +	for (i = 0; ; i++) {
> +		struct sk_buff *skb = __skb_dequeue(&dev->rx_pkts);
> +
> +		if (!skb)
> +			break;
> +
> +		if (i <= pkt_span->n_pkts)
> +			KUNIT_EXPECT_EQ(test, skb->len, pkt_span->pkts[i] - 4);
> +
> +		kfree_skb(skb);
> +	}

[Low]
Should this comparison be i < pkt_span->n_pkts rather than
i <= pkt_span->n_pkts?

With <=, when i == n_pkts the test reads pkts[n_pkts], which is a
zero-initialized tail slot of pkts[6]; the expected length then
underflows to (size_t)0 - 4 and produces a confusing diagnostic instead
of a clean failure if the queue ever holds more SKBs than expected.

> +
[ ... ]
-- 
This is an AI-generated review.