[PATCH RFC net v3 0/7] net/iucv: serialize the HiperSockets receive path with the socket lock

[PATCH RFC net v3 0/7] net/iucv: serialize the HiperSockets receive path with the socket lock

[Bryam Vargas via B4 Relay]

v3 of stage 1 of the socket-locking rework [1], cut down as Alexandra
asked in her review [2] of v2 [3]: on net, with nothing under it, and
without the iucv_send_ctrl() rewrite, the window update only while
connected and the credit reset. It also fixes the three items of the AI
review of v2 [4], except the window update after the FIN that the
second one ends on, which is under "Not in v3" below.

  1/7  a msglimit changed while the SYN is out no longer closes the
       socket on a peer that kept to its window, nor stalls that peer
  2/7  sendmsg() fails with -EPIPE when sending is shut down while it
       waits for the window
  3/7  recvmsg() under lock_sock()
  4/7  the frame dispatch and three state changes moved into functions
       of their own, no functional change
  5/7  the HiperSockets rx path under the socket lock: a control frame
       waits for the task that owns the socket
  6/7  shutdown(SHUT_RD) against a frame in flight
  7/7  EOF, not -ENOTCONN, for a reader parked when the peer closes

2/7 and 7/7 are fixes in their own right, here because v2 had them (7/7
was its 2/2). 2/7 applies to net by itself. 7/7 is written on top of
5/7: its two hunks are in functions that 3/7 and 4/7 add.

RFC because one thing is missing that I can't provide. Hidayath,
Alexandra: could one of you run v3 on z/VM the way Hidayath ran v1 [5]?
I depend on those numbers: my setup was tuned against that run, and v3
moves enough code to need one again before this is a PATCH. What I'd
compare, with net as the baseline (the numbers from [5] in parentheses,
on net-next unless marked):

- recvrecv (3917 backwards reads): 0 from 3/7 on
- sk_shutdown (565 of 7281 cycles): 0 from 3/7 on
- shut_rd (32 of 212): as net up to 4/7, 0 from 5/7 on. 5/7 leaves the
  window KCSAN reported on v1 [6], which shut_rd did not hit there and
  hits here only with a udelay() added to widen it; 6/7 closes it
- the blocked reader (-ENOTCONN in 13627 of 13627): EOF in all with 7/7
- rxclose (51 of 906): 0 with 7/7
- SO_MSGLIMIT lowered in IUCV_BOUND (1 message lost in 6297 cycles on
  v1; none on net-next, which has no budget): none lost at any step.
  The budget comes back with 5/7 and counts against the window of 1/7
- KCSAN: of the 29 reports on net-next, the 23 that name
  iucv_sock_shutdown() gone: 19 against recvmsg() and 2 against
  iucv_send_ctrl() with 3/7, 2 against afiucv_hs_rcv() with 6/7. The 6
  between afiucv_hs_rcv() and iucv_send_ctrl() may stay: v3 no longer
  rewrites that function, which still clears and restores SEND_SHUTDOWN
  around its allocation while the softirq tests RCV_SHUTDOWN in the same
  byte. Neither write changes the bit the softirq tests. The hs_dev
  report of the v1 run [6] is not one of the 29; 1/2 of the two fixes
  posted for net on 28 Oct [7] is for it, and is not in this series

My setup is QEMU TCG with an emulated HiperSockets adapter and CP, so
rates and real SMP come only from the hardware, and I have no KCSAN
build of it. Where the two disagree, the hardware is the reference.

Changes in v3 (details below the --- of each patch):

- on net main, with nothing under it. Of the two net fixes [7], 1/2
  applies on top as it is and 2/2 needs a rebase (Alexandra)
- split into seven, as listed above; the EOF in the receive wait went to
  7/7 (Alexandra)
- out: the iucv_send_ctrl() rewrite, the window update only while
  connected, the credit reset (Alexandra), and the purge of
  backlog_skb_q that v2 added to iucv_sock_destruct(), which is not
  about the lock and has no repro
- AI review of v2 [4]: a FIN older than the connection no longer
  disconnects it, and frames behind a recorded FIN are freed (both in
  5/7). A socket connected by a SYN|ACK it never asked for reads with
  msglimit as its window, as on net; v2 gave it a window of 0 (1/7).
  That net connects a socket on such a SYN|ACK is not changed by v3
- connect() applies a reply to its SYN that came in while it owned the
  socket. In v1 and v2 a non-blocking connect() returned -EAGAIN for a
  connection already made, or already refused
- found while reviewing v3 and fixed by it: a MSG_PEEK read that a
  second reader overtakes leaks its skb (3/7), and a SYN for a bound
  socket that does not listen leaks a child (5/7). The second is one of
  the refusal paths Hidayath flagged in August [8]; the others are
  untouched
- one forward declaration is left of v2's two, afiucv_hs_backlog_rcv(),
  which iucv_sock_alloc() installs. It could go if iucv_sock_create()
  installed it and a child copied it from its listener. But
  iucv_sock_alloc() also puts the socket on iucv_sk_list, and I'd rather
  not have one there without its backlog handler. Say if you prefer it
  the other way (Jakub on v1 [9])
- stable: "# 6.18.x" with two prerequisites: aa5ec571b93f for 3/7, and
  f38bce9af9e2 for 5/7, whose backlog queues an skb that has to be
  private. With each picked right before the patch that names it, the
  nine apply and af_iucv.o builds on 7.3.4 and 6.18.63 (built, not
  run). 6.12.y has no sk_drops_inc(), which 5/7 calls, so it and the
  older trees need a backport. Whether 4/7 to 6/7 are stable material
  at their size is for you to say; 1/7 to 3/7 do not need the rest, and
  7/7 as posted needs 3/7 to 5/7
- kernel-doc on every function added, comments in net style and short,
  the labels 3/7 adds are unlock and err_unlock, and each message says
  what is seen, when, and what it costs (Alexandra). The diffstat is
  548+/102- against v2's 444+/125-; 199 of the added lines are
  kernel-doc and comments

Not in v3, and what that costs: five of my 53 default tests (the set v2
was tested with [3]) fail on v3, as they do on net. With the first
three of the pieces dropped above applied on top again, all 53 pass.

- iucv_send_ctrl() still needs room in sk_sndbuf for every control
  frame. With the send memory used up, which on HiperSockets lasts until
  TX completion, shutdown(SHUT_WR) sends no SHT and the peer never reads
  EOF (two tests), and the FIN of NETDEV_GOING_DOWN does not go out
  (one)
- recvmsg() still sends a window update after the peer's FIN, a frame
  nobody uses (one); [7] 2/2 has the test for it
- a window update that finds no room leaves its credit, and the SHT read
  next is counted past the window: WARN_ON() and -EFAULT where EOF was
  due (one). [7] 2/2 ends that by not counting the SHT

net fails three more: two that 2/7 fixes and one that 7/7 fixes. The
dropped purge shows in no test: without it a data frame queued on
backlog_skb_q after __iucv_sock_close() emptied it is never freed, on
net as on v3. That one is by inspection, not reproduced.

Tested on s390x under QEMU TCG, KASAN + PROVE_LOCKING +
DEBUG_ATOMIC_SLEEP, on net main, against the emulated adapter under the
real qeth and qdio drivers and the emulated CP, as v2 was [3]. Two
vCPUs under -icount, which runs one at a time; the race tests also with
seeded short slices, and the two-reader test of 3/7 under MTTCG. No
KASAN or lockdep splat on net, at any step of the series or on the
kernels that follow. Of 20 kernels each with one piece of the series
taken out or changed (2 for 1/7, 3 for 2/7, 1 for 3/7, 10 for 5/7, 1
for 6/7, 3 for 7/7), 18 fail a test the series passes. The other two
show only with a delay added to hold their window open: the one without
6/7's spinlock and the one without the receive-queue test in 7/7's
wait. The numbers are below the --- of each patch. Logs on request.

Still open, as in v2: afiucv_hs_callback_txnotify() and the z/VM
callbacks change sk_state without checking whether a task owns the
socket, and so does afiucv_netdev_event(), whose fix Nagamani is holding
for this series [10]. backlog_skb_q stays unbounded for a peer that
ignores its window. A listener with a full accept queue still leaks the
child of the SYN it refuses (measured on every kernel, numbers under
5/7), and so does a SYN|ACK that fails to go out (by inspection) [8].

Seen while reviewing v3, in net as well and not touched here: on a z/VM
socket iucv_sock_shutdown() updates sk_shutdown under lock_sock() and
iucv_callback_shutdown() under bh_lock_sock(), so either can lose its
bit, and iucv_callback_rx() tests RCV_SHUTDOWN with no lock: 6/7 takes
the spinlock on the HiperSockets branch only. And
iucv_callback_connreq() answers a path request for a listener that
close() is emptying, the race 5/7 closes for a HiperSockets SYN.

[1] https://lore.kernel.org/all/20260724222917.134769-1-hexlabsecurity@proton.me/
[2] https://lore.kernel.org/netdev/2886dcb1-2741-4b29-86e6-dba3ccf2125a@linux.ibm.com/
[3] https://lore.kernel.org/netdev/20261022-b4-disp-1d26b87a-v2-0-7ee1264d812a@proton.me/
[4] https://lore.kernel.org/all/sashiko-outbox-183527@kernel.org/
[5] https://lore.kernel.org/netdev/51a4a40a-f34b-4d41-b049-69aa53f4dcf5@linux.ibm.com/
[6] https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/
[7] https://lore.kernel.org/netdev/20261028-b4-disp-dd3e4ebb-v2-0-3747658d59bd@proton.me/
[8] https://lore.kernel.org/netdev/5f88daf4-cd8e-4464-b132-ac45be205121@linux.ibm.com/
[9] https://lore.kernel.org/netdev/20261019183223.49a59d6f@kernel.org/
[10] https://lore.kernel.org/netdev/1d9d3cc1-24fb-40e0-a4c7-6fbfbcf4dc37@linux.ibm.com/

---
Bryam Vargas (7):
      net/iucv: keep the window given to the peer in a field of its own
      net/iucv: fail sendmsg() with EPIPE once sending is shut down
      net/iucv: run recvmsg() under the socket lock
      net/iucv: factor out the HiperSockets frame dispatch and state changes
      net/iucv: serialize HiperSockets receive with the socket lock
      net/iucv: set RCV_SHUTDOWN under the socket spinlock
      net/iucv: return EOF to a reader parked when the peer closes

 include/net/iucv/af_iucv.h |   6 +
 net/iucv/af_iucv.c         | 644 ++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 548 insertions(+), 102 deletions(-)
---
base-commit: 8373c4e8863ac5f5120d200527aca9880330f343
change-id: 20261110-b4-disp-2a6b4364-b8e24a7bdf60

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

[PATCH RFC net v3 1/7] net/iucv: keep the window given to the peer in a field of its own

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

A HiperSockets SYN carries msglimit as the receive window, and
SO_MSGLIMIT is accepted while the socket is IUCV_BOUND, which it is
until the SYN|ACK arrives. A msglimit changed in between leaves the peer
with the window the SYN carried, while recvmsg() counts the frames it
reads against the new value:

- Lowered, a peer that keeps to its window has the socket closed on it
  once the receive buffer is full and frames wait in backlog_skb_q,
  where no window update resets the count: WARN_ON(), -EFAULT, the rest
  of the stream lost.
