Messages in this thread | | | From | Dragos Tatulea <> | Subject | Re: [RFC PATCH] net: Fix one page_pool page leak from skb_frag_unref | Date | Mon, 29 Apr 2024 07:39:09 +0000 |
| |
On Fri, 2024-04-26 at 16:05 -0700, Jakub Kicinski wrote: > On Thu, 25 Apr 2024 08:17:28 +0000 Dragos Tatulea wrote: > > > The unref path always dropped a regular page > > > ref, thanks to this commit as you point out: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2cc3aeb5ecccec0d266813172fcd82b4b5fa5803 > > > > > > AFAICT the correct fix is to actually revert commit 2cc3aeb5eccc > > > ("skbuff: Fix a potential race while recycling page_pool packets"). > > > The reason is that now that skb_frag_ref() can grab page-pool refs, we > > > don't need to make sure there is only 1 SKB that triggers the recycle > > > path anymore. All the skb and its clones can obtain page-pool refs, > > > and in the unref path we drop the page-pool refs. page_pool_put_page() > > > detects correctly that the last page-pool ref is put and recycles the > > > page only then. > > > > > I don't think this is a good way forward. For example, skb->pp_recycle is used > > as a hint in skb_gro_receive to avoid coalescing skbs with different pp_recycle > > flag states. This could interfere with that. > > That's a bit speculative, right? The simple invariant we are trying to > hold is that if skb->pp_recycle && skb_frag_is_pp(skb, i) then the > reference skb is holding on that frag is a pp reference, not page > reference. > Yes, it was a speculative statement. After re-reading it and the code of skb_gro_receive() it makes less sense now.
Mina's suggestion to revert commit 2cc3aeb5eccc ("skbuff: Fix a potential race while recycling page_pool packets") seems less scary now. I just hope we don't bump into too many scenarios similar to the ipsec one...
> skb_gro_receive() needs to maintain that invariant, if it doesn't > we need to fix it.. >
Thanks, Dragos
| |