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

[PATCH net 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 f4377bf85887. The same two patches apply on net-next
8df0638138d3, where the test set ran, and on mainline (7.3-rc7); the
SHT case below ran on this base as well. Tested on an s390x guest
under QEMU TCG with KASAN and lockdep on. QEMU has no HiperSockets
adapter and its s390x CPU emulation lacked things this needs: the
adapter is one I wrote, under the real qeth and qdio drivers, the CPU
carries my patches, 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.

[1] https://lore.kernel.org/netdev/20261011-b4-disp-410518f4-v1-1-753bf3258292@proton.me/
[2] https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/

---
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 | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)
---
base-commit: f4377bf85887f57b59abb5479f7660a81ffe4449
change-id: 20261022-b4-disp-b35a9153-ad04dc76713a

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

[PATCH net 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. Nothing user-visible is known from it: 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 listener's child's, which 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.

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 now on a new, 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 db261ecd19af..501e758c5e68 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -447,9 +447,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() */
@@ -630,7 +637,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;
@@ -1912,9 +1919,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;
@@ -2087,7 +2099,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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (3) · Medium: 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] In afiucv_hs_callback_syn(), the child now takes a reference
  on skb->dev every time, instead of copying the listener's hs_dev.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  afiucv_netdev_event() walks iucv_sk_list with sk_for_each() and holds…
- [High] This problem wasn't introduced by this patch, but
  afiucv_hs_callback_syn() leaks the child socket it allocates on every…
- [High] This problem wasn't introduced by this patch, but the SYN
  callback can publish a child into a listener's accept_q while that…

--

net/iucv: clear hs_dev before dropping its reference

This patch changes iucv_sock_close() so that iucv->hs_dev is cleared
before dev_put(). It also marks the racing hs_dev and sk_bound_dev_if
accesses with READ_ONCE()/WRITE_ONCE(). Finally, afiucv_hs_callback_syn()
now takes the child's device reference and sk_bound_dev_if 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 db261ecd19af1..501e758c5e68a 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -447,9 +447,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: High]
This is a pre-existing issue, but afiucv_netdev_event() also reads this
field, and it holds neither iucv_sk_list.lock nor the socket lock:

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

iucv_sock_close() runs under lock_sock() only. Can it move the socket out
of IUCV_CONNECTED and run this clear after the notifier's check but before
its call to iucv_send_ctrl()?

iucv_send_ctrl() reads hs_dev again and uses it without a NULL check:

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

The list walk itself also looks unprotected. iucv_sock_link() and
iucv_sock_unlink() change the list under write_lock_bh(&iucv_sk_list.lock),
and rtnl does not exclude them.

Could a concurrent close() followed by iucv_sock_kill() unlink and free
the node the notifier is on?

Once this patch is applied, the notifier is the only unlocked,
unannotated reader of hs_dev left. The notes below the commit message
leave it to Nagamani PV's "net/iucv: fix races in afiucv_netdev_event()".

[ ... ]

> @@ -1912,9 +1919,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 leak a reference on skb->dev when the SYN races with close() of
the listener?

The callback holds only bh_lock_sock(sk). That does not exclude
iucv_sock_close() of the listener, which holds lock_sock(), and the
callback never checks sock_owned_by_user(). This sequence looks possible:

CPU1 (softirq)
afiucv_hs_callback_syn()
  bh_lock_sock(sk);
  sk->sk_state == IUCV_LISTEN, check passes

CPU2 (process context)
iucv_sock_close(listener)
  iucv_sock_cleanup_listen()
    drains accept_q
  WRITE_ONCE(iucv->hs_dev, NULL);
  dev_put(dev);

CPU1
  dev_hold(skb->dev);
  WRITE_ONCE(niucv->hs_dev, skb->dev);
  dev_queue_xmit(skb);
  iucv_accept_enqueue(sk, nsk);
  nsk->sk_state = IUCV_CONNECTED;

The child then sits in IUCV_CONNECTED on the accept_q of a listener that
has already been drained. Nothing accepts, closes or kills it.

Before this patch, the callback read the listener's hs_dev as NULL in
this interleaving, so dev_hold(NULL) did nothing and only the struct sock
leaked. Now the child holds the ingress device.

