[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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 2 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The clear-before-put reorder in __iucv_sock_close() does not
  give the guarantee its new comments describe.
- [Medium] afiucv_hs_callback_syn() now always takes a device reference
  from skb->dev (line 1936) instead of copying the listener's hs_dev.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  afiucv_hs_callback_syn()'s error paths leak the child socket, and on…
- [High] This problem wasn't introduced by this patch, but
  afiucv_netdev_event() can dereference a NULL hs_dev.
- [Medium] afiucv_hs_callback_syn() checks the raw dev_queue_xmit()
  result with `if (!err)` (lines 1942-1948).

--

net/iucv: clear hs_dev before dropping its reference

This patch changes __iucv_sock_close() to clear iucv->hs_dev and
sk_bound_dev_if before calling dev_put(). It marks the accesses that race
with the afiucv_hs_rcv() lookup with READ_ONCE()/WRITE_ONCE(). It also
changes afiucv_hs_callback_syn() to take the child's device and ifindex
from skb->dev instead of from the listener's fields.

Closes: https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 5ba875798aa41..5bab6aea910e1 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);

[Severity: Medium]
Does this ordering give the guarantee the comment describes? The clear
and the dev_put() run under lock_sock(sk) only. afiucv_hs_rcv() reads
hs_dev under read_lock(&iucv_sk_list.lock), which this path never takes,
so the two sides don't exclude each other:

CPU1 (softirq)                        CPU2 (process context)
afiucv_hs_rcv()
  read_lock(&iucv_sk_list.lock)
  READ_ONCE(iucv_sk(sk)->hs_dev) == dev
                                      __iucv_sock_close()
                                        WRITE_ONCE(iucv->hs_dev, NULL);
                                        dev_put(dev);
  memcmp() name checks match
  sock_hold(sk)

In that case the lookup still matches the socket after the device
reference is gone. That is the situation the commit message describes
("Between the two it compares a pointer the socket no longer holds"),
with a smaller window.

