Re: [PATCH net v2 3/3] net/iucv: send the window update outside message_q.lock

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

New issues:
- [High] Redundant concurrent window updates cause spurious socket disconnections due to transient errors.
- [High] Lockless modification of `sk->sk_state` to `IUCV_DISCONN` in error path corrupts the socket state machine.

Pre-existing issues:
- [Critical] Concurrent execution of `iucv_sock_recvmsg` and `iucv_sock_close` can lead to a NULL pointer dereference of `iucv->hs_dev`.
- [High] Lockless read-modify-write sequence on `sk->sk_shutdown` in `iucv_send_ctrl` permanently loses concurrent state updates.
--

--- Patch [3]: [PATCH net v2 3/3] net/iucv: send the window update outside message_q.lock ---
commit 38dc50c2f4dfcebf699da12476bf1a5fb529484e
Author: Bryam Vargas <[email protected]>

net/iucv: send the window update outside message_q.lock

This patch moves the window update control message dispatch outside the
message_q.lock in iucv_sock_recvmsg(). It avoids sleeping in atomic context
since iucv_send_ctrl() performs a GFP_KERNEL allocation. A boolean flag is
used to defer the send until after the lock is released.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
[ ... ]
> @@ -1336,15 +1337,18 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>  				iucv_process_message_q(sk);
>  			if (iucv->transport == AF_IUCV_TRANS_HIPER &&
>  			    atomic_read(&iucv->msg_recv) >=
> -							iucv->msglimit / 2) {
> -				err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
> -				if (err) {
> -					sk->sk_state = IUCV_DISCONN;
> -					sk->sk_state_change(sk);
> -				}
> -			}
> +							iucv->msglimit / 2)
> +				send_win = true;
>  		}
>  		spin_unlock_bh(&iucv->message_q.lock);
> +
> +		if (send_win) {
> +			err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);

[Severity: High]
Because atomic_read(&iucv->msg_recv) is checked inside the lock, but the
counter is reset later inside afiucv_hs_send() without holding
message_q.lock, can multiple concurrent calls to iucv_sock_recvmsg() set
send_win to true and trigger redundant window updates?

If redundant updates are sent, could transient send buffer exhaustion cause
the socket to unintentionally hit the error path below and disconnect?

[Severity: Critical]
This isn't a bug introduced by this patch, but calling iucv_send_ctrl()
concurrently here exposes a pre-existing race condition.

In iucv_send_ctrl(), LL_RESERVED_SPACE() dereferences iucv->hs_dev:

iucv_send_ctrl()
    blen = sizeof(struct af_iucv_trans_hdr) +
           LL_RESERVED_SPACE(iucv->hs_dev);

If a concurrent thread executes iucv_sock_close() and sets iucv->hs_dev
to NULL, does this cause a NULL pointer dereference?

[Severity: High]
This is a pre-existing issue, but there appears to be a concurrency
regression in iucv_send_ctrl() when it caches sk->sk_shutdown.

iucv_send_ctrl()
    if (sk->sk_shutdown & SEND_SHUTDOWN) {
        shutdown = sk->sk_shutdown;
        sk->sk_shutdown &= RCV_SHUTDOWN;
    }
    skb = sock_alloc_send_skb(sk, blen, 1, &err);
    ...
    if (shutdown)
        sk->sk_shutdown = shutdown;

Since sock_alloc_send_skb() can sleep, if another thread updates
sk->sk_shutdown (for example, via shutdown(fd, SHUT_RD)) during the
allocation, does the unconditional restore of the cached shutdown state
overwrite and permanently lose the concurrent update?

> +			if (err) {
> +				sk->sk_state = IUCV_DISCONN;
> +				sk->sk_state_change(sk);
> +			}
> +		}

[Severity: High]
Does modifying sk->sk_state and calling sk->sk_state_change(sk) locklessly
outside of message_q.lock or lock_sock() corrupt the socket state machine?

If multiple threads hit this error path simultaneously, could it lead to
unordered execution of state change callbacks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3