[PATCH net v2 0/3] net/iucv: fix the recvmsg window update

[PATCH net v2 0/3] net/iucv: fix the recvmsg window update

[Bryam Vargas via B4 Relay]

v2 answers Jakub's reviews of v1 with changes rather than replies.

1/3 keeps its hunk and states the impact properly. SO_MSGLIMIT accepts 1,
which makes msglimit / 2 zero, and msg_recv never leaves zero on a classic
socket, so the NULL read is one recvmsg() away for an unprivileged process
on a socket of its own. With relocate_lowcore it faults.

Its Fixes: tag was wrong as well. The unconditional send does date to
3881ac441f64, but iucv_send_ctrl() sized the skb with a constant ETH_HLEN
until 238965b71b96, so before that it just returned -ENODEV. Backport
window is v5.3, not v3.1.

2/3 is new. v1 admitted it widened the msg_recv race and left it open,
which isn't good enough under Cc: stable. afiucv_hs_send() samples the
counter and settles it after dev_queue_xmit(); sendmsg reaches that under
lock_sock() and recvmsg reaches it under no socket lock, so the two don't
exclude each other and both can subtract the same value. Negative counter,
WARN_ON, and the same credit advertised twice to a peer whose
afiucv_hs_callback_win() subtracts the wire value from msg_sent unchecked.

Taking the socket lock around the deferred send would close it too, since
recvmsg is the only unlocked side. I didn't: recvmsg has never held that
lock, and making it do so changes the receive path for every caller. That
belongs in the locking rework, not in a stable fix.

The claim sits after the last error exit rather than in the header build,
so the counter reads zero only while the transmit is in flight, and a
concurrent sender isn't talked out of its own update.

3/3 is v1's patch, and it must not be applied without 2/3: hoisting the
send out of message_q.lock drops the serialisation that lock gave two
concurrent recvmsg(). Backported alone it recreates what 2/3 fixes. Both
changelogs say so.

Litmus test under LKMM: the counter reaches -2 before 2/3 and cannot after.
No hardware run; I have no IBM Z, and CONFIG_AFIUCV is s390-only.

The unlocked hs_dev and sk_shutdown window, the unbounded backlog_skb_q and
the WARN_ON a flooding peer can reach are pre-existing and wider than this
series. They belong with the locking rework Alexandra has open.

Thanks for the reviews.

---
Bryam Vargas (3):
      net/iucv: only send the window update on HiperSockets sockets
      net/iucv: claim the receive credit atomically
      net/iucv: send the window update outside message_q.lock

 net/iucv/af_iucv.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)
---
base-commit: 746fc0787f616da418ffc04a110296fe95d53491
change-id: 20260821-b4-disp-3a6e8695-c1f4a6069169

Best regards,
--  
Bryam Vargas <hexlabsecurity@proton.me>

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

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

iucv_sock_recvmsg() sends AF_IUCV_FLAG_WIN without testing the transport,
but that flag exists only on HiperSockets. On a classic z/VM socket
iucv->hs_dev is NULL and iucv_send_ctrl() sizes the skb from
LL_RESERVED_SPACE(iucv->hs_dev), so the read goes through NULL. It is one
recvmsg() away for an unprivileged process on a socket of its own:
SO_MSGLIMIT accepts 1, so msglimit / 2 is 0 and msg_recv never leaves 0 on
a classic socket. The read lands in mapped lowcore on a default kernel and
the socket takes a spurious disconnect; with relocate_lowcore it faults.

Test the transport. The unconditional send is older than that, but stayed
harmless while iucv_send_ctrl() used a constant ETH_HLEN.

