[PATCH net v2 0/2] net/iucv: two fixes in the HiperSockets receive path

[PATCH net v2 0/2] net/iucv: two fixes in the HiperSockets receive path

[Bryam Vargas via B4 Relay]

Two bugs in the HiperSockets receive path that predate the socket-locking
rework [1]; review and testing of that rework turned them up. Neither
depends on it.

1/2 clears hs_dev before the socket drops its reference on the device and
marks the accesses that race with it: afiucv_hs_rcv() compares hs_dev
under iucv_sk_list.lock while close() clears it under lock_sock().
Hidayath Khan caught the racing pair with KCSAN under z/VM [2] and
suggested the fix. The listener's child now takes its device from the
frame instead of reading the listener's field in softirq. This is an
ordering fix with no known crash.

2/2 stops counting the SHT's empty frame against the receive window (the
peer does not count it) and drops the WARN_ON on the overrun path, which
a peer reaches from the network by sending past its window. Only frames
with data count now; a zero-length data frame, which already ends the
stream for its reader, is no longer counted either. 2/2 applies on its
own and goes to stable.

Based on net at fcc00b15cb21, after the net-next merge (iucv_sock_close()
is split into __iucv_sock_close() there). The same two patches apply on
mainline. Tested on an s390x guest under QEMU TCG with KASAN and lockdep
on, on net f4377bf85887 and net-next 8df0638138d3; the base moved to
fcc00b15cb21 afterwards, same hunks. QEMU has no HiperSockets adapter:
the one here is mine, under the real qeth and qdio drivers, on a QEMU
with my own patches to the s390x machine (the adapter's interruptions
through the FLIC, a z/VM CP model), and there is no IBM Z to compare
against. With 2/2 an SHT read while frames wait in backlog_skb_q gives
EOF instead of WARN plus -EFAULT. 1/2 does not change any result in the
rework's test set; its race shows under KCSAN, not here. Logs on
request.

v2:
- 2/2: an empty frame read after the close over the window reached
  iucv_send_ctrl() with hs_dev already cleared (the AI review of v1
  [3]); the window update now goes out only while the socket is
  connected. Repro on the same machine (SO_MSGLIMIT 2, SO_RCVBUF 6144,
  two sends, one frame injected over the window, the SHT, 16 more
  behind it; the parameters are below 2/2), counting iucv_send_ctrl()
  entries and the frames at the adapter: v1 attempts one update after
  the close and nothing leaves; v2 attempts none. The reads are the same
  on both (1, 1, -EFAULT, -ECONNRESET, 0, -EFAULT; the last is a data
  frame behind the SHT closing again); net without the patch reads
  -EFAULT at the SHT with its WARN.
- 2/2: the commit message now says "a zero-length data frame that
  reaches recvmsg()": a zero-length send with a piggybacked credit is a
  window update and never reaches it [3].
- 1/2: code unchanged. The review's point on it (the child's device
  reference when a SYN races the listener's close) is the pre-existing
  orphan the notes below 1/2 describe; the rework [1] closes that race.
- Both commit messages say how the patch was tested.
- Rebased on net after the net-next merge.

v1: https://lore.kernel.org/netdev/20261022-b4-disp-b35a9153-v1-0-d74645876195@proton.me/

[1] https://lore.kernel.org/netdev/20261022-b4-disp-1d26b87a-v2-0-7ee1264d812a@proton.me/
[2] https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/
[3] https://lore.kernel.org/netdev/179306942509.3134.1109951482403010755@kernel.org/

---
Bryam Vargas (2):
      net/iucv: clear hs_dev before dropping its reference
      net/iucv: do not count the SHT frame against the receive window

 net/iucv/af_iucv.c | 45 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 11 deletions(-)
---
base-commit: fcc00b15cb21a6de76826e5012b2c758505f5e65
change-id: 20261028-b4-disp-dd3e4ebb-fe118f9d09aa

Best regards,
--  
bryamzxz <hexlabsecurity@proton.me>

[PATCH net v2 0/2] net/iucv: two fixes in the HiperSockets receive path

[Bryam Vargas]

Two bugs in the HiperSockets receive path that predate the socket-locking
rework [1]; review and testing of that rework turned them up. Neither
depends on it.

1/2 clears hs_dev before the socket drops its reference on the device and
marks the accesses that race with it: afiucv_hs_rcv() compares hs_dev
under iucv_sk_list.lock while close() clears it under lock_sock().
Hidayath Khan caught the racing pair with KCSAN under z/VM [2] and
suggested the fix. The listener's child now takes its device from the
frame instead of reading the listener's field in softirq. This is an
ordering fix with no known crash.