- Raised far enough, the threshold of the window update lies above the
  window: the count never reaches it, the peer uses up its window and
  its writer waits for an update that only a data frame from us would
  carry.

Record the window where it goes out, in the SYN and in a listener's
SYN|ACK, and let recvmsg() count frames against it and take the
window-update threshold from it. A msglimit set after the SYN no longer
changes anything for that connection; getsockopt() still returns it. A
socket connected by a SYN|ACK without having sent a SYN keeps msglimit
as its window.

Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Fixes: 800c5eb7b5eb ("af_iucv: change net_device handling for HS transport")
Cc: stable@vger.kernel.org # 6.18.x
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
New in v3 as a patch of its own (Alexandra); v2 had it inside the lock
patch. It goes first because the backlog of 5/7 counts against this
window: on the live msglimit it would drop data without a trace, as in
Hidayath's SO_MSGLIMIT scenario on v1.

Both cases repro in the emulated setup, with the SYN|ACK held back in
the adapter while the limit changes:

- SO_MSGLIMIT 8 when the SYN goes out, lowered to 2, minimum SO_RCVBUF,
  the peer sends its 8 frames. net reads 2, then takes the WARN_ON() in
  iucv_sock_recvmsg() and returns -EFAULT; with this patch all 8 arrive.
  With the default SO_RCVBUF net delivers the 8 as well: a window update
  goes out after each read and resets the count.
- SO_MSGLIMIT 2 when the SYN goes out, raised to 8. The peer sends 2 and
  the socket reads both. On net no window update leaves and the peer's
  next send returns -EAGAIN; with this patch the update leaves after
  the first read and the send goes through. With the limit left at 2
  both kernels send the update.

The last sentence of the message is the third item of the AI review of
v2. net connects an IUCV_BOUND socket on any SYN|ACK, whether or not it
sent a SYN. v3 leaves that as it is; against v2 it changes what such a
socket reads afterwards. With a forged SYN|ACK and one byte for a socket
that never called connect(), net and this patch return the byte; with
the window left at 0, as in v2, the read takes the WARN_ON() and returns
-EFAULT.

With afiucv_hs_window() back on the live msglimit, the lowered case
loses data in the backlog of 5/7 and the raised one stalls as on net.
None of this shows in the 53 default tests: with this patch the same
eight fail as on net, with the same result.
---
 include/net/iucv/af_iucv.h |  1 +
 net/iucv/af_iucv.c         | 25 ++++++++++++++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/include/net/iucv/af_iucv.h b/include/net/iucv/af_iucv.h
index df85d19fbf84..6decbb1f34b3 100644
--- a/include/net/iucv/af_iucv.h
+++ b/include/net/iucv/af_iucv.h
@@ -130,6 +130,7 @@ struct iucv_sock {
 	u8			flags;
 	u16			msglimit;
 	u16			msglimit_peer;
+	u16			msglimit_adv;	/* window given to the peer */
 	atomic_t		skbs_in_xmit;
 	atomic_t		msg_sent;
 	atomic_t		msg_recv;
diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index ce13fe3c85b5..657c61161dbd 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -176,6 +176,20 @@ static inline int iucv_below_msglim(struct sock *sk)
 			(atomic_read(&iucv->pendings) <= 0));
 }
 
+/**
+ * afiucv_hs_window() - the window a HiperSockets socket gave its peer
+ * @iucv:	the socket
+ *
+ * The msglimit its SYN or SYN|ACK carried. A socket that sent neither, but
+ * was connected by a SYN|ACK, has msglimit as its window.
+ *
+ * Return: the frames the peer may send before it needs a window update.
+ */
+static u16 afiucv_hs_window(const struct iucv_sock *iucv)
+{
+	return iucv->msglimit_adv ?: iucv->msglimit;
+}
+
 /*
  * iucv_sock_wake_msglim() - Wake up thread waiting on msg limit
  */
@@ -208,8 +222,11 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
 	phs_hdr->magic = ETH_P_AF_IUCV;
 	phs_hdr->version = 1;
 	phs_hdr->flags = flags;
-	if (flags == AF_IUCV_FLAG_SYN)
+	if (flags == AF_IUCV_FLAG_SYN) {
 		phs_hdr->window = iucv->msglimit;
+		/* what the peer may send us, whatever msglimit becomes */
+		iucv->msglimit_adv = iucv->msglimit;
+	}
 	memcpy(phs_hdr->destUserID, iucv->dst_user_id, 8);
 	memcpy(phs_hdr->destAppName, iucv->dst_name, 8);
 	memcpy(phs_hdr->srcUserID, iucv->src_user_id, 8);
@@ -1320,7 +1337,8 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		consume_skb(skb);
 		if (iucv->transport == AF_IUCV_TRANS_HIPER) {
 			atomic_inc(&iucv->msg_recv);
-			if (atomic_read(&iucv->msg_recv) > iucv->msglimit) {
+			if (atomic_read(&iucv->msg_recv) >
+			    afiucv_hs_window(iucv)) {
 				WARN_ON(1);
 				iucv_sock_close(sk);
 				return -EFAULT;
@@ -1345,7 +1363,7 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 				iucv_process_message_q(sk);
 			if (iucv->transport == AF_IUCV_TRANS_HIPER &&
 			    atomic_read(&iucv->msg_recv) >=
-							iucv->msglimit / 2)
+						afiucv_hs_window(iucv) / 2)
 				send_win = true;
 		}
 		spin_unlock_bh(&iucv->message_q.lock);
@@ -1926,6 +1944,7 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
 	afiucv_swap_src_dest(skb);
 	trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_ACK;
 	trans_hdr->window = niucv->msglimit;
+	niucv->msglimit_adv = niucv->msglimit;
 	/* if receiver acks the xmit connection is established */
 	err = dev_queue_xmit(skb);
 	if (!err) {

-- 
2.56.0

Re: [PATCH RFC net v3 1/7] net/iucv: keep the window given to the peer in a field of its own

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net/iucv: premature and unconditional msglimit_adv write in
  afiucv_hs_send()
--

commit 8534ba296d230681179bb7448129eaebb9ecfc26
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/iucv: keep the window given to the peer in a field of its own

This commit records the receive window advertised in HiperSockets SYN and
SYN|ACK frames in a separate msglimit_adv field, and uses it in
iucv_sock_recvmsg() so later SO_MSGLIMIT changes do not alter window
accounting for that connection.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ce13fe3c85b52..657c61161dbdc 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c

[ ... ]

> @@ -208,8 +222,11 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
>  	phs_hdr->magic = ETH_P_AF_IUCV;
>  	phs_hdr->version = 1;
>  	phs_hdr->flags = flags;
> -	if (flags == AF_IUCV_FLAG_SYN)
> +	if (flags == AF_IUCV_FLAG_SYN) {
>  		phs_hdr->window = iucv->msglimit;
> +		/* what the peer may send us, whatever msglimit becomes */
> +		iucv->msglimit_adv = iucv->msglimit;
> +	}

[Severity: Medium]
Can setting iucv->msglimit_adv here cause it to diverge from the window the
peer actually received?

afiucv_hs_send() writes iucv->msglimit_adv before the device and MTU checks
and dev_queue_xmit(), and overwrites it on every AF_IUCV_FLAG_SYN call.
After a non-blocking or interrupted connect() sends a SYN and leaves
sk->sk_state as IUCV_BOUND, changing SO_MSGLIMIT and calling connect() again
overwrites iucv->msglimit_adv even if the peer's SYN|ACK to the first SYN is
already in flight or the second send fails with -ENETDOWN.  When the peer
sends frames up to the window from the first SYN, iucv_sock_recvmsg() then
hits WARN_ON(1), closes the socket and returns -EFAULT if SO_MSGLIMIT was
lowered, or fails to send AF_IUCV_FLAG_WIN updates if it was raised.

Similarly, if an initial connect() fails before transmitting a SYN and
SO_MSGLIMIT is changed before an incoming SYN|ACK connects the bound socket,
afiucv_hs_window() returns the stale msglimit_adv from the failed connect()
instead of iucv->msglimit.  In addition, iucv_sock_connect() checks
sk->sk_state before lock_sock(sk) without rechecking under the lock, so a
concurrent connect() can still enter afiucv_hs_send() and overwrite
iucv->msglimit_adv after the socket reaches IUCV_CONNECTED.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-b4-disp-2a6b4364-v3-0-35677b68e697@proton.me?part=1

[PATCH RFC net v3 2/7] net/iucv: fail sendmsg() with EPIPE once sending is shut down

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

sendmsg() tests SEND_SHUTDOWN on entry only, then waits for the peer's
window without the socket lock. When sending is shut down during that
wait, by shutdown(SHUT_WR) in another thread or by the peer's quiesce
over z/VM, the wait does not end and the call does not fail: the writer
waits out SO_SNDTIMEO, or, when the window opens first, sends its
record after the socket's SHT and the peer reads data after EOF. With
the peer's FIN in as well it returns -ECONNRESET for a stream its own
socket shut down.

End the wait when sending is shut down and test SEND_SHUTDOWN again
after it, ahead of the sk_state test, so the call fails with -EPIPE as
it does on entry.

Fixes: 0ea920d211e0 ("af_iucv: Return -EAGAIN if iucv msg limit is exceeded")
Cc: stable@vger.kernel.org # 6.18.x
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
New in v3 as a patch of its own: the new condition of sendmsg()'s
window wait and the test after it, placed ahead of the state test, were
in the lock patch. It goes before 3/7, which lets sendmsg() wait for
send memory without the socket lock and would otherwise open the same
gap a second time.

Three of the 53 default tests are about this, and net fails all three.
A writer parked on a full window sends its record after the socket's
own SHT when the window opens, so the peer reads data after EOF; it
waits out SO_SNDTIMEO when the window stays shut; and with the peer's
FIN in as well it gets -ECONNRESET. With this patch the first and the
third pass and the second returns -EPIPE; that test still fails, as its
SHT finds no room in sk_sndbuf (one of the five in the cover letter).
With only the wait changed the writer sends after the shutdown in the
first two and gets -ECONNRESET in the third; with only the test after
it, it waits out SO_SNDTIMEO again; with the test placed after the
state test it gets -ECONNRESET again.

Not changed: iucv_send_ctrl() still clears SEND_SHUTDOWN while it
allocates a control frame, so a caller without the socket lock can make
the new test miss a shutdown. On a connected socket there are two:
recvmsg()'s window update, until 3/7 puts recvmsg() under the lock, and
the NETDEV_GOING_DOWN notifier, which sets IUCV_DISCONN right after, so
the call then fails with -ECONNRESET.
---
 net/iucv/af_iucv.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 657c61161dbd..133c1d1886b2 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1055,10 +1055,16 @@ static int iucv_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	/* wait if outstanding messages for iucv path has reached */
 	timeo = sock_sndtimeo(sk, noblock);
-	err = iucv_sock_wait(sk, iucv_below_msglim(sk), timeo);
+	err = iucv_sock_wait(sk, iucv_below_msglim(sk) ||
+			     (sk->sk_shutdown & SEND_SHUTDOWN), timeo);
 	if (err)
 		goto fail;
 
+	/* return -EPIPE if sending has been shut down meanwhile */
+	if (sk->sk_shutdown & SEND_SHUTDOWN) {
+		err = -EPIPE;
+		goto fail;
+	}
 	/* return -ECONNRESET if the socket is no longer connected */
 	if (sk->sk_state != IUCV_CONNECTED) {
 		err = -ECONNRESET;

-- 
2.56.0

Re: [PATCH RFC net v3 2/7] net/iucv: fail sendmsg() with EPIPE once sending is shut down

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> sendmsg() tests SEND_SHUTDOWN on entry only, then waits for the peer's
> window without the socket lock. When sending is shut down during that
> wait, by shutdown(SHUT_WR) in another thread or by the peer's quiesce
> over z/VM, the wait does not end and the call does not fail: the writer
> waits out SO_SNDTIMEO, or, when the window opens first, sends its
> record after the socket's SHT and the peer reads data after EOF. With
> the peer's FIN in as well it returns -ECONNRESET for a stream its own
> socket shut down.
> 
> End the wait when sending is shut down and test SEND_SHUTDOWN again
> after it, ahead of the sk_state test, so the call fails with -EPIPE as
> it does on entry.
> 
> Fixes: 0ea920d211e0 ("af_iucv: Return -EAGAIN if iucv msg limit is exceeded")
> Cc: stable@vger.kernel.org # 6.18.x
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-b4-disp-2a6b4364-v3-0-35677b68e697@proton.me?part=2

[PATCH RFC net v3 3/7] net/iucv: run recvmsg() under the socket lock

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

recvmsg() takes no socket lock, on either transport:

- Two threads reading one socket get its data out of order.
- A read that races shutdown() can write back an older sk_shutdown and
  lose the bit shutdown() set, so a shutdown() that returned 0 has no
  effect.
- A MSG_PEEK read leaves its skb on the receive queue and holds a
  reference to it. When a second reader consumes the skb meanwhile,
  that reference is the last one and skb_unref() frees nothing: the skb
  and its receive memory leak, and iucv_sock_destruct() warns.

All three need only a second thread at the wrong moment. No
memory-safety report is known from any of them.

Take lock_sock() in recvmsg(). It sleeps for data without the lock, in
sk_wait_event(), so a reader with nothing to read keeps no other call
on the socket waiting.

sendmsg() now waits for send memory without the socket lock as well:
recvmsg() needs that lock, and over z/VM the memory comes back only when
the peer reads, which may wait for our own reader. That wait is one more
point at which shutdown(SHUT_WR) can find the send memory used up:
iucv_send_ctrl() then has no room for the SHT and the peer reads no EOF,
as before this patch whenever the memory is used up when shutdown()
runs.

Fixes: eac3731bd04c ("[S390]: Add AF_IUCV socket support")
Suggested-by: Alexandra Winter <wintera@linux.ibm.com>
Link: https://lore.kernel.org/all/20260724222917.134769-1-hexlabsecurity@proton.me/
Cc: stable@vger.kernel.org # 6.18.x: aa5ec571b93f: net/iucv: Add lock nesting annotation in iucv_sock_close()
Cc: stable@vger.kernel.org # 6.18.x
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
New in v3 as a patch of its own: the half of v2's lock patch that does
not touch the softirq side. Without 5/7 the softirq still changes
sk_state under a task that owns the socket, as on net.

In the emulated setup: two readers on one socket under MTTCG, three
runs of 838 messages each. On net and with 1/7 and 2/7 every run has
reads that go backwards, 9 to 18 of them; from 3/7 on there are none.

The lost bit: a task reads on a socket shut down for sending, with a
window of 2 so that nearly every read sends a window update, and
another calls shutdown(SHUT_RD). Over four runs of 200 cycles poll()
then shows no EPOLLRDHUP in 15 to 53 cycles of a run on net and with
1/7 and 2/7, and in none from 3/7 on.

Repro of the MSG_PEEK item: one process is parked in the copy of a
recv(MSG_PEEK), its buffer a userfaultfd page, and a second one reads
the same message. On net, and with 1/7 and 2/7, the second read returns
the byte while the first is still parked; once both are back the socket
has receive memory charged and nothing to read, and iucv_sock_destruct()
takes its WARN_ON() at close. From 3/7 on the second reader waits for
the first, then reads the byte, and nothing stays charged. With the two
reads one after the other every kernel is clean.

sendmsg()'s memory wait: with it left under the socket lock, a
non-blocking read is held up for as long as a writer waits for send
memory, 6 s in the test.

The prerequisite on the stable line, aa5ec571b93f, brings
__iucv_sock_close(), which recvmsg() now calls with the lock held where
it used to close the socket on a peer that overran its window.

By inspection: the memory wait now runs without the socket lock, so
iucv_send_ctrl() clearing SEND_SHUTDOWN around its allocation, e.g. for
recvmsg()'s window update, can make that wait miss the wake-up of a
shutdown(SHUT_WR). The writer then sleeps on until memory comes back,
where 2/7's test fails the call, or SO_SNDTIMEO runs out. No data goes
out after the SHT.

Hidayath's 19 KCSAN reports of recvmsg() against shutdown() on
net-next, and the 2 of iucv_send_ctrl() against shutdown(), are what
this patch should end.
---
 net/iucv/af_iucv.c | 94 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 78 insertions(+), 16 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 133c1d1886b2..639725917f3f 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1041,7 +1041,17 @@ static int iucv_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 		}
 	}
 	skb = sock_alloc_send_pskb(sk, headroom + linear, len - linear,
-				   noblock, &err, 0);
+				   1, &err, 0);
+	if (!skb && err == -EAGAIN && !noblock) {
+		/* Wait without the socket lock: recvmsg() takes it, and over
+		 * z/VM the memory comes back only when the peer reads, which
+		 * may wait for our own reader.
+		 */
+		release_sock(sk);
+		skb = sock_alloc_send_pskb(sk, headroom + linear,
+					   len - linear, 0, &err, 0);
+		lock_sock(sk);
+	}
 	if (!skb)
 		goto out;
 	if (headroom)
