netdev - Re: Qdisc->u32_node

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, 7 Aug 2017 21:06:24 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     jhs@...atatu.com, xiyou.wangcong@...il.com, davem@...emloft.net,
        netdev@...r.kernel.org, mlxsw@...lanox.com
Subject: Re: Qdisc->u32_node - licence to kill

Mon, Aug 07, 2017 at 07:47:14PM CEST, john.fastabend@...il.com wrote:
>On 08/07/2017 09:41 AM, Jiri Pirko wrote:
>> Hi Jamal/Cong/David/all.
>> 
>> Digging in the u32 code deeper now. I need to get rid of tp->q for shared
>> blocks, but I found out about this:
>> 
>> struct Qdisc {
>> 	......
>>         void                    *u32_node;
>> 	......
>> };
>> 
>> Yeah, ugly. u32 uses it to store some shared data, tp_c. It actually
>> stores a linked list of all hashtables added to one qdiscs.
>> 
>> So basically what you have is, you have 1 root ht per prio/pref. Then
>> you can have multiple hts, linked from any other ht, does not matter in
>> which prio/pref they are.
>> 
>
>We can create arbitrary hash tables here independent of prio/pref via
>TCA_U32_DIVISOR. Then these can be linked to other hash tables via
>TCA_U32_LINK commands.

Yeah, that's what I thought.


>
>prio/pref does not really play any part here from my reading, except as
>a further specifier in the walk callbacks. Making it a useful filter on
>dump operations.

Not correct. prio/pref is one level up priority, independent on specific
cls implementation. You can have cls_u32 instance on prio 10 and
cls_flower instance on prio 20. Both work.

In fact, the current u32 "linking" ignores the upper level
prio/pref and breakes user assumptions when he inserts rules with
specific prio.


>
>> Do I understand that correctly that prio/pref only has meaning if
>> linking does not take place, because if there is linking, the prio/pref
>> of inserted rule is simply ignored?
>
>I think even then the prio/pref meaning is dubious, from u32_change,

Please see tc_ctl_tfilter. That is where prio/pref is processed. What
you describe is one level down.


>
>                for (pins = rtnl_dereference(*ins); pins;
>                     ins = &pins->next, pins = rtnl_dereference(*ins))
>                        if (TC_U32_NODE(handle) < TC_U32_NODE(pins->handle))
>                                break;
>
>I think the list insert is done via handle not via prio/pref.
>
>> 
>> That is the most confusing thing I saw in net/sched/ so far. 
>> Is this a bug? Sounds like one.
>> 
>
>I don't think this is a bug at very least I don't see how we can
>change it without breaking users. I know people depend on the hash map
>capabilities and linking logic.

Do they insert rules into multiple hashtables with different prio? Why?
What is the usecase?


>
>> Did someone introduce *u32_node (formerly static struct tc_u_common
>> *u32_list;) just to allow this weirdness?
>> 
>> Can I just remove this shared tp_c and make the linking to other
>> hashtables only possible within the same prio/pref? That would make
>> sense to me.
>> 
>
>The idea to make linking hash tables only possible within the same
>prio/pref will break existing programs. We can't do this its part of
>UAPI now and people depend on it.

That's why I asked if that is a bug. I still feel it is. But I
definitelly understand your concern. I'm just trying to figure out how
to resolve this misdesign :(