[Cake] Re: [PATCH net-next v7 4/4] net/sched: cake: hash before segmenting and reject an overlong split

[Jamal Hadi Salim <jhs@mojatatu.com>]

On Tue, Oct 20, 2026 at 4:47 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> >> > +
> >> > +             segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
> >> > +             if (IS_ERR_OR_NULL(segs))
> >> > +                     return qdisc_drop(skb, sch, to_free);
> >> > +
> >> > +             /* The split path accounts the segment sum, not the
> >> > +              * stab-adjusted qdisc_pkt_len(), so check it here.
> >> > +              */
> >> > +             skb_list_walk_safe(segs, seg, nskb)
> >> > +                     slen += seg->len;
> >> > +
> >> > +             if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) {
> >> > +                     qdisc_qstats_overlimit(sch);
> >> > +                     /* __skb_gso_segment() puts SKB_GSO_CB at cb
> >> > +                      * offset 32, over tc_skb_cb.drop_reason; every
> >> > +                      * segment inherits it. Tag each one with the
> >> > +                      * qdisc drop reason.
> >> > +                      */
> >> > +                     skb_list_walk_safe(segs, seg, nskb)
> >> > +                             tcf_set_qdisc_drop_reason(seg,
> >> > +                                                       QDISC_DROP_OVERLIMIT);
> >> > +                     /* skb_gso_segment() moved sock_wfree to the tail
> >> > +                      * segment, so defer the list to to_free rather
> >> > +                      * than free it under the qdisc lock.
> >>
> >> I don't think this part of the comment is needed - we do all the drops
> >> using to_free, so the sock_wfree thing is irrelevant.
> >
> > I was trying to rationalize "the segs == skb fraglist". How about:
> > /* Fraglist GSO returns the original skb as head  (segs == skb), so
> > drop our ref first */
> > I can drop it if it seems obvious
>
> Yeah, that bit is fine to keep :)
>
> >> > +                      * Fraglist GSO returns the original skb as head
> >> > +                      * (segs == skb), so drop our ref first.
> >> > +                      */
> >> > +                     if (unlikely(segs == skb)) {
> >> > +                             consume_skb(skb);
> >> > +                             return qdisc_drop_all(segs, sch, to_free);
> >> > +                     }
> >> > +                     __qdisc_drop_all(segs, to_free);
> >> > +                     return qdisc_drop_reason(skb, sch, to_free,
> >> > +                                              QDISC_DROP_OVERLIMIT);
> >>
> >> Since you already set the drop reason above, there's not really any
> >> reason to use qdisc_drop_reason() anymore; so this logic can be
> >> simplified to simply:
> >>
> >>                         if (unlikely(segs == skb))
> >>                                 consume_skb(skb);
> >>                         return qdisc_drop_all(segs, sch, to_free);
> >>
> >
> > How does this look ?
> > tcf_set_qdisc_drop_reason(skb, QDISC_DROP_OVERLIMIT);
> > skb_list_walk_safe(segs, seg, nskb)
> >      tcf_set_qdisc_drop_reason(seg, QDISC_DROP_OVERLIMIT);
> >
> > if (unlikely(segs == skb)) {
> >         consume_skb(skb);
> >         return qdisc_drop_all(segs, sch, to_free);
> > }
> > __qdisc_drop_all(segs, to_free);
> > return qdisc_drop(skb, sch, to_free);
>
> Ah, right, got the 'segs' and 'skb' variables a bit confused, sorry
> about that.
>
> But, thinking about it some more:
>
> In both branches of your code above, we have a drop of the skb, and a
> drop_all of the segs. And the existing code already has an unconditional
> consume_skb() after segmentation, where none of the post-segmentation
> uses the 'skb' pointer.
>
> So wouldn't it be simpler to just add the consume_skb() unconditionally
> immediately after the skb_gso_segment() call? Something like this, on
> top of your patch:
>
> diff --git i/net/sched/sch_cake.c w/net/sched/sch_cake.c
> index 730c90af14bd..20173aaf973c 100644
> --- i/net/sched/sch_cake.c
> +++ w/net/sched/sch_cake.c
> @@ -1819,13 +1819,17 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>         cake_hash_keys(skb, flow_mode, flow_override, host_override, &keys);
>
>         if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_SPLIT_GSO) {
> -               struct sk_buff *nskb, *seg;
>                 netdev_features_t features = netif_skb_features(skb);
> +               struct sk_buff *nskb, *seg;
>
>                 segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
>                 if (IS_ERR_OR_NULL(segs))
>                         return qdisc_drop(skb, sch, to_free);
>
> +               /* we never use the original skb after splitting */
> +               consume_skb(skb);
> +               skb = NULL;
> +
>                 /* The split path accounts the segment sum, not the
>                  * stab-adjusted qdisc_pkt_len(), so check it here.
>                  */
> @@ -1842,20 +1846,8 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>                         skb_list_walk_safe(segs, seg, nskb)
>                                 tcf_set_qdisc_drop_reason(seg,
>                                                           QDISC_DROP_OVERLIMIT);
> -                       /* skb_gso_segment() moved sock_wfree to the tail
> -                        * segment, so defer the list to to_free rather
> -                        * than free it under the qdisc lock.
> -                        *
> -                        * Fraglist GSO returns the original skb as head
> -                        * (segs == skb), so drop our ref first.
> -                        */
> -                       if (unlikely(segs == skb)) {
> -                               consume_skb(skb);
> -                               return qdisc_drop_all(segs, sch, to_free);
> -                       }
> -                       __qdisc_drop_all(segs, to_free);
> -                       return qdisc_drop_reason(skb, sch, to_free,
> -                                                QDISC_DROP_OVERLIMIT);
> +
> +                       return qdisc_drop_all(segs, sch, to_free);
>                 }
>         }
>
> @@ -1917,9 +1909,8 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>                 WRITE_ONCE(b->backlogs[idx], b->backlogs[idx] + slen);
>
>                 qdisc_tree_reduce_backlog(sch, 1-numsegs, len-slen);
> -               consume_skb(skb);
>         } else {
> -               /* not splitting */
> +               /* not splitting - skb pointer still valid */
>                 int ack_pkt_len = 0;
>
>                 cobalt_set_enqueue_time(skb, now);
>
> >> > +             }
> >> > +     }
> >> > +
> >> > +     /* Admitted: resolve the flow. The mode is read once above, so
> >> > +      * extraction and resolution agree if cake_change() intervenes.
> >> > +      */
> >>
> >> The last sentence here is just distracting. Please drop.
> >>
> >
> > Ok.Will reword to: /* Admitted: resolve the flow. */
> >
> > I wont wait for the 7 days for sashiko to respond if we can reach a
> > quick consensus.
>
> Fine with me :)

I will resend with your suggestions.

pw-bot: cr

cheers,
jamal
> -Toke