@@ -1268,6 +1278,54 @@ static void iucv_process_message_q(struct sock *sk)
 	}
 }
 
+/**
+ * iucv_sock_recv_skb() - get the next skb for recvmsg(), waiting for one
+ * @sk:		the socket, locked by the caller
+ * @flags:	recvmsg() flags; MSG_PEEK leaves the skb queued and returns
+ *		it with a reference
+ * @err:	set when no skb is returned
+ *
+ * skb_recv_datagram() would sleep with the socket lock held. This sleeps in
+ * sk_wait_event(), which drops it.
+ *
+ * Context: Process context, socket lock held; sleeps with it released.
+ * Return: the skb, or NULL with @err set. At RCV_SHUTDOWN the caller
+ * returns EOF whatever @err is.
+ */
+static struct sk_buff *iucv_sock_recv_skb(struct sock *sk, int flags, int *err)
+{
+	long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+	struct sk_buff_head *queue = &sk->sk_receive_queue;
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
+	struct sk_buff *skb;
+
+	for (;;) {
+		skb = skb_recv_datagram(sk, flags | MSG_DONTWAIT, err);
+		if (skb || *err != -EAGAIN)
+			break;
+		if (sk->sk_shutdown & RCV_SHUTDOWN)
+			break;
+		if (!timeo)
+			break;
+		if (sk->sk_state != IUCV_CONNECTED) {
+			*err = -ENOTCONN;
+			break;
+		}
+		if (signal_pending(current)) {
+			*err = sock_intr_errno(timeo);
+			break;
+		}
+		add_wait_queue(sk_sleep(sk), &wait);
+		sk_wait_event(sk, &timeo,
+			      !skb_queue_empty_lockless(queue) ||
+			      READ_ONCE(sk->sk_err) ||
+			      (READ_ONCE(sk->sk_shutdown) & RCV_SHUTDOWN) ||
+			      sk->sk_state != IUCV_CONNECTED, &wait);
+		remove_wait_queue(sk_sleep(sk), &wait);
+	}
+	return skb;
+}
+
 static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 			     size_t len, int flags)
 {
@@ -1279,24 +1337,25 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 	int err = 0;
 	u32 offset;
 
+	lock_sock(sk);
 	if ((sk->sk_state == IUCV_DISCONN) &&
 	    skb_queue_empty(&iucv->backlog_skb_q) &&
 	    skb_queue_empty(&sk->sk_receive_queue) &&
-	    list_empty(&iucv->message_q.list))
-		return 0;
+	    list_empty(&iucv->message_q.list)) {
+		copied = 0;
+		goto unlock;
+	}
 
-	if (flags & (MSG_OOB))
-		return -EOPNOTSUPP;
+	if (flags & (MSG_OOB)) {
+		err = -EOPNOTSUPP;
+		goto err_unlock;
+	}
 
-	/* receive/dequeue next skb:
-	 * the function understands MSG_PEEK and, thus, does not dequeue skb
-	 * only refcount is increased.
-	 */
-	skb = skb_recv_datagram(sk, flags, &err);
+	skb = iucv_sock_recv_skb(sk, flags, &err);
 	if (!skb) {
 		if (sk->sk_shutdown & RCV_SHUTDOWN)
-			return 0;
-		return err;
+			err = 0;
+		goto err_unlock;
 	}
 
 	offset = IUCV_SKB_CB(skb)->offset;
@@ -1346,8 +1405,9 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 			if (atomic_read(&iucv->msg_recv) >
 			    afiucv_hs_window(iucv)) {
 				WARN_ON(1);
-				iucv_sock_close(sk);
-				return -EFAULT;
+				__iucv_sock_close(sk);
+				err = -EFAULT;
+				goto err_unlock;
 			}
 		}
 
@@ -1389,7 +1449,8 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		copied = rlen;
 	if (flags & MSG_PEEK)
 		skb_unref(skb);
-
+unlock:
+	release_sock(sk);
 	return copied;
 
 err_out:
@@ -1397,7 +1458,8 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		skb_queue_head(&sk->sk_receive_queue, skb);
 	else
 		skb_unref(skb);
-
+err_unlock:
+	release_sock(sk);
 	return err;
 }
 

-- 
2.56.0

Re: [PATCH RFC net v3 3/7] net/iucv: run recvmsg() under the socket lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net/iucv: missed shutdown wakeup in iucv_sock_sendmsg()
  during wmem wait
--

commit 16ffdad8f70763ae645ac7eca51d7e6e91eed12b
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/iucv: run recvmsg() under the socket lock

This commit acquires the socket lock in iucv_sock_recvmsg() while dropping it
in sk_wait_event() when waiting for data, and drops the socket lock in
iucv_sock_sendmsg() while waiting for send buffer memory.

Link: https://lore.kernel.org/all/20260724222917.134769-1-hexlabsecurity@proton.me/

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 133c1d1886b2a..639725917f3f8 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -1041,7 +1041,17 @@ static int iucv_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>  		}
>  	}
>  	skb = sock_alloc_send_pskb(sk, headroom + linear, len - linear,
> -				   noblock, &err, 0);
> +				   1, &err, 0);
> +	if (!skb && err == -EAGAIN && !noblock) {
> +		/* Wait without the socket lock: recvmsg() takes it, and over
> +		 * z/VM the memory comes back only when the peer reads, which
> +		 * may wait for our own reader.
> +		 */
> +		release_sock(sk);
> +		skb = sock_alloc_send_pskb(sk, headroom + linear,
> +					   len - linear, 0, &err, 0);
> +		lock_sock(sk);
> +	}

[Severity: Medium]
Can a writer waiting in sock_alloc_send_pskb() without lock_sock(sk) miss a
concurrent shutdown(SHUT_WR) wakeup when iucv_send_ctrl() temporarily clears
SEND_SHUTDOWN?

