[PATCH net 1/1] tcp: reject non zerocopy devmem tx

[PATCH net 1/1] tcp: reject non zerocopy devmem tx

[Pavel Begunkov]

Device memory send doesn't work without zero copy, prevent
tcp_sendmsg_locked() from falling back to the copy mode for devmem in
case there is no NETIF_F_SG. Currently, it'd try to copy from iovec
filled with dma-buf offsets, which would normally fail with EFAULT, but
it's still better to handle it more explicitly. A recent net-iov / page
mixing fix also needs this patch.

Fixes: bd61848900bff ("net: devmem: Implement TX path")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 net/ipv4/tcp.c | 4 ++++
 1 file changed, 4 insertions(+)

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) {
-- 
2.54.0

Re: [PATCH net 1/1] tcp: reject non zerocopy devmem tx

[Mina Almasry]

On Sat, Aug 22, 2026 at 3:57 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> Device memory send doesn't work without zero copy, prevent
> tcp_sendmsg_locked() from falling back to the copy mode for devmem in
> case there is no NETIF_F_SG. Currently, it'd try to copy from iovec
> filled with dma-buf offsets, which would normally fail with EFAULT, but
> it's still better to handle it more explicitly. A recent net-iov / page
> mixing fix also needs this patch.
>
> Fixes: bd61848900bff ("net: devmem: Implement TX path")
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  net/ipv4/tcp.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> 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) {
> --
> 2.54.0
>

Thanks, I was indeed putting together patches to fix all the
pre-existing issues. I think the code assumes devmem send means
binding is not NULL, and the fact that we efault on copying is lucky,
we should not even attempt copying.

How about we reduce the complexity of an already-long
tcp_sendmsg_locked function by combining this check with the other
ones? I was thinking this:

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index b4237d0e994d6..1e34f50770446 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1170,7 +1170,8 @@ int tcp_sendmsg_locked(struct sock *sk, struct
msghdr *msg, size_t size)
        }

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

It will return EINVAL instead of EOPNOTSUPP but I think that is OK.

-- 
Thanks,
Mina

Re: [PATCH net 1/1] tcp: reject non zerocopy devmem tx

[David Laight]

On Sun, 23 Aug 2026 08:58:32 -0700
Mina Almasry <almasrymina@google.com> wrote:

> On Sat, Aug 22, 2026 at 3:57 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
> >
> > Device memory send doesn't work without zero copy, prevent
> > tcp_sendmsg_locked() from falling back to the copy mode for devmem in
> > case there is no NETIF_F_SG. Currently, it'd try to copy from iovec
> > filled with dma-buf offsets, which would normally fail with EFAULT, but
> > it's still better to handle it more explicitly. A recent net-iov / page
> > mixing fix also needs this patch.
> >
> > Fixes: bd61848900bff ("net: devmem: Implement TX path")
> > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> > ---
> >  net/ipv4/tcp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > 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) {
> > --
> > 2.54.0
> >  
> 
> Thanks, I was indeed putting together patches to fix all the
> pre-existing issues. I think the code assumes devmem send means
> binding is not NULL, and the fact that we efault on copying is lucky,
> we should not even attempt copying.
> 
> How about we reduce the complexity of an already-long
> tcp_sendmsg_locked function by combining this check with the other
> ones? I was thinking this:
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index b4237d0e994d6..1e34f50770446 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1170,7 +1170,8 @@ int tcp_sendmsg_locked(struct sock *sk, struct
> msghdr *msg, size_t size)
>         }
> 
>         if (!sockc_err && sockc.dmabuf_id &&
> -           (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY))) {
> +           (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY) ||
> +            zc != MSG_ZEROCOPY)) {

That might be more readable with the ! moved outside the ():
> +           !((flags & MSG_ZEROCOPY) && sock_flag(sk, SOCK_ZEROCOPY) &&
> +            zc == MSG_ZEROCOPY)) {

David

>                 err = -EINVAL;
>                 goto out_err;
>         }
> 
> It will return EINVAL instead of EOPNOTSUPP but I think that is OK.
> 

Re: [PATCH net 1/1] tcp: reject non zerocopy devmem tx

[Pavel Begunkov]

