Re: [BUG] net/sched: Race Condition and Null Dereference in codel_change, pie_change, fq_pie_change, fq_codel_change, hhf_change

[Cong Wang <xiyou.wangcong@gmail.com>]

On Tue, Apr 29, 2025 at 01:41:19PM +0000, Savy wrote:
> 
> On Monday, April 28th, 2025 at 7:53 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> 
> > 
> > 
> > Excellent analysis!
> > 
> > Do you mind testing the following patch?
> > 
> > Note:
> > 
> > 1) We can't just test NULL, because otherwise we would leak the skb's
> > in gso_skb list.
> > 
> > 2) I am totally aware that maybe there are some other cases need the
> > same fix, but I want to be conservative here since this will be
> > targeting for -stable. It is why I intentionally keep my patch minimum.
> > 
> > Thanks!
> > 
> > --------------->
> > 
> 
> Hi Cong,
> 
> Thank you for the reply. We have tested your patch and can confirm that it resolves the issue.
> However, regarding point [1], we conducted some tests to verify if there is a skb leak in the gso_skb list, 
> but the packet remains in the list only for a limited amount of time.
> 
> In our POC we set a very low TBF rate, so when the Qdisc runs out of tokens, 
> it reschedules itself via qdisc watchdog after approximately 45 seconds.
> 
> Returning to the example above, here is what happens when the watchdog timer fires:
> 
> [ ... ]
> 
> Packet 2 is sent:
> 
>     [ ... ]
> 
>     tbf_dequeue()
>         qdisc_peek_dequeued()
>             skb_peek(&sch->gso_skb) // sch->gso_skb is empty
>             codel_qdisc_dequeue() // Codel qlen is 1
>                 qdisc_dequeue_head()
>                 // Packet 2 is removed from the queue
>                 // Codel qlen = 0
>             __skb_queue_head(&sch->gso_skb, skb); // Packet 2 is added to gso_skb list
>             sch->q.qlen++ // Codel qlen = 1
> 
>         // TBF runs out of tokens and reschedules itself for later
>         qdisc_watchdog_schedule_ns() 
> 
>     codel_change() // Patched, (!skb) break;, does not crash
> 
>     // ... ~45 seconds later the qdisc watchdog timer fires
> 
>     tbf_dequeue()
>         qdisc_peek_dequeued()
>             skb_peek(&sch->gso_skb) // sch->gso_skb is _not_ empty (contains Packet 2)
>         // TBF now has enough tokens
>         qdisc_dequeue_peeked()
>             skb = __skb_dequeue(&sch->gso_skb) // Packet 2 is removed from the gso_skb list
>             sch->q.qlen-- // Codel qlen = 0
> 
> Notice how the gso_skb list is correctly cleaned up when the watchdog timer fires.
> We also examined some edge cases, such as when the watchdog is canceled 
> and there are still packets left in the gso_skb list, and it is always cleaned up:
> 
> Qdisc destruction case:
> 
>     tbf_destroy()
>         qdisc_put()
>             __qdisc_destroy()
>                qdisc_reset()
>                    __skb_queue_purge(&qdisc->gso_skb);
> 
> Qdisc reset case:
> 
>     tbf_reset()
>         qdisc_reset()
>             __skb_queue_purge(&qdisc->gso_skb);
> 
> Perhaps the skb leak you mentioned occurs in another edge case that we overlooked? 


You are right, it is inaccurate to say it is a leak, probably just a
matter of ordering, because ->gso_list is logically the "head" of a
Qdisc.

> In any case, we believe your patch is technically more correct,
> as it makes sense to clean up packets in the gso_skb list first when the limit changes.
> 

Thank you for testing! I will add your Reported-and-Tested-by and send
out the patch.

Regards,
Cong