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

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Fri, Jul 24, 2026 at 10:26:42AM -0700, Mina Almasry wrote:
> On Thu, Jul 23, 2026 at 4:10 PM Bobby Eshleman <bobbyeshleman@gmail.com> wrote:
> >
> > 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.
> >
> 
> Yes, you're reproducing a different edge case that results in mixing.
> Do you plan to send a fix for that or should I take a look?

I have a fix in the works and plan on sending it soon.

Best,
Bobby