Looks good.
I'm planning to get rid of this function entirely in the fairly near future.
Ethan
On Thu, Dec 15, 2011 at 16:24, Ben Pfaff <b...@nicira.com> wrote:
> 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
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
