net/iucv: serialize the HiperSockets receive path with the socket lock

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/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/
[3] https://lore.kernel.org/netdev/[email protected]/
[4] https://lore.kernel.org/all/[email protected]/
[5] https://lore.kernel.org/netdev/[email protected]/
[6] https://lore.kernel.org/netdev/[email protected]/
[7] https://lore.kernel.org/netdev/[email protected]/
[8] https://lore.kernel.org/netdev/[email protected]/
[9] https://lore.kernel.org/netdev/[email protected]/
[10] https://lore.kernel.org/netdev/[email protected]/

---
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 <[email protected]>

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.

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

index df85d19fbf84..6decbb1f34b3 100644
@@ -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;

index ce13fe3c85b5..657c61161dbd 100644
@@ -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

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.

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

index 657c61161dbd..133c1d1886b2 100644
@@ -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

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.

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

index 133c1d1886b2..639725917f3f 100644
@@ -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

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.

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

index 639725917f3f..1677cc65ad06 100644
@@ -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

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.

---
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/[email protected]/
v1: https://lore.kernel.org/netdev/[email protected]/

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

index 6decbb1f34b3..6b2cb6c41b9e 100644
@@ -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);
 };

2.56.0

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.

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

index f31b037f28c1..2118effc137f 100644
@@ -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

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.

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

index 2118effc137f..9ae57cbd3efc 100644
@@ -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