On 8/23/26 16:58, Mina Almasry wrote:
> On Sat, Aug 22, 2026 at 3:57 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
...> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index b4237d0e994d6..1e34f50770446 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1170,7 +1170,8 @@ int tcp_sendmsg_locked(struct sock *sk, struct
> msghdr *msg, size_t size)
>          }
> 
>          if (!sockc_err && sockc.dmabuf_id &&
> -           (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY))) {
> +           (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY) ||

This check before your diff fells off. From a quick look, via io_uring
you can have a sendmsg call with dmabuf_id that would happily pass this
check but never goes through net_devmem_get_binding() and
msg_zerocopy_realloc(). It's not much too problematic for now but feels
dangerous to rely on it in the current form. I think we want sth like
this in the end.

if (sockc.dmabuf_id && !sockc_err) {
	if (!binding || zc != MSG_ZEROCOPY)
		// fail;
}

> +            zc != MSG_ZEROCOPY)) {
>                  err = -EINVAL;
>                  goto out_err;
>          }
> 
> It will return EINVAL instead of EOPNOTSUPP but I think that is OK.

-- 
Pavel Begunkov

Re: [PATCH net 1/1] tcp: reject non zerocopy devmem tx

[Mina Almasry]

On Wed, Aug 26, 2026 at 7:36 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 8/23/26 16:58, Mina Almasry wrote:
> > On Sat, Aug 22, 2026 at 3:57 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
> ...> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index b4237d0e994d6..1e34f50770446 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -1170,7 +1170,8 @@ int tcp_sendmsg_locked(struct sock *sk, struct
> > msghdr *msg, size_t size)
> >          }
> >
> >          if (!sockc_err && sockc.dmabuf_id &&
> > -           (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY))) {
> > +           (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY) ||
>
> This check before your diff fells off. From a quick look, via io_uring
> you can have a sendmsg call with dmabuf_id that would happily pass this
> check but never goes through net_devmem_get_binding() and
> msg_zerocopy_realloc(). It's not much too problematic for now but feels
> dangerous to rely on it in the current form. I think we want sth like
> this in the end.
>
> if (sockc.dmabuf_id && !sockc_err) {
>         if (!binding || zc != MSG_ZEROCOPY)
>                 // fail;
> }
>

OK it sorta makes sense to me. I think we're making a mistake checking
SOCK_ZEROCOPY/MSG_ZEROCOPY when instead all these are conveniently
condensed into 'zc == MSG_ZEROCOPY' when everything lines up. The
point of this line was roughly 'if the user is trying to send devmem
but not MSG_ZEROCOPY'd, fail'.

-- 
Thanks,
Mina

Re: [PATCH net 1/1] tcp: reject non zerocopy devmem tx

[Pavel Begunkov]

On 8/26/26 18:29, Mina Almasry wrote:
> On Wed, Aug 26, 2026 at 7:36 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> On 8/23/26 16:58, Mina Almasry wrote:
>>> On Sat, Aug 22, 2026 at 3:57 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>> ...> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>>> index b4237d0e994d6..1e34f50770446 100644
>>> --- a/net/ipv4/tcp.c
>>> +++ b/net/ipv4/tcp.c
>>> @@ -1170,7 +1170,8 @@ int tcp_sendmsg_locked(struct sock *sk, struct
>>> msghdr *msg, size_t size)
>>>           }
>>>
>>>           if (!sockc_err && sockc.dmabuf_id &&
>>> -           (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY))) {
>>> +           (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY) ||
>>
>> This check before your diff fells off. From a quick look, via io_uring
>> you can have a sendmsg call with dmabuf_id that would happily pass this
>> check but never goes through net_devmem_get_binding() and
>> msg_zerocopy_realloc(). It's not much too problematic for now but feels
>> dangerous to rely on it in the current form. I think we want sth like
>> this in the end.
>>
>> if (sockc.dmabuf_id && !sockc_err) {
>>          if (!binding || zc != MSG_ZEROCOPY)
>>                  // fail;
>> }
>>
> 
> OK it sorta makes sense to me. I think we're making a mistake checking
> SOCK_ZEROCOPY/MSG_ZEROCOPY when instead all these are conveniently
> condensed into 'zc == MSG_ZEROCOPY' when everything lines up. The
> point of this line was roughly 'if the user is trying to send devmem
> but not MSG_ZEROCOPY'd, fail'.

Alright, good to know as I was assuming that. Let's do it like that
then, I'll send a patch.

-- 
Pavel Begunkov