Fixes: 238965b71b96 ("net/af_iucv: build proper skbs for HiperTransport")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/iucv/af_iucv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index ea047bab65e7..0bc4a15f4b56 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);
 				if (err) {

-- 
2.55.0

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

[sashiko-bot]

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 <hexlabsecurity@proton.me>

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: stable@vger.kernel.org

> 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/20260821-b4-disp-3a6e8695-v2-0-37597ff723a8@proton.me?part=1

[PATCH net v2 2/3] net/iucv: claim the receive credit atomically

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

afiucv_hs_send() samples msg_recv, advertises it to the peer as the window,
and subtracts it once dev_queue_xmit() has returned. Nothing owns the
counter across the two: iucv_sock_sendmsg() reaches it under lock_sock()
and iucv_sock_recvmsg() reaches it under no socket lock, so an unprivileged
process running both on one socket can have them subtract the same value.
msg_recv goes negative and trips the WARN_ON(); the same interleaving puts
that credit on the wire twice, and the peer's afiucv_hs_callback_win()
subtracts the wire value from msg_sent unchecked.

Claim it with atomic_xchg(), after the last error exit so the counter reads
zero only while the transmit is in flight, and hand it back if that fails.
Nothing subtracts now, so the WARN_ON() goes too.

Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/iucv/af_iucv.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 0bc4a15f4b56..492a45bb2bba 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -210,12 +210,6 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
 	phs_hdr->flags = flags;
 	if (flags == AF_IUCV_FLAG_SYN)
 		phs_hdr->window = iucv->msglimit;
-	else if ((flags == AF_IUCV_FLAG_WIN) || !flags) {
-		confirm_recv = atomic_read(&iucv->msg_recv);
-		phs_hdr->window = confirm_recv;
-		if (confirm_recv)
-			phs_hdr->flags = phs_hdr->flags | AF_IUCV_FLAG_WIN;
-	}
 	memcpy(phs_hdr->destUserID, iucv->dst_user_id, 8);
 	memcpy(phs_hdr->destAppName, iucv->dst_name, 8);
 	memcpy(phs_hdr->srcUserID, iucv->src_user_id, 8);
@@ -250,13 +244,22 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
 	}
 	skb->protocol = cpu_to_be16(ETH_P_AF_IUCV);
 
+	/* Claim the receive credit here, not while building the header: every
+	 * way this frame can be dropped has now been ruled out, so the window
+	 * is zeroed only for as long as the transmit itself takes.
+	 */
+	if (flags == AF_IUCV_FLAG_WIN || !flags) {
+		confirm_recv = atomic_xchg(&iucv->msg_recv, 0);
+		phs_hdr->window = confirm_recv;
+		if (confirm_recv)
+			phs_hdr->flags = phs_hdr->flags | AF_IUCV_FLAG_WIN;
+	}
+
 	atomic_inc(&iucv->skbs_in_xmit);
 	err = dev_queue_xmit(skb);
 	if (net_xmit_eval(err)) {
 		atomic_dec(&iucv->skbs_in_xmit);
-	} else {
-		atomic_sub(confirm_recv, &iucv->msg_recv);
-		WARN_ON(atomic_read(&iucv->msg_recv) < 0);
+		atomic_add(confirm_recv, &iucv->msg_recv);
 	}
 	return net_xmit_eval(err);
 

-- 
2.55.0

Re: [PATCH net v2 2/3] net/iucv: claim the receive credit atomically

[sashiko-bot]

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

Pre-existing issues:
- [High] Positive network drop errors leak to userspace as partial success
- [High] Silent data loss for SOCK_STREAM when packet length exceeds MTU
--

--- Patch [2]: [PATCH net v2 2/3] net/iucv: claim the receive credit atomically ---
commit 20117da9fdf439cdc2d9a9928d88d8e19d4f0e83
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/iucv: claim the receive credit atomically

This commit addresses a race condition in the af_iucv HiperSockets
transport where the receive credit counter could be subtracted multiple times.
It modifies the code to claim the window using atomic_xchg() right before
queueing the packet to ensure the counter reads zero while the transmit
is in flight.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 0bc4a15f4b565..492a45bb2bba6 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c

[ ... ]

> @@ -250,13 +244,22 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
>  	}
>  	skb->protocol = cpu_to_be16(ETH_P_AF_IUCV);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential issue with
the MTU check just above this line in afiucv_hs_send():

	if (skb->len > skb->dev->mtu) {
		if (sock->sk_type == SOCK_SEQPACKET) {
			err = -EMSGSIZE;
			goto err_free;
		}
		err = pskb_trim(skb, skb->dev->mtu);
		if (err)
			goto err_free;
	}

For SOCK_STREAM sockets, if the packet is larger than the MTU, the skb is
silently truncated via pskb_trim().

However, iucv_sock_sendmsg() isn't aware of this truncation and returns the full
original length to userspace on success:

	err = afiucv_hs_send(&txmsg, sk, skb, 0);
	...
	release_sock(sk);
	return len;

Does this result in silent data loss, since userspace believes the entire buffer
was successfully sent?

>  
> +	/* Claim the receive credit here, not while building the header: every
> +	 * way this frame can be dropped has now been ruled out, so the window
> +	 * is zeroed only for as long as the transmit itself takes.
> +	 */
> +	if (flags == AF_IUCV_FLAG_WIN || !flags) {
> +		confirm_recv = atomic_xchg(&iucv->msg_recv, 0);
> +		phs_hdr->window = confirm_recv;
> +		if (confirm_recv)
> +			phs_hdr->flags = phs_hdr->flags | AF_IUCV_FLAG_WIN;
> +	}
> +
>  	atomic_inc(&iucv->skbs_in_xmit);
>  	err = dev_queue_xmit(skb);
>  	if (net_xmit_eval(err)) {
>  		atomic_dec(&iucv->skbs_in_xmit);
> -	} else {
> -		atomic_sub(confirm_recv, &iucv->msg_recv);
> -		WARN_ON(atomic_read(&iucv->msg_recv) < 0);
> +		atomic_add(confirm_recv, &iucv->msg_recv);
>  	}
>  	return net_xmit_eval(err);

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the return value here.

