On Tue, Aug 18, 2026 at 3:26 AM Paolo Abeni <[email protected]> wrote: > > 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.
Pre-existing issue I think. We indeed assume binding != NULL means the send is devmem and zc is set. We missed that we may not set zc if NETIF_F_SG is not enabled. Probably the code setting binding to non-NULL should check if zc is set. I think this is best reserved for its own follow up fix, but let me know if not Paolo. -- Thanks, Mina _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
