Re: [PATCH net v2 1/3] net/iucv: only send the window update on HiperSockets sockets

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

Pre-existing issues:
- [High] `iucv_sock_recvmsg` and `iucv_send_ctrl` perform unprotected read-modify-restore operations on `sk->sk_shutdown`, clobbering concurrent state changes from `shutdown()`.
- [High] Concurrent execution within `iucv_sock_recvmsg()` can cause a data race leading to a NULL pointer dereference of `iucv->hs_dev`.
- [High] Iterating over `iucv_sk_list.head` without acquiring the associated lock leads to race conditions and potential use-after-free or list corruption.
--

--- Patch [1]: [PATCH net v2 1/3] net/iucv: only send the window update on HiperSockets sockets ---
commit 50e666866fa1b6d2153e6efa369820e5171cf305
Author: Bryam Vargas <[email protected]>

net/iucv: only send the window update on HiperSockets sockets

This commit fixes an issue where an unconditional window update on classic
z/VM sockets can result in a spurious disconnect or a fault. It adds a check
to only send the window update if the transport is HiperSockets.

Cc: [email protected]

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7c..0bc4a15f4b565 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -1331,7 +1331,8 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>  		if (skb_queue_empty(&iucv->backlog_skb_q)) {
>  			if (!list_empty(&iucv->message_q.list))
>  				iucv_process_message_q(sk);
> -			if (atomic_read(&iucv->msg_recv) >=
> +			if (iucv->transport == AF_IUCV_TRANS_HIPER &&
> +			    atomic_read(&iucv->msg_recv) >=
>  							iucv->msglimit / 2) {
>  				err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);

[Severity: High]
This is a pre-existing issue, but could concurrent execution within
iucv_sock_recvmsg() cause a data race leading to a NULL pointer dereference
of iucv->hs_dev?

If multiple threads execute recvmsg() concurrently on the same socket, they
can dequeue packets and increment iucv->msg_recv without mutual exclusion
earlier in the function:

    if (atomic_read(&iucv->msg_recv) > iucv->msglimit) {
        WARN_ON(1);
        iucv_sock_close(sk);
        return -EFAULT;
    }

Calling iucv_sock_close() acquires the socket lock, drops the device
reference, and sets iucv->hs_dev = NULL.

Another concurrent thread executing recvmsg() might evaluate the send_win
condition as true and subsequently call iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN).

Since iucv_send_ctrl() accesses iucv->hs_dev via LL_RESERVED_SPACE() without
holding the socket lock:

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

wouldn't this dereference NULL if the first thread nullified the device
pointer?

[Severity: High]
This is a pre-existing issue, but is it possible for concurrent updates to
sk_shutdown to be lost?

Looking at iucv_sock_recvmsg(), we modify sk_shutdown directly without
holding the socket lock:

    if (!rlen)
        sk->sk_shutdown = sk->sk_shutdown | RCV_SHUTDOWN;

Additionally, in iucv_send_ctrl(), the stale state is saved, modified, and
blindly restored without locking:

    shutdown = sk->sk_shutdown;
    sk->sk_shutdown &= RCV_SHUTDOWN;
    ...
    if (shutdown)
        sk->sk_shutdown = shutdown;

If Thread A executes recvmsg() while Thread B concurrently executes shutdown(),
could Thread A overwrite and erase the flags (e.g., SEND_SHUTDOWN) set by
Thread B?

[Severity: High]
This is a pre-existing issue, but does iterating over iucv_sk_list.head
without acquiring the associated lock in afiucv_netdev_event() lead to
race conditions?

net/iucv/af_iucv.c:afiucv_netdev_event() {
    ...
    case NETDEV_GOING_DOWN:
        sk_for_each(sk, &iucv_sk_list.head) {
            iucv = iucv_sk(sk);
            if ((iucv->hs_dev == event_dev) &&
    ...
}

Concurrently, if a socket is created (iucv_sock_bind) or destroyed
(iucv_sock_kill), the list is mutated under write_lock_bh(&iucv_sk_list.lock).

Could this lockless traversal dereference invalid pointers or freed socket
structures during a NETDEV_GOING_DOWN or NETDEV_REBOOT event?

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

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