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

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Thu, Jul 23, 2026 at 11:35:15AM -0700, Mina Almasry wrote:
> 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?

I think for the above case it is okay because if the current sendmsg()
is zc==0, then we don't care if the tail skb is iou or devmem as
skb->unreadable tells us enough to avoid appending the non-zc sendmsg.

I'm realizing this a different mixing issue than what Pavel is seeing
though, probably needs a separate patch.

Best,
Bobby

> 
> 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