Re: [RFC] net: esp: fix bad handling of pages from page_pool

[Mina Almasry]

On Wed, Mar 6, 2024 at 9:41 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> On Wed, 6 Mar 2024 17:09:57 +0000 Dragos Tatulea wrote:
> > > Does the caller need to check skb->pp_recycle? pp_recycle seems like a
> > > redundant bit. We can tell whether the page is pp by checking
> > > is_pp_page(page). the pages in the frag must be pp pages when
> > > skb->pp_recycle is set and must be non pp pages when the
> > > skb->pp_recycle is not set, so it all seems redundant to me.
> > >
> > AFAIU we don't have to check for pp_recycle, at least not in this specific case.
>
> Definitely not something we assuming in a fix that needs to go
> to stable.
>
> So far, AFAIU, it's legal to have an skb without skb->pp_recycle
> set, which holds full page refs to a PP page.

Interesting. I apologize then I did not realize this combination is
legal. But I have a follow up question: what is the ref code supposed
to do in this combination? AFAIU:

- skb->pp_recycle && is_pp_page()
ref via page_pool_ref_page()
unref via page_pool_unref_page()

- !skb->pp_recycle && !is_pp_page()
ref via get_page()
unref via put_page()

- !skb->pp_recycle && is_pp_page()
ref via ?
unref via?

Also is this combination also legal you think? and if so what to do?
- skb->pp_recycle && !is_pp_page()
ref via ?
unref via?

I'm asking because I'm starting to wonder if this patch has some issue in it:
https://patchwork.kernel.org/project/netdevbpf/patch/20231215033011.12107-4-liangchen.linux@xxxxxxxxx/

Because in this patch, if we have a !skb->pp_recycle & is_pp_page()
combination we end up doing in skb_try_coalesce():
ref via page_pool_ref_page()
unref via put_page() via eventual napi_frag_unref()

which seems like an issue, no?

-- 
Thanks,
Mina