[PATCH net v2] net: devmem: prevent net-iov / page mixing

[PATCH net v2] net: devmem: prevent net-iov / page mixing

[Pavel Begunkov]

We should either have net_iov or page backed frags in a single skb,
otherwise it blows up down the stack. Don't allow mixing in
zerocopy_fill_skb_from_devmem().

Fixes: bd61848900bff ("net: devmem: Implement TX path")
Cc: stable@vger.kernel.org
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---

v2: EEXIST -> EFAULT, as skb_zerocopy_iter_stream() doesn't handle
    the former + for consistency. 

 net/core/datagram.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index c285c6465923..173b5d97bd40 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -712,6 +712,9 @@ zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
 	size_t virt_addr, size, off;
 	struct net_iov *niov;
 
+	if (i && skb_frags_readable(skb))
+		return -EFAULT;
+
 	/* Devmem filling works by taking an IOVEC from the user where the
 	 * iov_addrs are interpreted as an offset in bytes into the dma-buf to
 	 * send from. We do not support other iter types.
-- 
2.54.0

Re: [PATCH net v2] net: devmem: prevent net-iov / page mixing

[Bobby Eshleman]

On Mon, Jul 27, 2026 at 12:19:37PM +0100, Pavel Begunkov wrote:
> We should either have net_iov or page backed frags in a single skb,
> otherwise it blows up down the stack. Don't allow mixing in
> zerocopy_fill_skb_from_devmem().
> 
> Fixes: bd61848900bff ("net: devmem: Implement TX path")
> Cc: stable@vger.kernel.org
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
> 
> v2: EEXIST -> EFAULT, as skb_zerocopy_iter_stream() doesn't handle
>     the former + for consistency. 
> 
>  net/core/datagram.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index c285c6465923..173b5d97bd40 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -712,6 +712,9 @@ zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
>  	size_t virt_addr, size, off;
>  	struct net_iov *niov;
>  
> +	if (i && skb_frags_readable(skb))
> +		return -EFAULT;
> +

Doesn't this still break the allowed scenario where the user does
sendmsg(not devmem) before sendmsg(devmem), and tcp tries to append to
the write tail skb?

Best,
Bobby

Re: [PATCH net v2] net: devmem: prevent net-iov / page mixing

[Pavel Begunkov]

On 7/27/26 17:07, Bobby Eshleman wrote:
> On Mon, Jul 27, 2026 at 12:19:37PM +0100, Pavel Begunkov wrote:
>> We should either have net_iov or page backed frags in a single skb,
>> otherwise it blows up down the stack. Don't allow mixing in
>> zerocopy_fill_skb_from_devmem().
>>
>> Fixes: bd61848900bff ("net: devmem: Implement TX path")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>
>> v2: EEXIST -> EFAULT, as skb_zerocopy_iter_stream() doesn't handle
>>      the former + for consistency.
>>
>>   net/core/datagram.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/net/core/datagram.c b/net/core/datagram.c
>> index c285c6465923..173b5d97bd40 100644
>> --- a/net/core/datagram.c
>> +++ b/net/core/datagram.c
>> @@ -712,6 +712,9 @@ zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
>>   	size_t virt_addr, size, off;
>>   	struct net_iov *niov;
>>   
>> +	if (i && skb_frags_readable(skb))
>> +		return -EFAULT;
>> +
> 
> Doesn't this still break the allowed scenario where the user does
> sendmsg(not devmem) before sendmsg(devmem), and tcp tries to append to
> the write tail skb?
Considering that it crashes the kernel, no, it doesn't.

-- 
Pavel Begunkov

Re: [PATCH net v2] net: devmem: prevent net-iov / page mixing

[Stanislav Fomichev]

On 07/27, Pavel Begunkov wrote:
> We should either have net_iov or page backed frags in a single skb,
> otherwise it blows up down the stack. Don't allow mixing in
> zerocopy_fill_skb_from_devmem().
> 
> Fixes: bd61848900bff ("net: devmem: Implement TX path")
> Cc: stable@vger.kernel.org
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
> 
> v2: EEXIST -> EFAULT, as skb_zerocopy_iter_stream() doesn't handle
>     the former + for consistency. 
> 
>  net/core/datagram.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index c285c6465923..173b5d97bd40 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -712,6 +712,9 @@ zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
>  	size_t virt_addr, size, off;
>  	struct net_iov *niov;
>  
> +	if (i && skb_frags_readable(skb))
> +		return -EFAULT;
> +

Maybe we should do -EMSGSIZE? It is already properly plumbed via
skb_zerocopy_iter_stream (and you'll hit 'skb->len == orig_len')
and it hits 'new_segment' in tcp_sendmsg_locked?

Re: [PATCH net v2] net: devmem: prevent net-iov / page mixing

[Bobby Eshleman]

On Mon, Jul 27, 2026 at 05:39:11PM +0100, Pavel Begunkov wrote:
> On 7/27/26 17:07, Bobby Eshleman wrote:
> > On Mon, Jul 27, 2026 at 12:19:37PM +0100, Pavel Begunkov wrote:
> > > We should either have net_iov or page backed frags in a single skb,
> > > otherwise it blows up down the stack. Don't allow mixing in
> > > zerocopy_fill_skb_from_devmem().
> > > 
> > > Fixes: bd61848900bff ("net: devmem: Implement TX path")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> > > ---
> > > 
> > > v2: EEXIST -> EFAULT, as skb_zerocopy_iter_stream() doesn't handle
> > >      the former + for consistency.
> > > 
> > >   net/core/datagram.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/net/core/datagram.c b/net/core/datagram.c
> > > index c285c6465923..173b5d97bd40 100644
> > > --- a/net/core/datagram.c
> > > +++ b/net/core/datagram.c
> > > @@ -712,6 +712,9 @@ zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
> > >   	size_t virt_addr, size, off;
> > >   	struct net_iov *niov;
> > > +	if (i && skb_frags_readable(skb))
> > > +		return -EFAULT;
> > > +
> > 
> > Doesn't this still break the allowed scenario where the user does
> > sendmsg(not devmem) before sendmsg(devmem), and tcp tries to append to
> > the write tail skb?
> Considering that it crashes the kernel, no, it doesn't.

Both are broken IMHO. Before this patch, the kernel crashes. After this
patch, the socket errs out to the user under a condition that it is
supposed to just handle. Why not just propagate up an errno that
triggers new_segment so it gets handled? 

Best,
Bobby

Re: [PATCH net v2] net: devmem: prevent net-iov / page mixing

[Pavel Begunkov]

On 7/27/26 17:46, Stanislav Fomichev wrote:
> On 07/27, Pavel Begunkov wrote:
>> We should either have net_iov or page backed frags in a single skb,
>> otherwise it blows up down the stack. Don't allow mixing in
>> zerocopy_fill_skb_from_devmem().
>>
>> Fixes: bd61848900bff ("net: devmem: Implement TX path")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>
>> v2: EEXIST -> EFAULT, as skb_zerocopy_iter_stream() doesn't handle
>>      the former + for consistency.
>>
>>   net/core/datagram.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/net/core/datagram.c b/net/core/datagram.c
>> index c285c6465923..173b5d97bd40 100644
>> --- a/net/core/datagram.c
>> +++ b/net/core/datagram.c
>> @@ -712,6 +712,9 @@ zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
>>   	size_t virt_addr, size, off;
>>   	struct net_iov *niov;
>>   
>> +	if (i && skb_frags_readable(skb))
>> +		return -EFAULT;
>> +
> 
> Maybe we should do -EMSGSIZE? It is already properly plumbed via
> skb_zerocopy_iter_stream (and you'll hit 'skb->len == orig_len')
> and it hits 'new_segment' in tcp_sendmsg_locked?

I made it consistent with zerocopy_fill_skb_from_iter(), don't see
the point of making it behaving differently from combination to
combination. Do you have some use case for that?

-- 
Pavel Begunkov

Re: [PATCH net v2] net: devmem: prevent net-iov / page mixing

[Stanislav Fomichev]

On 07/27, Pavel Begunkov wrote:
> On 7/27/26 17:46, Stanislav Fomichev wrote:
> > On 07/27, Pavel Begunkov wrote:
> > > We should either have net_iov or page backed frags in a single skb,
> > > otherwise it blows up down the stack. Don't allow mixing in
> > > zerocopy_fill_skb_from_devmem().
> > > 
> > > Fixes: bd61848900bff ("net: devmem: Implement TX path")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> > > ---
> > > 
> > > v2: EEXIST -> EFAULT, as skb_zerocopy_iter_stream() doesn't handle
> > >      the former + for consistency.
> > > 
> > >   net/core/datagram.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/net/core/datagram.c b/net/core/datagram.c
> > > index c285c6465923..173b5d97bd40 100644
> > > --- a/net/core/datagram.c
> > > +++ b/net/core/datagram.c
> > > @@ -712,6 +712,9 @@ zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
> > >   	size_t virt_addr, size, off;
> > >   	struct net_iov *niov;
> > > +	if (i && skb_frags_readable(skb))
> > > +		return -EFAULT;
> > > +
> > 
> > Maybe we should do -EMSGSIZE? It is already properly plumbed via
> > skb_zerocopy_iter_stream (and you'll hit 'skb->len == orig_len')
> > and it hits 'new_segment' in tcp_sendmsg_locked?
> 
> I made it consistent with zerocopy_fill_skb_from_iter(), don't see
> the point of making it behaving differently from combination to
> combination. Do you have some use case for that?

Ah, ok, yeah, that makes sense, let's go with that!

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net v2] net: devmem: prevent net-iov / page mixing

[Mina Almasry]

On Mon, Jul 27, 2026 at 4:19 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> We should either have net_iov or page backed frags in a single skb,
> otherwise it blows up down the stack. Don't allow mixing in
> zerocopy_fill_skb_from_devmem().
>
> Fixes: bd61848900bff ("net: devmem: Implement TX path")
> Cc: stable@vger.kernel.org
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>

I actually really liked the EEXIST because it looked to me like
tcp_sendmsg would handle that elegantly by sending the current skb,
and then starting a new skb with the new mem type. But it doesn't work
and as you noted there is some value in consistently. Also this is
much better than crashing.

We can follow up with another low priority improvement that allows
mixing and matching types in separate sendmsg calls and separate skbs
actually work. I think that would be nice.

Reviewed-by: Mina Almasry <almasrymina@google.com>

-- 
Thanks,
Mina

Re: [PATCH net v2] net: devmem: prevent net-iov / page mixing

[Bobby Eshleman]

On Mon, Jul 27, 2026 at 12:19:37PM +0100, Pavel Begunkov wrote:
> We should either have net_iov or page backed frags in a single skb,
> otherwise it blows up down the stack. Don't allow mixing in
> zerocopy_fill_skb_from_devmem().
> 
> Fixes: bd61848900bff ("net: devmem: Implement TX path")
> Cc: stable@vger.kernel.org
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
> 
> v2: EEXIST -> EFAULT, as skb_zerocopy_iter_stream() doesn't handle
>     the former + for consistency. 
> 
>  net/core/datagram.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index c285c6465923..173b5d97bd40 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -712,6 +712,9 @@ zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
>  	size_t virt_addr, size, off;
>  	struct net_iov *niov;
>  
> +	if (i && skb_frags_readable(skb))
> +		return -EFAULT;
> +
>  	/* Devmem filling works by taking an IOVEC from the user where the
>  	 * iov_addrs are interpreted as an offset in bytes into the dma-buf to
>  	 * send from. We do not support other iter types.
> -- 
> 2.54.0
> 

Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>