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

[PATCH net 1/1] 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>
---
 net/core/datagram.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index c285c6465923..35febc1c25fa 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 -EEXIST;
+
 	/* 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 1/1] net: devmem: prevent net-iov / page mixing

[Bobby Eshleman]

On Wed, Jul 22, 2026 at 11:58:46AM +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>
> ---
>  net/core/datagram.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index c285c6465923..35febc1c25fa 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 -EEXIST;

Are we trying to hit the -EEXIST handler in tcp_sendmsg_locked() so that
we start a new skb? If so, I think we might need to plumb this -EEXIST
case through skb_zerocopy_iter_stream() too?

Best,
Bobby

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

[Pavel Begunkov]

On 7/22/26 18:59, Bobby Eshleman wrote:
> On Wed, Jul 22, 2026 at 11:58:46AM +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>
>> ---
>>   net/core/datagram.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/net/core/datagram.c b/net/core/datagram.c
>> index c285c6465923..35febc1c25fa 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 -EEXIST;
> 
> Are we trying to hit the -EEXIST handler in tcp_sendmsg_locked() so that
> we start a new skb? If so, I think we might need to plumb this -EEXIST
> case through skb_zerocopy_iter_stream() too?

Easier to EFAULT. It'd more consistent, and I don't care how tcp takes
it specifically.

-- 
Pavel Begunkov

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

[Mina Almasry]

On Wed, Jul 22, 2026 at 3:59 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>

It's true that we don't support mixing niov types and doing so would
blow up, but this is an unnecessary defensive check imo. The calling
code should not (and does not, I hope) have an edge case where it
tries to mix and match niov types.

Maybe a DEBUG_NET_WARN_ON check to help catch bugs in the calling code
may be fine?

Also we don't really support mixing different niov sub-types in an skb
(like IO_URING + DMABUF) afaict, so might as well go the extra mile
and check that it's all devmem niovs specifically.

And might as well put the check in skb_add_rx_frag_netmem. It's the
same situation on RX, we don't support mixing there (and I hope no
code path leads to mixing today).

-- 
Thanks,
Mina

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

[Pavel Begunkov]

On 7/22/26 20:49, Mina Almasry wrote:
> On Wed, Jul 22, 2026 at 3:59 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>
> 
> It's true that we don't support mixing niov types and doing so would
> blow up, but this is an unnecessary defensive check imo. The calling
> code should not (and does not, I hope) have an edge case where it
> tries to mix and match niov types.

Nope, that can easily happen.

> Maybe a DEBUG_NET_WARN_ON check to help catch bugs in the calling code
> may be fine?
> 
> Also we don't really support mixing different niov sub-types in an skb
> (like IO_URING + DMABUF) afaict, so might as well go the extra mile
> and check that it's all devmem niovs specifically.

All those type intermixing rules might be flaky, maybe we should
start with DEBUG_NET_WARN_ON, but at least for tx from a quick
look it's handled by ubuf_info checks.

> And might as well put the check in skb_add_rx_frag_netmem. It's the
> same situation on RX, we don't support mixing there (and I hope no
> code path leads to mixing today).

A DEBUG_NET_WARN_ON there might be a very good idea, I agree.

-- 
Pavel Begunkov

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

[Bobby Eshleman]

On Wed, Jul 22, 2026 at 12:49:07PM -0700, Mina Almasry wrote:
> On Wed, Jul 22, 2026 at 3:59 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>
> 
> It's true that we don't support mixing niov types and doing so would
> blow up, but this is an unnecessary defensive check imo. The calling
> code should not (and does not, I hope) have an edge case where it
> tries to mix and match niov types.
> 
> Maybe a DEBUG_NET_WARN_ON check to help catch bugs in the calling code
> may be fine?
> 
> Also we don't really support mixing different niov sub-types in an skb
> (like IO_URING + DMABUF) afaict, so might as well go the extra mile
> and check that it's all devmem niovs specifically.
> 
> And might as well put the check in skb_add_rx_frag_netmem. It's the
> same situation on RX, we don't support mixing there (and I hope no
> code path leads to mixing today).

Hey Mina and Pavel,

I was able to confirm this mixing case does exist.

It looks like in tcp_sendmsg_locked() when new message is non-devmem (zc
== 0) and tcp_write_queue_tail is devmem, the skb collapsing is allowed
(though same-frag merge is disallowed).

Adding a mode to ncdevmem that sends mixed devmem and non-devmem
messages, it can be caught hacking this in:

/* DEBUG (DROP ME): detect a TX skb that mixes devmem (net_iov, unreadable)
 * and non-devmem (page, readable) fragments. Such an skb must never exist.
 */
static bool tcp_dbg_skb_frags_mixed(const struct sk_buff *skb)
{
	bool readable = false, unreadable = false;
	int i;

	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
		if (skb_frag_is_net_iov(&skb_shinfo(skb)->frags[i]))
			unreadable = true;
		else
			readable = true;
	}
	return readable && unreadable;
}

static int __tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, ...)
{
	...
	BUG_ON(!skb || !tcp_skb_pcount(skb));

	WARN_ONCE(tcp_dbg_skb_frags_mixed(skb),
		  "DEBUG: TX skb mixes devmem+non-devmem frags (nr_frags=%u len=%u)\n",
		  skb_shinfo(skb)->nr_frags, skb->len);
	...
}


Resulting in:

