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

From: Felix Fietkau
Date: Wed Jan 25 2023 - 13:43:40 EST

On 25.01.23 19:26, Alexander H Duyck wrote:
On Wed, 2023-01-25 at 18:32 +0100, Felix Fietkau wrote:
On 25.01.23 18:11, Alexander H Duyck wrote:
> On Tue, 2023-01-24 at 22:30 +0100, Felix Fietkau wrote:
> > On 24.01.23 22:10, Alexander H Duyck wrote:
> > > 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@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?
> > > > 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.
> > The driver patches for page pool are here:
> > https://patchwork.kernel.org/project/linux-wireless/patch/64abb23f4867c075c19d704beaae5a0a2f8e8821.1673963374.git.lorenzo@xxxxxxxxxx/
> > https://patchwork.kernel.org/project/linux-wireless/patch/68081e02cbe2afa2d35c8aa93194f0adddbd0f05.1673963374.git.lorenzo@xxxxxxxxxx/
> > > > They are also applied in my mt76 tree at:
> > https://github.com/nbd168/wireless
> > > > - Felix
> > So one thing I am thinking is that we may be seeing an issue where we
> are somehow getting a mix of frag and non-frag based page pool pages.
> That is the only case I can think of where we might be underflowing
> negative. If you could add some additional debug info on the underflow
> WARN_ON case in page_pool_defrag_page that might be useful.
> Specifically I would be curious what the actual return value is. I'm
> assuming we are only hitting negative 1, but I would want to verify we
> aren't seeing something else.
I'll try to run some more tests soon. However, I think I found the piece of code that is incompatible with using pp_frag_count.
When receiving an A-MSDU packet (multiple MSDUs within a single 802.11 packet), and it is not split by the hardware, a cfg80211 function extracts the individual MSDUs into separate skbs. In that case, a fragment can be shared across multiple skbs, and get_page is used to increase the refcount.
You can find this in net/wireless/util.c: ieee80211_amsdu_to_8023s (and its helper functions).

I'm not sure if it is problematic or not. Basically it is trading off
by copying over the frags, calling get_page on each frag, and then
using dev_kfree_skb to disassemble and release the pp_frag references.
There should be other paths in the kernel that are doing something
similar.

This code also has a bug where it doesn't set pp_recycle on the newly allocated skb if the previous one has it, but that's a separate matter and fixing it doesn't make the crash go away.

Adding the recycle would cause this bug. So one thing we might be
seeing is something like that triggering this error. Specifically if
the page is taken via get_page when assembling the new skb then we
cannot set the recycle flag in the new skb otherwise it will result in
the reference undercount we are seeing. What we are doing is shifting
the references away from the pp_frag_count to the page reference count
in this case. If we set the pp_recycle flag then it would cause us to
decrement pp_frag_count instead of the page reference count resulting
in the underrun.
Couldn't leaving out the pp_recycle flag potentially lead to a case where the last user of the page drops it via page_frag_free instead of page_pool_return_skb_page? Is that valid?

Is there any way I can make that part of the code work with the current page pool frag implementation?

The current code should work. Basically as long as the references are
taken w/ get_page and skb->pp_recycle is not set then we shouldn't run
into this issue because the pp_frag_count will be dropped when the
original skb is freed and the page reference count will be decremented
when the new one is freed.

For page pool page fragments the main thing to keep in mind is that if
pp_recycle is set it will update the pp_frag_count and if it is not
then it will just decrement the page reference count.
What takes care of DMA unmap and other cleanup if the last reference to the page is dropped via page_frag_free?

- Felix