[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