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

["Toke Høiland-Jørgensen" <toke@toke.dk>]

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

-Toke