net/iucv/af_iucv.c:iucv_send_ctrl() {
    ...
	if (sk->sk_shutdown & SEND_SHUTDOWN) {
		/* controlling flags should be sent anyway */
		shutdown = sk->sk_shutdown;
		sk->sk_shutdown &= RCV_SHUTDOWN;
	}
	skb = sock_alloc_send_skb(sk, blen, 1, &err);
	if (skb) {
		skb_reserve(skb, blen);
		err = afiucv_hs_send(NULL, sk, skb, flags);
	}
	if (shutdown)
		sk->sk_shutdown = shutdown;
    ...
}

Because sock_wait_for_wmem() now runs unlocked, it can race with
iucv_send_ctrl() (called under lock_sock(sk) by iucv_sock_recvmsg() for a
window update):

CPU0                              CPU1
iucv_sock_sendmsg()
  release_sock(sk)
  sock_alloc_send_pskb()
    sock_wait_for_wmem()
      prepare_to_wait(...)
                                  iucv_sock_shutdown(SHUT_WR)
                                    sk->sk_shutdown |= SEND_SHUTDOWN
                                    sk->sk_state_change(sk)
                                  iucv_sock_recvmsg()
                                    iucv_send_ctrl()
                                      sk->sk_shutdown &= RCV_SHUTDOWN
      if (sk->sk_shutdown & SEND_SHUTDOWN)
        /* false */
                                      sk->sk_shutdown = shutdown
      schedule_timeout(timeo)  <-- sleeps despite SHUT_WR

Since iucv_send_ctrl() restores SEND_SHUTDOWN without waking sk_sleep(sk),
iucv_sock_sendmsg() stays blocked in schedule_timeout() until send memory is
freed or SO_SNDTIMEO expires instead of returning -EPIPE immediately.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-b4-disp-2a6b4364-v3-0-35677b68e697@proton.me?part=3

[PATCH RFC net v3 4/7] net/iucv: factor out the HiperSockets frame dispatch and state changes

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

Move the switch on the frame type out of afiucv_hs_rcv() into
afiucv_hs_dispatch(), and the state changes a SYN|ACK, a SYN|FIN and a
FIN make into afiucv_hs_synack(), afiucv_hs_synfin() and
afiucv_hs_fin(). The three are still called under the bh_lock_sock() of
their callbacks, which now test sk where they tested iucv_sk(sk), a
cast of it. They are defined ahead of iucv_proto: the next patch
reaches them from proto->release_cb. afiucv_hs_synfin() and
afiucv_hs_fin() are the same for now and stay two: a later patch sets
RCV_SHUTDOWN in the second only.

No functional change. The next patch runs the dispatch from a second
place and the three state changes from the task that owns the socket.

Cc: stable@vger.kernel.org # 6.18.x, needed by the next patch
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
New in v3: v2 moved this code and changed it in one diff. Here it only
moves, so that 5/7 shows what changes.

checkpatch asks for a Fixes: tag because of the stable line. There is
none to give: the patch fixes nothing and is marked for stable only so
that 5/7 applies there.

The kernel with 1/7 to 4/7 gives the result of the one with 1/7 to 3/7
in every test: the 53 default ones and the 28 opt-in ones (window,
strays, connect(), leaks).
---
 net/iucv/af_iucv.c | 151 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 97 insertions(+), 54 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 639725917f3f..1677cc65ad06 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -39,6 +39,44 @@
 
 static char iucv_userid[80];
 
