Re: [RFC PATCH v3 02/20] tcp: don't allow non-devmem originated ppiov

[Pavel Begunkov <asml.silence@gmail.com>]

On 12/19/23 23:24, Mina Almasry wrote:
> On Tue, Dec 19, 2023 at 1:04 PM David Wei <dw@davidwei.uk> wrote:
>>
>> 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 <dw@davidwei.uk>
>> ---
>>   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