Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

On Thu, Feb 2, 2017 at 9:48 AM, Jiri Pirko [off-list ref] wrote:

Thu, Feb 02, 2017 at 06:24:40PM CET, tom@herbertland.com wrote:

quoted

On Thu, Feb 2, 2017 at 7:58 AM, Simon Horman [off-list ref] wrote:

quoted

[Repost due to gmail account problem]

On Thu, Feb 02, 2017 at 04:31:33AM -0800, Eric Dumazet wrote:

quoted

On Thu, 2017-02-02 at 11:37 +0100, Simon Horman wrote:

quoted

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 <redacted>
---

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.

quoted

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

True, but the code doesn't separate out the critical path from all
these extended features which is resulted in a jumbled mess with no
modularity to speak of :-(

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

Sure, but that just makes this code corner cases which means it's hard
to maintain and harder to find bugs in the long run.

quoted

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

In this case it makes sense though, we can't just continue accepting
every poor little narrow use-case protocol that comes along into the
kernel :-)

quoted

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...

It's going to be a problem with a whole host of application level
protocols especially those run over UDP. QUIC is a great example. The
actual protocol will probably only ever run in userspace, but it is
inevitable that we want to provide targeted kernel support for packet
steering. filtering, GRO/GSO if they have such things. Instead of
implementing this in a specialized QUIC module, it will most likely
make everyone happier to add these in a generic protocol-agnostic way.

From QUIC POV they want to minimize any dependencies on the kernel and

be able to iterate quickly, from a kernel POV we really don't want to
have to explicitly support an endless stream of protocols like this.

Tom