+/**
+ * afiucv_hs_synack() - connect a socket, on the SYN|ACK to its SYN
+ * @sk:		the socket, in IUCV_BOUND
+ * @window:	the window in the SYN|ACK: the frames the peer takes from us
+ *
+ * Context: Softirq, under bh_lock_sock().
+ */
+static void afiucv_hs_synack(struct sock *sk, u16 window)
+{
+	iucv_sk(sk)->msglimit_peer = window;
+	sk->sk_state = IUCV_CONNECTED;
+	sk->sk_state_change(sk);
+}
+
+/**
+ * afiucv_hs_synfin() - refuse a socket, on the SYN|FIN to its SYN
+ * @sk:		the socket, in IUCV_BOUND
+ *
+ * Context: As afiucv_hs_synack().
+ */
+static void afiucv_hs_synfin(struct sock *sk)
+{
+	sk->sk_state = IUCV_DISCONN;
+	sk->sk_state_change(sk);
+}
+
+/**
+ * afiucv_hs_fin() - disconnect a socket, on the FIN of its peer
+ * @sk:		the socket, in IUCV_CONNECTED
+ *
+ * Context: As afiucv_hs_synack().
+ */
+static void afiucv_hs_fin(struct sock *sk)
+{
+	sk->sk_state = IUCV_DISCONN;
+	sk->sk_state_change(sk);
+}
+
 static struct proto iucv_proto = {
 	.name		= "AF_IUCV",
 	.owner		= THIS_MODULE,
@@ -2032,17 +2070,13 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
  */
 static int afiucv_hs_callback_synack(struct sock *sk, struct sk_buff *skb)
 {
-	struct iucv_sock *iucv = iucv_sk(sk);
-
-	if (!iucv || sk->sk_state != IUCV_BOUND) {
+	if (!sk || sk->sk_state != IUCV_BOUND) {
 		kfree_skb(skb);
 		return NET_RX_SUCCESS;
 	}
 
 	bh_lock_sock(sk);
-	iucv->msglimit_peer = iucv_trans_hdr(skb)->window;
-	sk->sk_state = IUCV_CONNECTED;
-	sk->sk_state_change(sk);
+	afiucv_hs_synack(sk, iucv_trans_hdr(skb)->window);
 	bh_unlock_sock(sk);
 	consume_skb(skb);
 	return NET_RX_SUCCESS;
@@ -2053,16 +2087,13 @@ static int afiucv_hs_callback_synack(struct sock *sk, struct sk_buff *skb)
  */
 static int afiucv_hs_callback_synfin(struct sock *sk, struct sk_buff *skb)
 {
-	struct iucv_sock *iucv = iucv_sk(sk);
-
-	if (!iucv || sk->sk_state != IUCV_BOUND) {
+	if (!sk || sk->sk_state != IUCV_BOUND) {
 		kfree_skb(skb);
 		return NET_RX_SUCCESS;
 	}
 
 	bh_lock_sock(sk);
-	sk->sk_state = IUCV_DISCONN;
-	sk->sk_state_change(sk);
+	afiucv_hs_synfin(sk);
 	bh_unlock_sock(sk);
 	consume_skb(skb);
 	return NET_RX_SUCCESS;
@@ -2073,19 +2104,15 @@ static int afiucv_hs_callback_synfin(struct sock *sk, struct sk_buff *skb)
  */
 static int afiucv_hs_callback_fin(struct sock *sk, struct sk_buff *skb)
 {
-	struct iucv_sock *iucv = iucv_sk(sk);
-
 	/* other end of connection closed */
-	if (!iucv) {
+	if (!sk) {
 		kfree_skb(skb);
 		return NET_RX_SUCCESS;
 	}
 
 	bh_lock_sock(sk);
-	if (sk->sk_state == IUCV_CONNECTED) {
-		sk->sk_state = IUCV_DISCONN;
-		sk->sk_state_change(sk);
-	}
+	if (sk->sk_state == IUCV_CONNECTED)
+		afiucv_hs_fin(sk);
 	bh_unlock_sock(sk);
 	consume_skb(skb);
 	return NET_RX_SUCCESS;
@@ -2153,6 +2180,57 @@ static int afiucv_hs_callback_rx(struct sock *sk, struct sk_buff *skb)
 	return NET_RX_SUCCESS;
 }
 
+/**
+ * afiucv_hs_dispatch() - run the callback of a frame
+ * @sk:		the socket the frame is for, or NULL if there is none
+ * @skb:	the frame
+ *
+ * Context: Softirq.
+ * Return: NET_RX_SUCCESS.
+ */
+static int afiucv_hs_dispatch(struct sock *sk, struct sk_buff *skb)
+{
+	struct af_iucv_trans_hdr *trans_hdr = iucv_trans_hdr(skb);
+	int err = NET_RX_SUCCESS;
+
+	switch (trans_hdr->flags) {
+	case AF_IUCV_FLAG_SYN:
+		/* connect request */
+		err = afiucv_hs_callback_syn(sk, skb);
+		break;
+	case (AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_ACK):
+		/* connect request confirmed */
+		err = afiucv_hs_callback_synack(sk, skb);
+		break;
+	case (AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_FIN):
+		/* connect request refused */
+		err = afiucv_hs_callback_synfin(sk, skb);
+		break;
+	case (AF_IUCV_FLAG_FIN):
+		/* close request */
+		err = afiucv_hs_callback_fin(sk, skb);
+		break;
+	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;
+		}
+		fallthrough;	/* and receive non-zero length data */
+	case (AF_IUCV_FLAG_SHT):
+		/* shutdown request */
+		fallthrough;	/* and receive zero length data */
+	case 0:
+		/* plain data frame */
+		IUCV_SKB_CB(skb)->class = trans_hdr->iucv_hdr.class;
+		err = afiucv_hs_callback_rx(sk, skb);
+		break;
+	default:
+		kfree_skb(skb);
+	}
+	return err;
+}
+
 /*
  * afiucv_hs_rcv() - base function for arriving data through HiperSockets
  *                   transport
@@ -2233,42 +2311,7 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
 	 data -> send FIN
 	 SYN|ACK, SYN|FIN, FIN -> no action? */
 
-	switch (trans_hdr->flags) {
-	case AF_IUCV_FLAG_SYN:
-		/* connect request */
-		err = afiucv_hs_callback_syn(sk, skb);
-		break;
-	case (AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_ACK):
-		/* connect request confirmed */
-		err = afiucv_hs_callback_synack(sk, skb);
-		break;
-	case (AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_FIN):
-		/* connect request refused */
-		err = afiucv_hs_callback_synfin(sk, skb);
-		break;
-	case (AF_IUCV_FLAG_FIN):
-		/* close request */
-		err = afiucv_hs_callback_fin(sk, skb);
-		break;
-	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;
-		}
-		fallthrough;	/* and receive non-zero length data */
-	case (AF_IUCV_FLAG_SHT):
-		/* shutdown request */
-		fallthrough;	/* and receive zero length data */
-	case 0:
-		/* plain data frame */
-		IUCV_SKB_CB(skb)->class = trans_hdr->iucv_hdr.class;
-		err = afiucv_hs_callback_rx(sk, skb);
-		break;
-	default:
-		kfree_skb(skb);
-	}
-
+	err = afiucv_hs_dispatch(sk, skb);
 	if (sk)
 		sock_put(sk);
 	return err;

-- 
2.56.0

Re: [PATCH RFC net v3 4/7] net/iucv: factor out the HiperSockets frame dispatch and state changes

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> Move the switch on the frame type out of afiucv_hs_rcv() into
> afiucv_hs_dispatch(), and the state changes a SYN|ACK, a SYN|FIN and a
> FIN make into afiucv_hs_synack(), afiucv_hs_synfin() and
> afiucv_hs_fin(). The three are still called under the bh_lock_sock() of
> their callbacks, which now test sk where they tested iucv_sk(sk), a
> cast of it. They are defined ahead of iucv_proto: the next patch
> reaches them from proto->release_cb. afiucv_hs_synfin() and
> afiucv_hs_fin() are the same for now and stay two: a later patch sets
> RCV_SHUTDOWN in the second only.
> 
> No functional change. The next patch runs the dispatch from a second
> place and the three state changes from the task that owns the socket.
> 
> Cc: stable@vger.kernel.org # 6.18.x, needed by the next patch
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-b4-disp-2a6b4364-v3-0-35677b68e697@proton.me?part=4

[PATCH RFC net v3 5/7] net/iucv: serialize HiperSockets receive with the socket lock

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

The HiperSockets receive callbacks change sk_state under bh_lock_sock()
without testing sock_owned_by_user(). A SYN|ACK, SYN|FIN or FIN that
arrives during a system call on the socket is applied under the task
that holds lock_sock(), which goes on with the state it read. A SYN is
answered for a listener that close() is emptying: the child is left on
the accept queue of a socket that is then freed, and nothing can close
it. It keeps the listener's name, so bind() to that name fails with
-EADDRINUSE from then on, and it keeps its reference on the net device.

Either needs only a frame from the peer at the wrong moment. No
memory-safety report is known.

Take bh_lock_sock() once in afiucv_hs_rcv(), for every frame that finds
a socket. While a task owns it:

- A SYN|ACK, SYN|FIN or FIN is recorded in the socket and applied by
  the owner from proto->release_cb. It is not queued: nothing resends a
  control frame, and the socket backlog can refuse one under memory
  pressure, since it does not take a pfmemalloc skb.
  connect() applies a recorded reply before it tests the state, so a
  non-blocking connect() still sees a reply that is already in.
- Data, SHT and WIN frames do not change sk_state and are handled on
  arrival, as before. They wait in the socket backlog only while the
  reply to our SYN is recorded and not applied yet, and behind frames
  that wait already, up to the window given to the peer plus one.
- A SYN for a listener waits in the backlog, cut to its header, up to
  the accept queue plus one. Past that, or if the listener closed or
  its queue filled meanwhile, it is refused without a child.

A frame that no state of the socket can use is freed in afiucv_hs_rcv()
and such a SYN refused there: a SYN|ACK or SYN|FIN outside IUCV_BOUND,
a FIN for a socket neither connected nor about to be, and data, SHT and
WIN frames behind a recorded FIN. A SYN for a socket that does not
listen was refused only after a child had been allocated for it, and
that child was never freed; it is now refused without one. A listener
whose accept queue is full still leaks the child when no task owns it,
as before.

Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Suggested-by: Alexandra Winter <wintera@linux.ibm.com>
Link: https://lore.kernel.org/all/20260724222917.134769-1-hexlabsecurity@proton.me/
Cc: stable@vger.kernel.org # 6.18.x: f38bce9af9e2: net/iucv: take a private, writable frame before rewriting it in place
Cc: stable@vger.kernel.org # 6.18.x
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v3:
- only the softirq side of the lock is left here; 1/7 to 4/7, 6/7 and
  7/7 took the rest, and the cover letter lists what was dropped. Up to
  this patch the parked reader still gets -ENOTCONN at the peer's FIN,
  as on net; 7/7 changes that
- afiucv_hs_stray() calls a FIN a stray unless the socket is
  IUCV_CONNECTED or a recorded SYN|ACK is about to connect it. v2
  recorded a FIN that reached a socket in IUCV_BOUND and applied it
  after the SYN|ACK (AI review of v2, first item)
- data, SHT and WIN frames behind a recorded FIN are strays too, as
  afiucv_hs_callback_rx() frees them once the FIN is applied. v2 handled
  them as connected (second item)
- connect() applies a recorded reply to its SYN before it tests the
  state. v1 and v2 left the reply for release_sock(): a non-blocking
  connect() returned -EAGAIN with the SYN|ACK, or the SYN|FIN, already
  in, where net returns 0 or -ECONNREFUSED
- kernel-doc on every function added, with its context; the comment on
  afiucv_hs_defer() is rewritten (the reason v2 gave there was stale),
  the one on afiucv_hs_backlog_add() says why frames are counted
- on net, with nothing under it
v2: https://lore.kernel.org/netdev/20261022-b4-disp-1d26b87a-v2-1-7ee1264d812a@proton.me/
v1: https://lore.kernel.org/netdev/20261011-b4-disp-410518f4-v1-1-753bf3258292@proton.me/

Repro of the listener in the message's first paragraph. The window is a
few instructions wide, between the last child taken off the accept
queue and the listener's IUCV_CLOSED, so the test kernels have an
mdelay(400) there and a SYN is sent 100 ms into the close(). net with
the delay: connect() returns 0, nothing ever accepts or closes the
child, one socket stays in use after both ends are closed, and a new
bind() to the listener's name fails with -EADDRINUSE. The series with
the same delay: the SYN waits in the listener's backlog, is refused
when the close is done, and connect() returns -ECONNREFUSED; nothing is
left and the name binds again. With the SYN sent before the close()
both kernels free the child. Not seen without the delay, and no KASAN
splat with it.

Strays are freed in afiucv_hs_rcv(): a FIN with consume_skb(), as
afiucv_hs_callback_fin() frees it on net, the rest as NO_SOCKET drops.
With the drop trace on, a second FIN for a socket the first one
disconnected leaves no kfree_skb event, on net and here; the series
with that FIN dropped like the other strays leaves one NO_SOCKET drop.
A data byte sent the same way is a drop on every kernel.

The SYN for a socket that does not listen, measured: 16 connect() calls
to a socket that is bound and does not listen, each refused. On net and
up to 4/7 they leave 16 sockets in use after everything is closed, from
this patch on none. The full accept queue is not fixed: 1056 SYNs
refused by a listener whose queue is full leave 1056 sockets in use, on
net and with the series.

The backlog's budget is the window of 1/7 plus one, so a socket
connected by a SYN|ACK without a SYN of its own counts against msglimit
there too. With the socket held, three one-byte messages behind such a
SYN|ACK are all read after the release. With the budget read from
msglimit_adv alone, one is read and two are SOCKET_BACKLOG drops.

The first two items of the AI review repro on v2 as sent and are gone
here. Both hold the socket with a task parked in setsockopt(), its
filter a userfaultfd page, and send it one forged frame over the
emulated adapter. First: the socket's SYN is held
back, so it is IUCV_BOUND and no SYN|ACK exists; a FIN arrives, then the
SYN is released and the SYN|ACK comes in. v2 connects and disconnects
at the release; v3 frees the FIN as it arrives and a byte goes each way
afterwards. With the FIN sent after the SYN|ACK instead, both apply it.
Second: the peer sends one byte and closes, then a forged data frame
arrives behind its FIN. v2 delivers the forged byte after the real one;
v3 frees it and the reader gets EOF. net and v1 behave as v3 in both.
v1 queued control frames and ran them in order, so both came in when v2
began to record them instead. With any FIN recorded again the first
test fails; with frames let through behind a recorded FIN, the second.

With any one of these pieces taken out, at least one test fails: the
release callback that applies what was recorded, the wait for the
owner, the reply to our SYN applied before the backlog runs, connect()
applying that reply, either half of afiucv_hs_stray()'s rule for a FIN,
its rule for frames behind one, the whole of afiucv_hs_stray(), the
budget read through the window helper, and the stray FIN consumed. Of
the 53 default tests the kernel with 1/7 to 5/7 fails six: the five in
the cover letter and the parked reader's -ENOTCONN, which 7/7 changes.

By inspection only, no repro: a pfmemalloc skb refused by the backlog,
the device reference a waiting SYN holds, and the SOCK_ZAPPED test in
afiucv_hs_apply().
---
 include/net/iucv/af_iucv.h |   5 +
 net/iucv/af_iucv.c         | 332 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 314 insertions(+), 23 deletions(-)

diff --git a/include/net/iucv/af_iucv.h b/include/net/iucv/af_iucv.h
index 6decbb1f34b3..6b2cb6c41b9e 100644
--- a/include/net/iucv/af_iucv.h
+++ b/include/net/iucv/af_iucv.h
@@ -128,14 +128,19 @@ struct iucv_sock {
 	struct sock_msg_q	message_q;
 	unsigned int		send_tag;
 	u8			flags;
+	u8			hs_pending;	/* AF_IUCV_PEND_*, af_iucv.c */
 	u16			msglimit;
 	u16			msglimit_peer;
 	u16			msglimit_adv;	/* window given to the peer */
+	u16			hs_pending_window;
 	atomic_t		skbs_in_xmit;
 	atomic_t		msg_sent;
 	atomic_t		msg_recv;
 	atomic_t		pendings;
 	int			transport;
+	/* frames in sk_backlog since it was empty */
+	unsigned int		hs_backlog_data;
+	unsigned int		hs_backlog_syn;
 	void			(*sk_txnotify)(struct sock *sk,
 					       enum iucv_tx_notify n);
 };
diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 1677cc65ad06..f31b037f28c1 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -44,7 +44,8 @@ static char iucv_userid[80];
  * @sk:		the socket, in IUCV_BOUND
  * @window:	the window in the SYN|ACK: the frames the peer takes from us
  *
- * Context: Softirq, under bh_lock_sock().
+ * Context: sk_lock.slock held: softirq with no task owning the socket, or
+ * the owner in afiucv_hs_apply().
  */
 static void afiucv_hs_synack(struct sock *sk, u16 window)
 {
@@ -77,10 +78,58 @@ static void afiucv_hs_fin(struct sock *sk)
 	sk->sk_state_change(sk);
 }
 
+/* iucv_sock.hs_pending: control frames recorded for the owner */
+#define AF_IUCV_PEND_SYNACK	0x01
+#define AF_IUCV_PEND_SYNFIN	0x02
+#define AF_IUCV_PEND_SYNREPLY	(AF_IUCV_PEND_SYNACK | AF_IUCV_PEND_SYNFIN)
+#define AF_IUCV_PEND_FIN	0x04
+#define AF_IUCV_PEND_ALL	(AF_IUCV_PEND_SYNREPLY | AF_IUCV_PEND_FIN)
+
+/**
+ * afiucv_hs_apply() - apply the control frames afiucv_hs_record() kept
+ * @sk:		the socket, owned by the caller
+ * @mask:	the AF_IUCV_PEND_* bits to apply
+ *
+ * A socket closed meanwhile takes none. The test is SOCK_ZAPPED, since
+ * __iucv_sock_close() leaves IUCV_BOUND as it is.
+ *
+ * Context: Process context, under sk_lock.slock.
+ */
+static void afiucv_hs_apply(struct sock *sk, u8 mask)
+{
+	struct iucv_sock *iucv = iucv_sk(sk);
+	u8 pending = iucv->hs_pending & mask;
+
+	iucv->hs_pending &= ~pending;
+	if (sock_flag(sk, SOCK_ZAPPED))
+		return;
+	if ((pending & AF_IUCV_PEND_SYNACK) && sk->sk_state == IUCV_BOUND)
+		afiucv_hs_synack(sk, iucv->hs_pending_window);
+	if ((pending & AF_IUCV_PEND_SYNFIN) && sk->sk_state == IUCV_BOUND)
+		afiucv_hs_synfin(sk);
+	if ((pending & AF_IUCV_PEND_FIN) && sk->sk_state == IUCV_CONNECTED)
+		afiucv_hs_fin(sk);
+}
+
+/**
+ * afiucv_hs_release_cb() - proto->release_cb, apply what was recorded
+ * @sk:		the socket, still owned by the caller
+ *
+ * Runs after the backlog, so a recorded FIN follows the data before it.
+ *
+ * Context: Process context, under sk_lock.slock.
+ */
+static void afiucv_hs_release_cb(struct sock *sk)
+{
+	if (iucv_sk(sk)->hs_pending)
+		afiucv_hs_apply(sk, AF_IUCV_PEND_ALL);
+}
+
 static struct proto iucv_proto = {
 	.name		= "AF_IUCV",
 	.owner		= THIS_MODULE,
 	.obj_size	= sizeof(struct iucv_sock),
+	.release_cb	= afiucv_hs_release_cb,
 };
 
 static struct iucv_interface *pr_iucv;
@@ -131,6 +180,7 @@ static void iucv_sock_kill(struct sock *sk);
 static void __iucv_sock_close(struct sock *sk);
 
 static void afiucv_hs_callback_txnotify(struct sock *sk, enum iucv_tx_notify);
+static int afiucv_hs_backlog_rcv(struct sock *sk, struct sk_buff *skb);
 
 static struct iucv_sock_list iucv_sk_list = {
 	.lock = __RW_LOCK_UNLOCKED(iucv_sk_list.lock),
@@ -562,6 +612,8 @@ static struct sock *iucv_sock_alloc(struct socket *sock, int proto, gfp_t prio,
 		iucv->transport = AF_IUCV_TRANS_HIPER;
 
 	sk->sk_destruct = iucv_sock_destruct;
+	/* not from iucv_proto: sk_alloc() does not copy proto->backlog_rcv */
+	sk->sk_backlog_rcv = afiucv_hs_backlog_rcv;
 	sk->sk_sndtimeo = IUCV_CONN_TIMEOUT;
 
 	sock_reset_flag(sk, SOCK_ZAPPED);
@@ -836,6 +888,15 @@ static int iucv_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 	if (err)
 		goto done;
 
+	/* The reply may be in already, recorded while we own the socket:
+	 * apply it, or a non-blocking connect() misses it.
+	 */
+	if (iucv->transport == AF_IUCV_TRANS_HIPER) {
+		spin_lock_bh(&sk->sk_lock.slock);
+		afiucv_hs_apply(sk, AF_IUCV_PEND_SYNREPLY);
+		spin_unlock_bh(&sk->sk_lock.slock);
+	}
+
 	if (sk->sk_state != IUCV_CONNECTED)
 		err = iucv_sock_wait(sk, iucv_sock_in_state(sk, IUCV_CONNECTED,
 							    IUCV_DISCONN),
@@ -1324,7 +1385,8 @@ static void iucv_process_message_q(struct sock *sk)
  * @err:	set when no skb is returned
  *
  * skb_recv_datagram() would sleep with the socket lock held. This sleeps in
- * sk_wait_event(), which drops it.
+ * sk_wait_event(), which drops it; a FIN recorded for the owner is applied
+ * then and ends the wait.
  *
  * Context: Process context, socket lock held; sleeps with it released.
  * Return: the skb, or NULL with @err set. At RCV_SHUTDOWN the caller
@@ -2018,7 +2080,6 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
 	}
 
 	nsk = iucv_sock_alloc(NULL, sk->sk_protocol, GFP_ATOMIC, 0);
-	bh_lock_sock(sk);
 	if ((sk->sk_state != IUCV_LISTEN) ||
 	    sk_acceptq_is_full(sk) ||
 	    !nsk) {
@@ -2028,7 +2089,6 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
 		err = dev_queue_xmit(skb);
 		if (nsk)
 			iucv_sock_kill(nsk);
-		bh_unlock_sock(sk);
 		goto out;
 	}
 
@@ -2059,7 +2119,6 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
 		sk->sk_data_ready(sk);
 	} else
 		iucv_sock_kill(nsk);
-	bh_unlock_sock(sk);
 
 out:
 	return NET_RX_SUCCESS;
@@ -2075,9 +2134,7 @@ static int afiucv_hs_callback_synack(struct sock *sk, struct sk_buff *skb)
 		return NET_RX_SUCCESS;
 	}
 
-	bh_lock_sock(sk);
 	afiucv_hs_synack(sk, iucv_trans_hdr(skb)->window);
-	bh_unlock_sock(sk);
 	consume_skb(skb);
 	return NET_RX_SUCCESS;
 }
@@ -2092,9 +2149,7 @@ static int afiucv_hs_callback_synfin(struct sock *sk, struct sk_buff *skb)
 		return NET_RX_SUCCESS;
 	}
 
-	bh_lock_sock(sk);
 	afiucv_hs_synfin(sk);
-	bh_unlock_sock(sk);
 	consume_skb(skb);
 	return NET_RX_SUCCESS;
 }
@@ -2110,10 +2165,8 @@ static int afiucv_hs_callback_fin(struct sock *sk, struct sk_buff *skb)
 		return NET_RX_SUCCESS;
 	}
 
