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

[sashiko-bot@kernel.org]

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