On 12/19/23 23:24, Mina Almasry wrote:

On Tue, Dec 19, 2023 at 1:04 PM David Wei [off-list ref] wrote:

quoted

From: Pavel Begunkov <asml.silence@gmail.com>

NOT FOR UPSTREAM

There will be more users of struct page_pool_iov, and ppiovs from one
subsystem must not be used by another. That should never happen for any
sane application, but we need to enforce it in case of bufs and/or
malicious users.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
Signed-off-by: David Wei <redacted>
---
  net/ipv4/tcp.c | 7 +++++++
  1 file changed, 7 insertions(+)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 33a8bb63fbf5..9c6b18eebb5b 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c

@@ -2384,6 +2384,13 @@ static int tcp_recvmsg_devmem(const struct sock *sk, const struct sk_buff *skb,
                         }

                         ppiov = skb_frag_page_pool_iov(frag);
+
+                       /* Disallow non devmem owned buffers */
+                       if (ppiov->pp->p.memory_provider != PP_MP_DMABUF_DEVMEM) {
+                               err = -ENODEV;
+                               goto out;
+                       }
+

Instead of this, I maybe recommend modifying the skb->dmabuf flag? My
mental model is that flag means all the frags in the skb are

That's a good point, we need to separate them, and I have it in my
todo list.

specifically dmabuf, not general ppiovs or net_iovs. Is it possible to
add skb->io_uring or something?

->io_uring flag is not feasible, converting ->devmem into a type
{page,devmem,iouring} is better but not great either.

If that bloats the skb headers, then maybe we need another place to
put this flag. Maybe the [page_pool|net]_iov should declare whether
it's dmabuf or otherwise, and we can check frag[0] and assume all

ppiov->pp should be enough, either not mixing buffers from different
pools or comparing pp->ops or some pp->type.

frags are the same as frag0.

I think I like this one the most. I think David Ahern mentioned
before, but would be nice having it on per frag basis and kill
->devmem flag. That would still stop collapsing if frags are
from different pools or so.

But IMO the page pool internals should not leak into the
implementation of generic tcp stack functions.

-- 
Pavel Begunkov