2/2 stops counting the SHT's empty frame against the receive window (the
peer does not count it) and drops the WARN_ON on the overrun path, which
a peer reaches from the network by sending past its window. Only frames
with data count now; a zero-length data frame, which already ends the
stream for its reader, is no longer counted either. 2/2 applies on its
own and goes to stable.

Based on net at fcc00b15cb21, after the net-next merge (iucv_sock_close()
is split into __iucv_sock_close() there). The same two patches apply on
mainline. Tested on an s390x guest under QEMU TCG with KASAN and lockdep
on, on net f4377bf85887 and net-next 8df0638138d3; the base moved to
fcc00b15cb21 afterwards, same hunks. QEMU has no HiperSockets adapter:
the one here is mine, under the real qeth and qdio drivers, on a QEMU
with my own patches to the s390x machine (the adapter's interruptions
through the FLIC, a z/VM CP model), and there is no IBM Z to compare
against. With 2/2 an SHT read while frames wait in backlog_skb_q gives
EOF instead of WARN plus -EFAULT. 1/2 does not change any result in the
rework's test set; its race shows under KCSAN, not here. Logs on
request.

v2:
- 2/2: an empty frame read after the close over the window reached
  iucv_send_ctrl() with hs_dev already cleared (the AI review of v1
  [3]); the window update now goes out only while the socket is
  connected. Repro on the same machine (SO_MSGLIMIT 2, SO_RCVBUF 6144,
  two sends, one frame injected over the window, the SHT, 16 more
  behind it; the parameters are below 2/2), counting iucv_send_ctrl()
  entries and the frames at the adapter: v1 attempts one update after
  the close and nothing leaves; v2 attempts none. The reads are the same
  on both (1, 1, -EFAULT, -ECONNRESET, 0, -EFAULT; the last is a data
  frame behind the SHT closing again); net without the patch reads
  -EFAULT at the SHT with its WARN.
- 2/2: the commit message now says "a zero-length data frame that
  reaches recvmsg()": a zero-length send with a piggybacked credit is a
  window update and never reaches it [3].
- 1/2: code unchanged. The review's point on it (the child's device
  reference when a SYN races the listener's close) is the pre-existing
  orphan the notes below 1/2 describe; the rework [1] closes that race.
- Both commit messages say how the patch was tested.
- Rebased on net after the net-next merge.

v1: https://lore.kernel.org/netdev/20261022-b4-disp-b35a9153-v1-0-d74645876195@proton.me/

[1] https://lore.kernel.org/netdev/20261022-b4-disp-1d26b87a-v2-0-7ee1264d812a@proton.me/
[2] https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/
[3] https://lore.kernel.org/netdev/179306942509.3134.1109951482403010755@kernel.org/

---
Bryam Vargas (2):
      net/iucv: clear hs_dev before dropping its reference
      net/iucv: do not count the SHT frame against the receive window

 net/iucv/af_iucv.c | 45 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 11 deletions(-)
---
base-commit: fcc00b15cb21a6de76826e5012b2c758505f5e65
change-id: 20261028-b4-disp-dd3e4ebb-fe118f9d09aa

Best regards,
--  
bryamzxz <hexlabsecurity@proton.me>

[PATCH net v2 1/2] net/iucv: clear hs_dev before dropping its reference

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

iucv_sock_close() drops the socket's reference on its HiperSockets
device and only then clears iucv->hs_dev, while afiucv_hs_rcv() compares
that field against the ingress device for every socket in the list,
under iucv_sk_list.lock and not the socket lock. Between the two it
compares a pointer the socket no longer holds; KCSAN reports the plain
read against the plain write. The order dates from 800c5eb7b5eb, the
compare from 80230a18c164. No user-visible effect is known: the receive
path holds the ingress device, so the stale pointer is compared, never
dereferenced.

Clear the pointer first, through a local, then drop the reference, and
mark the lookup's read and the two stores that race with it: the clear
in close() and the child's store in afiucv_hs_callback_syn() (the child
is in iucv_sk_list from iucv_sock_alloc() on). The child now takes its
device and sk_bound_dev_if from the frame the lookup matched, held
before it is published, instead of reading the listener's fields:
close() clears them under lock_sock(), which the bh_lock_sock() in
afiucv_hs_callback_syn() does not exclude, so the callback can pass its
IUCV_LISTEN check and then read a field close() has cleared. That a SYN
can be accepted while the listener closes is left to the socket-locking
rework; this is its KCSAN-visible part. bind()'s hs_dev store runs
under the list's write lock and stays plain. sk_bound_dev_if, which
getsockopt() reads without the lock, is marked where close(), bind()
and the child store it; annotation only.

Tested on s390x under QEMU TCG with a patched QEMU: an emulated
HiperSockets adapter under the real qeth and qdio drivers. The
socket-locking rework's test set gives the same results with and
without this patch; the race itself shows under KCSAN only.

