netdev - Re: [Patch net 1/2] sch_htb: make htb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <74017437-48d2-4518-9888-e74c14e01a5d@redhat.com>
Date: Fri, 2 May 2025 11:59:21 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Victor Nogueira <victor@...atatu.com>,
 Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org
Cc: jhs@...atatu.com, jiri@...nulli.us, alan@...ie.me.uk
Subject: Re: [Patch net 1/2] sch_htb: make htb_deactivate() idempotent

On 4/30/25 5:19 PM, Victor Nogueira wrote:
> If that's so, couldn't we instead of doing:
> 
>> @@ -348,7 +348,8 @@ static void htb_add_to_wait_tree(struct htb_sched *q,
>>    */
>>   static inline void htb_next_rb_node(struct rb_node **n)
>>   {
>> -	*n = rb_next(*n);
>> +	if (*n)
>> +		*n = rb_next(*n);
>>   }
> 
> do something like:
> 
> @@ -921,7 +921,9 @@ static struct sk_buff *htb_dequeue_tree(struct 
> htb_sched *q, const int prio,
>                  cl->leaf.deficit[level] -= qdisc_pkt_len(skb);
>                  if (cl->leaf.deficit[level] < 0) {
>                          cl->leaf.deficit[level] += cl->quantum;
> -                       htb_next_rb_node(level ? 
> &cl->parent->inner.clprio[prio].ptr :
> +                       /* Account for (fq_)codel child deactivating 
> after dequeue */
> +                       if (likely(cl->prio_activity))
> +                               htb_next_rb_node(level ? 
> &cl->parent->inner.clprio[prio].ptr :
>   
> &q->hlevel[0].hprio[prio].ptr);
>                  }
>                  /* this used to be after charge_class but this constelation
> 
> That way it's clear that the issue is a corner case where the
> child qdisc deactivates the parent. Otherwise it seems like
> we need to check for (*n) being NULL for every call to
> htb_next_rb_node.

@Victor, I think that the Cong's suggested patch is simpler to very/tie
to code change to the actually addressed issue. I started at your
suggest code for a bit, and out of sheer htb ignorance I'm less
confident about it fixing the thing.

Do you have strong feeling vs Cong's suggested approach?

Thanks,

Paolo