netdev - Re: [PATCH v4 net-next 0/8] flow_dissector: Protocol specific flow dissector offload

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: <20171004064532.GD1895@nanopsycho>
Date:   Wed, 4 Oct 2017 08:45:32 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Tom Herbert <tom@...ntonium.net>
Cc:     Tom Herbert <tom@...bertland.com>,
        David Miller <davem@...emloft.net>,
        Hannes Frederic Sowa <hannes@...essinduktion.org>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Rohit Seth <rohit@...ntonium.net>
Subject: Re: [PATCH v4 net-next 0/8] flow_dissector: Protocol specific flow
 dissector offload

Tue, Oct 03, 2017 at 08:35:54PM CEST, tom@...ntonium.net wrote:
>On Tue, Oct 3, 2017 at 12:46 AM, Jiri Pirko <jiri@...nulli.us> wrote:
>> Fri, Sep 29, 2017 at 07:59:35PM CEST, tom@...bertland.com wrote:
>>>On Fri, Sep 29, 2017 at 10:42 AM, David Miller <davem@...emloft.net> wrote:
>>>> From: Tom Herbert <tom@...bertland.com>
>>>> Date: Fri, 29 Sep 2017 08:48:55 -0700
>>>>
>>>>> The flow_dissector interface is not a uAPI.
>>>>
>>>> That's not true, insofar as cls_flower.c uses the flow_dissector
>>>> therefore if you change the flow_dissector in certain ways then
>>>> cls_flower.c might have it's behavior changed and that is in fact UAPI
>>>> facing.
>>>
>>>Then I would suggest adding another flag like FLOW_DISSECTOR_F_FLOWER
>>>and when anyone puts new code into flow_dissector they can wrap it
>>>with "if !(flags & FLOW_DISSECTOR_F_FLOWER)". If the flower uAPI is
>>>subsequently update then the conditional can be removed. This way
>>>flower can support maintain its APIs, but we can still still extend
>>>and improve flow_dissector for othersuse cases.
>>
>> This is not flower-specific problem. Flow_dissector is a servant of many.
>
>Besides flower, what other use cases of flow_dissector have made
>flow_dissector interface a uAPI? Any use of hashing does not do this.
>Maybe OVS does?

It may be that currently it affects only flower. That does not mean you
should add flower-specific quirk. All I say is this should be handled in
a generic way, independent on the caller.


>
>> As such, it is instructed what should it do. If you want to
>> change the way inner headers are parsed, you should either:
>
>Why would that only affect the way inner headers are parsed? Wouldn't
>we need to consider any change to flow_dissector that might affect the
>output in any way. For instance, the depth limits I added would change
>to output for someone that was parsing thirty-five layers of
>encapsulation so it it looks like that feature needs a flag. What if
>someone adds a new Ethernet protocol or a new encap protocol?

Sure, what I ment was any change of behaviour.


>
>> 1) change the callers so they are behaving the same as before
>> 2) make the flow_dissection change optional so the caller can say if he
>>    wants original or new behaviour.
>
>I guess we can do that, but am concerned about the overhead this will
>generate if were adding a flag each time anyone modifies the function.
>There are performance critical use cases of flow_dissector that will
>be impacted by such changes.

I don't think that the overhead would be much different from what you
proposed.


>
>Tom
>
>
>>