Fixes: 800c5eb7b5eb ("af_iucv: change net_device handling for HS transport")
Fixes: 80230a18c164 ("net/iucv: filter frames in afiucv_hs_rcv() by ingress device")
Reported-by: Hidayath Khan <hidayath@linux.ibm.com>
Closes: https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/
Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Nagamani PV's "net/iucv: fix races in afiucv_netdev_event()" [1] marks
the same store in close(); Alexandra Winter asked him to hold it for the
locking series and he agreed [2]. This one goes out now, on a new and
independent KCSAN report, and its close() hunk subsumes his, which marks
the store but keeps dev_put() first; once this is applied, the
afiucv_netdev_event() read is what remains for his patch.

The loop's memcmp()s on the name fields are plain as well, against
connect()'s stores under lock_sock(); they were not in Hidayath's report
and are not touched here. The child that afiucv_hs_callback_syn() leaks
on its error paths (iucv_sock_kill() on a socket that was never closed
is a no-op), with the device reference it holds when dev_queue_xmit()
fails, is pre-existing; Hidayath flagged it in August [3] and his fix
for it is pending. A child the callback enqueues after
iucv_sock_cleanup_listen() has drained the accept queue is never
accepted or killed either; it now holds the device the frame arrived on,
where before it held the listener's device or nothing, depending on
which side of the clear it read.

Tested on s390x under QEMU TCG (net-next 8df0638138d3, KASAN +
PROVE_LOCKING) with an emulated HiperSockets adapter under the real qeth
and qdio drivers. The rework's test set, 53 tests, gives the same result
with and without this patch: the 8 that fail on net-next are the bugs
the rework fixes, and they fail the same way. That setup has no KCSAN
and nothing that drives afiucv_hs_rcv()'s lookup against a close() at
the right instant. Hidayath's KCSAN report covers the lookup's read and
proposed the clear-before-put order through a local; the child's device
source comes from reading the code.

[1] https://lore.kernel.org/netdev/20260917071706.23831-1-nagamani@linux.ibm.com/
[2] https://lore.kernel.org/netdev/1d9d3cc1-24fb-40e0-a4c7-6fbfbcf4dc37@linux.ibm.com/
[3] https://lore.kernel.org/netdev/5f88daf4-cd8e-4464-b132-ac45be205121@linux.ibm.com/
---
 net/iucv/af_iucv.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 5ba875798aa4..5bab6aea910e 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -450,9 +450,16 @@ static void __iucv_sock_close(struct sock *sk)
 	}
 
 	if (iucv->hs_dev) {
-		dev_put(iucv->hs_dev);
-		iucv->hs_dev = NULL;
-		sk->sk_bound_dev_if = 0;
+		struct net_device *dev = iucv->hs_dev;
+
+		/* afiucv_hs_rcv() matches frames on hs_dev under
+		 * iucv_sk_list.lock, not the socket lock: clear it before the
+		 * reference goes, so the match can no longer find a device
+		 * this socket does not hold.
+		 */
+		WRITE_ONCE(iucv->hs_dev, NULL);
+		WRITE_ONCE(sk->sk_bound_dev_if, 0);
+		dev_put(dev);
 	}
 
 	/* mark socket for deletion by iucv_sock_kill() */
