Re: [PATCH net v3 2/2] net: tcp: block standard payload injection into devmem skbs

On Wed, Aug 12, 2026 at 5:01 AM Eric Dumazet <[email protected]> wrote:
>
> On Tue, Aug 11, 2026 at 9:54 PM Mina Almasry <[email protected]> wrote:
> >
> > Protect tcp_sendmsg_locked() from mistakenly appending non-zerocopy
> > page fragments to unreadable devmem skbs. Create a new segment instead.
> >
> > Fixes: bd61848900bff ("net: devmem: Implement TX path")
> > Cc: Pavel Begunkov <[email protected]>
> > Cc: Stanislav Fomichev <[email protected]>
> > Cc: Bobby Eshleman <[email protected]>
> > Reviewed-by: Pavel Begunkov <[email protected]>
> > Reviewed-by: Bobby Eshleman <[email protected]>
> > Signed-off-by: Mina Almasry <[email protected]>
> > ---
> >  net/ipv4/tcp.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 455441f1b6949..186a36c698798 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -1278,6 +1278,11 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
> >                 if (copy > msg_data_left(msg))
> >                         copy = msg_data_left(msg);
> >
> > +               if (zc != MSG_ZEROCOPY && unlikely(!skb_frags_readable(skb))) {
>
> This seems wrong, as @binding could be NULL or not ?
> Also testing the condition right after a fresh skb was allocated is
> adding unecessary cost.
>

Ah, my bad. We were indeed checking zc only and assuming if zc ==
MSG_ZEROCOPY, then also binding is non-NULL and the send is devmem.
That is not correct.

> > +                       tcp_mark_push(tp, skb);
> > +                       goto new_segment;
> > +               }
> > +
> >                 if (zc == 0) {
> >                         bool merge = true;
> >                         int i = skb_shinfo(skb)->nr_frags;
> > --
> > 2.55.0.679.g6767b8d81c-goog
> >
>
> What about instead:
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 455441f1b694904172cfa1d8e7bac7076b60cb24..b4237d0e994d6f9d754d2167023e3981a40b58f4
> 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:


This fix is much better and looks like it will work. FWIW I also
checked other call sites of tcp_skb_can_collapse_to like
tcp_should_autocork and mptcp_skb_can_collapse_to and it seems both
work as-is and don't need any changes. I'll rev v4 after the cooldown.
Thanks!

-- 
Thanks,
Mina