-	bh_lock_sock(sk);
 	if (sk->sk_state == IUCV_CONNECTED)
 		afiucv_hs_fin(sk);
-	bh_unlock_sock(sk);
 	consume_skb(skb);
 	return NET_RX_SUCCESS;
 }
@@ -2128,6 +2181,7 @@ static int afiucv_hs_callback_win(struct sock *sk, struct sk_buff *skb)
 	if (!iucv)
 		return NET_RX_SUCCESS;
 
+	/* also runs while a task owns the socket, see afiucv_hs_defer() */
 	if (sk->sk_state != IUCV_CONNECTED)
 		return NET_RX_SUCCESS;
 
@@ -2148,6 +2202,7 @@ static int afiucv_hs_callback_rx(struct sock *sk, struct sk_buff *skb)
 		return NET_RX_SUCCESS;
 	}
 
+	/* also runs while a task owns the socket, see afiucv_hs_defer() */
 	if (sk->sk_state != IUCV_CONNECTED) {
 		kfree_skb(skb);
 		return NET_RX_SUCCESS;
@@ -2180,12 +2235,171 @@ static int afiucv_hs_callback_rx(struct sock *sk, struct sk_buff *skb)
 	return NET_RX_SUCCESS;
 }
 
+/**
+ * afiucv_hs_stray() - test for a frame that no state of the socket can use
+ * @sk:		the socket the frame is for, under bh_lock_sock()
+ * @flags:	the flags of the frame
+ *
+ * A stray never waits for the owner. A SYN is one outside IUCV_LISTEN and a
+ * SYN|ACK or SYN|FIN outside IUCV_BOUND, the state tests of their
+ * callbacks; a full accept queue is not tested here. A FIN is one unless
+ * the socket is connected or a recorded SYN|ACK is about to connect it.
+ * Data, SHT and WIN are strays once the peer's FIN is recorded: the socket
+ * stays IUCV_CONNECTED until the owner applies it.
+ *
+ * Context: Softirq, under bh_lock_sock().
+ * Return: true if the frame is to be freed or, for a SYN, refused.
+ */
+static bool afiucv_hs_stray(struct sock *sk, u8 flags)
+{
+	struct iucv_sock *iucv = iucv_sk(sk);
+
+	switch (flags) {
+	case 0:
+	case AF_IUCV_FLAG_SHT:
+	case AF_IUCV_FLAG_WIN:
+		return iucv->hs_pending & AF_IUCV_PEND_FIN;
+	case AF_IUCV_FLAG_FIN:
+		return sk->sk_state != IUCV_CONNECTED &&
+		       !(iucv->hs_pending & AF_IUCV_PEND_SYNACK);
+	case AF_IUCV_FLAG_SYN:
+		return sk->sk_state != IUCV_LISTEN;
+	case AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_ACK:
+	case AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_FIN:
+		return sk->sk_state != IUCV_BOUND;
+	}
+	return true;
+}
+
+/**
+ * afiucv_hs_defer() - test whether a data, SHT or WIN frame has to wait
+ * @sk:		the socket, owned by a task, under bh_lock_sock()
+ *
+ * These frames do not change sk_state and are handled on arrival, also
+ * while a task owns the socket. They wait in the socket backlog only if:
+ *
+ * - the reply to our SYN is recorded and not applied yet: the socket is
+ *   still IUCV_BOUND, where data is freed and a WIN ignored;
+ * - frames wait already: a later one must not overtake them, and
+ *   sk_backlog.len stays non-zero until __release_sock() has run them all.
+ *
+ * Context: Softirq, under bh_lock_sock().
+ * Return: true if the frame has to wait in the backlog.
+ */
+static bool afiucv_hs_defer(struct sock *sk)
+{
+	return (iucv_sk(sk)->hs_pending & AF_IUCV_PEND_SYNREPLY) ||
+	       sk->sk_backlog.len != 0;
+}
+
+/**
+ * afiucv_hs_record() - keep a control frame for the task that owns the socket
+ * @sk:		the socket, owned by a task, under bh_lock_sock()
+ * @skb:	the frame, consumed here
+ * @flags:	the flags of the frame: SYN|ACK, SYN|FIN or FIN
+ *
+ * Not queued: nothing resends a control frame, and the socket backlog has
+ * to refuse a pfmemalloc skb. A bit in hs_pending is kept instead, and the
+ * window of a SYN|ACK. The first reply to our SYN decides; a FIN is applied
+ * after it.
+ *
+ * Context: Softirq, under bh_lock_sock().
+ */
+static void afiucv_hs_record(struct sock *sk, struct sk_buff *skb, u8 flags)
+{
+	struct iucv_sock *iucv = iucv_sk(sk);
+
+	switch (flags) {
+	case AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_ACK:
+		if (!(iucv->hs_pending & AF_IUCV_PEND_SYNREPLY)) {
+			iucv->hs_pending_window = iucv_trans_hdr(skb)->window;
+			iucv->hs_pending |= AF_IUCV_PEND_SYNACK;
+		}
+		break;
+	case AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_FIN:
+		if (!(iucv->hs_pending & AF_IUCV_PEND_SYNREPLY))
+			iucv->hs_pending |= AF_IUCV_PEND_SYNFIN;
+		break;
+	case AF_IUCV_FLAG_FIN:
+		iucv->hs_pending |= AF_IUCV_PEND_FIN;
+		break;
+	}
+	consume_skb(skb);
+}
+
+/**
+ * afiucv_hs_backlog_add() - make a frame wait for the task that owns the socket
+ * @sk:		the socket, owned by a task, under bh_lock_sock()
+ * @skb:	the frame
+ * @flags:	the flags of the frame: a SYN, or a data, SHT or WIN frame
+ *
+ * sk_add_backlog() is not used: its limit is in bytes and counts the
+ * receive queue too, while the window is in frames. So frames are counted:
+ *
+ * - data, SHT and WIN up to the window given to the peer plus one. They
+ *   wait only from the reply to our SYN until the owner has run them, and
+ *   a peer that keeps to its window sends at most that many data frames
+ *   and its SHT by then, and no WIN;
+ * - SYNs up to the accept queue plus the one sk_acceptq_is_full() admits.
+ *
+ * The counts restart only when __release_sock() has run every frame, so a
+ * peer cannot keep the owner in its loop. Past the limit a frame is
+ * dropped and a SYN refused; so is a pfmemalloc skb, on which
+ * __sk_backlog_rcv() BUG()s unless the socket is SOCK_MEMALLOC. A waiting
+ * SYN is cut to its header, all the reply needs, and holds its device:
+ * close() drops hs_dev before the release runs the SYN.
+ *
+ * Context: Softirq, under bh_lock_sock().
+ */
+static void afiucv_hs_backlog_add(struct sock *sk, struct sk_buff *skb,
+				  u8 flags)
+{
+	struct iucv_sock *iucv = iucv_sk(sk);
+	unsigned int *count;
+	unsigned int limit;
+
+	if (!sk->sk_backlog.len) {
+		iucv->hs_backlog_data = 0;
+		iucv->hs_backlog_syn = 0;
+	}
+	if (flags == AF_IUCV_FLAG_SYN) {
+		count = &iucv->hs_backlog_syn;
+		limit = sk->sk_max_ack_backlog + 1;
+	} else {
+		count = &iucv->hs_backlog_data;
+		limit = afiucv_hs_window(iucv) + 1;
+	}
+	if (*count >= limit ||
+	    (skb_pfmemalloc(skb) && !sock_flag(sk, SOCK_MEMALLOC)))
+		goto refuse;
+	if (flags == AF_IUCV_FLAG_SYN) {
+		if (pskb_trim(skb, sizeof(struct af_iucv_trans_hdr)))
+			goto refuse;
+		dev_hold(skb->dev);
+	}
+	__sk_add_backlog(sk, skb);
+	/* a window of 65535 frames of 64K each would wrap the int */
+	sk->sk_backlog.len = min_t(u32, sk->sk_backlog.len + skb->truesize,
+				   INT_MAX);
+	(*count)++;
+	return;
+
+refuse:
+	if (flags == AF_IUCV_FLAG_SYN) {
+		afiucv_hs_callback_syn(NULL, skb);
+	} else {
+		sk_drops_inc(sk);
+		kfree_skb_reason(skb, SKB_DROP_REASON_SOCKET_BACKLOG);
+	}
+}
+
 /**
  * afiucv_hs_dispatch() - run the callback of a frame
  * @sk:		the socket the frame is for, or NULL if there is none
  * @skb:	the frame
  *
- * Context: Softirq.
+ * Context: BH disabled. @sk is under bh_lock_sock(), or owned by the caller
+ * in afiucv_hs_backlog_rcv().
  * Return: NET_RX_SUCCESS.
  */
 static int afiucv_hs_dispatch(struct sock *sk, struct sk_buff *skb)
