Thu, Feb 02, 2017 at 06:24:40PM CET, [email protected] wrote: >On Thu, Feb 2, 2017 at 7:58 AM, Simon Horman <[email protected]> >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 <[email protected]> >>> > --- >>> >>> 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...
