| Message ID | 20220831092103.442868-1-toke@toke.dk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 90fabae8a2c225c4e4936723c38857887edde5cc |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] sch_cake: Return __NET_XMIT_STOLEN when consuming enqueued skb | expand |
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 >
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
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 You are awesome, thank you!
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
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.
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
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)
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(-)