This RFC is a proposal to avoid the manual assert in dp_packet_resize__() for DPBUF_DPDK packets. It has been reported before, in [1], as something that can lead to crashes.
Overall I can see two approaches to this problem: 1. Modify dp_packet's API entirely to deal with errors and report them to callers appropriately; 2. For DPDK packets only, check a priori if the prealloc_tailroom() and prealloc_headroom() operations are possible or not (the only callers of dp_packet_resize__(), which leads to the manual assert). Approach 1. would lead to a fair amount of change without giving much in return when using non-DPDK packets (as DPBUF_MALLOC). For example, any packet created with dp_packet_new() / dp_packet_init() / dp_packet_use_stub(), or even cloned (a DPBUF_DPDK is cloned into a DPBUF_MALLOC, for example), wouldn't have the mentioned limitations. Instead, this approach follows 2. and modifies dp-packet.c so that dp_packet_put_uninit() and dp_packet_push_uninit() don't call neither dp_packet_prealloc_tailroom() nor dp_packet_prealloc_headroom() for DPBUF_DPDK packets. Instead, they check if there's enough space beforehand, and if not return NULL, instead of the normal pointer to the data. Otherwise the operations would continue as expected. This means we would need to deal with the NULL case only where we may be using DPBUF_DPDK packets (all the other cases remain unaffected as NULL won't be possible there). The cases I've identified it happens is where headers are being pushed (which seems to be what's causing the crashes in [2] as well): - In eth_push_vlan(), push_eth(), push_mpls() and push_nsh(), which get called from odp_execute_actions(); - In netdev_tnl_push_ip_header(), which gets called by the native tunnels implementations, such as netdev_gre_push_header() for GRE, that is ultimately called by push_tnl_action(). I haven't found a case where dp_packet_put_uninit() is actually used in a DPBUF_DPDK packet. In all cases it seems to be a result of a packet created locally by using dp_packet_new() or some variant. The system-userspace-testsuite tests have been run successfully with theses changes, but no other tests have been performed. [1] https://mail.openvswitch.org/pipermail/ovs-dev/2018-May/346649.html Tiago Lam (1): dp-packet: Don't resize DPBUF_DPDK packets. lib/dp-packet.c | 126 +++++++++++++++++++++++++++++++++++++++++------- lib/netdev-native-tnl.c | 24 +++++++-- lib/netdev-native-tnl.h | 6 +-- lib/netdev-provider.h | 2 +- lib/netdev.c | 16 ++++-- lib/odp-execute.c | 49 +++++++++++++++---- lib/packets.c | 38 +++++++++++++-- lib/packets.h | 8 +-- 8 files changed, 224 insertions(+), 45 deletions(-) -- 2.7.4 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
