Messages in this thread | | | Subject | Re: [PATCH] net: page_pool: fix refcounting issues with fragmented allocation | From | Alexander H Duyck <> | Date | Tue, 24 Jan 2023 13:10:24 -0800 |
| |
On Tue, 2023-01-24 at 18:22 +0100, Felix Fietkau wrote: > On 24.01.23 15:11, Ilias Apalodimas wrote: > > Hi Felix, > > > > ++cc Alexander and Yunsheng. > > > > Thanks for the report > > > > On Tue, 24 Jan 2023 at 14:43, Felix Fietkau <nbd@nbd.name> 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? > I don't see how the approch taken in my patch would blow up. From what I > can tell, it should be fairly close to how refcount is handled in > page_frag_alloc. The main improvement it adds is to prevent it from > blowing up if pool-allocated fragments get shared across multiple skbs > with corresponding get_page and page_pool_return_skb_page calls. > > - Felix >
Do you have the patch available to review as an RFC? From what I am seeing it looks like you are underrunning on the pp_frag_count itself. I would suspect the issue to be something like starting with a bad count in terms of the total number of references, or deducing the wrong amount when you finally free the page assuming you are tracking your frag count using a non-atomic value in the driver.
Thanks,
- Alex
| |