skbuff: Fix a race between coalescing and releasing SKBs

Commit 1effe8ca4e34 allowed coalescing to proceed with non page pool page and page pool page when @from is cloned, i.e.

to->pp_recycle    --> false
from->pp_recycle  --> true
skb_cloned(from)  --> true

However, it actually requires skb_cloned(@from) to hold true until coalescing finishes in this situation. If the other cloned SKB is released while the merging is in process, from_shinfo->nr_frags will be set to 0 toward the end of the function, causing the increment of frag page _refcount to be unexpectedly skipped resulting in inconsistent reference counts. Later when SKB(@to) is released, it frees the page directly even though the page pool page is still in use, leading to use-after-free or double-free errors. So it should be prohibitted.

The double-free error message below prompted us to investigate:

BUG: Bad page state in process swapper/1  pfn:0e0d1
page:00000000c6548b28 refcount:-1 mapcount:0 mapping:0000000000000000
index:0x2 pfn:0xe0d1
flags: 0xfffffc0000000(node=0|zone=1|lastcpupid=0x1fffff)
raw: 000fffffc0000000 0000000000000000 ffffffff00000101 0000000000000000
raw: 0000000000000002 0000000000000000 ffffffffffffffff 0000000000000000
page dumped because: nonzero _refcount

CPU: 1 PID: 0 Comm: swapper/1 Tainted: G            E      6.2.0+
Call Trace:
 <IRQ>
dump_stack_lvl+0x32/0x50
bad_page+0x69/0xf0
free_pcp_prepare+0x260/0x2f0
free_unref_page+0x20/0x1c0
skb_release_data+0x10b/0x1a0
napi_consume_skb+0x56/0x150
net_rx_action+0xf0/0x350

? __napi_schedule+0x79/0x90
__do_softirq+0xc8/0x2b1
__irq_exit_rcu+0xb9/0xf0
common_interrupt+0x82/0xa0
</IRQ>
<TASK>
asm_common_interrupt+0x22/0x40
RIP: 0010:default_idle+0xb/0x20

Signed-off-by Liang Chen <[email protected]>

---
 net/core/skbuff.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

net/core/skbuff.c +7 -10

index 050a875d09c5..9be23ece5f03 100644
@@ -5598,17 +5598,14 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 		return false;
 
 	/* In general, avoid mixing slab allocated and page_pool allocated
-	 * pages within the same SKB. However when @to is not pp_recycle and
-	 * @from is cloned, we can transition frag pages from page_pool to
-	 * reference counted.
-	 *
-	 * On the other hand, don't allow coalescing two pp_recycle SKBs if
-	 * @from is cloned, in case the SKB is using page_pool fragment
-	 * references (PP_FLAG_PAGE_FRAG). Since we only take full page
-	 * references for cloned SKBs at the moment that would result in
-	 * inconsistent reference counts.
+	 * pages within the same SKB. However don't allow coalescing two
+	 * pp_recycle SKBs if @from is cloned, in case the SKB is using
+	 * page_pool fragment references (PP_FLAG_PAGE_FRAG). Since we only
+	 * take full page references for cloned SKBs at the moment that would
+	 * result in inconsistent reference counts.
 	 */