In the other interleaving, where hs_dev was read before the clear, the
old code already leaked the listener's device reference. This change
extends the device leak to the case that used to leak only the sock.

afiucv_netdev_event() does nothing on NETDEV_UNREGISTER. On
NETDEV_GOING_DOWN it only moves the child to IUCV_DISCONN.

Would unregister_netdevice() of that HiperSockets device then wait
forever in netdev_wait_allrefs()?

The notes below the commit message mention this orphaned child, and
that it now holds the device the frame arrived on.

>  	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 does
afiucv_hs_callback_syn() leak the child socket nsk on every refused SYN?

iucv_sock_alloc() runs before any of the checks. It clears SOCK_ZAPPED
and links nsk into iucv_sk_list:

net/iucv/af_iucv.c:iucv_sock_alloc() {
    ...
	sock_reset_flag(sk, SOCK_ZAPPED);

	sk->sk_protocol = proto;
	sk->sk_state	= IUCV_OPEN;

	iucv_sock_link(&iucv_sk_list, sk);
    ...
}

Two branches call iucv_sock_kill(nsk): the refusal branch (listener not
in IUCV_LISTEN, or sk_acceptq_is_full()) and the dev_queue_xmit() failure
branch. iucv_sock_kill() returns at once for a socket that is not zapped:

net/iucv/af_iucv.c:iucv_sock_kill() {
	if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket)
		return;
    ...
}

So nsk is never unlinked or freed. A HiperSockets peer can keep sending
SYNs to a bound socket, or to a listener with a full backlog. Each leaked
entry also makes the sk_for_each() walk in afiucv_hs_rcv() longer.

On the dev_queue_xmit() failure branch, the leaked child has already done
dev_hold(skb->dev) above.

Can that reference also block unregister_netdevice() of the ingress
device forever?

The notes below the commit message say Hidayath Khan reported this in
August and that his fix is pending.

[Severity: High]
This is a pre-existing issue, but can the child be published into the
listener's accept_q while the listener is being torn down, and then be
handled before it is fully set up?

The callback holds only bh_lock_sock(sk), while iucv_sock_close() holds
lock_sock(sk). iucv_accept_enqueue() makes the child visible before it
sets the child's parent:

net/iucv/af_iucv.c:iucv_accept_enqueue() {
    ...
	spin_lock_irqsave(&par->accept_q_lock, flags);
	list_add_tail(&iucv_sk(sk)->accept_q, &par->accept_q);
	spin_unlock_irqrestore(&par->accept_q_lock, flags);
	iucv_sk(sk)->parent = parent;
	sk_acceptq_added(parent);
}

iucv_sock_cleanup_listen() calls iucv_accept_dequeue(parent, NULL). That
function walks accept_q without accept_q_lock and, with newsock == NULL,
takes any child whatever its state. It then calls iucv_accept_unlink():

net/iucv/af_iucv.c:iucv_accept_unlink() {
    ...
	struct iucv_sock *par = iucv_sk(iucv_sk(sk)->parent);

	spin_lock_irqsave(&par->accept_q_lock, flags);
    ...
}

sk_alloc() zeroes the sock, so parent is still NULL if the store in
iucv_accept_enqueue() has not happened yet.

Is that a NULL pointer dereference?

If the parent store has happened, cleanup goes on to iucv_sock_close()
and iucv_sock_kill() the child, which drops both references. The callback
then still does:

		iucv_accept_enqueue(sk, nsk);
		nsk->sk_state = IUCV_CONNECTED;

Would the write to nsk->sk_state then be a use-after-free?

The commit message leaves the case of a SYN accepted while the listener
closes to the socket-locking rework.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-b4-disp-b35a9153-v1-0-d74645876195%40proton.me

[PATCH net 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 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.

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>
---
Reproduced on s390x under QEMU TCG (net-next 8df0638138d3 and net
f4377bf85887, 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] runs
against the rework's backlog budget and passes on 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/
---
 net/iucv/af_iucv.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 501e758c5e68..a5142081bb6a 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1317,10 +1317,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;
 			}

-- 
2.56.0

Re: [PATCH net 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: 1 (1) · Medium: 0 (3) · 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).

