From: Bryam Vargas 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 --- 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 From: Bryam Vargas 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 --- 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 From: Bryam Vargas 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 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 --- 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 From: Bryam Vargas 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 --- 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 From: Bryam Vargas 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 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 --- 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 From: Bryam Vargas 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 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 --- 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 From: Bryam Vargas 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 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 --- 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