Re: [PATCH] net: page_pool: fix refcounting issues with fragmented allocation

[Alexander H Duyck]

On Tue, 2023-01-24 at 16:11 +0200, Ilias Apalodimas wrote:
> Hi Felix,
> 
> ++cc Alexander and Yunsheng.
> 
> Thanks for the report
> 
> On Tue, 24 Jan 2023 at 14:43, Felix Fietkau <nbd@xxxxxxxx> wrote:
> > 
> > While testing fragmented page_pool allocation in the mt76 driver, I was able
> > to reliably trigger page refcount underflow issues, which did not occur with
> > full-page page_pool allocation.
> > It appears to me, that handling refcounting in two separate counters
> > (page->pp_frag_count and page refcount) is racy when page refcount gets
> > incremented by code dealing with skb fragments directly, and
> > page_pool_return_skb_page is called multiple times for the same fragment.
> > 
> > Dropping page->pp_frag_count and relying entirely on the page refcount makes
> > these underflow issues and crashes go away.
> > 
> 
> This has been discussed here [1].  TL;DR changing this to page
> refcount might blow up in other colorful ways.  Can we look closer and
> figure out why the underflow happens?
> 
> [1] https://lore.kernel.org/netdev/1625903002-31619-4-git-send-email-linyunsheng@xxxxxxxxxx/
> 
> Thanks
> /Ilias
> 
> 

The logic should be safe in terms of the page pool itself as it should
be holding one reference to the page while the pp_frag_count is non-
zero. That one reference is what keeps the two halfs in sync as the
page shouldn't be able to be freed until we exhaust the pp_frag_count.

To have an underflow there are two possible scenarios. One is that
either put_page or free_page is being called somewhere that the
page_pool freeing functions should be used. The other possibility is
that a pp_frag_count reference was taken somewhere a page reference
should have.

Do we have a backtrace for the spots that are showing this underrun? If
nothing else we may want to look at tracking down the spots that are
freeing the page pool pages via put_page or free_page to determine what
paths these pages are taking.