Re: [PATCH 1/1 v2] skbuff: Fix a potential race while recycling page_pool packets

On Thu, 15 Jul 2021 at 13:48, Yunsheng Lin [off-list ref] wrote:

On 2021/7/15 18:38, Ilias Apalodimas wrote:

quoted

On Thu, 15 Jul 2021 at 13:00, Ilias Apalodimas
[off-list ref] wrote:

quoted

On Thu, 15 Jul 2021 at 07:01, Yunsheng Lin [off-list ref] wrote:

quoted

On 2021/7/9 14:29, Ilias Apalodimas wrote:

quoted

As Alexander points out, when we are trying to recycle a cloned/expanded
SKB we might trigger a race.  The recycling code relies on the
pp_recycle bit to trigger,  which we carry over to cloned SKBs.
If that cloned SKB gets expanded or if we get references to the frags,
call skbb_release_data() and overwrite skb->head, we are creating separate
instances accessing the same page frags.  Since the skb_release_data()
will first try to recycle the frags,  there's a potential race between
the original and cloned SKB, since both will have the pp_recycle bit set.

Fix this by explicitly those SKBs not recyclable.
The atomic_sub_return effectively limits us to a single release case,
and when we are calling skb_release_data we are also releasing the
option to perform the recycling, or releasing the pages from the page pool.

Fixes: 6a5bcd84e886 ("page_pool: Allow drivers to hint on SKB recycling")
Reported-by: Alexander Duyck <alexanderduyck@fb.com>
Suggested-by: Alexander Duyck <alexanderduyck@fb.com>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes since v1:
- Set the recycle bit to 0 during skb_release_data instead of the
  individual fucntions triggering the issue, in order to catch all
  cases
 net/core/skbuff.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 12aabcda6db2..f91f09a824be 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -663,7 +663,7 @@ static void skb_release_data(struct sk_buff *skb)
      if (skb->cloned &&
          atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,
                            &shinfo->dataref))
-             return;
+             goto exit;

Is it possible this patch may break the head frag page for the original skb,
supposing it's head frag page is from the page pool and below change clears
the pp_recycle for original skb, causing a page leaking for the page pool?

So this would leak eventually dma mapping if the skb is cloned (and
the dataref is now +1) and we are freeing the original skb first?

Apologies for the noise, my description was not complete.
The case you are thinking is clone an SKB and then expand the original?

Yes.
It seems we might need different pp_recycle bit for head frag and data frag.

We could just reset the pp_recycle flag on pskb_carve_inside_header,
pskb_expand_header and pskb_carve_inside_nonlinear which were the
three functions that might trigger the race to begin with.  The point
on adding it on skb_release_data was to have a catch all for all
future cases ...
Let me stare at itt a bit more in case I can come up with something better

Thanks
/Ilias

quoted

thanks
/Ilias

quoted

quoted

quoted

      skb_zcopy_clear(skb, true);

@@ -674,6 +674,8 @@ static void skb_release_data(struct sk_buff *skb)
              kfree_skb_list(shinfo->frag_list);

      skb_free_head(skb);
+exit:
+     skb->pp_recycle = 0;
 }

 /*

.