Re: [PATCH net-next] net/iucv: serialize HiperSockets receive with the socket lock

[netdev-bot+sashiko@kernel.org]

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

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

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

- [Medium] afiucv_hs_backlog_add() takes the data budget from the live
  READ_ONCE(iucv->msglimit) + 1, on the assumption that this equals the…
- [Medium] The comments on afiucv_hs_defer() and afiucv_hs_backlog_add()
  size both budgets on one claim: data and WIN frames only ever wait…
- [Medium] Repeated SHT frames can still push msg_recv past msglimit, so
  recvmsg() takes WARN_ON(1), __iucv_sock_close() and -EFAULT instead of…
- [Low] The commit message describes user-visible bugs that date back to
  3881ac441f64 ("af_iucv: add HiperSockets transport"): a FIN or SYN|ACK…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is high
  severity.
- [Low] In iucv_sock_recv_skb(), a reader that is asleep when the peer's
  FIN arrives gets -ENOTCONN, while a recvmsg() that starts after the…

--

net/iucv: serialize HiperSockets receive with the socket lock

This patch makes afiucv_hs_rcv() take bh_lock_sock() once. While a task
owns the socket, frames are deferred to the socket backlog, which is
bounded by frame count and drained by a per-socket sk_backlog_rcv. It also
runs recvmsg() under lock_sock(), lets sendmsg() wait for send memory
without the lock, and sends the FIN and first SHT past sk_sndbuf.

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

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 5ba875798aa41..1fcb47a087551 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c

[ ... ]

> @@ -374,26 +377,28 @@ static void iucv_sever_path(struct sock *sk, int with_user_data)
>  static int iucv_send_ctrl(struct sock *sk, u8 flags)
>  {

[ ... ]

> -	skb = sock_alloc_send_skb(sk, blen, 1, &err);
> -	if (skb) {
> -		skb_reserve(skb, blen);
> -		err = afiucv_hs_send(NULL, sk, skb, flags);
> +	/* Control frames go out after a shutdown too. The FIN and the first SHT
> +	 * end the stream and may exceed sk_sndbuf, like TCP's FIN; at sk_sndbuf
> +	 * the others get -EAGAIN and arm SIGIO, as sock_alloc_send_skb() does.
> +	 */
> +	force = flags == AF_IUCV_FLAG_FIN ||
> +		(flags == AF_IUCV_FLAG_SHT &&
> +		 !(sk->sk_shutdown & SEND_SHUTDOWN));

[Severity: Low]
The only trailers are Suggested-by, Link and Signed-off-by. There is no
Fixes: tag.

The changelog describes bugs users can see that go back to 3881ac441f64
("af_iucv: add HiperSockets transport"):

- a SHT or FIN lost to a full send buffer
- a window update that fails and keeps its credit, so recvmsg() hits
  WARN_ON(1) and returns -EFAULT

The notes below the --- line say this iucv_send_ctrl() change could go to
net on its own with Fixes: 3881ac441f64. They also say the credit drop in
recvmsg() fixes a bug in net today.

If both stay folded into one net-next patch, will stable pick them up?
Could the iucv_send_ctrl() fix, and possibly the credit drop, go to net
as separate patches with Fixes: tags?

[ ... ]

> @@ -1245,6 +1267,47 @@ static void iucv_process_message_q(struct sock *sk)
>  	}
>  }
>
> +/*
> + * Like skb_recv_datagram(), but sleep with the socket lock released, so that
> + * release_sock() runs the backlog and a deferred FIN reaches the reader.
> + */
> +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;
> +			break;
> +		}

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. A
reader that is asleep here when the peer's FIN arrives gets -ENOTCONN. A
recvmsg() that starts after the FIN returns 0 through the IUCV_DISCONN
shortcut at the top of iucv_sock_recvmsg().

The old path, skb_recv_datagram()->__skb_wait_for_more_packets(), also
returned -ENOTCONN in this case, because IUCV_CONNECTED equals
TCP_ESTABLISHED. So this keeps the existing behaviour.