-	if (to->pp_recycle != (from->pp_recycle && !skb_cloned(from)))
+	if ((to->pp_recycle != from->pp_recycle)
+		|| (from->pp_recycle && skb_cloned(from)))
 		return false;
 
 	if (len <= skb_tailroom(to)) {

Commit 1effe8ca4e34 allowed coalescing to proceed with non page pool page and page pool page when @from is cloned, i.e.

to->pp_recycle    --> false
from->pp_recycle  --> true
skb_cloned(from)  --> true

However, it actually requires skb_cloned(@from) to hold true until coalescing finishes in this situation. If the other cloned SKB is released while the merging is in process, from_shinfo->nr_frags will be set to 0 toward the end of the function, causing the increment of frag page _refcount to be unexpectedly skipped resulting in inconsistent reference counts. Later when SKB(@to) is released, it frees the page directly even though the page pool page is still in use, leading to use-after-free or double-free errors. So it should be prohibitted.

The double-free error message below prompted us to investigate:

BUG: Bad page state in process swapper/1  pfn:0e0d1
page:00000000c6548b28 refcount:-1 mapcount:0 mapping:0000000000000000
index:0x2 pfn:0xe0d1
flags: 0xfffffc0000000(node=0|zone=1|lastcpupid=0x1fffff)
raw: 000fffffc0000000 0000000000000000 ffffffff00000101 0000000000000000
raw: 0000000000000002 0000000000000000 ffffffffffffffff 0000000000000000
page dumped because: nonzero _refcount

CPU: 1 PID: 0 Comm: swapper/1 Tainted: G            E      6.2.0+
Call Trace:
 <IRQ>
dump_stack_lvl+0x32/0x50
bad_page+0x69/0xf0
free_pcp_prepare+0x260/0x2f0
free_unref_page+0x20/0x1c0
skb_release_data+0x10b/0x1a0
napi_consume_skb+0x56/0x150
net_rx_action+0xf0/0x350
? __napi_schedule+0x79/0x90
__do_softirq+0xc8/0x2b1
__irq_exit_rcu+0xb9/0xf0
common_interrupt+0x82/0xa0
</IRQ>
<TASK>
asm_common_interrupt+0x22/0x40
RIP: 0010:default_idle+0xb/0x20

Signed-off-by Liang Chen <[email protected]>

---
 net/core/skbuff.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

net/core/skbuff.c +2 -9

index 050a875d09c5..9be23ece5f03 100644
@@ -5598,17 +5598,14 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 		return false;
 
 	/* In general, avoid mixing slab allocated and page_pool allocated
-	 * pages within the same SKB. However when @to is not pp_recycle and
-	 * @from is cloned, we can transition frag pages from page_pool to
-	 * reference counted.
-	 *
-	 * On the other hand, don't allow coalescing two pp_recycle SKBs if
-	 * @from is cloned, in case the SKB is using page_pool fragment
-	 * references (PP_FLAG_PAGE_FRAG). Since we only take full page
-	 * references for cloned SKBs at the moment that would result in
-	 * inconsistent reference counts.
+	 * pages within the same SKB. However don't allow coalescing two
+	 * pp_recycle SKBs if @from is cloned, in case the SKB is using

I'm not quite sure I agree with the fix. Couldn't we just modify the check further down that does:

        if (!skb_cloned(from))
                from_shinfo->nr_frags = 0;

And instead just make that:

	if (!skb->cloned || (!skb_cloned(from) && !from->pp_recycle))
                from_shinfo->nr_frags = 0;

With that we would retain the existing behavior and in the case of cloned from frames we would take the references and let the original from skb freed to take care of pulling the pages from the page pool.

Quoted 6 lines · On Tue, 04 Apr 2023 08:51:18 -0700 Alexander H Duyck wrote:

I'm not quite sure I agree with the fix. Couldn't we just modify the check further down that does:

        if (!skb_cloned(from))
                from_shinfo->nr_frags = 0;

Quoted 7 lines

And instead just make that:

      if (!skb->cloned || (!skb_cloned(from) && !from->pp_recycle))
                from_shinfo->nr_frags = 0;

With that we would retain the existing behavior and in the case of cloned from frames we would take the references and let the original from skb freed to take care of pulling the pages from the page pool.

Sounds like a better fix, indeed. But this sort of code will require another fat comment above to explain why. This:

        if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

is much easier to understand, no?

We should at least include that in the explanatory comment, I reckon...

Quoted 7 lines

Sounds like a better fix, indeed. But this sort of code will require another fat comment above to explain why. This:

        if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

is much easier to understand, no?

Quoted 1 line

We should at least include that in the explanatory comment, I reckon...

Sure, the idea of dealing with the case where @from transitioned into non cloned skb in the function retains the existing behavior, and gives more opportunities to
coalesce skbs. And it seems (!skb_cloned(from) && !from->pp_recycle) is enough here.

I will take a closer look at the code path for the fragstolen case before making v2
patch - If @from transitioned into non cloned skb before "if (skb_head_is_locked(from))"

Thanks for the reviews.

Quoted 7 lines · On Wed, 5 Apr 2023 16:18:47 +0800 Liang Chen wrote:

Quoted 7 lines

Sounds like a better fix, indeed. But this sort of code will require another fat comment above to explain why. This:

        if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

is much easier to understand, no?

Quoted 7 lines

Quoted 1 line

We should at least include that in the explanatory comment, I reckon...

Sure, the idea of dealing with the case where @from transitioned into non cloned skb in the function retains the existing behavior, and gives more opportunities to
coalesce skbs. And it seems (!skb_cloned(from) && !from->pp_recycle) is enough here.

Well, that's pretty much what Alex suggested minus the optimization he put in for "was never cloned" which is probably worth having. So if you're gonna do this just use his code.

My point was that !from->pp_recycle requires the reader to understand the relationship between this check and the previous condition at entry. While to->pp_recycle == from->pp_recycle seems much more obvious to me - directly shifting frags between skbs with different refcount styles is dangerous.

Maybe it's just me, so whatever.
Make sure you write a good comment.

Quoted 6 lines

I will take a closer look at the code path for the fragstolen case before making v2
patch - If @from transitioned into non cloned skb before "if (skb_head_is_locked(from))"

Quoted 1 line

Thanks for the reviews.

Quoted 16 lines · Jakub Kicinski

Quoted 6 lines · On Tue, 04 Apr 2023 08:51:18 -0700 Alexander H Duyck wrote:

I'm not quite sure I agree with the fix. Couldn't we just modify the check further down that does:

        if (!skb_cloned(from))
                from_shinfo->nr_frags = 0;

Quoted 7 lines

And instead just make that:

      if (!skb->cloned || (!skb_cloned(from) && !from->pp_recycle))
                from_shinfo->nr_frags = 0;

With that we would retain the existing behavior and in the case of cloned from frames we would take the references and let the original from skb freed to take care of pulling the pages from the page pool.

Quoted 8 lines

Sounds like a better fix, indeed. But this sort of code will require another fat comment above to explain why. This:

        if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

is much easier to understand, no?

We should at least include that in the explanatory comment, I reckon...

Sure, the idea of dealing with the case where @from transitioned into non cloned skb in the function retains the existing behavior, and gives more opportunities to
coalesce skbs. And it seems (!skb_cloned(from) && !from->pp_recycle) is enough here.
I will take a closer look at the code path for the fragstolen case before making v2
patch - If @from transitioned into non cloned skb before "if (skb_head_is_locked(from))"

Thanks for the reviews.

Quoted 37 lines · Liang Chen

Quoted 16 lines · Jakub Kicinski

Quoted 6 lines · On Tue, 04 Apr 2023 08:51:18 -0700 Alexander H Duyck wrote:

I'm not quite sure I agree with the fix. Couldn't we just modify the check further down that does:

        if (!skb_cloned(from))
                from_shinfo->nr_frags = 0;

Quoted 7 lines

And instead just make that:

      if (!skb->cloned || (!skb_cloned(from) && !from->pp_recycle))
                from_shinfo->nr_frags = 0;

With that we would retain the existing behavior and in the case of cloned from frames we would take the references and let the original from skb freed to take care of pulling the pages from the page pool.

Quoted 8 lines

Sounds like a better fix, indeed. But this sort of code will require another fat comment above to explain why. This:

        if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

is much easier to understand, no?

We should at least include that in the explanatory comment, I reckon...

Sure, the idea of dealing with the case where @from transitioned into non cloned skb in the function retains the existing behavior, and gives more opportunities to
coalesce skbs. And it seems (!skb_cloned(from) && !from->pp_recycle) is enough here.
I will take a closer look at the code path for the fragstolen case before making v2
patch - If @from transitioned into non cloned skb before "if (skb_head_is_locked(from))"

Quoted 1 line

Thanks for the reviews.

Actually I am not sure that works now that I look at it closer. The problem with using (!skb_cloned(from) && !from->pp_recycle) is that it breaks the case where both from and to are pp_recycle without being cloned.

So it probably needs to be something actually the setup Jakub suggested would probably work better:
if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

Basically we just have to guarantee that if we are copying frags over the frag destructor is the same for the origin and destination. Otherwise we can take a reference and convert them to being reference counted.

Quoted 39 lines · Alexander Duyck

Quoted 37 lines · Liang Chen

Quoted 16 lines · Jakub Kicinski

Quoted 6 lines · On Tue, 04 Apr 2023 08:51:18 -0700 Alexander H Duyck wrote:

I'm not quite sure I agree with the fix. Couldn't we just modify the check further down that does:

        if (!skb_cloned(from))
                from_shinfo->nr_frags = 0;

Quoted 7 lines

And instead just make that:

      if (!skb->cloned || (!skb_cloned(from) && !from->pp_recycle))
                from_shinfo->nr_frags = 0;

With that we would retain the existing behavior and in the case of cloned from frames we would take the references and let the original from skb freed to take care of pulling the pages from the page pool.

Quoted 8 lines

Sounds like a better fix, indeed. But this sort of code will require another fat comment above to explain why. This:

        if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

is much easier to understand, no?

We should at least include that in the explanatory comment, I reckon...

Sure, the idea of dealing with the case where @from transitioned into non cloned skb in the function retains the existing behavior, and gives more opportunities to
coalesce skbs. And it seems (!skb_cloned(from) && !from->pp_recycle) is enough here.
I will take a closer look at the code path for the fragstolen case before making v2
patch - If @from transitioned into non cloned skb before "if (skb_head_is_locked(from))"

Quoted 6 lines

Quoted 1 line

Thanks for the reviews.

Actually I am not sure that works now that I look at it closer. The problem with using (!skb_cloned(from) && !from->pp_recycle) is that it breaks the case where both from and to are pp_recycle without being cloned.

Yeah, it would break that case. Thanks!

Quoted 5 lines

So it probably needs to be something actually the setup Jakub suggested would probably work better:
if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

I agree. That's better.

Quoted 56 lines · Liang Chen

Quoted 39 lines · Alexander Duyck

Quoted 37 lines · Liang Chen

Quoted 16 lines · Jakub Kicinski

Quoted 6 lines · On Tue, 04 Apr 2023 08:51:18 -0700 Alexander H Duyck wrote:

I'm not quite sure I agree with the fix. Couldn't we just modify the check further down that does:

        if (!skb_cloned(from))
                from_shinfo->nr_frags = 0;

Quoted 7 lines

And instead just make that:

      if (!skb->cloned || (!skb_cloned(from) && !from->pp_recycle))
                from_shinfo->nr_frags = 0;

With that we would retain the existing behavior and in the case of cloned from frames we would take the references and let the original from skb freed to take care of pulling the pages from the page pool.

Quoted 8 lines

Sounds like a better fix, indeed. But this sort of code will require another fat comment above to explain why. This:

        if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

is much easier to understand, no?

We should at least include that in the explanatory comment, I reckon...

Sure, the idea of dealing with the case where @from transitioned into non cloned skb in the function retains the existing behavior, and gives more opportunities to
coalesce skbs. And it seems (!skb_cloned(from) && !from->pp_recycle) is enough here.
I will take a closer look at the code path for the fragstolen case before making v2
patch - If @from transitioned into non cloned skb before "if (skb_head_is_locked(from))"

Quoted 6 lines

Quoted 1 line

Thanks for the reviews.

Actually I am not sure that works now that I look at it closer. The problem with using (!skb_cloned(from) && !from->pp_recycle) is that it breaks the case where both from and to are pp_recycle without being cloned.

Yeah, it would break that case. Thanks!

Quoted 5 lines

So it probably needs to be something actually the setup Jakub suggested would probably work better:
if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

Quoted 1 line

I agree. That's better.

Same feeling on my side.
I prefer not trying to merge mixed pp_recycle skbs "just because we could" at the expense
of adding more code in a fast path.

Quoted 58 lines · Eric Dumazet

Quoted 56 lines · Liang Chen

Quoted 39 lines · Alexander Duyck

Quoted 37 lines · Liang Chen

Quoted 16 lines · Jakub Kicinski

Quoted 6 lines · On Tue, 04 Apr 2023 08:51:18 -0700 Alexander H Duyck wrote:

I'm not quite sure I agree with the fix. Couldn't we just modify the check further down that does:

        if (!skb_cloned(from))
                from_shinfo->nr_frags = 0;

Quoted 7 lines

And instead just make that:

      if (!skb->cloned || (!skb_cloned(from) && !from->pp_recycle))
                from_shinfo->nr_frags = 0;

With that we would retain the existing behavior and in the case of cloned from frames we would take the references and let the original from skb freed to take care of pulling the pages from the page pool.

Quoted 8 lines

Sounds like a better fix, indeed. But this sort of code will require another fat comment above to explain why. This:

        if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

is much easier to understand, no?

We should at least include that in the explanatory comment, I reckon...

Sure, the idea of dealing with the case where @from transitioned into non cloned skb in the function retains the existing behavior, and gives more opportunities to
coalesce skbs. And it seems (!skb_cloned(from) && !from->pp_recycle) is enough here.
I will take a closer look at the code path for the fragstolen case before making v2
patch - If @from transitioned into non cloned skb before "if (skb_head_is_locked(from))"

Quoted 6 lines

Quoted 1 line

Thanks for the reviews.

Actually I am not sure that works now that I look at it closer. The problem with using (!skb_cloned(from) && !from->pp_recycle) is that it breaks the case where both from and to are pp_recycle without being cloned.

Yeah, it would break that case. Thanks!

Quoted 5 lines

So it probably needs to be something actually the setup Jakub suggested would probably work better:
if (to->pp_recycle == from->pp_recycle && !skb_cloned(from))

Quoted 6 lines

Quoted 1 line

I agree. That's better.

Same feeling on my side.
I prefer not trying to merge mixed pp_recycle skbs "just because we could" at the expense
of adding more code in a fast path.

+1 here. The intention of recycling was to affect the normal path as less as possible. On top of that, we've some amount of race conditions over the years, trying to squeeze more performance with similar tricks. I'd much rather be safe here, since recycling by itself is a great performance boost

Regards
/Ilias

Quoted 5 lines

Quoted 4 lines

Same feeling on my side.
I prefer not trying to merge mixed pp_recycle skbs "just because we could" at the expense
of adding more code in a fast path.

Quoted 5 lines

+1 here. The intention of recycling was to affect the normal path as less as possible. On top of that, we've some amount of race conditions over the years, trying to squeeze more performance with similar tricks. I'd much rather be safe here, since recycling by itself is a great performance boost

Sorry, I didn't check my inbox before sending out the v2 patch.

Yeah, It is a bit complicated as we expected. The patch is sent out. Please take a look to see if it is the way to go, or We should stay with the current patch for simplicity reasons. Thanks!

Quoted 5 lines · On Thu, 6 Apr 2023 19:54:23 +0800 Liang Chen wrote:

Quoted 5 lines

Quoted 4 lines

Same feeling on my side.
I prefer not trying to merge mixed pp_recycle skbs "just because we could" at the expense
of adding more code in a fast path.

Quoted 7 lines

Quoted 5 lines

+1 here. The intention of recycling was to affect the normal path as less as possible. On top of that, we've some amount of race conditions over the years, trying to squeeze more performance with similar tricks. I'd much rather be safe here, since recycling by itself is a great performance boost

Sorry, I didn't check my inbox before sending out the v2 patch.

I can discard v2 from patchwork, let's continue the conversation here.

Quoted 3 lines

Yeah, It is a bit complicated as we expected. The patch is sent out. Please take a look to see if it is the way to go, or We should stay with the current patch for simplicity reasons. Thanks!

Sounds like you know what Eric and Ilias agreed with, I'm a bit confused.. are we basically going back to v1? (hopefully with coding style fixed)

net/core/skbuff.c bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from, · line 5609

Quoted 4 lines · On Tue, 4 Apr 2023 15:47:33 +0800 Liang Chen wrote:

net/core/skbuff.c +2 -1

-     if (to->pp_recycle != (from->pp_recycle && !skb_cloned(from)))
+     if ((to->pp_recycle != from->pp_recycle)
+             || (from->pp_recycle && skb_cloned(from)))
              return false;

It should be formatted like this:

        if (to->pp_recycle != from->pp_recycle ||
            (from->pp_recycle && skb_cloned(from)))

Sure. I have addressed it and submitted the updated patch for your review as v3. Thank you for pointing it out.