- [High] The patch adds `&& rlen` to the counting branch in
  iucv_sock_recvmsg() (net/iucv/af_iucv.c:1324).

Pre-existing issues:
- [High] A peer can grow iucv->backlog_skb_q without limit.
- [Medium] A conforming HiperSockets peer can still end up in the
  `msg_recv > msglimit` close in iucv_sock_recvmsg().
- [Medium] On HiperSockets, a zero-length sendmsg() leaks one slot of the
  peer's window when a receive credit is piggybacked on it.
- [Medium] The classic IUCV transport still has a WARN that a peer can
  trigger.
- [Low] Two unsynchronized writes to sk->sk_shutdown can lose
  RCV_SHUTDOWN.

--

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

This patch changes iucv_sock_recvmsg() so that zero-length HiperSockets
frames, such as the SHT frame sent by shutdown(SHUT_WR), no longer count
against msg_recv. It also removes the WARN_ON(1) from the over-window
path, which still closes the socket and returns -EFAULT.

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

> A zero-length data frame 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.

[Severity: Medium]
This is a pre-existing issue, but does a zero-length data frame always
arrive at the receiver as an empty data frame?

iucv_sock_sendmsg() has no zero-length guard, and it counts the send
before the frame goes out:

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

With flags == 0, afiucv_hs_send() piggybacks any pending credit:

    if (flags == AF_IUCV_FLAG_WIN || !flags) {
            confirm_recv = atomic_xchg(&iucv->msg_recv, 0);
            phs_hdr->window = confirm_recv;
            if (confirm_recv)
                    phs_hdr->flags = phs_hdr->flags | AF_IUCV_FLAG_WIN;

On the receiving side, afiucv_hs_rcv() then handles it as a pure window
update:

    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 afiucv_hs_callback_rx() or recvmsg(). It does not
set RCV_SHUTDOWN, the receiver never credits it back, and the sender's
msg_sent stays one too high.

If zero-length sends repeat while msg_recv is non-zero, they use up
msglimit_peer. Eventually sendmsg() blocks in iucv_sock_wait() or returns
-EAGAIN.

This patch does not touch that path. The sentence quoted above, though,
does not hold when a credit is piggybacked.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 501e758c5e68a..a5142081bb6a6 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -1317,10 +1317,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: High]
Can an empty frame read after the over-window close reach
iucv_send_ctrl() while iucv->hs_dev is NULL?

When a data frame pushes msg_recv over msglimit, iucv_sock_close() runs
and clears hs_dev:

iucv_sock_close()
    ...
    skb_queue_purge(&iucv->send_skb_q);
    skb_queue_purge(&iucv->backlog_skb_q);
    ...
    WRITE_ONCE(iucv->hs_dev, NULL);

It does not purge sk_receive_queue, and it does not reset msg_recv.

The next recvmsg() returns -ECONNRESET through sock_error() in
__skb_try_recv_datagram(), and that call also clears sk_err. The
recvmsg() after it dequeues whatever is still in sk_receive_queue, even
though the socket is IUCV_CLOSED.

If that frame is empty (an SHT, for example), rlen == 0 and the new gate
skips the over-window return. backlog_skb_q is empty because close
purged it, and msg_recv is still above msglimit, so this code runs:

    if (iucv->transport == AF_IUCV_TRANS_HIPER &&
        atomic_read(&iucv->msg_recv) >=
                    iucv->msglimit / 2)
            send_win = true;
    ...
    if (send_win) {
            err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);

iucv_send_ctrl() then computes:

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

This reads hard_header_len and needed_headroom through a NULL pointer,
before afiucv_hs_send() gets a chance to check the device.

For example, say the receiver has msglimit 2 and a small SO_RCVBUF, and
the peer sends d1, d2, d3 and then SHTs:

1. Reading d1 and d2 moves d3 and S1 into sk_receive_queue.
2. Reading d3 closes the socket.
3. The next read returns -ECONNRESET.
4. The read after that consumes S1.

Before this patch, every frame read after the close went through the
msg_recv > msglimit branch again and returned -EFAULT. send_win could not
be reached on this path.

On s390 with relocate_lowcore this would oops. On a default boot, blen
would be computed from lowcore bytes and afiucv_hs_send() would fail with
-ENODEV. recvmsg() would then move the IUCV_CLOSED socket back to
IUCV_DISCONN.

