netdev - Re: Fwd: u32 ht filters

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]

Date:   Mon, 12 Feb 2018 14:23:52 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Jiri Pirko <jiri@...lanox.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Paweł Staszewski <pstaszewski@...are.pl>
Subject: Re: Fwd: u32 ht filters

Sat, Feb 10, 2018 at 09:41:57PM CET, xiyou.wangcong@...il.com wrote:
>On Wed, Feb 7, 2018 at 11:38 PM, Jiri Pirko <jiri@...nulli.us> wrote:
>> Thu, Feb 08, 2018 at 12:08:36AM CET, xiyou.wangcong@...il.com wrote:
>>>On Tue, Feb 6, 2018 at 11:01 PM, Jiri Pirko <jiri@...nulli.us> wrote:
>>>> Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangcong@...il.com wrote:
>>>>>Hi, Jiri
>>>>>
>>>>>Your  commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c
>>>>>breaks the tc script by Paweł. Please find below for details.
>>>>
>>>> Did you do the bisection?
>>>> The commit just uses block struct instead of q, but since they
>>>> are in 1:1 relation, that should be equvivalent. So basically you still
>>>> have per-qdisc hashtables for u32.
>>>
>>>Well, at least the following fixes the problem here. But I am not sure
>>>if it is expected too for shared block among multiple qdiscs.
>>
>> For shared block, block->q is null.
>
>According to this comment:
>/* block_index not 0 means the shared block is requested */
>
>and the code,
>
>        if (!block) {
>                block = tcf_block_create(net, q, extack);
>
>block->q is set to q, and q is always non-NULL AFAIU.

Yep, that is a bug. Fixing it now.

>
>Also, I don't know if it is intended, but block->q always points to
>the parent qdisc rather than the qdisc attached to a class.

You are right. That is incorrect. Fixing now.

>
>
>>>
>>>
>>>@@ -338,7 +330,7 @@ static struct hlist_head *tc_u_common_hash;
>>>
>>> static unsigned int tc_u_hash(const struct tcf_proto *tp)
>>> {
>>>-       return hash_ptr(tp->chain->block, U32_HASH_SHIFT);
>>>+       return hash_ptr(tp->chain->block->q, U32_HASH_SHIFT);
>>> }
>>>
>>> static struct tc_u_common *tc_u_common_find(const struct tcf_proto *tp)
>>>@@ -348,7 +340,7 @@ static struct tc_u_common *tc_u_common_find(const
>>>struct tcf_proto *tp)
>>>
>>>        h = tc_u_hash(tp);
>>>        hlist_for_each_entry(tc, &tc_u_common_hash[h], hnode) {
>>>-               if (tc->block == tp->chain->block)
>>>+               if (tc->block->q == tp->chain->block->q)
>>
>> :O I don't get it. tc->block is pointer, tc->block->q is pointer. And
>> they are different at the same time for non-shared block.
>
>If you look into Pawel's script, a new block is created for each class
>therefore a different tc_u_common is created which causes the
>ht 9:22 can't be found.

Yeah :/ this tc_u_common thing is going to haunt me. This is resolvable
now by using block->q. There is no support for block sharing for other
qdiscs than ingress and clsact so we don't have to take care of the
multi-class&block sharing now. Will fix.

Thanks!