[Cake] Re: [PATCH V2 RESEND] net/sched: sch_cake: Fix incorrect qlen reduction in cake_drop

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

Xiang Mei <xmei5@asu.edu> writes:

> In cake_drop(), the function qdisc_tree_reduce_backlog() is called to
> decrement the qlen of the qdisc hierarchy. However, it may incorrectly
> reduce the qlen when the dropped packet was not enqueued, leading to
> a potential null-pointer dereference (e.g., when using qfq sched as
> the parent qdisc).
>
> This issue occurs when the caller (cake_enqueue()) returns NET_XMIT_CN,
> causing the parent qdisc not to enqueue the current packet, while
> qdisc_tree_reduce_backlog() still decrements the backlog.
>
> This patch prevents invalid qlen reduction by verifying that the
> dropped packet was actually enqueued before adjusting the backlog.
>
> Fixes: 15de71d06a40 ("net/sched: Make cake_enqueue return NET_XMIT_CN when past buffer_limit")
> Signed-off-by: Xiang Mei <xmei5@asu.edu>

Thank you for the patch! I think the issue is valid, but I don't think
the fix is quite right - see below.

Also, this should probably go through netdev? Please include
netdev@vger.kernel.org and add a 'net' tag to the patch subject when you
respin (so it'll look like [PATCH net v3]).

> ---
> v2: add missing cc
>
>  net/sched/sch_cake.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index 32bacfc314c2..3a2ba9dfc22d 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -1548,7 +1548,7 @@ static int cake_advance_shaper(struct cake_sched_data *q,
>  	return len;
>  }
>  
> -static unsigned int cake_drop(struct Qdisc *sch, struct sk_buff **to_free)
> +static unsigned int cake_drop(struct Qdisc *sch, struct sk_buff **to_free, u32 current_flow)
>  {
>  	struct cake_sched_data *q = qdisc_priv(sch);
>  	ktime_t now = ktime_get();
> @@ -1597,7 +1597,8 @@ static unsigned int cake_drop(struct Qdisc *sch, struct sk_buff **to_free)
>  
>  	qdisc_drop_reason(skb, sch, to_free, SKB_DROP_REASON_QDISC_OVERLIMIT);
>  	sch->q.qlen--;
> -	qdisc_tree_reduce_backlog(sch, 1, len);
> +	if (likely(current_flow != idx + (tin << 16)))
> +		qdisc_tree_reduce_backlog(sch, 1, len);

Since cake_drop() is called in a loop, the number of bytes/packets
dropped may not match the packet coming in. So we can't just skip this.

I think the simplest would probably be to move the
qdisc_tree_reduce_backlog() out of cake_drop entirely, and then
replicate the logic from fq_codel in the caller. I.e., the bit where it
saves the qlen and backlog before dropping and calls
qdisc_tree_reduce_backlog() once after. So we'll end up with something
like:

prev_backlog = sch->qstats.backlog;
prev_qlen = sch->q.qlen;
while (q->buffer_used > q->buffer_limit) {
/*...*/
}
prev_qlen -= sch->q.qlen;
prev_backlog -= sch->qstats.backlog;
if (same_flow)
   qdisc_tree_reduce_backlog(sch, prev_qlen - 1,
				  prev_backlog - pkt_len);
   return NET_XMIT_CN;
}
qdisc_tree_reduce_backlog(sch, prev_qlen,b
				  prev_backlog);

/* etc */

-Toke