Could this return early once the socket is closed, or skip the window
update after a close?

>  			atomic_inc(&iucv->msg_recv);
>  			if (atomic_read(&iucv->msg_recv) > iucv->msglimit) {
> -				WARN_ON(1);

[Severity: Medium]
This is a pre-existing issue, but the classic IUCV transport still has a
WARN that a peer can trigger, of the same kind as the one removed here.

alloc_iucv_recv_skb() does:

    WARN_ONCE(!skb,
              "alloc of recv iucv skb len=%lu failed with errcode=%d\n",
              len, err);

len comes from the peer's message via iucv_msg_length(), called from
iucv_callback_rx() or iucv_process_message_q(). iucv_callback_rx() checks
it only against sk_rcvbuf, and iucv_process_message_q() does not check it
at all.

A message larger than MAX_SKB_FRAGS * PAGE_SIZE plus the linear part
makes alloc_skb_with_frags() fail with -EMSGSIZE. A GFP_ATOMIC | GFP_DMA
allocation failure under memory pressure has the same effect.

With panic_on_warn, a peer can turn this into a panic. Should this
WARN_ONCE() be removed as well?

>  				iucv_sock_close(sk);
>  				return -EFAULT;

[Severity: High]
This isn't a bug introduced by this patch, but this read-time check is
the only window enforcement on the receive side. Can a peer that ignores
its window grow iucv->backlog_skb_q without limit?

afiucv_hs_callback_rx() queues frames like this:

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

Once sk_rcvbuf is full, or once the backlog holds any frames, every later
data, SHT or WIN-with-payload frame goes onto backlog_skb_q. That queue
has no length or memory limit, and these skbs are not charged to
sk_rmem_alloc.

msg_recv is only incremented in iucv_sock_recvmsg(). If the reader is
idle or slow, kernel memory keeps growing until the reader runs or the
system runs out of memory.

[Severity: Medium]
This is a pre-existing issue, but can a conforming peer still reach this
close?

The new comment says "a peer reaches this from the network". However,
the check compares against the current iucv->msglimit, while the peer is
bound by the window advertised in the SYN. afiucv_hs_send() fills that
window in as:

    phs_hdr->window = iucv->msglimit;

iucv_sock_connect() sends the SYN and then waits in IUCV_BOUND. A
blocking connect drops the socket lock inside iucv_sock_wait(), and a
non-blocking connect returns -EAGAIN. In either case
iucv_sock_setsockopt() still accepts SO_MSGLIMIT in that state:

    case IUCV_BOUND:
            if (val < 1 || val > U16_MAX)
                    rc = -EINVAL;
            else
                    iucv->msglimit = val;

So msglimit can be lowered below the window that was already advertised.

While frames wait in backlog_skb_q, recvmsg() sends no WIN, so msg_recv
climbs past the lowered limit. A peer that stayed inside its window then
gets closed: the reader sees -EFAULT and the consumed frame is lost.

The notes below the cut line mention this case and defer it to a rework
that stores the advertised window separately. Could the commit message
and the new comment also say that a conforming peer can still reach this
path?

>  			}

[Severity: Low]
This is a pre-existing issue, but can the RCV_SHUTDOWN set by the
empty-frame read be lost?

iucv_sock_recvmsg() runs without lock_sock() and does a plain
read-modify-write:

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

iucv_send_ctrl() saves and restores the whole mask. It can be called from
a repeated shutdown(SHUT_WR) under lock_sock(), or from a concurrent
recvmsg() sending WIN:

    if (sk->sk_shutdown & SEND_SHUTDOWN) {
            /* controlling flags should be sent anyway */
            shutdown = sk->sk_shutdown;
            sk->sk_shutdown &= RCV_SHUTDOWN;
    }
    ...
    if (shutdown)
            sk->sk_shutdown = shutdown;

If the SHT read sets RCV_SHUTDOWN between the save and the restore, the
restore clears it again. The current read still returns 0, but later
reads may block or return -EAGAIN instead of EOF. afiucv_hs_callback_rx()
would also start accepting data frames again.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-b4-disp-b35a9153-v1-0-d74645876195%40proton.me