Re: [PATCH net-next v9 11/14] tcp: RX path for devmem TCP

[Pavel Begunkov]

On 5/28/24 18:36, Mina Almasry wrote:
> On Wed, May 22, 2024 at 11:02 PM David Wei <dw@xxxxxxxxxxx> wrote:
..
> > > +                      */
> > > +                     if (!skb_frag_net_iov(frag)) {
> > > +                             net_err_ratelimited("Found non-dmabuf skb with net_iov");
> > > +                             err = -ENODEV;
> > > +                             goto out;
> > > +                     }
> > > +
> > > +                     niov = skb_frag_net_iov(frag);
> >
> > Sorry if we've already discussed this.
> >
> > We have this additional hunk:
> >
> > + if (niov->pp->mp_ops != &dmabuf_devmem_ops) {
> > +       err = -ENODEV;
> > +       goto out;
> > + }
> >
> > In case one of our skbs end up here, skb_frag_is_net_iov() and
> > !skb_frags_readable(). Does this even matter? And if so then is there a
> > better way to distinguish between our two types of net_iovs?
>
> Thanks for bringing this up, yes, maybe we do need a way to
> distinguish, but it's not 100% critical, no? It's mostly for debug
> checking?

Not really. io_uring definitely wouldn't want the devmem completion path
taking an iov and basically stashing it into a socket (via refcount),
that's a lifetime problem. Nor we'd have all the binding/chunk_owner
parts you have and probably use there.

Same the other way around, you don't want io_uring grabbing your iov
and locking it up, it won't even be possible to return it back. We
also may want to have access to backing pages for different fallback
purposes, for which we need to know the iov came from this particular
ring.

It shouldn't happen for a behaving user, but most of it would likely
be exploitable one way or another.

> I would say add a helper, like net_iov_is_dmabuf() or net_iov_is_io_uring().

We're verifying that the context the iov bound to is the current
context (e.g. io_uring instance) we're executing from. If we can
agree that mp_priv should be a valid pointer, the check would look
like:

if (pp->mp_priv == io_uring_ifq)

> Checking for niov->pp->mp_ops seems a bit hacky to me, and may be
> outright broken. IIRC niov's can be disconnected from the page_pool
> via page_pool_clear_pp_info(), and niov->pp may be null. Abstractly

It's called in the release path like page_pool_return_page(),
I can't imagine someone can sanely clear it while inflight ...

> speaking the niov type maybe should be a property of the niov itself,
> and not the pp the niov is attached to.

.. but I can just stash all that in niov->owner,
struct dmabuf_genpool_chunk_owner you have. That might be even
cleaner. And regardless of it I'll be making some minor changes
to the structure to make it generic.

> It is not immediately obvious to me what the best thing to do here is,
> maybe it's best to add a flag to niov or to use niov->pp_magic for
> this.
>
> I would humbly ask that your follow up patchset takes care of this
> bit, if possible. I think mine is doing quite a bit of heavy lifting
> as is (and I think may be close to ready?), when it comes to concerns
> of devmem + io_uring coexisting if you're able to take care, awesome,
> if not, I can look into squashing some fix.

Let it be this way then. It's not a problem while there is
only one such a provider.

--
Pavel Begunkov