[PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Liang Chen]

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 <liangchen.linux@gmail.com>
---
 net/core/skbuff.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 050a875d09c5..9be23ece5f03 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -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)) {
-- 
2.18.2

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Yunsheng Lin]

On 2023/4/4 15:47, Liang Chen wrote:
> 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.

Nice catch.

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

A Fixes tag is needed for the backport of the fix.
Fixes: 1effe8ca4e34 ("skbuff: fix coalescing for page_pool fragment recycling")

And target the net branch for bugfix:
 [PATCH net] skbuff: Fix a race between coalescing and releasing SKBs

Otherwise, it looks good to me:
Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>

> 
> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
> ---
>  net/core/skbuff.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 050a875d09c5..9be23ece5f03 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -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)) {
> 

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Alexander H Duyck]

On Tue, 2023-04-04 at 15:47 +0800, Liang Chen wrote:
> 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 <liangchen.linux@gmail.com>
> ---
>  net/core/skbuff.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 050a875d09c5..9be23ece5f03 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -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)) {

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.

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Jakub Kicinski]

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

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Liang Chen]

On Wed, Apr 5, 2023 at 9:21 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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;
> >
> > 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...

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.

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Jakub Kicinski]

On Wed, 5 Apr 2023 16:18:47 +0800 Liang Chen wrote:
> > 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.

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.

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

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Alexander Duyck]

On Wed, Apr 5, 2023 at 1:19 AM Liang Chen <liangchen.linux@gmail.com> wrote:
>
> On Wed, Apr 5, 2023 at 9:21 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > 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;
> > >
> > > 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...
>
> 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.

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.

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Liang Chen]

On Wed, Apr 5, 2023 at 10:50 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 5 Apr 2023 16:18:47 +0800 Liang Chen wrote:
> > > 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.
>
> 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.
>

Yeah, I agree with the point that to->pp_recycle == from->pp_recycle
is easier to understand, and will use it in the next iteration of the
patch. Thanks!

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

Sure.
>
> > 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))"
> >

I took a closer look at the code path for the "fragstolen" case, and
it indeed requires a special handle for the situation addressed in
this patch. Something like,

    if( to->pp_recycle != from->pp_recycle )
        get_page(page);

before  "*fragstolen = true;".   But this makes the logic a bit
complicated. Anyway, I will include the logic in the v2 patch. Let's
see if it looks better.

> > Thanks for the reviews.

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Liang Chen]

On Wed, Apr 5, 2023 at 11:06 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Wed, Apr 5, 2023 at 1:19 AM Liang Chen <liangchen.linux@gmail.com> wrote:
> >
> > On Wed, Apr 5, 2023 at 9:21 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > 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;
> > > >
> > > > 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...
> >
> > 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.
>
> 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!
>
> 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.
> 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.

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Eric Dumazet]

On Thu, Apr 6, 2023 at 5:28 AM Liang Chen <liangchen.linux@gmail.com> wrote:
>
> On Wed, Apr 5, 2023 at 11:06 PM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
> >
> > On Wed, Apr 5, 2023 at 1:19 AM Liang Chen <liangchen.linux@gmail.com> wrote:
> > >
> > > On Wed, Apr 5, 2023 at 9:21 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > >
> > > > 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;
> > > > >
> > > > > 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...
> > >
> > > 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.
> >
> > 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!
> >
> > 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.

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.

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Ilias Apalodimas]

On Thu, 6 Apr 2023 at 12:56, Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Apr 6, 2023 at 5:28 AM Liang Chen <liangchen.linux@gmail.com> wrote:
> >
> > On Wed, Apr 5, 2023 at 11:06 PM Alexander Duyck
> > <alexander.duyck@gmail.com> wrote:
> > >
> > > On Wed, Apr 5, 2023 at 1:19 AM Liang Chen <liangchen.linux@gmail.com> wrote:
> > > >
> > > > On Wed, Apr 5, 2023 at 9:21 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > >
> > > > > 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;
> > > > > >
> > > > > > 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...
> > > >
> > > > 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.
> > >
> > > 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!
> > >
> > > 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.
>
> 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

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Liang Chen]

On Thu, Apr 6, 2023 at 6:46 PM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Thu, 6 Apr 2023 at 12:56, Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Apr 6, 2023 at 5:28 AM Liang Chen <liangchen.linux@gmail.com> wrote:
> > >
> > > On Wed, Apr 5, 2023 at 11:06 PM Alexander Duyck
> > > <alexander.duyck@gmail.com> wrote:
> > > >
> > > > On Wed, Apr 5, 2023 at 1:19 AM Liang Chen <liangchen.linux@gmail.com> wrote:
> > > > >
> > > > > On Wed, Apr 5, 2023 at 9:21 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > > >
> > > > > > 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;
> > > > > > >
> > > > > > > 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...
> > > > >
> > > > > 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.
> > > >
> > > > 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!
> > > >
> > > > 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.
> >
> > 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

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!

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Jakub Kicinski]

On Thu, 6 Apr 2023 19:54:23 +0800 Liang Chen wrote:
> > > 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
> 
> 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.

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

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Liang Chen]

On Thu, Apr 6, 2023 at 10:59 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 6 Apr 2023 19:54:23 +0800 Liang Chen wrote:
> > > > 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
> >
> > 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.
>
> > 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)

Sorry for making the confusion. I just felt the v2 patch complicated
the code a bit.  I am not quite sure if the performance gain justifies
the added complexity, and would really appreciate it if you could make
the decision on which patch to pick up. Thanks a lot!

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Jakub Kicinski]

On Tue,  4 Apr 2023 15:47:33 +0800 Liang Chen wrote:
> -	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)))

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Liang Chen]

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

Thanks,
Liang

On Tue, Apr 11, 2023 at 8:26 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue,  4 Apr 2023 15:47:33 +0800 Liang Chen wrote:
> > -     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)))

Re: [PATCH] skbuff: Fix a race between coalescing and releasing SKBs

[Jakub Kicinski]

On Wed, 12 Apr 2023 15:27:13 +0800 Liang Chen wrote:
> Sure. I have addressed it and submitted the updated patch for your
> review as v3. Thank you for pointing it out.

I know, I don't understand why you have to send this note, I can see
the patch. And it's still in patchwork which as you know (as I trust
you read our process documentation) means it's going to be reviewed.
The judeo-christiano-muslim world is going thru its spring celebrations
so the reviews will be slower than usual. 

Please be patient.

And please don't top post.