Re: [PATCH net-next] net/iucv: Add lock nesting annotation in iucv_sock_close()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Lockless traversal of accept_q causes list corruption and use-after-free due to concurrent modifications from softirq context.
- [High] Newly allocated sockets are permanently leaked on error paths because iucv_sock_kill() returns early if SOCK_ZAPPED is not set.
- [High] Child socket leak in iucv_sock_cleanup_listen due to late state update leaving a race window.
- [High] Unprotected state modifications in iucv_callback_connack cause state desynchronization.
--

commit dd0ad2a384eeaa590848b51235abdb105f4560db
Author: Alexandra Winter <wintera@linux.ibm.com>

net/iucv: Add lock nesting annotation in iucv_sock_close()

This commit splits off __iucv_sock_close() to allow it to be called
without lock nesting. It also adds a lockdep annotation to
iucv_sock_close() to avoid a false-positive recursive locking warning.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index db261ecd19af1..dc906f5f7c42d 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c

[ ... ]

> @@ -302,7 +303,9 @@ static void iucv_sock_cleanup_listen(struct sock *parent)
>  
>  	/* Close non-accepted connections */
>  	while ((sk = iucv_accept_dequeue(parent, NULL))) {

[Severity: Critical]
This is a pre-existing issue, but I have a question about how
iucv_accept_dequeue() is interacting with the softirq context here.

Does lockless traversal of accept_q cause list corruption or use-after-free
due to concurrent modifications from softirq context?

iucv_accept_dequeue() (and iucv_accept_poll()) traverses the accept_q list
using list_for_each_entry_safe() without holding accept_q_lock:

net/iucv/af_iucv.c:iucv_accept_dequeue() {
    ...
	list_for_each_entry_safe(isk, n, &iucv_sk(parent)->accept_q, accept_q) {
    ...
}

Concurrently, an incoming connection triggers iucv_callback_connreq() in
softirq, which acquires accept_q_lock via iucv_accept_enqueue() and modifies
the list using list_add_tail(). Since list_for_each_entry_safe() is not safe
against concurrent modifications from another context, does this result in a
data race and list corruption when poll() or accept() are called?

> -		iucv_sock_close(sk);
> +		lock_sock(sk);
> +		__iucv_sock_close(sk);
> +		release_sock(sk);
>  		iucv_sock_kill(sk);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but looking at the loop above,
could this leave a race window where child sockets are permanently leaked?

iucv_sock_cleanup_listen() drains the accept_q completely, but since it is
executed under lock_sock(parent) (which releases the slock spinlock), could
a softirq executing iucv_callback_connreq() concurrently enqueue a new
socket?

Since parent->sk_state = IUCV_CLOSED is updated after the loop finishes:

net/iucv/af_iucv.c:iucv_sock_cleanup_listen() {
    ...
	while ((sk = iucv_accept_dequeue(parent, NULL))) {
		lock_sock(sk);
		__iucv_sock_close(sk);
		release_sock(sk);
		iucv_sock_kill(sk);
	}

	parent->sk_state = IUCV_CLOSED;
}

Can a softirq successfully enqueue a new child socket to accept_q just as the
loop terminates, leaving it leaked forever since iucv_sock_destruct() does
not purge accept_q?


[Severity: High]
This is another pre-existing issue, but does iucv_sock_kill() silently leak
newly allocated sockets on error paths?

In error paths, newly allocated sockets are passed to iucv_sock_kill():

net/iucv/af_iucv.c:iucv_callback_connreq() {
    ...
	if (err) {
		iucv_sever_path(nsk, 1);
		iucv_sock_kill(nsk);
		goto fail;
	}
    ...
}

And also in afiucv_hs_callback_syn():

net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
    ...
	err = dev_queue_xmit(skb);
	if (!err) {
		...
	} else
		iucv_sock_kill(nsk);
    ...
}

Since iucv_sock_alloc() creates sockets with the SOCK_ZAPPED flag cleared,
and iucv_sock_kill() returns early if !sock_flag(sk, SOCK_ZAPPED), doesn't
this mean the socket is left forever in iucv_sk_list without its reference
being dropped? Could this allow a remote unauthenticated user to cause a
Denial of Service via memory exhaustion by spamming SYN requests?

>  
> @@ -394,15 +397,16 @@ static int iucv_send_ctrl(struct sock *sk, u8 flags)
>  	return err;
>  }
>  
> -/* Close an IUCV socket */
> -static void iucv_sock_close(struct sock *sk)
> +/**
> + * __iucv_sock_close() - close socket, caller holds lock_sock(sk)
> + * @sk: network socket
> + */
> +static void __iucv_sock_close(struct sock *sk)
>  {
>  	struct iucv_sock *iucv = iucv_sk(sk);
>  	unsigned long timeo;
>  	int err = 0;
>  
> -	lock_sock(sk);
> -
>  	switch (sk->sk_state) {

[Severity: High]
This isn't a bug introduced by this patch, but could unprotected state
modifications in iucv_callback_connack() cause state desynchronization
and resource leaks?

iucv_callback_connack() executes in interrupt context when the peer accepts
a connection, and it directly modifies the socket state without acquiring
bh_lock_sock(sk):

net/iucv/af_iucv.c:iucv_callback_connack() {
    ...
	sk->sk_state = IUCV_CONNECTED;
	sk->sk_state_change(sk);
    ...
}

Since this write is unprotected, can it race with other operations (like
__iucv_sock_close() checking sk->sk_state here) that are concurrently
transitioning the socket to IUCV_CLOSING or IUCV_CLOSED under lock_sock(sk)?
Would this blindly overwrite the state with IUCV_CONNECTED during a
concurrent teardown sequence, leaving the socket in an inconsistent state
forever?

>  	case IUCV_LISTEN:
>  		iucv_sock_cleanup_listen(sk);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910162919.2100124-1-wintera@linux.ibm.com?part=1