[   85.908005] DEBUG: TX skb mixes devmem+non-devmem frags (nr_frags=2 len=24)
[   85.908342] WARNING: net/ipv4/tcp_output.c:1570 at __tcp_transmit_skb+0x9bb/0x1030, CPU#2: ncdevmem/278
[   85.910646] RIP: 0010:__tcp_transmit_skb+0x9c2/0x1030
   ...
[   85.915617]  tcp_write_xmit+0x47b/0x17d0
[   85.915802]  __tcp_push_pending_frames+0x38/0x100
[   85.916025]  tcp_sendmsg_locked+0xe51/0x1280
[   85.916244]  tcp_sendmsg+0x2c/0x50
[   85.916903]  do_syscall_64+0x11c/0x610


My feeling is that we should guard against this when
tcp_sendmsg_locked() is doing its "new segment or not" calculus.  In the
zc == 0 case, I think we need to check if the queue tail is unreadable
and 'goto new_segment' if it is?

This is a different case than Pavel's patch addresses though, where the
new sendmsg is devmem and write queue tail is readable.

Best,
Bobby

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

[Mina Almasry]

On Thu, Jul 23, 2026 at 10:24 AM Bobby Eshleman <bobbyeshleman@gmail.com> wrote:
>
> On Wed, Jul 22, 2026 at 12:49:07PM -0700, Mina Almasry wrote:
> > On Wed, Jul 22, 2026 at 3:59 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>
> >
> > It's true that we don't support mixing niov types and doing so would
> > blow up, but this is an unnecessary defensive check imo. The calling
> > code should not (and does not, I hope) have an edge case where it
> > tries to mix and match niov types.
> >
> > Maybe a DEBUG_NET_WARN_ON check to help catch bugs in the calling code
> > may be fine?
> >
> > Also we don't really support mixing different niov sub-types in an skb
> > (like IO_URING + DMABUF) afaict, so might as well go the extra mile
> > and check that it's all devmem niovs specifically.
> >
> > And might as well put the check in skb_add_rx_frag_netmem. It's the
> > same situation on RX, we don't support mixing there (and I hope no
> > code path leads to mixing today).
>
> Hey Mina and Pavel,
>
> I was able to confirm this mixing case does exist.
>
> It looks like in tcp_sendmsg_locked() when new message is non-devmem (zc
> == 0) and tcp_write_queue_tail is devmem, the skb collapsing is allowed
> (though same-frag merge is disallowed).
>
> Adding a mode to ncdevmem that sends mixed devmem and non-devmem
> messages, it can be caught hacking this in:
>
> /* DEBUG (DROP ME): detect a TX skb that mixes devmem (net_iov, unreadable)
>  * and non-devmem (page, readable) fragments. Such an skb must never exist.
>  */
> static bool tcp_dbg_skb_frags_mixed(const struct sk_buff *skb)
> {
>         bool readable = false, unreadable = false;
>         int i;
>
>         for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
>                 if (skb_frag_is_net_iov(&skb_shinfo(skb)->frags[i]))
>                         unreadable = true;
>                 else
>                         readable = true;
>         }
>         return readable && unreadable;
> }
>
> static int __tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, ...)
> {
>         ...
>         BUG_ON(!skb || !tcp_skb_pcount(skb));
>
>         WARN_ONCE(tcp_dbg_skb_frags_mixed(skb),
>                   "DEBUG: TX skb mixes devmem+non-devmem frags (nr_frags=%u len=%u)\n",
>                   skb_shinfo(skb)->nr_frags, skb->len);
>         ...
> }
>
>
> Resulting in:
>
> [   85.908005] DEBUG: TX skb mixes devmem+non-devmem frags (nr_frags=2 len=24)
> [   85.908342] WARNING: net/ipv4/tcp_output.c:1570 at __tcp_transmit_skb+0x9bb/0x1030, CPU#2: ncdevmem/278
> [   85.910646] RIP: 0010:__tcp_transmit_skb+0x9c2/0x1030
>    ...
> [   85.915617]  tcp_write_xmit+0x47b/0x17d0
> [   85.915802]  __tcp_push_pending_frames+0x38/0x100
> [   85.916025]  tcp_sendmsg_locked+0xe51/0x1280
> [   85.916244]  tcp_sendmsg+0x2c/0x50
> [   85.916903]  do_syscall_64+0x11c/0x610
>
>
> My feeling is that we should guard against this when
> tcp_sendmsg_locked() is doing its "new segment or not" calculus.  In the
> zc == 0 case, I think we need to check if the queue tail is unreadable
> and 'goto new_segment' if it is?
>
> This is a different case than Pavel's patch addresses though, where the
> new sendmsg is devmem and write queue tail is readable.
>

:( Yep looks like we have a couple of bugs in the TX mixing. Sorry
about that. We do indeed need to fix this ASAP.

I don't think it's enough to check readable vs unreadable, no? Because
I think appending io_uring niovs to a devmem skb will still blow up
and vise versa, even though both are unreadable, right? Or is io_uring
saved from this somehow in both cases?

If io_uring is vulnerable to this as well, then fixing this becomes a
bit more hairy because this is a TCP fast path. We don't have bits in
the skb header telling us exactly what the skb memtype is (only
readable vs not), so we'll have to check skb_frag_is_net_iov(frags[0])
and netmem_to_net_iov(frags[0]->netmem)->niov_type to check the exact
type of the skb memtype, and that may be a lot of cachelines to fetch
in the fast path. :(


-- 
Thanks,
Mina