@@ -2231,6 +2445,71 @@ static int afiucv_hs_dispatch(struct sock *sk, struct sk_buff *skb)
 	return err;
 }
 
+/**
+ * afiucv_hs_backlog_rcv() - sk_backlog_rcv, run a frame that waited
+ * @sk:		the socket, owned by the caller
+ * @skb:	the frame
+ *
+ * Frames run in the order of arrival. The reply to our SYN came before any
+ * of them, so it is applied first. A SYN is refused if the listener was
+ * closed or its accept queue filled meanwhile.
+ *
+ * Context: Process context, socket owned by the caller, no sk_lock.slock.
+ * Return: 0.
+ */
+static int afiucv_hs_backlog_rcv(struct sock *sk, struct sk_buff *skb)
+{
+	struct net_device *dev = NULL;
+
+	spin_lock_bh(&sk->sk_lock.slock);
+	afiucv_hs_apply(sk, AF_IUCV_PEND_SYNREPLY);
+	spin_unlock_bh(&sk->sk_lock.slock);
+	if (iucv_trans_hdr(skb)->flags == AF_IUCV_FLAG_SYN) {
+		dev = skb->dev;		/* held by afiucv_hs_backlog_add() */
+		if (sk->sk_state != IUCV_LISTEN || sk_acceptq_is_full(sk)) {
+			afiucv_hs_callback_syn(NULL, skb);
+			dev_put(dev);
+			return 0;
+		}
+	}
+	/* BH off: the callbacks take message_q.lock with spin_lock() */
+	local_bh_disable();
+	afiucv_hs_dispatch(sk, skb);
+	local_bh_enable();
+	dev_put(dev);
+	return 0;
+}
+
+/**
+ * afiucv_hs_rcv_owned() - handle a frame while a task owns the socket
+ * @sk:		the socket, under bh_lock_sock()
+ * @skb:	the frame
+ * @flags:	the flags of the frame
+ *
+ * A SYN|ACK, SYN|FIN or FIN is recorded, a SYN waits in the backlog, and a
+ * data, SHT or WIN frame is handled now unless afiucv_hs_defer() says wait.
+ *
+ * Context: Softirq, under bh_lock_sock().
+ * Return: NET_RX_SUCCESS.
+ */
+static int afiucv_hs_rcv_owned(struct sock *sk, struct sk_buff *skb, u8 flags)
+{
+	switch (flags) {
+	case AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_ACK:
+	case AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_FIN:
+	case AF_IUCV_FLAG_FIN:
+		afiucv_hs_record(sk, skb, flags);
+		return NET_RX_SUCCESS;
+	case AF_IUCV_FLAG_SYN:
+		break;
+	default:
+		if (!afiucv_hs_defer(sk))
+			return afiucv_hs_dispatch(sk, skb);
+	}
+	afiucv_hs_backlog_add(sk, skb, flags);
+	return NET_RX_SUCCESS;
+}
+
 /*
  * afiucv_hs_rcv() - base function for arriving data through HiperSockets
  *                   transport
@@ -2302,18 +2581,25 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!iucv)
 		sk = NULL;
 
-	/* no sock
-	how should we send with no sock
-	1) send without sock no send rc checking?
-	2) introduce default sock to handle this cases
-
-	 SYN -> send SYN|ACK in good case, send SYN|FIN in bad case
-	 data -> send FIN
-	 SYN|ACK, SYN|FIN, FIN -> no action? */
+	/* no socket: a SYN is refused, anything else freed */
+	if (!sk)
+		return afiucv_hs_dispatch(NULL, skb);
 
-	err = afiucv_hs_dispatch(sk, skb);
-	if (sk)
-		sock_put(sk);
+	bh_lock_sock(sk);
+	if (afiucv_hs_stray(sk, trans_hdr->flags)) {
+		if (trans_hdr->flags == AF_IUCV_FLAG_SYN)
+			afiucv_hs_callback_syn(NULL, skb);
+		else if (trans_hdr->flags == AF_IUCV_FLAG_FIN)
+			consume_skb(skb);
+		else
+			kfree_skb_reason(skb, SKB_DROP_REASON_NO_SOCKET);
+	} else if (sock_owned_by_user(sk)) {
+		err = afiucv_hs_rcv_owned(sk, skb, trans_hdr->flags);
+	} else {
+		err = afiucv_hs_dispatch(sk, skb);
+	}
+	bh_unlock_sock(sk);
+	sock_put(sk);
 	return err;
 }
 

-- 
2.56.0

Re: [PATCH RFC net v3 5/7] net/iucv: serialize HiperSockets receive with the socket lock

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> The HiperSockets receive callbacks change sk_state under bh_lock_sock()
> without testing sock_owned_by_user(). A SYN|ACK, SYN|FIN or FIN that
> arrives during a system call on the socket is applied under the task
> that holds lock_sock(), which goes on with the state it read. A SYN is
> answered for a listener that close() is emptying: the child is left on
> the accept queue of a socket that is then freed, and nothing can close
> it. It keeps the listener's name, so bind() to that name fails with
> -EADDRINUSE from then on, and it keeps its reference on the net device.
> 
> Either needs only a frame from the peer at the wrong moment. No
> memory-safety report is known.
> 
> Take bh_lock_sock() once in afiucv_hs_rcv(), for every frame that finds
> [ ... ]
> Suggested-by: Alexandra Winter <wintera@linux.ibm.com>
> Link: https://lore.kernel.org/all/20260724222917.134769-1-hexlabsecurity@proton.me/
> Cc: stable@vger.kernel.org # 6.18.x: f38bce9af9e2: net/iucv: take a private, writable frame before rewriting it in place
> Cc: stable@vger.kernel.org # 6.18.x
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-b4-disp-2a6b4364-v3-0-35677b68e697@proton.me?part=5

[PATCH RFC net v3 6/7] net/iucv: set RCV_SHUTDOWN under the socket spinlock

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

shutdown(SHUT_RD) sets RCV_SHUTDOWN and purges the receive queue under
lock_sock() alone, and nothing orders that against a HiperSockets data
frame: a frame that tested RCV_SHUTDOWN before the bit was set is
queued after the purge, and recvmsg() returns its data after
shutdown(SHUT_RD) returned.

Since the previous patch such a frame is received under sk_lock.slock,
also while a task owns the socket; one that waited for the owner runs
when it releases the socket, which shutdown() does only after the purge.
On HiperSockets, set the bit and purge under that lock. recvmsg() sets
the same bit under lock_sock() only, when it reads a zero-length record,
which is how the peer's SHT arrives; that write and the frame's test are
marked WRITE_ONCE() and READ_ONCE(), and only a frame the peer sent
after that record can race with it. iucv_send_ctrl() still clears and
restores SEND_SHUTDOWN in that byte with plain writes. The z/VM branch
is unchanged, less a line that was commented out.

Without the previous patch this one changes nothing: the frame then
takes no lock that shutdown() could share with it.

Fixes: 82492a355fac ("af_iucv: add shutdown for HS transport")
Reported-by: Hidayath Khan <hidayath@linux.ibm.com>
Closes: https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/
Cc: stable@vger.kernel.org # 6.18.x, with the previous patch only
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
New in v3 as a patch: the hunk was in the lock patch of v2.

This is the first of the two races KCSAN reported on v1 in the mail
the Closes: tag points at, afiucv_hs_callback_rx() against
iucv_sock_shutdown() on sk_shutdown. The second there, on hs_dev, is
fixed by 1/2 of the two fixes posted for net on 28 Oct ([7] in the
cover letter), which are not in this series.

In the emulated setup, 200 cycles a run and four runs a kernel (QEMU's
default slice and three seeded short ones): net, the kernel with 1/7
and 2/7 and the one with 1/7 to 3/7 read data after shutdown(SHUT_RD)
in 0 to 7 cycles of a run, 39 of 2400 in all. The series reads none.
Nor does the series minus this spinlock: since 5/7 only a frame that
takes sk_lock.slock between shutdown()'s lock_sock() and its write gets
through, and no run had one. With udelay(50) added after lock_sock() in
shutdown() and after the frame's test, the series minus this spinlock
reads late data in 10 to 28 of 200 cycles and the series in none.

On the hardware Hidayath could not get that window to fire with v1
either, and KCSAN cannot show it closed: shutdown()'s write is now
WRITE_ONCE(), which the readers that take no lock need, and that alone
silences the report. IOW only the udelay() kernels above show what the
spinlock does.

The z/VM branch is unchanged; what is open there is under "Seen while
reviewing v3" in the cover letter.
---
 net/iucv/af_iucv.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index f31b037f28c1..2118effc137f 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1462,7 +1462,7 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 	rlen   = skb->len - offset;		/* real length of skb */
 	copied = min_t(unsigned int, rlen, len);
 	if (!rlen)
-		sk->sk_shutdown = sk->sk_shutdown | RCV_SHUTDOWN;
+		WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);
 
 	cskb = skb;
 	if (skb_copy_datagram_msg(cskb, offset, msg, copied)) {
@@ -1665,16 +1665,28 @@ static int iucv_sock_shutdown(struct socket *sock, int how)
 			iucv_send_ctrl(sk, AF_IUCV_FLAG_SHT);
 	}
 
