Re: [PATCH net-next 3/3] llc: add KUnit tests for the connection state machine bounds

[Simon Horman <horms@kernel.org>]

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://sashiko.dev/#/patchset/20260901210308.1173180-3-kees@kernel.org?part=3
---
llc: add KUnit tests for the connection state machine bounds

This commit adds a KUnit test suite for the LLC type 2 connection state
machine. It introduces regression tests for state transitions and array
indexing invariants.

> diff --git a/net/llc/tests/conn_kunit.c b/net/llc/tests/conn_kunit.c
> new file mode 100644
> index 0000000000000..86d3f122df0a6
> --- /dev/null
> +++ b/net/llc/tests/conn_kunit.c
> @@ -0,0 +1,146 @@

[ ... ]

> +static struct sk_buff *llc_conn_test_rx_pdu(struct kunit *test, struct sock *sk)
> +{
> +	struct llc_conn_state_ev *ev;
> +	struct llc_pdu_sn *pdu;
> +	struct sk_buff *skb;
> +
> +	skb = alloc_skb(sizeof(*pdu), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, skb);

[Severity: Medium]
Will this leak unmanaged resources if the assertion fails?

If alloc_skb() fails, KUNIT_ASSERT_NOT_NULL() terminates the test case
abruptly. Any previously allocated resources that are not registered with
KUnit's cleanup mechanism (like kunit_add_action()) will bypass their
manual cleanup routines.

[ ... ]

> +static void llc_conn_state_process_out_of_svc(struct kunit *test)
> +{
> +	struct sk_buff *first, *second;
> +	struct socket *sock;
> +	struct sock *sk;
> +
> +	sock = llc_conn_test_socket(test);
> +	sk = sock->sk;
> +
> +	first = llc_conn_test_rx_pdu(test, sk);
> +	second = llc_conn_test_rx_pdu(test, sk);

[Severity: Medium]
For instance, if the second call to llc_conn_test_rx_pdu() triggers an
assertion failure, does this code leak the socket and the first sk_buff?

The sock is allocated in llc_conn_test_socket(), and the first sk_buff is
allocated just above. Because KUNIT_ASSERT_NOT_NULL() causes an immediate
abort on failure, the execution would skip the sock_release(sock) call at the
end of this function, permanently leaking both the socket and the sk_buff.

> +
> +	lock_sock(sk);
> +	KUNIT_EXPECT_EQ(test, llc_sk(sk)->state, LLC_CONN_STATE_ADM);