netdev - Re: [PATCH net v5] net: sched: cls_u32: Fix u32's systematic failure to free IDR entries for hnodes.

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] [day] [month] [year] [list]

Message-ID: <3be2edef-f3b0-4a02-b794-193e29047de2@orange.com>
Date: Fri, 8 Nov 2024 15:46:44 +0100
From: Alexandre Ferrieux <alexandre.ferrieux@...il.com>
To: Cong Wang <xiyou.wangcong@...il.com>,
 Alexandre Ferrieux <alexandre.ferrieux@...il.com>
Cc: edumazet@...gle.com, jhs@...atatu.com, jiri@...nulli.us,
 netdev@...r.kernel.org
Subject: Re: [PATCH net v5] net: sched: cls_u32: Fix u32's systematic failure
 to free IDR entries for hnodes.

On 08/11/2024 02:34, Cong Wang wrote:
> On Fri, Nov 08, 2024 at 12:11:23AM +0100, Alexandre Ferrieux wrote:
>> To generate hnode handles (in gen_new_htid()), u32 uses IDR and
>> encodes the returned small integer into a structured 32-bit
>> word. Unfortunately, at disposal time, the needed decoding
>> is not done. As a result, idr_remove() fails, and the IDR
>> fills up. [...]
>> This patch adds the missing decoding logic for handles that
>> deserve it, along with a corresponding tdc test.
> 
> Good catch! I have a few comments below.
> [...]
> It seems you missed the idr_replace() case?
> 
>  - idr_replace(&ht->handle_idr, n, n->handle);
>  + idr_replace(&ht->handle_idr, n, handle2id(n->handle));

Unless I'm mistaken, there are two different kinds of IDR:
 - the "toplevel" IDR tp_c->handle_idr used to generate the IDs of hnodes
(hashtables)
 - the individual IDR ht->handle_idr holding IDs of knodes
As it turns out, hnodes use small integers in [1..0x7FF] in the IDR space, and
carry them along "encoded" (with id2handle() now), while knodes are directly
allocated in a higher range of the IDR space. As a consequence, it does not make
sense to encode/decode them with the hnode-oriented scheme.
It looks like idr_replace() is used on knodes, so it should not make sense to
add the decoding here.

Now, this is all based on reverse engineering, as I'm not aware of detailed
documentation (beyond code comments). Please correct me if I'm wrong.

>> +        "cmdUnderTest": "bash -c 'for i in {1..2048} ;do $TC filter delete dev $DEV1 pref 3;$TC filter add dev $DEV1 parent 10:0 protocol ip prio 3 u32 match ip src 0.0.0.3/32 action drop || exit 1;i=`expr $i + 1`;done'",
> 
> Any reason using this for loop instead of tdc_multibatch.py?

I have zero experience with tdc, so I'm just working on the README and
user-other oriented documentation, suggesting to extend the provided JSON test
specification. So quite possibly I missed some "other ways". However, I have
just posted a v6 of the patch that uses tc in batch mode (tc -b) and reduces the
execution time of this test from over 10 secs to a split second, and to me the
resulting line remains readable:

	for i in {1..2048} ;do echo filter delete dev $DEV1 pref 3;echo filter add dev
$DEV1 parent 10:0 protocol ip prio 3 u32 match ip src 0.0.0.3/32 action
drop;done | $TC -b -

Please tell me if you deem necessary to switch to another method and/or syntax.