[PATCH] iucv: fix lockless sk_for_each in afiucv_netdev_event

[PATCH] iucv: fix lockless sk_for_each in afiucv_netdev_event

[Haoran Wang]

afiucv_netdev_event() traverses iucv_sk_list.head with sk_for_each()
without holding iucv_sk_list.lock:

    /* net/iucv/af_iucv.c:2218 */
    sk_for_each(sk, &iucv_sk_list.head) {
        iucv = iucv_sk(sk);
        if ((iucv->hs_dev == event_dev) &&
            (sk->sk_state == IUCV_CONNECTED)) {
            if (event == NETDEV_GOING_DOWN)
                iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
            sk->sk_state = IUCV_DISCONN;
            sk->sk_state_change(sk);
        }
    }

The list is mutated under write_lock_bh() by iucv_sock_link() and
iucv_sock_unlink() (called from the socket create/close paths), so a
concurrent close can unlink an sk while the notifier is traversing the
hlist.  The sk_for_each() next pointer may dereference an sk that is
being freed, and the per-socket work then runs on an sk whose lifetime
is not held. 

The other traversals of the same list already take the lock.
iucv_callback_connreq() and afiucv_hs_rcv() both wrap sk_for_each() in
read_lock(&iucv_sk_list.lock) ... read_unlock(). 

Take iucv_sk_list.lock around the traversal, matching the other two
sites.  The per-socket work (iucv_send_ctrl() reaching
sock_alloc_send_skb(), and the state change) may sleep on allocation
and cannot run under read_lock_bh(), so the fix collects the matching
sockets in two passes under the lock.  Count them first, then on the
second pass take a reference with sock_hold() on each and store it
into a kmalloc'd array, drop the lock, and run iucv_send_ctrl(),
sk_state_change() and sock_put() outside it.  sock_hold() pins each
sk for the out-of-lock work, so a concurrent close can no longer free
it from under the notifier.


