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

[Pavel Begunkov <asml.silence@gmail.com>]

On 8/12/26 13:01, Eric Dumazet wrote:
> On Tue, Aug 11, 2026 at 9:54 PM Mina Almasry <almasrymina@google.com> 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 <asml.silence@gmail.com>
>> Cc: Stanislav Fomichev <sdf@fomichev.me>
>> Cc: Bobby Eshleman <bobbyeshleman@gmail.com>
>> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
>> Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
>> Signed-off-by: Mina Almasry <almasrymina@google.com>
>> ---
>>   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 ?

I'd say it is *supposed* to be null as device memory without zero
copy doesn't make sense, but it looks like that can happen if there
is no NETIF_F_SG. How about rejecting it? It'd EFAULT somewhere in
skb_copy_to_page_nocache() anyway.

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 455441f1b694..f403830af65f 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1162,6 +1162,10 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
  					binding = NULL;
  					goto out_err;
  				}
+				if (zc != MSG_ZEROCOPY) {
+					err = -EOPNOTSUPP;
+					goto out_err;
+				}
  			}
  		}
  	} else if (unlikely(msg->msg_flags & MSG_SPLICE_PAGES) && size) {


> Also testing the condition right after a fresh skb was allocated is
> adding unecessary cost.

FWIW, we can even remove all extra overhead with a new SKBFL flag and
checking it together with likes of skb_zcopy_pure(), but IMHO it's
better to be done on top if needed.

-- 
Pavel Begunkov