@@ -638,7 +645,7 @@ static int iucv_sock_bind(struct socket *sock, struct sockaddr_unsized *addr,
 				__iucv_auto_name(iucv);
 			else
 				memcpy(iucv->src_name, sa->siucv_name, 8);
-			sk->sk_bound_dev_if = dev->ifindex;
+			WRITE_ONCE(sk->sk_bound_dev_if, dev->ifindex);
 			iucv->hs_dev = dev;
 			dev_hold(dev);
 			sk->sk_state = IUCV_BOUND;
@@ -1920,9 +1927,14 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
 	memcpy(niucv->dst_user_id, trans_hdr->srcUserID, 8);
 	memcpy(niucv->src_name, iucv->src_name, 8);
 	memcpy(niucv->src_user_id, iucv->src_user_id, 8);
-	nsk->sk_bound_dev_if = sk->sk_bound_dev_if;
-	niucv->hs_dev = iucv->hs_dev;
-	dev_hold(niucv->hs_dev);
+	/* skb->dev is the device the lookup matched against the listener's
+	 * hs_dev; the listener's own fields are cleared by close() under
+	 * lock_sock(), which this softirq does not hold. nsk is in
+	 * iucv_sk_list already: hold, then publish.
+	 */
+	WRITE_ONCE(nsk->sk_bound_dev_if, skb->dev->ifindex);
+	dev_hold(skb->dev);
+	WRITE_ONCE(niucv->hs_dev, skb->dev);
 	afiucv_swap_src_dest(skb);
 	trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_ACK;
 	trans_hdr->window = niucv->msglimit;
@@ -2095,7 +2107,11 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
 	sk = NULL;
 	read_lock(&iucv_sk_list.lock);
 	sk_for_each(sk, &iucv_sk_list.head) {
-		if (iucv_sk(sk)->hs_dev != dev)
+		/* Cleared before iucv_sock_close() drops the reference:
+		 * whichever value is read is a device the socket still
+		 * holds, or NULL.
+		 */
+		if (READ_ONCE(iucv_sk(sk)->hs_dev) != dev)
 			continue;
 		if (trans_hdr->flags == AF_IUCV_FLAG_SYN) {
 			if ((!memcmp(&iucv_sk(sk)->src_name,

-- 
2.56.0

[PATCH net v2 1/2] net/iucv: clear hs_dev before dropping its reference

[Bryam Vargas]

iucv_sock_close() drops the socket's reference on its HiperSockets
device and only then clears iucv->hs_dev, while afiucv_hs_rcv() compares
that field against the ingress device for every socket in the list,
under iucv_sk_list.lock and not the socket lock. Between the two it
compares a pointer the socket no longer holds; KCSAN reports the plain
read against the plain write. The order dates from 800c5eb7b5eb, the
compare from 80230a18c164. No user-visible effect is known: the receive
path holds the ingress device, so the stale pointer is compared, never
dereferenced.

Clear the pointer first, through a local, then drop the reference, and
mark the lookup's read and the two stores that race with it: the clear
in close() and the child's store in afiucv_hs_callback_syn() (the child
is in iucv_sk_list from iucv_sock_alloc() on). The child now takes its
device and sk_bound_dev_if from the frame the lookup matched, held
before it is published, instead of reading the listener's fields:
close() clears them under lock_sock(), which the bh_lock_sock() in
afiucv_hs_callback_syn() does not exclude, so the callback can pass its
IUCV_LISTEN check and then read a field close() has cleared. That a SYN
can be accepted while the listener closes is left to the socket-locking
rework; this is its KCSAN-visible part. bind()'s hs_dev store runs
under the list's write lock and stays plain. sk_bound_dev_if, which
getsockopt() reads without the lock, is marked where close(), bind()
and the child store it; annotation only.

Tested on s390x under QEMU TCG with a patched QEMU: an emulated
HiperSockets adapter under the real qeth and qdio drivers. The
socket-locking rework's test set gives the same results with and
without this patch; the race itself shows under KCSAN only.

Fixes: 800c5eb7b5eb ("af_iucv: change net_device handling for HS transport")
Fixes: 80230a18c164 ("net/iucv: filter frames in afiucv_hs_rcv() by ingress device")
Reported-by: Hidayath Khan <hidayath@linux.ibm.com>
Closes: https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/
Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Nagamani PV's "net/iucv: fix races in afiucv_netdev_event()" [1] marks
the same store in close(); Alexandra Winter asked him to hold it for the
locking series and he agreed [2]. This one goes out now, on a new and
independent KCSAN report, and its close() hunk subsumes his, which marks
the store but keeps dev_put() first; once this is applied, the
afiucv_netdev_event() read is what remains for his patch.

The loop's memcmp()s on the name fields are plain as well, against
connect()'s stores under lock_sock(); they were not in Hidayath's report
and are not touched here. The child that afiucv_hs_callback_syn() leaks
on its error paths (iucv_sock_kill() on a socket that was never closed
is a no-op), with the device reference it holds when dev_queue_xmit()
fails, is pre-existing; Hidayath flagged it in August [3] and his fix
for it is pending. A child the callback enqueues after
iucv_sock_cleanup_listen() has drained the accept queue is never
accepted or killed either; it now holds the device the frame arrived on,
where before it held the listener's device or nothing, depending on
which side of the clear it read.

Tested on s390x under QEMU TCG (net-next 8df0638138d3, KASAN +
PROVE_LOCKING) with an emulated HiperSockets adapter under the real qeth
and qdio drivers. The rework's test set, 53 tests, gives the same result
with and without this patch: the 8 that fail on net-next are the bugs
the rework fixes, and they fail the same way. That setup has no KCSAN
and nothing that drives afiucv_hs_rcv()'s lookup against a close() at
the right instant. Hidayath's KCSAN report covers the lookup's read and
proposed the clear-before-put order through a local; the child's device
source comes from reading the code.

[1] https://lore.kernel.org/netdev/20260917071706.23831-1-nagamani@linux.ibm.com/
[2] https://lore.kernel.org/netdev/1d9d3cc1-24fb-40e0-a4c7-6fbfbcf4dc37@linux.ibm.com/
[3] https://lore.kernel.org/netdev/5f88daf4-cd8e-4464-b132-ac45be205121@linux.ibm.com/
---
 net/iucv/af_iucv.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 5ba875798aa4..5bab6aea910e 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -450,9 +450,16 @@ static void __iucv_sock_close(struct sock *sk)
 	}
 
 	if (iucv->hs_dev) {
-		dev_put(iucv->hs_dev);
-		iucv->hs_dev = NULL;
-		sk->sk_bound_dev_if = 0;
+		struct net_device *dev = iucv->hs_dev;
+
+		/* afiucv_hs_rcv() matches frames on hs_dev under
+		 * iucv_sk_list.lock, not the socket lock: clear it before the
+		 * reference goes, so the match can no longer find a device
+		 * this socket does not hold.
+		 */
+		WRITE_ONCE(iucv->hs_dev, NULL);
+		WRITE_ONCE(sk->sk_bound_dev_if, 0);
+		dev_put(dev);
 	}
 
 	/* mark socket for deletion by iucv_sock_kill() */
@@ -638,7 +645,7 @@ static int iucv_sock_bind(struct socket *sock, struct sockaddr_unsized *addr,
 				__iucv_auto_name(iucv);
 			else
 				memcpy(iucv->src_name, sa->siucv_name, 8);
-			sk->sk_bound_dev_if = dev->ifindex;
+			WRITE_ONCE(sk->sk_bound_dev_if, dev->ifindex);
 			iucv->hs_dev = dev;
 			dev_hold(dev);
 			sk->sk_state = IUCV_BOUND;
@@ -1920,9 +1927,14 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
 	memcpy(niucv->dst_user_id, trans_hdr->srcUserID, 8);
 	memcpy(niucv->src_name, iucv->src_name, 8);
 	memcpy(niucv->src_user_id, iucv->src_user_id, 8);
-	nsk->sk_bound_dev_if = sk->sk_bound_dev_if;
-	niucv->hs_dev = iucv->hs_dev;
-	dev_hold(niucv->hs_dev);
+	/* skb->dev is the device the lookup matched against the listener's
+	 * hs_dev; the listener's own fields are cleared by close() under
+	 * lock_sock(), which this softirq does not hold. nsk is in
+	 * iucv_sk_list already: hold, then publish.
+	 */
+	WRITE_ONCE(nsk->sk_bound_dev_if, skb->dev->ifindex);
+	dev_hold(skb->dev);
+	WRITE_ONCE(niucv->hs_dev, skb->dev);
 	afiucv_swap_src_dest(skb);
 	trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_ACK;
 	trans_hdr->window = niucv->msglimit;
@@ -2095,7 +2107,11 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
 	sk = NULL;
 	read_lock(&iucv_sk_list.lock);
 	sk_for_each(sk, &iucv_sk_list.head) {
-		if (iucv_sk(sk)->hs_dev != dev)
+		/* Cleared before iucv_sock_close() drops the reference:
+		 * whichever value is read is a device the socket still
+		 * holds, or NULL.
+		 */
+		if (READ_ONCE(iucv_sk(sk)->hs_dev) != dev)
 			continue;
 		if (trans_hdr->flags == AF_IUCV_FLAG_SYN) {
 			if ((!memcmp(&iucv_sk(sk)->src_name,

-- 
2.56.0

[PATCH net v2 2/2] net/iucv: do not count the SHT frame against the receive window

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

iucv_sock_recvmsg() counts every HiperSockets frame it consumes in
msg_recv, the SHT's empty one included, while the peer counts only what
sendmsg() sends against the window it was granted; the SHT goes out
through iucv_send_ctrl() uncounted, one per shutdown(SHUT_WR) call. The
next window update zeroes msg_recv and takes the surplus with it, but
recvmsg() sends none while frames wait in backlog_skb_q, and an SHT read
in that state pushes msg_recv past msglimit: WARN_ON(1) fires, the socket
is closed and the reader gets -EFAULT instead of EOF. A peer that sends
past its window reaches the same WARN from the network; with
panic_on_warn that is a remote panic. The SHT frame dates from
82492a355fac, the WARN and the close from 800c5eb7b5eb.

The SHT reaches recvmsg() as a zero-length frame, like a zero-length
data frame, so test the length: count only frames that still carry
data when the read consumes them (rlen; a partial SOCK_STREAM read
requeues above this point, so each data frame counts once), and close a
socket over the window without the WARN; -EFAULT is kept and nothing is
logged. A zero-length data frame that reaches recvmsg() sets
RCV_SHUTDOWN, after which afiucv_hs_callback_rx() drops every later
data frame uncounted; the one slot this patch no longer credits for it
is lost like the slots of the frames dropped after it.

The close over the window purges backlog_skb_q, not sk_receive_queue,
and nothing resets msg_recv; once empty frames are not counted, a read
of an empty frame from that queue after the close would skip the
-EFAULT branch and reach the window update, where iucv_send_ctrl()
sizes the skb through the cleared hs_dev. Send the update only while
the socket is connected.

Tested on s390x under QEMU TCG with a patched QEMU: an emulated
HiperSockets adapter under the real qeth and qdio drivers. An SHT read
with frames waiting in backlog_skb_q returns EOF instead of -EFAULT, and
a read after the close over the window attempts no window update.

Fixes: 82492a355fac ("af_iucv: add shutdown for HS transport")
Fixes: 800c5eb7b5eb ("af_iucv: change net_device handling for HS transport")
Reported-by: Sashiko <netdev-bot+sashiko@kernel.org>
Closes: https://lore.kernel.org/netdev/179183431327.434549.446348148837850426@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v2: the window update goes out only while the socket is connected. The
AI review of v1 found that, once empty frames are not counted, a read
from sk_receive_queue after the close over the window skips the -EFAULT
branch and reaches iucv_send_ctrl() with hs_dev NULL:
LL_RESERVED_SPACE() reads through the NULL pointer (lowcore on s390: no
fault on a default boot, an oops with a relocated lowcore),
afiucv_hs_send() fails with -ENODEV and recvmsg() moves the closed
socket to IUCV_DISCONN. Repro on the same machine with SO_MSGLIMIT 2 and
SO_RCVBUF 6144: the peer sends 1 byte twice (the window), then from a
tap on its interface one data frame over the window, its SHUT_WR for the
SHT, and 16 more data frames behind it, so that no read sends an update
before the close (a Linux peer blocks at the window; the overrun is
injected). The third read closes the socket, the fourth returns
-ECONNRESET, the fifth consumes the SHT, the sixth takes a data frame
behind it and closes again. On v1 the probe counts one iucv_send_ctrl()
entry after the close and the adapter sees no frame leave; with this
version there is none. The reads are the same on both (1, 1, -EFAULT,
-ECONNRESET, 0, -EFAULT), IOW nothing shows from user space on a default
boot; net without the patch reads -EFAULT at the SHT with its WARN.

Also in v2, the commit message now says "a zero-length data frame that
reaches recvmsg()": a zero-length send with a piggybacked credit is a
window update to afiucv_hs_rcv() and never reaches recvmsg(), so the
sentence did not hold for it.

Reproduced on s390x under QEMU TCG (net f4377bf85887 and net-next
8df0638138d3, same result) with an emulated HiperSockets adapter under
the real qeth and qdio drivers: the reader has SO_MSGLIMIT 2 and
SO_RCVBUF 2304 (the minimum); the peer sends 1 byte twice, which fills
the window, then calls shutdown(SHUT_WR) 32 times, one SHT each. The
reader starts once everything has arrived; the receive queue took two
frames, the rest wait in backlog_skb_q, and with that queue non-empty
recvmsg() sends no window update, so the first SHT read counts 3
against a limit of 2: WARN and -EFAULT without the patch, EOF with it.
With one SHT instead of 32 the queue drains, the update goes out after
the two reads, and both kernels read EOF.

Without the patch, the surplus a window update carries for a counted
SHT is credit the peer never spent, and it takes the peer's msg_sent
below zero; harmless, since a peer that sent an SHT has SEND_SHUTDOWN
and its sendmsg() returns -EPIPE. With the patch no SHT is counted, so
no update carries that surplus.

A HiperSockets socket stays in IUCV_BOUND from its SYN until the SYN|ACK
arrives, and SO_MSGLIMIT is accepted in that state, so msglimit can drop
below the window the SYN advertised. Hidayath's test for that [1],
written for the per-socket backlog budget of the socket-locking rework
[2], passes on bare net-next, where the update after every read keeps
msg_recv low; measured here with SO_RCVBUF at its minimum, so that
frames wait in backlog_skb_q, the same conforming peer reaches this
check (net-next: WARN and -EFAULT after two of eight messages; with this
patch -EFAULT without the WARN). The rework keeps the advertised window
in a field of its own for that, and this patch only takes the WARN out
of its way. That iucv_sock_shutdown() sends an SHT on every call is a
sender-side oddity left alone here; the receiver has to cope with any
peer.

[1] https://lore.kernel.org/netdev/51a4a40a-f34b-4d41-b049-69aa53f4dcf5@linux.ibm.com/
[2] https://lore.kernel.org/netdev/20261022-b4-disp-1d26b87a-v2-0-7ee1264d812a@proton.me/
---
 net/iucv/af_iucv.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 5bab6aea910e..266563fb0e0c 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1325,10 +1325,13 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		}
 
 		consume_skb(skb);
-		if (iucv->transport == AF_IUCV_TRANS_HIPER) {
+		/* The peer counts only the data it sends against its window;
+		 * the SHT's empty frame is not among them. Over the window,
+		 * close without a WARN: a peer reaches this from the network.
+		 */
+		if (iucv->transport == AF_IUCV_TRANS_HIPER && rlen) {
 			atomic_inc(&iucv->msg_recv);
 			if (atomic_read(&iucv->msg_recv) > iucv->msglimit) {
-				WARN_ON(1);
 				iucv_sock_close(sk);
 				return -EFAULT;
 			}
@@ -1357,7 +1360,11 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		}
 		spin_unlock_bh(&iucv->message_q.lock);
 
-		if (send_win) {
+		/* iucv_sock_close() clears hs_dev but leaves sk_receive_queue
+		 * readable: a frame read after the over-window close must not
+		 * send a window update.
+		 */
+		if (send_win && sk->sk_state == IUCV_CONNECTED) {
 			err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
 			if (err) {
 				sk->sk_state = IUCV_DISCONN;

-- 
2.56.0

[PATCH net v2 2/2] net/iucv: do not count the SHT frame against the receive window

[Bryam Vargas]

iucv_sock_recvmsg() counts every HiperSockets frame it consumes in
msg_recv, the SHT's empty one included, while the peer counts only what
sendmsg() sends against the window it was granted; the SHT goes out
through iucv_send_ctrl() uncounted, one per shutdown(SHUT_WR) call. The
next window update zeroes msg_recv and takes the surplus with it, but
recvmsg() sends none while frames wait in backlog_skb_q, and an SHT read
in that state pushes msg_recv past msglimit: WARN_ON(1) fires, the socket
is closed and the reader gets -EFAULT instead of EOF. A peer that sends
past its window reaches the same WARN from the network; with
panic_on_warn that is a remote panic. The SHT frame dates from
82492a355fac, the WARN and the close from 800c5eb7b5eb.

The SHT reaches recvmsg() as a zero-length frame, like a zero-length
data frame, so test the length: count only frames that still carry
data when the read consumes them (rlen; a partial SOCK_STREAM read
requeues above this point, so each data frame counts once), and close a
socket over the window without the WARN; -EFAULT is kept and nothing is
logged. A zero-length data frame that reaches recvmsg() sets
RCV_SHUTDOWN, after which afiucv_hs_callback_rx() drops every later
data frame uncounted; the one slot this patch no longer credits for it
is lost like the slots of the frames dropped after it.

The close over the window purges backlog_skb_q, not sk_receive_queue,
and nothing resets msg_recv; once empty frames are not counted, a read
of an empty frame from that queue after the close would skip the
-EFAULT branch and reach the window update, where iucv_send_ctrl()
sizes the skb through the cleared hs_dev. Send the update only while
the socket is connected.

Tested on s390x under QEMU TCG with a patched QEMU: an emulated
HiperSockets adapter under the real qeth and qdio drivers. An SHT read
with frames waiting in backlog_skb_q returns EOF instead of -EFAULT, and
a read after the close over the window attempts no window update.

Fixes: 82492a355fac ("af_iucv: add shutdown for HS transport")
Fixes: 800c5eb7b5eb ("af_iucv: change net_device handling for HS transport")
Reported-by: Sashiko <netdev-bot+sashiko@kernel.org>
Closes: https://lore.kernel.org/netdev/179183431327.434549.446348148837850426@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v2: the window update goes out only while the socket is connected. The
AI review of v1 found that, once empty frames are not counted, a read
from sk_receive_queue after the close over the window skips the -EFAULT
branch and reaches iucv_send_ctrl() with hs_dev NULL:
LL_RESERVED_SPACE() reads through the NULL pointer (lowcore on s390: no
fault on a default boot, an oops with a relocated lowcore),
afiucv_hs_send() fails with -ENODEV and recvmsg() moves the closed
socket to IUCV_DISCONN. Repro on the same machine with SO_MSGLIMIT 2 and
SO_RCVBUF 6144: the peer sends 1 byte twice (the window), then from a
tap on its interface one data frame over the window, its SHUT_WR for the
SHT, and 16 more data frames behind it, so that no read sends an update
before the close (a Linux peer blocks at the window; the overrun is
injected). The third read closes the socket, the fourth returns
-ECONNRESET, the fifth consumes the SHT, the sixth takes a data frame
behind it and closes again. On v1 the probe counts one iucv_send_ctrl()
entry after the close and the adapter sees no frame leave; with this
version there is none. The reads are the same on both (1, 1, -EFAULT,
-ECONNRESET, 0, -EFAULT), IOW nothing shows from user space on a default
boot; net without the patch reads -EFAULT at the SHT with its WARN.

Also in v2, the commit message now says "a zero-length data frame that
reaches recvmsg()": a zero-length send with a piggybacked credit is a
window update to afiucv_hs_rcv() and never reaches recvmsg(), so the
sentence did not hold for it.

Reproduced on s390x under QEMU TCG (net f4377bf85887 and net-next
8df0638138d3, same result) with an emulated HiperSockets adapter under
the real qeth and qdio drivers: the reader has SO_MSGLIMIT 2 and
SO_RCVBUF 2304 (the minimum); the peer sends 1 byte twice, which fills
the window, then calls shutdown(SHUT_WR) 32 times, one SHT each. The
reader starts once everything has arrived; the receive queue took two
frames, the rest wait in backlog_skb_q, and with that queue non-empty
recvmsg() sends no window update, so the first SHT read counts 3
against a limit of 2: WARN and -EFAULT without the patch, EOF with it.
With one SHT instead of 32 the queue drains, the update goes out after
the two reads, and both kernels read EOF.

Without the patch, the surplus a window update carries for a counted
SHT is credit the peer never spent, and it takes the peer's msg_sent
below zero; harmless, since a peer that sent an SHT has SEND_SHUTDOWN
and its sendmsg() returns -EPIPE. With the patch no SHT is counted, so
no update carries that surplus.

A HiperSockets socket stays in IUCV_BOUND from its SYN until the SYN|ACK
arrives, and SO_MSGLIMIT is accepted in that state, so msglimit can drop
below the window the SYN advertised. Hidayath's test for that [1],
written for the per-socket backlog budget of the socket-locking rework
[2], passes on bare net-next, where the update after every read keeps
msg_recv low; measured here with SO_RCVBUF at its minimum, so that
frames wait in backlog_skb_q, the same conforming peer reaches this
check (net-next: WARN and -EFAULT after two of eight messages; with this
patch -EFAULT without the WARN). The rework keeps the advertised window
in a field of its own for that, and this patch only takes the WARN out
of its way. That iucv_sock_shutdown() sends an SHT on every call is a
sender-side oddity left alone here; the receiver has to cope with any
peer.

[1] https://lore.kernel.org/netdev/51a4a40a-f34b-4d41-b049-69aa53f4dcf5@linux.ibm.com/
[2] https://lore.kernel.org/netdev/20261022-b4-disp-1d26b87a-v2-0-7ee1264d812a@proton.me/
---
 net/iucv/af_iucv.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 5bab6aea910e..266563fb0e0c 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1325,10 +1325,13 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		}
 
 		consume_skb(skb);
-		if (iucv->transport == AF_IUCV_TRANS_HIPER) {
+		/* The peer counts only the data it sends against its window;
+		 * the SHT's empty frame is not among them. Over the window,
+		 * close without a WARN: a peer reaches this from the network.
+		 */
+		if (iucv->transport == AF_IUCV_TRANS_HIPER && rlen) {
 			atomic_inc(&iucv->msg_recv);
 			if (atomic_read(&iucv->msg_recv) > iucv->msglimit) {
-				WARN_ON(1);
 				iucv_sock_close(sk);
 				return -EFAULT;
 			}
@@ -1357,7 +1360,11 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		}
 		spin_unlock_bh(&iucv->message_q.lock);
 
-		if (send_win) {
+		/* iucv_sock_close() clears hs_dev but leaves sk_receive_queue
+		 * readable: a frame read after the over-window close must not
+		 * send a window update.
+		 */
+		if (send_win && sk->sk_state == IUCV_CONNECTED) {
 			err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
 			if (err) {
 				sk->sk_state = IUCV_DISCONN;

-- 
2.56.0

Re: [PATCH net v2 2/2] net/iucv: do not count the SHT frame against the receive window

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> iucv_sock_recvmsg() counts every HiperSockets frame it consumes in
> msg_recv, the SHT's empty one included, while the peer counts only what
> sendmsg() sends against the window it was granted; the SHT goes out
> through iucv_send_ctrl() uncounted, one per shutdown(SHUT_WR) call. The
> next window update zeroes msg_recv and takes the surplus with it, but
> recvmsg() sends none while frames wait in backlog_skb_q, and an SHT read
> in that state pushes msg_recv past msglimit: WARN_ON(1) fires, the socket
> is closed and the reader gets -EFAULT instead of EOF. A peer that sends
> past its window reaches the same WARN from the network; with
> panic_on_warn that is a remote panic. The SHT frame dates from
> 82492a355fac, the WARN and the close from 800c5eb7b5eb.
> 
> The SHT reaches recvmsg() as a zero-length frame, like a zero-length
> [ ... ]
> Fixes: 800c5eb7b5eb ("af_iucv: change net_device handling for HS transport")
> Reported-by: Sashiko <netdev-bot+sashiko@kernel.org>
> Closes: https://lore.kernel.org/netdev/179183431327.434549.446348148837850426@kernel.org/
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-b4-disp-dd3e4ebb-v2-0-3747658d59bd@proton.me?part=2