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

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