When dev_queue_xmit() drops a packet, it returns NET_XMIT_DROP (0x01).
net_xmit_eval() translates NET_XMIT_DROP to 1, and returning this positive value
propagates to iucv_sock_sendmsg().

Since iucv_sock_sendmsg() passes positive error codes back to the socket layer:

	err = afiucv_hs_send(&txmsg, sk, skb, 0);
	if (err) {
		atomic_dec(&iucv->msg_sent);
		goto out;
	}

Could sys_sendmsg interpret the return value of 1 as 1 byte successfully sent,
causing userspace to incorrectly assume partial success and potentially enter
an infinite loop retrying the remaining data?

Should this be returning net_xmit_errno(err) instead of net_xmit_eval(err)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821-b4-disp-3a6e8695-v2-0-37597ff723a8@proton.me?part=2

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

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

iucv_sock_recvmsg() calls iucv_send_ctrl() with message_q.lock held, and
iucv_send_ctrl() allocates through sock_alloc_send_skb() with
sk->sk_allocation -- GFP_KERNEL here -- so the allocation may sleep inside
the spin_lock_bh() section; noblock suppresses only the wait for send
buffer space, not the allocation flags. CONFIG_DEBUG_ATOMIC_SLEEP reports
it.

Note that the update is due and send it once the lock is dropped. That
leaves two recvmsg() able to reach afiucv_hs_send() concurrently, which
message_q.lock used to prevent; the preceding patch is what makes that
safe, so do not apply this one without it.

Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/iucv/af_iucv.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 492a45bb2bba..a7c0f60bb5bf 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1244,6 +1244,7 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 	struct iucv_sock *iucv = iucv_sk(sk);
 	unsigned int copied, rlen;
 	struct sk_buff *skb, *rskb, *cskb;
+	bool send_win = false;
 	int err = 0;
 	u32 offset;
 
@@ -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);
+			if (err) {
+				sk->sk_state = IUCV_DISCONN;
+				sk->sk_state_change(sk);
+			}
+		}
 	}
 
 done:

-- 
2.55.0

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

[sashiko-bot]

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 <hexlabsecurity@proton.me>

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/20260821-b4-disp-3a6e8695-v2-0-37597ff723a8@proton.me?part=3

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

[Alexandra Winter]

On 22.08.26 13:17, sashiko-bot@kernel.org wrote:
> 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 <hexlabsecurity@proton.me>
> 
> 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?
> 

No, because of the previous patch that is mentioned as a precondition.


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

I'm not sure I understand the question. But I think this works as designed.

Re: [PATCH net v2 0/3] net/iucv: fix the recvmsg window update

[Alexandra Winter]

On 21.08.26 13:17, Bryam Vargas via B4 Relay wrote:
> v2 answers Jakub's reviews of v1 with changes rather than replies.
> 
> 1/3 keeps its hunk and states the impact properly. SO_MSGLIMIT accepts 1,
> which makes msglimit / 2 zero, and msg_recv never leaves zero on a classic
> socket, so the NULL read is one recvmsg() away for an unprivileged process
> on a socket of its own. With relocate_lowcore it faults.
> 
> Its Fixes: tag was wrong as well. The unconditional send does date to
> 3881ac441f64, but iucv_send_ctrl() sized the skb with a constant ETH_HLEN
> until 238965b71b96, so before that it just returned -ENODEV. Backport
> window is v5.3, not v3.1.
> 
> 2/3 is new. v1 admitted it widened the msg_recv race and left it open,
> which isn't good enough under Cc: stable. afiucv_hs_send() samples the
> counter and settles it after dev_queue_xmit(); sendmsg reaches that under
> lock_sock() and recvmsg reaches it under no socket lock, so the two don't
> exclude each other and both can subtract the same value. Negative counter,
> WARN_ON, and the same credit advertised twice to a peer whose
> afiucv_hs_callback_win() subtracts the wire value from msg_sent unchecked.
> 
> Taking the socket lock around the deferred send would close it too, since
> recvmsg is the only unlocked side. I didn't: recvmsg has never held that
> lock, and making it do so changes the receive path for every caller. That
> belongs in the locking rework, not in a stable fix.
> 
> The claim sits after the last error exit rather than in the header build,
> so the counter reads zero only while the transmit is in flight, and a
> concurrent sender isn't talked out of its own update.
> 
> 3/3 is v1's patch, and it must not be applied without 2/3: hoisting the
> send out of message_q.lock drops the serialisation that lock gave two
> concurrent recvmsg(). Backported alone it recreates what 2/3 fixes. Both
> changelogs say so.
> 
> Litmus test under LKMM: the counter reaches -2 before 2/3 and cannot after.
> No hardware run; I have no IBM Z, and CONFIG_AFIUCV is s390-only.
> 
> The unlocked hs_dev and sk_shutdown window, the unbounded backlog_skb_q and
> the WARN_ON a flooding peer can reach are pre-existing and wider than this
> series. They belong with the locking rework Alexandra has open.
> 
> Thanks for the reviews.
> 
> ---
> Bryam Vargas (3):
>       net/iucv: only send the window update on HiperSockets sockets
>       net/iucv: claim the receive credit atomically
>       net/iucv: send the window update outside message_q.lock
> 
>  net/iucv/af_iucv.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> ---
> base-commit: 746fc0787f616da418ffc04a110296fe95d53491
> change-id: 20260821-b4-disp-3a6e8695-c1f4a6069169
> 
> Best regards,
> --  
> Bryam Vargas <hexlabsecurity@proton.me>
> 
> 
> 