The new comment in afiucv_hs_rcv() makes the same claim:

	/* 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)

Under LKMM, dev_put() is also not release-ordered after the WRITE_ONCE().
The KCSAN report goes away because of the READ_ONCE()/WRITE_ONCE()
marking. The reorder doesn't add a cross-CPU guarantee.

There's no memory-safety effect today, since the lookup only compares the
value against the ingress dev. Could these comments lead a later change to
dereference hs_dev from the lookup, which would then be a use-after-free?

If the guarantee is wanted, would it work to do the clear under
write_lock_bh(&iucv_sk_list.lock), the way iucv_sock_bind() stores
hs_dev, and call dev_put() after the unlock? Otherwise, could the comments
say that the change only annotates the race for KCSAN?

[Severity: High]
This is a pre-existing issue, not one introduced by this patch, but can
this NULL store race with afiucv_netdev_event()?

On NETDEV_GOING_DOWN the notifier walks iucv_sk_list holding only RTNL. It
takes neither the socket lock nor iucv_sk_list.lock, and it holds no
socket reference:

afiucv_netdev_event() {
	...
		if ((iucv->hs_dev == event_dev) &&
		    (sk->sk_state == IUCV_CONNECTED)) {
			if (event == NETDEV_GOING_DOWN)
				iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
	...
}

iucv_send_ctrl() loads hs_dev again and dereferences it before any NULL
check:

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

Suppose close() of a connected HIPER socket runs between the notifier's
check and that second load. It sends its FIN, goes through
DISCONN/CLOSING/CLOSED and iucv_sever_path(), then reaches the
WRITE_ONCE(iucv->hs_dev, NULL) above. Wouldn't LL_RESERVED_SPACE() then
dereference NULL? The !skb->dev check in afiucv_hs_send() comes too late
to help.

This can be triggered by a HiperSockets device going down (ip link set
down, or a qeth recovery or offline event) while a user closes a connected
socket on it.

The walk holds no socket reference. Can a socket that iucv_sock_kill()
unlinks and frees during the walk also be used after it is freed?

Neither the clear-before-put order nor marking the notifier's compare
serializes this check-then-use sequence. The cover letter leaves the
notifier to Nagamani PV's pending patch.

[ ... ]

> @@ -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);

[Severity: Medium]
Can this unconditional dev_hold(skb->dev) pin the HiperSockets device when
the child ends up orphaned?

afiucv_hs_callback_syn() only takes bh_lock_sock(sk) and doesn't check
sock_owned_by_user(). It can therefore pass its sk_state != IUCV_LISTEN
check while __iucv_sock_close() holds lock_sock() on the listener.
iucv_sock_cleanup_listen() drains accept_q only once:

	while ((sk = iucv_accept_dequeue(parent, NULL))) {
		...
	}

	parent->sk_state = IUCV_CLOSED;

If iucv_accept_enqueue(sk, nsk) runs after that drain, the child is never
accepted, closed or killed. Its sk_socket is NULL and SOCK_ZAPPED is never
set, so it never reaches the dev_put() in __iucv_sock_close().

afiucv_netdev_event() ignores NETDEV_UNREGISTER. Wouldn't unregistering
the device then wait forever for this reference ("waiting for <dev> to
become free")?

The orphan race and the pin when the read comes before the clear both
predate this patch, as the cover letter notes. Before this patch, though,
an orphan that read iucv->hs_dev after close() cleared it got NULL, and
dev_hold(NULL) did nothing. With skb->dev that case now holds a reference
too.

Would it help to refuse the SYN when the listener's hs_dev has already
been cleared?

>  	afiucv_swap_src_dest(skb);
>  	trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_ACK;
>  	trans_hdr->window = niucv->msglimit;

[Severity: High]
This isn't a bug introduced by this patch, but do the error paths in
afiucv_hs_callback_syn() leak nsk?

iucv_sock_alloc() links nsk into iucv_sk_list and clears SOCK_ZAPPED. Two
error paths call iucv_sock_kill(nsk):

  - the rejection path (listener not in IUCV_LISTEN, or accept queue
    full)
  - the dev_queue_xmit() failure path

On a never-closed child, iucv_sock_kill() returns at once:

static void iucv_sock_kill(struct sock *sk)
{
	if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket)
		return;
	...
}

Only __iucv_sock_close() sets SOCK_ZAPPED, so nsk is never unlinked or
freed. On the transmit-failure path the child also keeps the reference
taken by dev_hold(skb->dev) above. That reference then blocks
unregistration of the device.

nsk is allocated before the state and backlog checks. Can a peer on the
HiperSockets LAN send matching SYN frames to a bound but non-listening
socket, or to a listener with a full backlog, and leak one socket per
frame?

The leaked sockets stay on iucv_sk_list, so every afiucv_hs_rcv() lookup
and every bind() name scan gets slower.

The cover letter says Hidayath Khan flagged this in August and that his
fix is pending.

[Severity: Medium]
This is also a pre-existing issue, not one introduced by this patch.
Should the dev_queue_xmit() result below go through net_xmit_eval()?

afiucv_hs_callback_syn() {
	...
	err = dev_queue_xmit(skb);
	if (!err) {
		iucv_accept_enqueue(sk, nsk);
		nsk->sk_state = IUCV_CONNECTED;
	...
}

__dev_xmit_skb() returns the qdisc enqueue result unchanged.
fq_codel_enqueue() can return NET_XMIT_CN after it drops packets from the
head of the same flow, so a SYN-ACK queued at the tail can still be sent.

In that case the peer considers itself connected. Locally, the child is
never enqueued and stays in IUCV_OPEN, and afiucv_hs_callback_rx() drops
the peer's data.

afiucv_hs_send() in the same file already uses net_xmit_eval().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028-b4-disp-dd3e4ebb-v2-0-3747658d59bd%40proton.me

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The window update in iucv_sock_recvmsg() is now gated by
  `send_win && sk->sk_state == IUCV_CONNECTED`.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but frames a
  HiperSockets peer sends that do not fit in sk_receive_queue are…
- [Medium] Mismatch between the advertised window and the enforced limit.
- [Low] Mismatch in HiperSockets window accounting in the other direction
  (sender over-counts).

--

net/iucv: do not count the SHT frame against the receive window

This patch stops iucv_sock_recvmsg() from counting zero-length HiperSockets
frames, such as the SHT, in msg_recv. It also drops the WARN_ON(1) on the
over-window close. The window update is now sent only while sk_state is
IUCV_CONNECTED, so no update goes out through the hs_dev that
iucv_sock_close() cleared.

Closes: https://lore.kernel.org/netdev/179183431327.434549.446348148837850426@kernel.org/

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 5bab6aea910e1..266563fb0e0cb 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) {

[Severity: Low]
This is a pre-existing issue, but the sender side has the opposite
mismatch. iucv_sock_sendmsg() increments msg_sent for a zero-length send
as well:

    atomic_inc(&iucv->msg_sent);
    err = afiucv_hs_send(&txmsg, sk, skb, 0);

If the sender has receive credit pending (msg_recv > 0), afiucv_hs_send()
sets AF_IUCV_FLAG_WIN on that frame. afiucv_hs_rcv() then consumes it as
a credit only:

    case (AF_IUCV_FLAG_WIN):
        err = afiucv_hs_callback_win(sk, skb);
        if (skb->len == sizeof(struct af_iucv_trans_hdr)) {
            consume_skb(skb);
            break;
        }

The frame never reaches recvmsg(), so the receiver never counts it and
never credits it back.

Does the sender lose one slot of msglimit_peer on each such send? If so,
iucv_below_msglim() eventually stays false and its own sendmsg() blocks.

Only the local sender's socket is affected. The v2 notes already say
that this frame "is a window update to afiucv_hs_rcv() and never reaches
recvmsg()", and this patch does not touch that path.

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

[Severity: High]
This isn't a bug introduced by this patch, but the new comment says a
peer reaches this from the network. Is the window enforced anywhere on
the receive side before this read-time check?

Once the receive queue is full, afiucv_hs_callback_rx() appends to
backlog_skb_q with no length or memory limit:

    if (skb_queue_empty(&iucv->backlog_skb_q)) {
        if (__sock_queue_rcv_skb(sk, skb))
            /* handle rcv queue full */
            skb_queue_tail(&iucv->backlog_skb_q, skb);
    } else
        skb_queue_tail(&iucv_sk(sk)->backlog_skb_q, skb);