Should a blocked reader also see EOF here, like a reader that arrives
after the FIN?

[ ... ]

> @@ -1322,8 +1386,9 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>  			atomic_inc(&iucv->msg_recv);
>  			if (atomic_read(&iucv->msg_recv) > iucv->msglimit) {
>  				WARN_ON(1);
> -				iucv_sock_close(sk);
> -				return -EFAULT;
> +				__iucv_sock_close(sk);
> +				err = -EFAULT;
> +				goto out_err;
>  			}
>  		}
>
> @@ -1350,13 +1415,18 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>  		}
>  		spin_unlock_bh(&iucv->message_q.lock);
>
> -		if (send_win) {
> +		if (send_win && sk->sk_state == IUCV_CONNECTED) {
>  			err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
>  			if (err) {
>  				sk->sk_state = IUCV_DISCONN;
>  				sk->sk_state_change(sk);
>  			}
>  		}
> +		/* the peer won't take this credit: drop it, or a SHT's
> +		 * zero-length skb counts past msglimit
> +		 */
> +		if (send_win && sk->sk_state != IUCV_CONNECTED)
> +			atomic_set(&iucv->msg_recv, 0);

[Severity: Medium]
Can repeated SHT frames still drive recvmsg() into the WARN_ON(1) and
-EFAULT above?

On the peer, every shutdown(SHUT_WR) while connected sends another SHT:

iucv_sock_shutdown()
    ...
    } else
        iucv_send_ctrl(sk, AF_IUCV_FLAG_SHT);

These SHTs don't use any window, since only sendmsg() increments
msg_sent. Here, each one becomes a zero-length skb, and consuming it does
atomic_inc(&iucv->msg_recv).

msg_recv is only reset through send_win. send_win is only set when
backlog_skb_q is empty after the requeue loop, and the new credit drop
depends on send_win too.

Suppose the peer sends a full window of data, then enough repeated SHTs
that the receive queue spills into backlog_skb_q. backlog_skb_q may then
stay non-empty while msg_recv keeps climbing.

Would the first SHT read then push msg_recv past iucv->msglimit? That
would make recvmsg() call __iucv_sock_close() and return -EFAULT instead
of EOF. The comment in afiucv_hs_backlog_add() says "a repeated one
carries nothing", but such an SHT still seems to count here.

[ ... ]

> @@ -1894,7 +1966,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) {
> @@ -1904,7 +1975,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;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does this refusal path
leak nsk?

iucv_sock_kill() does nothing unless SOCK_ZAPPED is set:

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

iucv_sock_alloc() clears SOCK_ZAPPED, and only __iucv_sock_close() sets
it. None of these paths call __iucv_sock_close(). So each SYN refused
here seems to leave a whole iucv_sock linked on iucv_sk_list, still
holding its sk_alloc() reference. That covers a listener not in
IUCV_LISTEN and a full accept queue.

The same applies to the iucv_sock_kill(nsk) after a failed
dev_queue_xmit() of the SYN|ACK further down. There, the
dev_hold(niucv->hs_dev) reference would leak too, and
unregister_netdev() of that device could wait forever.

A peer on the HiperSockets LAN can repeat connect() against a listener
with a full accept queue, so this can grow without bound. The leaked
sockets also lengthen the iucv_sk_list walk in afiucv_hs_rcv() for every
frame.

With this patch, deferred SYNs reach the same path through
afiucv_hs_backlog_rcv()->afiucv_hs_dispatch()->afiucv_hs_callback_syn().
The changelog also names the full-accept-queue refusal as the model for
the new refusal past the budget.

[ ... ]

> @@ -2066,6 +2131,64 @@ static int afiucv_hs_callback_rx(struct sock *sk, struct sk_buff *skb)
>  	return NET_RX_SUCCESS;
>  }
>
> +/* data, SHT and WIN frames: they leave sk_state alone */
> +static bool afiucv_hs_is_data(u8 flags)
> +{
> +	return !flags || flags == AF_IUCV_FLAG_SHT || flags == AF_IUCV_FLAG_WIN;
> +}
> +
> +/*
> + * Frames that change sk_state wait while the user owns the socket. Data, SHT
> + * and WIN frames go through, but never past waiting ones: sk_backlog.len stays
> + * set until __release_sock() has run them all. So they wait only behind a FIN,
> + * which voids them, or a SYN|ACK, when we have sent nothing for a WIN to ack.
> + */
> +static bool afiucv_hs_defer(struct sock *sk, u8 flags)
> +{
> +	return !afiucv_hs_is_data(flags) || sk->sk_backlog.len != 0;
> +}

