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: <d56902b7-1e1a-402e-b0b8-9080e9115add@mojatatu.com>
Date: Fri, 2 May 2025 19:13:23 -0300
From: Victor Nogueira <victor@...atatu.com>
To: Paolo Abeni <pabeni@...hat.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 5/2/25 06:59, Paolo Abeni wrote:
> 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?

I have no strong feelings, I'm ok with merging the patch
as is.

cheers,
Victor