The backlog skbs are not charged to sk_rmem_alloc. msglimit is checked
only here, when the local application reads, and backlog_skb_q is only
purged by the close that follows.

Can a connected peer that ignores its window keep growing backlog_skb_q
while the application is not reading, until the system runs out of
memory?

The commit message leaves a per-socket backlog budget to the separate
socket-locking rework.

[Severity: Medium]
This is also a pre-existing issue. This check uses the current
iucv->msglimit, but the peer enforces the window it got in the SYN:

afiucv_hs_send():
    if (flags == AF_IUCV_FLAG_SYN)
        phs_hdr->window = iucv->msglimit;

afiucv_hs_callback_syn():
    niucv->msglimit_peer = trans_hdr->window;

iucv_sock_setsockopt(SO_MSGLIMIT) accepts IUCV_BOUND. A connecting socket
stays in that state until the SYN|ACK arrives, so msglimit can be lowered
after the SYN has gone out.

Can a peer that stays within its granted window then push msg_recv past
the lowered msglimit while frames wait in backlog_skb_q? That would close
the socket with -EFAULT.

Removing the WARN makes this less severe. The commit message says the
rework "keeps the advertised window in a field of its own for that".

> @@ -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);

[Severity: Medium]
Is this state check serialized against the over-window close?

iucv_sock_recvmsg() never takes lock_sock(), and message_q.lock is
dropped just above. __iucv_sock_close() runs under lock_sock_nested()
from iucv_sock_close(). After the state changes, it clears hs_dev:

    WRITE_ONCE(iucv->hs_dev, NULL);
    WRITE_ONCE(sk->sk_bound_dev_if, 0);
    dev_put(dev);

Consider two threads reading the same HiperSockets socket while msg_recv
goes over msglimit (a peer sending past its window, or a lowered
SO_MSGLIMIT):

reader A                            reader B
iucv_sock_recvmsg()
  send_win = true
  sk->sk_state == IUCV_CONNECTED
                                    iucv_sock_recvmsg()
                                      msg_recv > msglimit
                                      iucv_sock_close()
                                        __iucv_sock_close()
                                          hs_dev = NULL
  iucv_send_ctrl()
    LL_RESERVED_SPACE(iucv->hs_dev)

Wouldn't reader A still dereference a NULL hs_dev in iucv_send_ctrl()?
Then afiucv_hs_send() returns -ENODEV, and recvmsg() moves the CLOSED
socket back to IUCV_DISCONN.

The NULL check in afiucv_hs_send() reloads hs_dev. It also runs only
after LL_RESERVED_SPACE() has already dereferenced the pointer.

If A loads hs_dev just before B clears it, A also keeps using the
net_device after B's dev_put(), without holding a reference.

The same race existed before this patch, when the call was
unconditional. Here, sk_state is also read with a plain load.

Would taking lock_sock(sk) around the state test and iucv_send_ctrl()
close this? message_q.lock is already released at this point, and
__iucv_sock_close() already calls iucv_send_ctrl() under lock_sock.

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028-b4-disp-dd3e4ebb-v2-0-3747658d59bd%40proton.me