[Cake] [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb

[Cake] [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb

[Toke Høiland-Jørgensen]

When the GSO splitting feature of sch_cake is enabled, GSO superpackets
will be broken up and the resulting segments enqueued in place of the
original skb. In this case, CAKE calls consume_skb() on the original skb,
but still returns NET_XMIT_SUCCESS. This can confuse parent qdiscs into
assuming the original skb still exists, when it really has been freed. Fix
this by adding the __NET_XMIT_STOLEN flag to the return value in this case.

Fixes: 0c850344d388 ("sch_cake: Conditionally split GSO segments")
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 net/sched/sch_cake.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index a43a58a73d09..a04928082e4a 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -1713,6 +1713,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	}
 	idx--;
 	flow = &b->flows[idx];
+	ret = NET_XMIT_SUCCESS;
 
 	/* ensure shaper state isn't stale */
 	if (!b->tin_backlog) {
@@ -1771,6 +1772,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 
 		qdisc_tree_reduce_backlog(sch, 1-numsegs, len-slen);
 		consume_skb(skb);
+		ret |= __NET_XMIT_STOLEN;
 	} else {
 		/* not splitting */
 		cobalt_set_enqueue_time(skb, now);
@@ -1904,7 +1906,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		}
 		b->drop_overlimit += dropped;
 	}
-	return NET_XMIT_SUCCESS;
+	return ret;
 }
 
 static struct sk_buff *cake_dequeue_one(struct Qdisc *sch)
-- 
2.37.2

Re: [Cake] [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb

[Dave Taht]

hmm. what did this break above it? just stats? or a mem leak? or?

On Wed, Aug 31, 2022 at 2:25 AM Toke Høiland-Jørgensen via Cake
<cake@lists.bufferbloat.net> wrote:
>
> When the GSO splitting feature of sch_cake is enabled, GSO superpackets
> will be broken up and the resulting segments enqueued in place of the
> original skb. In this case, CAKE calls consume_skb() on the original skb,
> but still returns NET_XMIT_SUCCESS. This can confuse parent qdiscs into
> assuming the original skb still exists, when it really has been freed. Fix
> this by adding the __NET_XMIT_STOLEN flag to the return value in this case.
>
> Fixes: 0c850344d388 ("sch_cake: Conditionally split GSO segments")
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> ---
>  net/sched/sch_cake.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index a43a58a73d09..a04928082e4a 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -1713,6 +1713,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>         }
>         idx--;
>         flow = &b->flows[idx];
> +       ret = NET_XMIT_SUCCESS;
>
>         /* ensure shaper state isn't stale */
>         if (!b->tin_backlog) {
> @@ -1771,6 +1772,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>
>                 qdisc_tree_reduce_backlog(sch, 1-numsegs, len-slen);
>                 consume_skb(skb);
> +               ret |= __NET_XMIT_STOLEN;
>         } else {
>                 /* not splitting */
>                 cobalt_set_enqueue_time(skb, now);
> @@ -1904,7 +1906,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>                 }
>                 b->drop_overlimit += dropped;
>         }
> -       return NET_XMIT_SUCCESS;
> +       return ret;
>  }
>
>  static struct sk_buff *cake_dequeue_one(struct Qdisc *sch)
> --
> 2.37.2
>
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake



-- 
FQ World Domination pending: https://blog.cerowrt.org/post/state_of_fq_codel/
Dave Täht CEO, TekLibre, LLC

Re: [Cake] [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb

[Eric Dumazet]

On Wed, Aug 31, 2022 at 2:25 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> When the GSO splitting feature of sch_cake is enabled, GSO superpackets
> will be broken up and the resulting segments enqueued in place of the
> original skb. In this case, CAKE calls consume_skb() on the original skb,
> but still returns NET_XMIT_SUCCESS. This can confuse parent qdiscs into
> assuming the original skb still exists, when it really has been freed. Fix
> this by adding the __NET_XMIT_STOLEN flag to the return value in this case.
>

I think you forgot to give credits to the team who discovered this issue.

Something like this

Reported-by: zdi-disclosures@trendmicro.com # ZDI-CAN-18231



> Fixes: 0c850344d388 ("sch_cake: Conditionally split GSO segments")
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> ---
>  net/sched/sch_cake.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index a43a58a73d09..a04928082e4a 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -1713,6 +1713,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>         }
>         idx--;
>         flow = &b->flows[idx];
> +       ret = NET_XMIT_SUCCESS;
>
>         /* ensure shaper state isn't stale */
>         if (!b->tin_backlog) {
> @@ -1771,6 +1772,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>
>                 qdisc_tree_reduce_backlog(sch, 1-numsegs, len-slen);
>                 consume_skb(skb);
> +               ret |= __NET_XMIT_STOLEN;
>         } else {
>                 /* not splitting */
>                 cobalt_set_enqueue_time(skb, now);
> @@ -1904,7 +1906,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>                 }
>                 b->drop_overlimit += dropped;
>         }
> -       return NET_XMIT_SUCCESS;
> +       return ret;
>  }
>
>  static struct sk_buff *cake_dequeue_one(struct Qdisc *sch)
> --
> 2.37.2
>