-	sk->sk_shutdown |= how;
-	if (how == RCV_SHUTDOWN || how == SHUTDOWN_MASK) {
-		if ((iucv->transport == AF_IUCV_TRANS_IUCV) &&
-		    iucv->path) {
-			err = pr_iucv->path_quiesce(iucv->path, NULL);
-			if (err)
-				err = -ENOTCONN;
-/*			skb_queue_purge(&sk->sk_receive_queue); */
+	if (iucv->transport == AF_IUCV_TRANS_HIPER) {
+		/* Set the bit and purge under sk_lock.slock: a frame handled
+		 * on arrival tests RCV_SHUTDOWN under it, also while this
+		 * task owns the socket, and one that passed the test would
+		 * be queued after the purge. A frame that waited runs when
+		 * the owner releases the socket, not in between.
+		 */
+		spin_lock_bh(&sk->sk_lock.slock);
+		WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | how);
+		if (how == RCV_SHUTDOWN || how == SHUTDOWN_MASK)
+			skb_queue_purge(&sk->sk_receive_queue);
+		spin_unlock_bh(&sk->sk_lock.slock);
+	} else {
+		sk->sk_shutdown |= how;
+		if (how == RCV_SHUTDOWN || how == SHUTDOWN_MASK) {
+			if (iucv->path) {
+				err = pr_iucv->path_quiesce(iucv->path, NULL);
+				if (err)
+					err = -ENOTCONN;
+			}
+			skb_queue_purge(&sk->sk_receive_queue);
 		}
-		skb_queue_purge(&sk->sk_receive_queue);
 	}
 
 	/* Wake up anyone sleeping in poll */
@@ -2208,7 +2220,7 @@ static int afiucv_hs_callback_rx(struct sock *sk, struct sk_buff *skb)
 		return NET_RX_SUCCESS;
 	}
 
-	if (sk->sk_shutdown & RCV_SHUTDOWN) {
+	if (READ_ONCE(sk->sk_shutdown) & RCV_SHUTDOWN) {
 		kfree_skb(skb);
 		return NET_RX_SUCCESS;
 	}

-- 
2.56.0

Re: [PATCH RFC net v3 6/7] net/iucv: set RCV_SHUTDOWN under the socket spinlock

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> shutdown(SHUT_RD) sets RCV_SHUTDOWN and purges the receive queue under
> lock_sock() alone, and nothing orders that against a HiperSockets data
> frame: a frame that tested RCV_SHUTDOWN before the bit was set is
> queued after the purge, and recvmsg() returns its data after
> shutdown(SHUT_RD) returned.
> 
> Since the previous patch such a frame is received under sk_lock.slock,
> also while a task owns the socket; one that waited for the owner runs
> when it releases the socket, which shutdown() does only after the purge.
> On HiperSockets, set the bit and purge under that lock. recvmsg() sets
> the same bit under lock_sock() only, when it reads a zero-length record,
> which is how the peer's SHT arrives; that write and the frame's test are
> marked WRITE_ONCE() and READ_ONCE(), and only a frame the peer sent
> after that record can race with it. iucv_send_ctrl() still clears and
> restores SEND_SHUTDOWN in that byte with plain writes. The z/VM branch
> is unchanged, less a line that was commented out.
> 
> Without the previous patch this one changes nothing: the frame then
> takes no lock that shutdown() could share with it.
> 
> Fixes: 82492a355fac ("af_iucv: add shutdown for HS transport")
> Reported-by: Hidayath Khan <hidayath@linux.ibm.com>
> Closes: https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/
> Cc: stable@vger.kernel.org # 6.18.x, with the previous patch only
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-b4-disp-2a6b4364-v3-0-35677b68e697@proton.me?part=6

[PATCH RFC net v3 7/7] net/iucv: return EOF to a reader parked when the peer closes

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

A recvmsg() that starts after the peer closed returns 0. One that was
already waiting returns -ENOTCONN: its wait ends on sk_state and does
not ask whether anything is left to read. In a z/VM guest over
HiperSockets that was every parked reader, 13627 of 13627 on net-next
[1]. poll() does not report EPOLLRDHUP for the close either.

Return EOF from the wait too when the socket is IUCV_DISCONN with
nothing left to read. As for a call that starts afterwards, it does not
matter how the socket got there: the peer's FIN, a severed z/VM path, a
transmit error, a window update that could not be sent or the device
going down. EOF does not say that all the peer sent was read: a severed
z/VM path drops the messages not yet received, for either call. Set
RCV_SHUTDOWN where the HiperSockets FIN is applied, as TCP does: poll()
then reports EPOLLRDHUP and EPOLLRDNORM for the close, and EPOLLHUP if
the socket had shut down its own sending. The other ways still report
none of the three.

Fixes: 561e036006dc ("[AF_IUCV]: Implementation of a skb backlog queue")
Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Reported-by: Hidayath Khan <hidayath@linux.ibm.com>
Closes: https://lore.kernel.org/netdev/82691101-92d7-4838-b16e-2e41a83f65d0@linux.ibm.com/
Link: https://lore.kernel.org/netdev/51a4a40a-f34b-4d41-b049-69aa53f4dcf5@linux.ibm.com/ [1]
Cc: stable@vger.kernel.org # 6.18.x
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v3: this was 2/2. The EOF in the receive wait, which v2 had in the lock
patch, is here now, so both halves of one change in behaviour are in
one patch: the wait returns EOF, and the FIN sets RCV_SHUTDOWN.

Each half covers what the other does not. RCV_SHUTDOWN at the FIN gives
the parked reader EOF and poll() EPOLLRDHUP on HiperSockets, but a z/VM
path severed under a sleeping reader sets no RCV_SHUTDOWN; the test in
the wait covers the sever and gives poll() nothing.

In the emulated setup:

- the peer closes under a reader, 40 cycles, half with the reader
  parked and half with the close racing its recv(): -ENOTCONN in 34 on
  net, 29 with 1/7 and 2/7 and 32 with 1/7 to 6/7; EOF in all 40 with
  this patch
- poll() after the peer's close, 10 cycles of three cases (nothing
  queued, a message unread, the socket's own SHUT_WR first): on those
  three kernels no EPOLLRDHUP in any of the 30 and no EPOLLHUP in the
  10 with SHUT_WR; with this patch each case reports what an AF_UNIX
  socketpair reports on the same kernel. With RCV_SHUTDOWN at the FIN
  taken out again the reader still gets EOF, from the wait, and poll()
  reports as before the patch
- a reader parked while its own interface goes down, or while a send of
  its own fails in the adapter (SBALF15 0x10), and a z/VM reader whose
  path is severed (emulated CP): -ENOTCONN on those three kernels, and
  again with the EOF taken out of the wait; EOF with this patch

The interface going down and the TX error change sk_state with no
socket lock, so the test in the wait looks at the receive queue again.
The test kernel has an mdelay() that holds the reader between its look
at the queue and its look at the state; a byte arrives and the
interface goes down in that time. The reader's call returns -ENOTCONN
and its next two the byte and EOF. With the receive queue left out of
the test the first call returns 0 with the byte still queued. FWIW net
returns -ENOTCONN the same way when the skb lands between its own two
tests, a narrower window. The window update that could not be sent is
by inspection.

After 5/7 the FIN is applied under sk_lock.slock on every path, by the
softirq with no owner or by the owner from release_cb, and 6/7 has
shutdown() write the same field under that lock.

One thing 5/7 does not serialize: the NETDEV_GOING_DOWN notifier calls
iucv_send_ctrl() with no socket lock, and on a socket shut down for
sending that function restores sk_shutdown from a copy. A FIN applied
in between loses its RCV_SHUTDOWN, and with it the EPOLLRDHUP; the
reader still gets EOF from the wait. net has the same save and restore.
---
 net/iucv/af_iucv.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 2118effc137f..9ae57cbd3efc 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -75,6 +75,8 @@ static void afiucv_hs_synfin(struct sock *sk)
 static void afiucv_hs_fin(struct sock *sk)
 {
 	sk->sk_state = IUCV_DISCONN;
+	/* as a TCP FIN: EOF for a reader, EPOLLRDHUP for poll() */
+	WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);
 	sk->sk_state_change(sk);
 }
 
@@ -1389,8 +1391,9 @@ static void iucv_process_message_q(struct sock *sk)
  * then and ends the wait.
  *
  * Context: Process context, socket lock held; sleeps with it released.
- * Return: the skb, or NULL with @err set. At RCV_SHUTDOWN the caller
- * returns EOF whatever @err is.
+ * Return: the skb, or NULL with @err set: 0 if the connection is gone with
+ * nothing left to read and the call could still wait. At RCV_SHUTDOWN the
+ * caller returns EOF whatever @err is.
  */
 static struct sk_buff *iucv_sock_recv_skb(struct sock *sk, int flags, int *err)
 {
@@ -1408,7 +1411,16 @@ static struct sk_buff *iucv_sock_recv_skb(struct sock *sk, int flags, int *err)
 		if (!timeo)
 			break;
 		if (sk->sk_state != IUCV_CONNECTED) {
-			*err = -ENOTCONN;
+			/* disconnected and nothing left to read: EOF, as at
+			 * the top of iucv_sock_recvmsg()
+			 */
+			if (sk->sk_state == IUCV_DISCONN &&
+			    skb_queue_empty(&iucv_sk(sk)->backlog_skb_q) &&
+			    skb_queue_empty(&sk->sk_receive_queue) &&
+			    list_empty(&iucv_sk(sk)->message_q.list))
+				*err = 0;
+			else
+				*err = -ENOTCONN;
 			break;
 		}
 		if (signal_pending(current)) {

-- 
2.56.0

Re: [PATCH RFC net v3 7/7] net/iucv: return EOF to a reader parked when the peer closes

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> A recvmsg() that starts after the peer closed returns 0. One that was
> already waiting returns -ENOTCONN: its wait ends on sk_state and does
> not ask whether anything is left to read. In a z/VM guest over
> HiperSockets that was every parked reader, 13627 of 13627 on net-next
> [1]. poll() does not report EPOLLRDHUP for the close either.
> 
> Return EOF from the wait too when the socket is IUCV_DISCONN with
> nothing left to read. As for a call that starts afterwards, it does not
> matter how the socket got there: the peer's FIN, a severed z/VM path, a
> transmit error, a window update that could not be sent or the device
> going down. EOF does not say that all the peer sent was read: a severed
> z/VM path drops the messages not yet received, for either call. Set
> RCV_SHUTDOWN where the HiperSockets FIN is applied, as TCP does: poll()
> then reports EPOLLRDHUP and EPOLLRDNORM for the close, and EPOLLHUP if
> the socket had shut down its own sending. The other ways still report
> none of the three.
> 
> Fixes: 561e036006dc ("[AF_IUCV]: Implementation of a skb backlog queue")
> Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
> Reported-by: Hidayath Khan <hidayath@linux.ibm.com>
> Closes: https://lore.kernel.org/netdev/82691101-92d7-4838-b16e-2e41a83f65d0@linux.ibm.com/
> Link: https://lore.kernel.org/netdev/51a4a40a-f34b-4d41-b049-69aa53f4dcf5@linux.ibm.com/ [1]
> Cc: stable@vger.kernel.org # 6.18.x
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-b4-disp-2a6b4364-v3-0-35677b68e697@proton.me?part=7