Re: [PATCH net v2 2/3] net/iucv: claim the receive credit atomically

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

Pre-existing issues:
- [High] Positive network drop errors leak to userspace as partial success
- [High] Silent data loss for SOCK_STREAM when packet length exceeds MTU
--

--- Patch [2]: [PATCH net v2 2/3] net/iucv: claim the receive credit atomically ---
commit 20117da9fdf439cdc2d9a9928d88d8e19d4f0e83
Author: Bryam Vargas <[email protected]>

net/iucv: claim the receive credit atomically

This commit addresses a race condition in the af_iucv HiperSockets
transport where the receive credit counter could be subtracted multiple times.
It modifies the code to claim the window using atomic_xchg() right before
queueing the packet to ensure the counter reads zero while the transmit
is in flight.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 0bc4a15f4b565..492a45bb2bba6 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c

[ ... ]

> @@ -250,13 +244,22 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
>  	}
>  	skb->protocol = cpu_to_be16(ETH_P_AF_IUCV);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential issue with
the MTU check just above this line in afiucv_hs_send():

	if (skb->len > skb->dev->mtu) {
		if (sock->sk_type == SOCK_SEQPACKET) {
			err = -EMSGSIZE;
			goto err_free;
		}
		err = pskb_trim(skb, skb->dev->mtu);
		if (err)
			goto err_free;
	}

For SOCK_STREAM sockets, if the packet is larger than the MTU, the skb is
silently truncated via pskb_trim().

However, iucv_sock_sendmsg() isn't aware of this truncation and returns the full
original length to userspace on success:

	err = afiucv_hs_send(&txmsg, sk, skb, 0);
	...
	release_sock(sk);
	return len;

Does this result in silent data loss, since userspace believes the entire buffer
was successfully sent?

>  
> +	/* Claim the receive credit here, not while building the header: every
> +	 * way this frame can be dropped has now been ruled out, so the window
> +	 * is zeroed only for as long as the transmit itself takes.
> +	 */
> +	if (flags == AF_IUCV_FLAG_WIN || !flags) {
> +		confirm_recv = atomic_xchg(&iucv->msg_recv, 0);
> +		phs_hdr->window = confirm_recv;
> +		if (confirm_recv)
> +			phs_hdr->flags = phs_hdr->flags | AF_IUCV_FLAG_WIN;
> +	}
> +
>  	atomic_inc(&iucv->skbs_in_xmit);
>  	err = dev_queue_xmit(skb);
>  	if (net_xmit_eval(err)) {
>  		atomic_dec(&iucv->skbs_in_xmit);
> -	} else {
> -		atomic_sub(confirm_recv, &iucv->msg_recv);
> -		WARN_ON(atomic_read(&iucv->msg_recv) < 0);
> +		atomic_add(confirm_recv, &iucv->msg_recv);
>  	}
>  	return net_xmit_eval(err);

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the return value here.

When dev_queue_xmit() drops a packet, it returns NET_XMIT_DROP (0x01).
net_xmit_eval() translates NET_XMIT_DROP to 1, and returning this positive value
propagates to iucv_sock_sendmsg().

Since iucv_sock_sendmsg() passes positive error codes back to the socket layer:

	err = afiucv_hs_send(&txmsg, sk, skb, 0);
	if (err) {
		atomic_dec(&iucv->msg_sent);
		goto out;
	}

Could sys_sendmsg interpret the return value of 1 as 1 byte successfully sent,
causing userspace to incorrectly assume partial success and potentially enter
an infinite loop retrying the remaining data?

Should this be returning net_xmit_errno(err) instead of net_xmit_eval(err)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2