Re: [Cake] [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb

[Toke Høiland-Jørgensen]

Eric Dumazet <edumazet@google.com> writes:

> On Wed, Aug 31, 2022 at 2:25 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>
>> When the GSO splitting feature of sch_cake is enabled, GSO superpackets
>> will be broken up and the resulting segments enqueued in place of the
>> original skb. In this case, CAKE calls consume_skb() on the original skb,
>> but still returns NET_XMIT_SUCCESS. This can confuse parent qdiscs into
>> assuming the original skb still exists, when it really has been freed. Fix
>> this by adding the __NET_XMIT_STOLEN flag to the return value in this case.
>>
>
> I think you forgot to give credits to the team who discovered this issue.
>
> Something like this
>
> Reported-by: zdi-disclosures@trendmicro.com # ZDI-CAN-18231

Ah, right; apologies, will respin!

It also looks like fixing it this way will actually break other things
(most notably sch_cake as a child of sch_htb), so will send a different
patch as v2...

-Toke

Re: [Cake] [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb

[Toke Høiland-Jørgensen]

patchwork-bot+netdevbpf@kernel.org writes:

> Hello:
>
> This patch was applied to netdev/net.git (master)
> by Jakub Kicinski <kuba@kernel.org>:
>
> On Wed, 31 Aug 2022 11:21:03 +0200 you wrote:
>> When the GSO splitting feature of sch_cake is enabled, GSO superpackets
>> will be broken up and the resulting segments enqueued in place of the
>> original skb. In this case, CAKE calls consume_skb() on the original skb,
>> but still returns NET_XMIT_SUCCESS. This can confuse parent qdiscs into
>> assuming the original skb still exists, when it really has been freed. Fix
>> this by adding the __NET_XMIT_STOLEN flag to the return value in this case.
>> 
>> [...]
>
> Here is the summary with links:
>   - [net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb
>     https://git.kernel.org/netdev/net/c/90fabae8a2c2

Ah, crossed streams (just sent v2[0]).

Hmm, okay, so as noted in the changelog to v2, just this patch will
break htb+cake (because htb will now skip htb_activate()); do you prefer
that I send a follow-up to fix HTB in this mode, or to revert this and
apply the fix to sfb in v2 instead?

-Toke


[0] https://lore.kernel.org/r/20220831215219.499563-1-toke@toke.dk

Re: [Cake] [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb

[Jakub Kicinski]

On Thu, 01 Sep 2022 00:13:24 +0200 Toke Høiland-Jørgensen wrote:
> Ah, crossed streams (just sent v2[0]).

Sorry about that, traveling knocked out my sense of time and I kept
thinking it's Thursday, and the discussion happened yesterday :S

> Hmm, okay, so as noted in the changelog to v2, just this patch will
> break htb+cake (because htb will now skip htb_activate()); do you prefer
> that I send a follow-up to fix HTB in this mode, or to revert this and
> apply the fix to sfb in v2 instead?

Reverted. Let's review v2 as if v1 was not applied.

Re: [Cake] [PATCH net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb

[Toke Høiland-Jørgensen]

Jakub Kicinski <kuba@kernel.org> writes:

> On Thu, 01 Sep 2022 00:13:24 +0200 Toke Høiland-Jørgensen wrote:
>> Ah, crossed streams (just sent v2[0]).
>
> Sorry about that, traveling knocked out my sense of time and I kept
> thinking it's Thursday, and the discussion happened yesterday :S

Haha, OK, no worries :)

>> Hmm, okay, so as noted in the changelog to v2, just this patch will
>> break htb+cake (because htb will now skip htb_activate()); do you prefer
>> that I send a follow-up to fix HTB in this mode, or to revert this and
>> apply the fix to sfb in v2 instead?
>
> Reverted. Let's review v2 as if v1 was not applied.

SGTM!

-Toke