Thank you for the fixes Bryam.
I have reviewed them and would technically give an R-b to all three.

However I propose to send them again as one single patch. They are
really about the same subject: iucv->msg_recv handling and even have
a dependency, as you rightfully noted.
It will be much easier to proceed and to backport them, if it is only
one patch.

I know it's a tradeoff, but I think we will do ourselves and the
upstream maintainers a favor, if we send not too small fragments
of fixes.

Re: [PATCH net v2 0/3] net/iucv: fix the recvmsg window update

[Aswin Karuvally]

On 21/08/26 16:47, Bryam Vargas via B4 Relay wrote:
> v2 answers Jakub's reviews of v1 with changes rather than replies.
> 
> 1/3 keeps its hunk and states the impact properly. SO_MSGLIMIT accepts 1,
> which makes msglimit / 2 zero, and msg_recv never leaves zero on a classic
> socket, so the NULL read is one recvmsg() away for an unprivileged process
> on a socket of its own. With relocate_lowcore it faults.
> 
> Its Fixes: tag was wrong as well. The unconditional send does date to
> 3881ac441f64, but iucv_send_ctrl() sized the skb with a constant ETH_HLEN
> until 238965b71b96, so before that it just returned -ENODEV. Backport
> window is v5.3, not v3.1.
> 
> 2/3 is new. v1 admitted it widened the msg_recv race and left it open,
> which isn't good enough under Cc: stable. afiucv_hs_send() samples the
> counter and settles it after dev_queue_xmit(); sendmsg reaches that under
> lock_sock() and recvmsg reaches it under no socket lock, so the two don't
> exclude each other and both can subtract the same value. Negative counter,
> WARN_ON, and the same credit advertised twice to a peer whose
> afiucv_hs_callback_win() subtracts the wire value from msg_sent unchecked.
> 
> Taking the socket lock around the deferred send would close it too, since
> recvmsg is the only unlocked side. I didn't: recvmsg has never held that
> lock, and making it do so changes the receive path for every caller. That
> belongs in the locking rework, not in a stable fix.
> 
> The claim sits after the last error exit rather than in the header build,
> so the counter reads zero only while the transmit is in flight, and a
> concurrent sender isn't talked out of its own update.
> 
> 3/3 is v1's patch, and it must not be applied without 2/3: hoisting the
> send out of message_q.lock drops the serialisation that lock gave two
> concurrent recvmsg(). Backported alone it recreates what 2/3 fixes. Both
> changelogs say so.
> 
> Litmus test under LKMM: the counter reaches -2 before 2/3 and cannot after.
> No hardware run; I have no IBM Z, and CONFIG_AFIUCV is s390-only.
> 
> The unlocked hs_dev and sk_shutdown window, the unbounded backlog_skb_q and
> the WARN_ON a flooding peer can reach are pre-existing and wider than this
> series. They belong with the locking rework Alexandra has open.
> 
> Thanks for the reviews.
> 
> ---
> Bryam Vargas (3):
>       net/iucv: only send the window update on HiperSockets sockets
>       net/iucv: claim the receive credit atomically
>       net/iucv: send the window update outside message_q.lock
> 
>  net/iucv/af_iucv.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> ---
> base-commit: 746fc0787f616da418ffc04a110296fe95d53491
> change-id: 20260821-b4-disp-3a6e8695-c1f4a6069169
> 
> Best regards,
> --  
> Bryam Vargas <hexlabsecurity@proton.me>

Thank you for the fixes Bryam!

Tested-by: Aswin Karuvally <aswin@linux.ibm.com>