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

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

[Alexandra Winter]

iucv_sock_close() acquires lock_sock(sk) and then, when the socket is in
IUCV_LISTEN state, calls iucv_sock_cleanup_listen() which acquires
lock_sock(child_sk) for each pending child socket. Moreover
iucv_sock_close(child_sk) is called on the pending child sockets.

Avoid lockdep warning by using lock_sock_nested() in iucv_sock_close().
Split off __iucv_sock_close(), so iucv_sock_cleanup_listen() can call it
without nesting.

This is a missing lockdep annotation - not a real deadlock.

Example warning:
[ 6690.906930] ============================================
[ 6690.906935] WARNING: possible recursive locking detected
[ 6690.906942] 7.3.0-rc1net_KCSAN_KASAN-00240-g641d03105cc0 #23 Not tainted
[ 6690.906948] --------------------------------------------
[ 6690.906953] afiucv_test_loc/5183 is trying to acquire lock:
[ 6690.906959] 000601ad13f20a78 (sk_lock-IUCV){+.+.}-{0:0}, at: iucv_accept_dequeue+0xdc/0x340 [af_iucv]
[ 6690.906984]
               but task is already holding lock:
[ 6690.906989] 000601ad1c115278 (sk_lock-IUCV){+.+.}-{0:0}, at: iucv_sock_close+0x82/0x7d0 [af_iucv]
[ 6690.907006]
               other info that might help us debug this:
[ 6690.907010]  Possible unsafe locking scenario:

[ 6690.907014]        CPU0
[ 6690.907018]        ----
[ 6690.907021]   lock(sk_lock-IUCV);
[ 6690.907028]   lock(sk_lock-IUCV);
[ 6690.907034]
                *** DEADLOCK ***

[ 6690.907038]  May be due to missing lock nesting notation

[ 6690.907043] locks held by afiucv_test_loc/5183: 2, last CPU#0:
[ 6690.907085]  #0: 000601ad137a5550 (&sb->s_type->i_mutex_key#12){+.+.}-{3:3}, at: __sock_release+0x7e/0x230
[ 6690.907109]  #1: 000601ad1c115278 (sk_lock-IUCV){+.+.}-{0:0}, at: iucv_sock_close+0x82/0x7d0 [af_iucv]
[ 6690.907127]
               stack backtrace:
[ 6690.907135] CPU: 0 UID: 0 PID: 5183 Comm: afiucv_test_loc Kdump: loaded Not tainted 7.3.0-rc1net_KCSAN_KASAN-00240-g641d03105cc0 #23 PREEMPT
[ 6690.907140] Hardware name: IBM 8561 T01 703 (z/VM 7.4.0)
[ 6690.907142] Call Trace:
[ 6690.907144]  [<00061ab555fd6208>] dump_stack_lvl+0xe8/0x140
[ 6690.907151]  [<00061ab55623a110>] print_deadlock_bug+0x340/0x350
[ 6690.907156]  [<00061ab55623f768>] __lock_acquire+0x11b8/0x15d0
[ 6690.907159]  [<00061ab55623fc9c>] lock_acquire.part.0+0x11c/0x290
[ 6690.907162]  [<00061ab55623fec4>] lock_acquire+0xb4/0x1e0
[ 6690.907164]  [<00061ab5580d6b96>] lock_sock_nested+0x46/0xf0
[ 6690.907167]  [<00061ab4d66e38dc>] iucv_accept_dequeue+0xdc/0x340 [af_iucv]
[ 6690.907171]  [<00061ab4d66e541c>] iucv_sock_close+0xdc/0x7d0 [af_iucv]
[ 6690.907174]  [<00061ab4d66e5b62>] iucv_sock_release+0x52/0x130 [af_iucv]
[ 6690.907177]  [<00061ab5580c5b24>] __sock_release+0xa4/0x230
[ 6690.907180]  [<00061ab5580c5cdc>] sock_close+0x2c/0x40
[ 6690.907183]  [<00061ab556ac7a00>] __fput+0x2f0/0x880
[ 6690.907187]  [<00061ab556ac8cc0>] fput_close_sync+0xd0/0x1c0
[ 6690.907189]  [<00061ab556abbc70>] __s390x_sys_close+0x90/0xf0
[ 6690.907192]  [<00061ab55894df0e>] __do_syscall+0x1be/0x5a0
[ 6690.907196]  [<00061ab558976a7a>] system_call+0x72/0x90
[ 6690.907200] INFO: lockdep is turned off.

Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
---
 net/iucv/af_iucv.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index db261ecd19af..dc906f5f7c42 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -91,6 +91,7 @@ static struct sock *iucv_accept_dequeue(struct sock *parent,
 					struct socket *newsock);
 static void iucv_sock_kill(struct sock *sk);
 static void iucv_sock_close(struct sock *sk);
