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

[Alexander H Duyck]

On Wed, 2023-01-25 at 19:42 +0100, Felix Fietkau wrote:
> 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?

No. What will happen is that when the pp_frag_count is exhausted the
page will be unmapped and evicted from the page pool. When the page is
then finally freed it will end up going back to the page allocator
instead of page pool.

Basically the idea is that until pp_frag_count reaches 0 there will be
at least 1 page reference held.

> > > 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

When the page is freed on the skb w/ pp_recycle set it will unmap the
page and evict it from the page pool. Basically in these cases the page
goes from the page pool back to the page allocator.

The general idea with this is that if we are using fragments that there
will be enough of them floating around that if one or two frags have a
temporeary detour through a non-recycling path that hopefully by the
time the last fragment is freed the other instances holding the
additional page reference will have let them go. If not then the page
will go back to the page allocator and it will have to be replaced in
the page pool.