netdev - Re: [PATCH net v2 1/5] net_sched: drr: Fix double list add in class with netem as child qdisc

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: <25909f3c-a263-4532-8d61-2bad391a1d79@mojatatu.com>
Date: Wed, 23 Apr 2025 11:41:08 -0300
From: Victor Nogueira <victor@...atatu.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
 jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com,
 pabeni@...hat.com, toke@...hat.com, gerrard.tai@...rlabs.sg,
 pctammela@...atatu.com
Subject: Re: [PATCH net v2 1/5] net_sched: drr: Fix double list add in class
 with netem as child qdisc

On 4/22/25 21:44, Jakub Kicinski wrote:
> On Wed, 16 Apr 2025 07:24:23 -0300 Victor Nogueira wrote:
>> +static bool cl_is_initialised(struct drr_class *cl)
> cl_is_active() ?
> Had to look at the code to figure out what it does, but doesn't seem to
> have much to do with being "initialised". The point is that the list
> node of this class is not on the list of active classes.

Ok, I believe cl_is_active describes it better. I'll make that change.

>>   static struct drr_class *drr_find_class(struct Qdisc *sch, u32 classid)
>>   {
>>   	struct drr_sched *q = qdisc_priv(sch);
>> @@ -357,7 +362,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>>   		return err;
>>   	}
>>   
>> -	if (first) {
>> +	if (first && !cl_is_initialised(cl)) {
> I think we can delete the "first" check and temp variable.
> The code under the if() does not touch the packet so it doesn't matter
> whether we execute it for the initial or the nested call, right?

I was being more conservative, but your suggestion is a good optimisation.

cheers,
Victor