+static void __iucv_sock_close(struct sock *sk);
 
 static void afiucv_hs_callback_txnotify(struct sock *sk, enum iucv_tx_notify);
 
@@ -302,7 +303,9 @@ static void iucv_sock_cleanup_listen(struct sock *parent)
 
 	/* Close non-accepted connections */
 	while ((sk = iucv_accept_dequeue(parent, NULL))) {
-		iucv_sock_close(sk);
+		lock_sock(sk);
+		__iucv_sock_close(sk);
+		release_sock(sk);
 		iucv_sock_kill(sk);
 	}
 
@@ -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) {
 	case IUCV_LISTEN:
 		iucv_sock_cleanup_listen(sk);
@@ -454,7 +458,12 @@ static void iucv_sock_close(struct sock *sk)
 
 	/* mark socket for deletion by iucv_sock_kill() */
 	sock_set_flag(sk, SOCK_ZAPPED);
+}
 
+static void iucv_sock_close(struct sock *sk)
+{
+	lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
+	__iucv_sock_close(sk);
 	release_sock(sk);
 }
 
-- 
2.53.0

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

[sashiko-bot]

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

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

[Simon Horman]

On Fri, Sep 11, 2026 at 04:29:45PM +0000, sashiko-bot@kernel.org wrote:
> 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.

My opinion is that the issues above are all orthogonal to this patch
and should not block progress of it.

...

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

[Simon Horman]

On Thu, Sep 10, 2026 at 06:29:19PM +0200, Alexandra Winter wrote:
> iucv_sock_close() acquires lock_sock(sk) and then, when the socket is in
> IUCV_LISTEN state, calls iucv_sock_cleanup_listen() which acquires
> lock_sock(child_sk) for each pending child socket. Moreover
> iucv_sock_close(child_sk) is called on the pending child sockets.
> 
> Avoid lockdep warning by using lock_sock_nested() in iucv_sock_close().
> Split off __iucv_sock_close(), so iucv_sock_cleanup_listen() can call it
> without nesting.
> 
> This is a missing lockdep annotation - not a real deadlock.
> 
> Example warning:
> [ 6690.906930] ============================================
> [ 6690.906935] WARNING: possible recursive locking detected
> [ 6690.906942] 7.3.0-rc1net_KCSAN_KASAN-00240-g641d03105cc0 #23 Not tainted
> [ 6690.906948] --------------------------------------------
> [ 6690.906953] afiucv_test_loc/5183 is trying to acquire lock:
> [ 6690.906959] 000601ad13f20a78 (sk_lock-IUCV){+.+.}-{0:0}, at: iucv_accept_dequeue+0xdc/0x340 [af_iucv]
> [ 6690.906984]
>                but task is already holding lock:
> [ 6690.906989] 000601ad1c115278 (sk_lock-IUCV){+.+.}-{0:0}, at: iucv_sock_close+0x82/0x7d0 [af_iucv]
> [ 6690.907006]
>                other info that might help us debug this:
> [ 6690.907010]  Possible unsafe locking scenario:
> 
> [ 6690.907014]        CPU0
> [ 6690.907018]        ----
> [ 6690.907021]   lock(sk_lock-IUCV);
> [ 6690.907028]   lock(sk_lock-IUCV);
> [ 6690.907034]
>                 *** DEADLOCK ***
> 
> [ 6690.907038]  May be due to missing lock nesting notation
> 
> [ 6690.907043] locks held by afiucv_test_loc/5183: 2, last CPU#0:
> [ 6690.907085]  #0: 000601ad137a5550 (&sb->s_type->i_mutex_key#12){+.+.}-{3:3}, at: __sock_release+0x7e/0x230
> [ 6690.907109]  #1: 000601ad1c115278 (sk_lock-IUCV){+.+.}-{0:0}, at: iucv_sock_close+0x82/0x7d0 [af_iucv]
> [ 6690.907127]
>                stack backtrace:
> [ 6690.907135] CPU: 0 UID: 0 PID: 5183 Comm: afiucv_test_loc Kdump: loaded Not tainted 7.3.0-rc1net_KCSAN_KASAN-00240-g641d03105cc0 #23 PREEMPT
> [ 6690.907140] Hardware name: IBM 8561 T01 703 (z/VM 7.4.0)
> [ 6690.907142] Call Trace:
> [ 6690.907144]  [<00061ab555fd6208>] dump_stack_lvl+0xe8/0x140
> [ 6690.907151]  [<00061ab55623a110>] print_deadlock_bug+0x340/0x350
> [ 6690.907156]  [<00061ab55623f768>] __lock_acquire+0x11b8/0x15d0
> [ 6690.907159]  [<00061ab55623fc9c>] lock_acquire.part.0+0x11c/0x290
> [ 6690.907162]  [<00061ab55623fec4>] lock_acquire+0xb4/0x1e0
> [ 6690.907164]  [<00061ab5580d6b96>] lock_sock_nested+0x46/0xf0
> [ 6690.907167]  [<00061ab4d66e38dc>] iucv_accept_dequeue+0xdc/0x340 [af_iucv]
> [ 6690.907171]  [<00061ab4d66e541c>] iucv_sock_close+0xdc/0x7d0 [af_iucv]
> [ 6690.907174]  [<00061ab4d66e5b62>] iucv_sock_release+0x52/0x130 [af_iucv]
> [ 6690.907177]  [<00061ab5580c5b24>] __sock_release+0xa4/0x230
> [ 6690.907180]  [<00061ab5580c5cdc>] sock_close+0x2c/0x40
> [ 6690.907183]  [<00061ab556ac7a00>] __fput+0x2f0/0x880
> [ 6690.907187]  [<00061ab556ac8cc0>] fput_close_sync+0xd0/0x1c0
> [ 6690.907189]  [<00061ab556abbc70>] __s390x_sys_close+0x90/0xf0
> [ 6690.907192]  [<00061ab55894df0e>] __do_syscall+0x1be/0x5a0
> [ 6690.907196]  [<00061ab558976a7a>] system_call+0x72/0x90
> [ 6690.907200] INFO: lockdep is turned off.
> 
> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>

