Date:   Thu, 2 Feb 2017 18:48:03 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Tom Herbert <tom@...bertland.com>
Cc:     Simon Horman <simon.horman@...ronome.com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        David Miller <davem@...emloft.net>,
        Dinan Gunawardena <dinan.gunawardena@...ronome.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        oss-drivers@...ronome.com
Subject: Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

Thu, Feb 02, 2017 at 06:24:40PM CET, tom@...bertland.com wrote:
>On Thu, Feb 2, 2017 at 7:58 AM, Simon Horman <simon.horman@...ronome.com> wrote:
>> [Repost due to gmail account problem]
>>
>> On Thu, Feb 02, 2017 at 04:31:33AM -0800, Eric Dumazet wrote:
>>> On Thu, 2017-02-02 at 11:37 +0100, Simon Horman wrote:
>>> > Allow dissection of Neighbour Discovery target IP, and source and
>>> > destination link-layer addresses for neighbour solicitation and
>>> > advertisement messages.
>>> >
>>> > Signed-off-by: Simon Horman <simon.horman@...ronome.com>
>>> > ---
>>>
>>> Hi Simon
>>>
>>> Why is this needed ?
>>>
>>> Any code added in flow dissector needs to be extra careful,
>>> we had various packet of deaths errors recently in this area.
>>
>> Hi Eric,
>>
>> there some activity to allow programming of OvS flows in hardware via TC
>> with the flower classifier. As the ND fields in this patch are part of the
>> OvS flow key I would like them considered for additions to flower and thus
>> the dissector to allow compatibility with OvS.
>>
>Given that ARP is already there it seems only "fair" to have ND also.
>But Eric is correct, this is quite a sensitive area of code.
>
>> I apologise if any 'deaths' have resulted from my recent work on the
>> dissector. I am of course very open to ideas on how to avoid any future
>> incidents.
>
>That's a tough problem. flow_dissector started off as simple mechanism
>to just identify actual flows (really just TCP and UDP packets) for
>the purposes of packet steering. But given the benefits of its
>location low in the stack and the open ended capabilities for parsing
>it seems to have mushroomed into a general catchall to parse a whole
>bunch of different protocols. A lot of these go beyond simply
>identifying flows (ICMP parsing, ARP, or ND as in your patches). These
>new use cases may be valid, but the result is a convoluted function (>
>500 LOC by my count) and it seems to be quite easy to have subtle bugs
>mostly in edge cases, several of which could have been exploited in
>DDOS attacks.

Agreed that we probably came to a point when we need to split
__skb_flow_dissect into modular and pluggable pieces. Will not be
trivial though.

Also note that it depends on the __skb_flow_dissect user which code is
actually used or not. For the critical path, that keys are defined by:
flow_keys_dissector_keys

Most of the code Simon is adding is noop for non-flower usecase if:
dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) == false


>
>At some point we need to stop adding new protocols to parse in
>__skb_flow_dissect and push the processing back into the protocol
>modules with a callout interface from flow_dissector (for instance if
>we ever want VXLAN parsing in flow dissector this is the only
>reasonable way to do it). That moves the complexity but doesn't solve
>the problem of buggy code in this critical path. An alternative might
>be to put a cap on flow_dissector and add a hook to BPF program to

Loks like BPF is becoming an answer for everything these days :O


>allow parsing of new protocols. This has the advantage of providing an
>constrained interface that could eliminate possibility of some types
>of bugs we've seen. Also, this allows adding support for "user"
>protocols that the kernel might not even know about (QUIC comes to
>mind).

Not sure it is wise to make life easier for the proprietary
out-of-tree beasts...

Powered by blists - more mailing lists