Signed-off-by: Haoran Wang <haoranwangsec@gmail.com>
---
 net/iucv/af_iucv.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 4e5cc9da6..d2977b018 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -2214,18 +2214,49 @@ static int afiucv_netdev_event(struct notifier_block *this,
 
 	switch (event) {
 	case NETDEV_REBOOT:
-	case NETDEV_GOING_DOWN:
+	case NETDEV_GOING_DOWN: {
+		struct sock **matches;
+		int i, n = 0, count = 0;
+
+		read_lock(&iucv_sk_list.lock);
+		sk_for_each(sk, &iucv_sk_list.head) {
+			iucv = iucv_sk(sk);
+			if (iucv->hs_dev == event_dev &&
+			    sk->sk_state == IUCV_CONNECTED)
+				count++;
+		}
+		read_unlock(&iucv_sk_list.lock);
+
+		if (!count)
+			break;
+		matches = kmalloc_array(count, sizeof(*matches), GFP_KERNEL);
+		if (!matches)
+			break;
+
+		read_lock(&iucv_sk_list.lock);
 		sk_for_each(sk, &iucv_sk_list.head) {
+			if (n == count)
+				break;
 			iucv = iucv_sk(sk);
-			if ((iucv->hs_dev == event_dev) &&
-			    (sk->sk_state == IUCV_CONNECTED)) {
-				if (event == NETDEV_GOING_DOWN)
-					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
-				sk->sk_state = IUCV_DISCONN;
-				sk->sk_state_change(sk);
+			if (iucv->hs_dev == event_dev &&
+			    sk->sk_state == IUCV_CONNECTED) {
+				sock_hold(sk);
+				matches[n++] = sk;
 			}
 		}
+		read_unlock(&iucv_sk_list.lock);
+
+		for (i = 0; i < n; i++) {
+			sk = matches[i];
+			if (event == NETDEV_GOING_DOWN)
+				iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
+			sk->sk_state = IUCV_DISCONN;
+			sk->sk_state_change(sk);
+			sock_put(sk);
+		}
+		kfree(matches);
 		break;
+	}
 	case NETDEV_DOWN:
 	case NETDEV_UNREGISTER:
 	default:

Re: [PATCH] iucv: fix lockless sk_for_each in afiucv_netdev_event

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] iucv: fix lockless sk_for_each in afiucv_netdev_event

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] iucv: softirq deadlock in afiucv_netdev_event() due to read_lock()
- [High] iucv: race with iucv_sock_close() in afiucv_netdev_event() out-of-lock loop
--

commit 7da7cd5e3cce2986228a27e227a58eb1b118ea15
Author: Haoran Wang <haoranwangsec@gmail.com>

iucv: fix lockless sk_for_each in afiucv_netdev_event

This commit acquires iucv_sk_list.lock around the sk_for_each() traversal in
afiucv_netdev_event(), collecting matching sockets with sock_hold() into a
dynamically allocated array before running iucv_send_ctrl() and
sk_state_change() outside the lock.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index db261ecd19af1..20c8943b85cd2 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2222,18 +2222,49 @@ static int afiucv_netdev_event(struct notifier_block *this,
>  
>  	switch (event) {
>  	case NETDEV_REBOOT:
> -	case NETDEV_GOING_DOWN:
> +	case NETDEV_GOING_DOWN: {
> +		struct sock **matches;
> +		int i, n = 0, count = 0;
> +
> +		read_lock(&iucv_sk_list.lock);
>  		sk_for_each(sk, &iucv_sk_list.head) {
>  			iucv = iucv_sk(sk);
> -			if ((iucv->hs_dev == event_dev) &&
> -			    (sk->sk_state == IUCV_CONNECTED)) {
> -				if (event == NETDEV_GOING_DOWN)
> -					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
> -				sk->sk_state = IUCV_DISCONN;
> -				sk->sk_state_change(sk);
> +			if (iucv->hs_dev == event_dev &&
> +			    sk->sk_state == IUCV_CONNECTED)
> +				count++;
> +		}
> +		read_unlock(&iucv_sk_list.lock);
> +
> +		if (!count)
> +			break;
> +		matches = kmalloc_array(count, sizeof(*matches), GFP_KERNEL);
> +		if (!matches)
> +			break;
> +
> +		read_lock(&iucv_sk_list.lock);
> +		sk_for_each(sk, &iucv_sk_list.head) {
> +			if (n == count)
> +				break;
> +			iucv = iucv_sk(sk);
> +			if (iucv->hs_dev == event_dev &&
> +			    sk->sk_state == IUCV_CONNECTED) {
> +				sock_hold(sk);
> +				matches[n++] = sk;
>  			}
>  		}
> +		read_unlock(&iucv_sk_list.lock);

[Severity: High]
Should both passes in afiucv_netdev_event() use read_lock_bh() and
read_unlock_bh() instead of bare read_lock() and read_unlock()?

afiucv_netdev_event() runs in process context with bottom halves enabled,
unlike the other two read_lock(&iucv_sk_list.lock) sites in afiucv_hs_rcv()
and iucv_callback_connreq(), which already run with bottom halves disabled.

Writers to iucv_sk_list can be invoked from softirq context:

afiucv_hs_rcv()
  afiucv_hs_callback_syn()
    iucv_sock_alloc()
      iucv_sock_link()
        write_lock_bh(&l->lock)

and on error in afiucv_hs_callback_syn():

    iucv_sock_kill()
      iucv_sock_unlink()
        write_lock_bh(&l->lock)

If NET_RX_SOFTIRQ interrupts afiucv_netdev_event() on the same CPU while it
holds read_lock(&iucv_sk_list.lock), can write_lock_bh(&iucv_sk_list.lock)
spin forever waiting for the preempted reader?

> +
> +		for (i = 0; i < n; i++) {
> +			sk = matches[i];
> +			if (event == NETDEV_GOING_DOWN)
> +				iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
> +			sk->sk_state = IUCV_DISCONN;
> +			sk->sk_state_change(sk);
> +			sock_put(sk);
> +		}

[Severity: High]
Can this loop race with a concurrent iucv_sock_close() and dereference a NULL
iucv->hs_dev pointer in iucv_send_ctrl()?

sock_hold(sk) pins the struct sock memory, but once iucv_sk_list.lock is
dropped (or before iucv_sock_kill() unlinks the socket), a concurrent
iucv_sock_release() -> iucv_sock_close() can run under lock_sock(sk):

net/iucv/af_iucv.c:iucv_sock_close() {
	...
	if (iucv->hs_dev) {
		dev_put(iucv->hs_dev);
		iucv->hs_dev = NULL;
		sk->sk_bound_dev_if = 0;
	}
	sk->sk_state = IUCV_CLOSED;
	...
}

When afiucv_netdev_event() then processes matches[i] without holding
lock_sock(sk) or rechecking sk->sk_state and iucv->hs_dev, iucv_send_ctrl()
evaluates LL_RESERVED_SPACE(iucv->hs_dev):

net/iucv/af_iucv.c:iucv_send_ctrl() {
	...
	blen = sizeof(struct af_iucv_trans_hdr) +
	       LL_RESERVED_SPACE(iucv->hs_dev);
	...
}

In addition, wouldn't unconditionally setting sk->sk_state = IUCV_DISCONN
outside lock_sock(sk) overwrite IUCV_CLOSING or IUCV_CLOSED set by a
concurrent close, and race on sk->sk_shutdown in iucv_send_ctrl()?

> +		kfree(matches);
>  		break;
> +	}
>  	case NETDEV_DOWN:
>  	case NETDEV_UNREGISTER:
>  	default:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179241612354.5916.12881323209252769976@whr?part=1