netdev - Re: [patch net-next 0/9] net: sched: introduce chain templates support with offloading to mlxsw

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:   Wed, 27 Jun 2018 09:50:17 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Simon Horman <simon.horman@...ronome.com>,
        John Hurley <john.hurley@...ronome.com>,
        David Ahern <dsahern@...il.com>, mlxsw@...lanox.com
Subject: Re: [patch net-next 0/9] net: sched: introduce chain templates
 support with offloading to mlxsw

Tue, Jun 26, 2018 at 11:18:58PM CEST, jakub.kicinski@...ronome.com wrote:
>On Tue, 26 Jun 2018 09:12:17 +0200, Jiri Pirko wrote:
>> Tue, Jun 26, 2018 at 09:00:45AM CEST, jakub.kicinski@...ronome.com wrote:
>> >On Mon, Jun 25, 2018 at 11:43 PM, Jiri Pirko <jiri@...nulli.us> wrote:  
>> >> Tue, Jun 26, 2018 at 06:58:50AM CEST, jakub.kicinski@...ronome.com wrote:  
>> >>>On Mon, 25 Jun 2018 23:01:39 +0200, Jiri Pirko wrote:  
>> >>>> From: Jiri Pirko <jiri@...lanox.com>
>> >>>>
>> >>>> For the TC clsact offload these days, some of HW drivers need
>> >>>> to hold a magic ball. The reason is, with the first inserted rule inside
>> >>>> HW they need to guess what fields will be used for the matching. If
>> >>>> later on this guess proves to be wrong and user adds a filter with a
>> >>>> different field to match, there's a problem. Mlxsw resolves it now with
>> >>>> couple of patterns. Those try to cover as many match fields as possible.
>> >>>> This aproach is far from optimal, both performance-wise and scale-wise.
>> >>>> Also, there is a combination of filters that in certain order won't
>> >>>> succeed.
>> >>>>
>> >>>> Most of the time, when user inserts filters in chain, he knows right away
>> >>>> how the filters are going to look like - what type and option will they
>> >>>> have. For example, he knows that he will only insert filters of type
>> >>>> flower matching destination IP address. He can specify a template that
>> >>>> would cover all the filters in the chain.  
>> >>>
>> >>>Perhaps it's lack of sleep, but this paragraph threw me a little off
>> >>>the track.  IIUC the goal of this set is to provide a way to inform the
>> >>>HW about expected matches before any rule is programmed into the HW.
>> >>>Not before any rule is added to a particular chain.  One can just use
>> >>>the first rule in the chain to make a guess about the chain, but thanks
>> >>>to this set user can configure *all* chains before any rules are added.  
>> >>
>> >> The template is per-chain. User can use template for chain x and
>> >> not-use it for chain y. Up to him.  
>> >
>> >Makes sense.
>> >
>> >I can't help but wonder if it'd be better to associate the
>> >constraints/rules with chains instead of creating a new "template"
>> >object.  It seems more natural to create a chain with specific
>> >constraints in place than add and delete template of which there can
>> >be at most one to a chain...  Perhaps that's more about the user space
>> >tc command line.  Anyway, not a strong objection, just a thought.  
>> 
>> Hmm. I don't think it is good idea. User should see the template in a
>> "show" command per chain. We would have to have 2 show commands, one to
>> list the template objects and one to list templates per chains. It makes
>> things more complicated for no good reason. I think that this simple
>> chain-lock is easier and serves the purpose.
>
>Hm, I think the dump is fine, what I was thinking about was:
>
># tc chain add dev dummy0 ingress chain_index 22 \
>     ^^^^^
>	template proto ip \
>	^^^^^^^^
>	flower dst_mac 00:00:00:00:00:00/00:00:00:00:FF:FF

Okay, I got it. I see 2 issues.
1) user might expect to add a chain without the template. But that does
   not make sense really. Chains are created/deleted implicitly
   according to refcount.
2) there is not chain object like this available to user. Adding it just
   for template looks odd. Also, the "filter" and "template" are very
   much alike. They both are added to a chain, they both implicitly
   create chain if it does not exist, etc.

if you don't like "tc filter template add dev dummy0 ingress", how
about:
"tc template add dev dummy0 ingress ..."
"tc template add dev dummy0 ingress chain 22 ..."
that makes more sense I think.


>
>instead of:
>
># tc filter template add dev dummy0 ingress \
>     ^^^^^^^^^^^^^^^
>	proto ip chain_index 22 \
>	flower dst_mac 00:00:00:00:00:00/00:00:00:00:FF:FF
>
>And then delete becomes:
>
># tc chain del dev dummy0 ingress chain_index 22
>Error: The chain is not empty.
>
>The fact that template is very much like a filter is sort of an
>implementation detail, from user perspective it may be more intuitive
>to model template as an attribute of the chain, not a filter object
>added to a chain.
>
>But I could well be the only person who feels that way :)