Reviewed-by: Simon Horman <horms@kernel.org>

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

[Alexandra Winter]

On 13.09.26 15:42, Simon Horman wrote:
> On Fri, Sep 11, 2026 at 04:29:45PM +0000, sashiko-bot@kernel.org wrote:
>> 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.
> 
> My opinion is that the issues above are all orthogonal to this patch
> and should not block progress of it.
> 
> ...

I agree. I think all of them were reported already by Bryam Vargas [1]
I need to get this one out of the way, so I can reproduce the real issues.


Linkk: https://lore.kernel.org/netdev/20260724222917.134769-1-hexlabsecurity@proton.me/ [1]

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

[Jakub Kicinski]

On Thu, 10 Sep 2026 18:29:19 +0200 Alexandra Winter wrote:
>  static void iucv_sock_close(struct sock *sk);

deleted this line when applying per clashiko

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 10 Sep 2026 18:29:19 +0200 you wrote:
> iucv_sock_close() acquires lock_sock(sk) and then, when the socket is in
> IUCV_LISTEN state, calls iucv_sock_cleanup_listen() which acquires
> lock_sock(child_sk) for each pending child socket. Moreover
> iucv_sock_close(child_sk) is called on the pending child sockets.
> 
> Avoid lockdep warning by using lock_sock_nested() in iucv_sock_close().
> Split off __iucv_sock_close(), so iucv_sock_cleanup_listen() can call it
> without nesting.
> 
> [...]

Here is the summary with links:
  - [net-next] net/iucv: Add lock nesting annotation in iucv_sock_close()
    https://git.kernel.org/netdev/net-next/c/aa5ec571b93f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Alexandra Winter]

On 15.09.26 03:53, Jakub Kicinski wrote:
> On Thu, 10 Sep 2026 18:29:19 +0200 Alexandra Winter wrote:
>>  static void iucv_sock_close(struct sock *sk);
> 
> deleted this line when applying per clashiko

Ack, thank you Jakub.