[Severity: Medium]
Does this classification hold up for frames the dispatcher doesn't act
on?

While the socket is owned, every flags value outside {0, SHT, WIN} is
deferred and charged to sk_backlog_ctrl. That includes values that
afiucv_hs_dispatch() only frees in its default case. It also includes a
SYN|ACK or SYN|FIN for a socket that isn't IUCV_BOUND, which the
callbacks just drop.

On a socket that isn't listening, the control limit is
sk_max_ack_backlog + 2, which is 2. Two such frames in one ownership
period would leave no room for a real FIN. The FIN would then go to
kfree_skb_reason(skb, SKB_DROP_REASON_SOCKET_BACKLOG) and never be
resent.

Each deferred stray frame also makes sk_backlog.len non-zero. Later data
and pure WIN frames are then deferred too and charged to the
msglimit + 1 data budget.

With traffic in both directions, msglimit data frames plus two or more
pure WINs, could data frames be dropped as well? Before this patch these
stray frames were freed right away.

Would it make sense to drop unknown flags in afiucv_hs_rcv() before
deciding whether to defer?

> +
> +/*
> + * Not sk_add_backlog(): its limit is in bytes, and nothing resends a frame it
> + * drops. Count frames instead. Data, SHT and WIN frames get msglimit + 1: a
> + * peer that keeps to the window has at most msglimit data frames and one SHT
> + * waiting (a repeated one carries nothing); WINs need no room (see above). The
> + * rest get sk_max_ack_backlog + 2: a SYN|ACK and a FIN, or a listener's SYNs up
> + * to its accept queue and one more. Both counts restart only once
> + * __release_sock() has run every frame: a flood cannot keep the owner draining.
> + */
> +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, limit;
> +
> +	if (!sk->sk_backlog.len) {
> +		iucv->sk_backlog_data = 0;
> +		iucv->sk_backlog_ctrl = 0;
> +	}
> +	if (afiucv_hs_is_data(flags)) {
> +		count = &iucv->sk_backlog_data;
> +		limit = READ_ONCE(iucv->msglimit) + 1;

[Severity: Medium]
Can this budget end up smaller than the window the peer was granted?

The SYN advertises the msglimit that is current at connect() time:

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

However, iucv_sock_setsockopt(SO_MSGLIMIT) still accepts the option in
IUCV_BOUND. A HiperSockets socket stays in IUCV_BOUND from the moment
iucv_sock_connect() sends its SYN until the SYN|ACK arrives. That window
is open after:

- a non-blocking, timed-out or interrupted connect()
- another thread calls setsockopt() while a blocking connect() sleeps in
  iucv_sock_wait() with the lock released

If msglimit is lowered in that window, the peer can still send a full
window of the old size. Those frames wait behind the deferred SYN|ACK.
Every frame past the new msglimit + 1 is dropped here with
SKB_DROP_REASON_SOCKET_BACKLOG and never resent, so stream data is lost
without any error. The msg_recv > iucv->msglimit check in
iucv_sock_recvmsg() would also use the lowered value.

The notes below the --- line call this the one remaining gap, but the
code still allows it. Could SO_MSGLIMIT be rejected once a HiperSockets
SYN has gone out? Alternatively, could the advertised window be saved and
used for this budget?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-b4-disp-410518f4-v1-1-753bf3258292%40proton.me