When a flow consists solely of an output to OFPP_CONTROLLER, we avoid a
round trip to the kernel and back by calling execute_controller_action()
from handle_flow_miss(). However, execute_controller_action() frees the
packet passed in. This is dangerous, because the packet and the upcall
key are in the same block of malloc()'d memory, as the comment on struct
dpif_upcall says:
/* A packet passed up from the datapath to userspace.
*
* If 'key' or 'actions' is nonnull, then it points into data owned by
* 'packet', so their memory cannot be freed separately. (This is hardly a
* great way to do things but it works out OK for the dpif providers and
* clients that exist so far.)
*/
Thus, we get a use-after-free later on in handle_flow_miss() and eventually
a double free.
This fixes the problem by making execute_controller_action() clone the
packet in this case.
---
ofproto/ofproto-dpif.c | 16 +++++++++++-----
1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index e68bec3..1043a5d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -318,7 +318,7 @@ static bool execute_controller_action(struct ofproto_dpif *,
const struct flow *,
const struct nlattr *odp_actions,
size_t actions_len,
- struct ofpbuf *packet);
+ struct ofpbuf *packet, bool clone);
static void facet_flush_stats(struct ofproto_dpif *, struct facet *);
@@ -2533,7 +2533,7 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct
flow_miss *miss,
}
if (!execute_controller_action(ofproto, &facet->flow,
subfacet->actions,
- subfacet->actions_len, packet)) {
+ subfacet->actions_len, packet, true)) {
struct flow_miss_op *op = &ops[(*n_ops)++];
struct dpif_execute *execute = &op->dpif_op.execute;
@@ -3059,11 +3059,17 @@ facet_free(struct facet *facet)
free(facet);
}
+/* If the 'actions_len' bytes of actions in 'odp_actions' are just a single
+ * OVS_ACTION_ATTR_USERSPACE action, executes it internally and returns true.
+ * Otherwise, returns false without doing anything.
+ *
+ * If 'clone' is true, the caller always retains ownership of 'packet'.
+ * Otherwise, ownership is transferred to this function if it returns true. */
static bool
execute_controller_action(struct ofproto_dpif *ofproto,
const struct flow *flow,
const struct nlattr *odp_actions, size_t actions_len,
- struct ofpbuf *packet)
+ struct ofpbuf *packet, bool clone)
{
if (actions_len
&& odp_actions->nla_type == OVS_ACTION_ATTR_USERSPACE
@@ -3079,7 +3085,7 @@ execute_controller_action(struct ofproto_dpif *ofproto,
nla = nl_attr_find_nested(odp_actions, OVS_USERSPACE_ATTR_USERDATA);
send_packet_in_action(ofproto, packet, nl_attr_get_u64(nla), flow,
- false);
+ clone);
return true;
} else {
return false;
@@ -3100,7 +3106,7 @@ execute_odp_actions(struct ofproto_dpif *ofproto, const
struct flow *flow,
int error;
if (execute_controller_action(ofproto, flow, odp_actions, actions_len,
- packet)) {
+ packet, false)) {
return true;
}
--
1.7.2.5
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
