Unconditional return may cause packet leak in case of 'may_steal == true'. Additionally, removed redundant checking for depth level and clarified ignoring of the 'false' value of 'may_steal'.
CC: Sugesh Chandran <[email protected]> Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by combining recirc actions at xlate.") Signed-off-by: Ilya Maximets <[email protected]> --- lib/dpif-netdev.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index f86ed2a..294cd87 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -5672,12 +5672,23 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_, break; case OVS_ACTION_ATTR_TUNNEL_PUSH: - if (*depth < MAX_RECIRC_DEPTH) { - dp_packet_batch_apply_cutlen(packets_); - push_tnl_action(pmd, a, packets_); - return; + /* + * XXX: 'may_steal' concept is broken here, because we're + * unconditionally changing the packets just like for other PUSH_* + * actions in 'odp_execute()'. 'false' value could be ignored, + * because we could reach here only after clone, but we still need + * to free the packets in case 'may_steal == true'. + */ + if (may_steal) { + /* We're requested to push tunnel header, but also we need to take + * the ownership of these packets. Thus, we can avoid performing + * the action, because the caller will not use the result anyway. + * Just break to free the batch. */ + break; } - break; + dp_packet_batch_apply_cutlen(packets_); + push_tnl_action(pmd, a, packets_); + return; case OVS_ACTION_ATTR_TUNNEL_POP: if (*depth < MAX_RECIRC_DEPTH) { -- 2.7.4 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
