Re: [PATCH net v4 2/2] net: tcp: block mixing readable and unreadable frags

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: tcp: block mixing readable and unreadable frags

This adds a check in tcp_sendmsg_locked() so an existing tail SKB is not
collapsed into when its readability does not match the presence of a
devmem binding, forcing a new segment instead.

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 455441f1b6949..b4237d0e994d6 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1240,7 +1240,8 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
>  
>  		trace_tcp_sendmsg_locked(sk, msg, skb, size_goal);
>  
> -		if (copy <= 0 || !tcp_skb_can_collapse_to(skb)) {
> +		if (copy <= 0 || !tcp_skb_can_collapse_to(skb) ||
> +		    unlikely(skb_frags_readable(skb) != !binding)) {
>  			bool first_skb;
>  
>  new_segment:

[Severity: Medium]
Is binding alone the right proxy here for "this send will append
unreadable frags"?

What actually selects the devmem filler is the zc dispatch in the loop
plus __zerocopy_sg_from_iter():

net/core/datagram.c:__zerocopy_sg_from_iter() {
	...
	else if (binding)
		ret = zerocopy_fill_skb_from_devmem(skb, from, length, binding);
	else
		ret = zerocopy_fill_skb_from_iter(skb, from, length);
}

and that is only reached from the zc == MSG_ZEROCOPY arm of
tcp_sendmsg_locked().

Can binding != NULL coexist with zc == 0?  In tcp_sendmsg_locked() the
binding is taken whenever the cmsg carries dmabuf_id:

		if (sk->sk_route_caps & NETIF_F_SG)
			zc = MSG_ZEROCOPY;
		else
			uarg_to_msgzc(uarg)->zerocopy = 0;

		if (!sockc_err && sockc.dmabuf_id) {
			binding = net_devmem_get_binding(sk, sockc.dmabuf_id);

while zc is only set under NETIF_F_SG.  The one dmabuf_id validation
does not look at SG at all:

	if (!sockc_err && sockc.dmabuf_id &&
	    (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY))) {
		err = -EINVAL;

And sk_setup_caps() in net/core/sock.c only force-adds NETIF_F_SG when
sk_can_gso() holds:

	if (unlikely(sk->sk_gso_disabled))
		sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
	if (sk_can_gso(sk)) {
		if (dst->header_len && !xfrm_dst_offload_ok(dst)) {
			sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
		} else {
			sk->sk_route_caps |= NETIF_F_SG | NETIF_F_HW_CSUM;

so a TCP-MD5/TCP-AO socket (sk_gso_disable()) or an IPsec dst on a device
with sg turned off ends up with binding != NULL and zc == 0.

In that state, does the new predicate go wrong in both directions?

If the tail is readable or newly allocated, skb_frags_readable(skb) !=
!binding evaluates to true != false, so every iteration takes
goto new_segment and the write is split into one skb per iteration, even
though the zc == 0 path only appends readable page frags via
skb_fill_page_desc().

If the tail is already unreadable from an earlier devmem send, the test
is false != false, collapsing is allowed, and skb_fill_page_desc() adds a
readable page frag to an skb that __skb_fill_netmem_desc() already marked
unreadable = 1 — the mixing this patch aims to prevent.  Would
validate_xmit_unreadable_skb() catch that?  It only looks at the first
frag:

net/core/dev.c:validate_xmit_unreadable_skb() {
	if (shinfo->nr_frags > 0) {
		niov = netmem_to_net_iov(skb_frag_netmem(&shinfo->frags[0]));

Would keying the new clause on binding && zc == MSG_ZEROCOPY, or
rejecting dmabuf_id when the route cannot do SG, line the predicate up
with the code that actually fills the frags?

Note the mixing described in the second direction pre-dates this patch
(there was no check at all before); the incorrect predicate and the
forced re-segmentation are new.  Reaching this also needs a local
privileged setup (dmabuf TX binding plus MD5/AO or IPsec plus an ethtool
feature change), and in that configuration a devmem send through the copy
path would likely already fail in skb_copy_to_page_nocache() since the
iovec holds dmabuf offsets rather than user addresses, so this is stated
as an observation rather than a demonstrated corruption path.
-- 
This is an AI-generated review.