Re: [BUG] Possible unsafe page_pool usage in octeontx2

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

[...]

> >
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index 7ff80b80a6f9f..b50e219470a36 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -612,7 +612,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
> >                       page_pool_dma_sync_for_device(pool, page,
> >                                                     dma_sync_size);
> >
> > -             if (allow_direct && in_softirq() &&
> > +             if (allow_direct && in_serving_softirq() &&
>
> This is the "return/free/put" code path, where we have "allow_direct" as
> a protection in the API.  API users are suppose to use
> page_pool_recycle_direct() to indicate this, but as some point we
> allowed APIs to expose 'allow_direct'.
>
> The PP-alloc side is more fragile, and maybe the in_serving_softirq()
> belongs there.
>
> >                   page_pool_recycle_in_cache(page, pool))
> >                       return NULL;
> >
> > because the intention (as I understand it) is to be invoked from within
> > the NAPI callback (while softirq is served) and not if BH is just
> > disabled due to a lock or so.
> >
>
> True, and it used-to-be like this (in_serving_softirq), but as Ilias
> wrote it was changed recently.  This was to support threaded-NAPI (in
> 542bcea4be866b ("net: page_pool: use in_softirq() instead")), which
> I understood was one of your (Sebastian's) use-cases.
>
>
> > It would also make sense to a add WARN_ON_ONCE(!in_serving_softirq()) to
> > page_pool_alloc_pages() to spot usage outside of softirq. But this will
> > trigger in every driver since the same function is used in the open
> > callback to initially setup the HW.
> >
>
> I'm very open to ideas of detecting this.  Since mentioned commit PP is
> open to these kind of miss-uses of the API.
>
> One idea would be to leverage that NAPI napi->list_owner will have been
> set to something else than -1, when this is NAPI context.  Getting hold
> of napi object, could be done via pp->p.napi (but as Jakub wrote this is
> opt-in ATM).

I mentioned this earlier, but can't we add the softirq check in
__page_pool_get_cached()?
In theory, when a driver comes up and allocates pages to fill in its
descriptors it will call page_pool_alloc_pages().  That will go
through the slow allocation path, fill up the caches, and return the
last page.  After that, most of the allocations will be served by
__page_pool_get_cached(), and this is supposed to be running during
the driver Rx routine which runs under NAPI.  So eventually we will
hit that warning.

Thanks
/Ilias

>
> --Jesper