[ovs-dev] [PATCH v2] ofproto-dpif: Fix memory leak sending packets to controller.

Justin Pettit jpettit at nicira.com
Wed May 1 16:55:58 UTC 2013


The code's tricky, but I looked over it a couple of times, and it looks reasonable to me.

Thanks,

--Justin


On Apr 25, 2013, at 11:23 AM, Ben Pfaff <blp at nicira.com> wrote:

> In the case where execute_controller_action() returned true to
> handle_flow_miss(), indicating that the packet had been sent to the
> controller, nothing ever freed the packet, causing a memory leak.
> 
> One plausible solution seemed to be to turn over ownership of the packet to
> execute_controller_action(), by passing false instead of true as its
> 'clone' argument.  Another is to add an else case to the "if" statement
> that calls execute_controller_action() to free the packet.
> 
> However, neither of these solutions is actually correct, for a subtle
> reason.  The block of memory that includes the packet also includes the
> key for the flow miss.  At the end of handle_flow_miss(), past the end of
> the loop, the key is normally needed to install the flow in the datapath.
> Thus, by destroying the packet we potentially corrupt the key, and this
> has actually been seen while testing memory leak fixes.
> 
> This commit uses a different approach: instead of trying to destroy the
> packets one at a time when it becomes possible, it destroys all the packets
> together at the end.
> 
> Reported-by: Zoltan Kiss <zoltan.kiss at citrix.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> v1->v2: Fix typo in commit fix, fix double-free of 'todo' hmap.
> 
> ofproto/ofproto-dpif.c |   19 ++++++++++---------
> 1 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 2949085..1189919 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira Networks.
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
> @@ -2516,10 +2516,9 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
>                                miss->key_fitness, miss->key, miss->key_len,
>                                miss->initial_tci);
> 
> -    LIST_FOR_EACH_SAFE (packet, next_packet, list_node, &miss->packets) {
> +    LIST_FOR_EACH (packet, list_node, &miss->packets) {
>         struct dpif_flow_stats stats;
> 
> -        list_remove(&packet->list_node);
>         ofproto->n_matches++;
> 
>         if (facet->rule->up.cr.priority == FAIL_OPEN_PRIORITY) {
> @@ -2710,14 +2709,10 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, struct dpif_upcall *upcalls,
>     /* Process each element in the to-do list, constructing the set of
>      * operations to batch. */
>     n_ops = 0;
> -    HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
> +    HMAP_FOR_EACH (miss, hmap_node, &todo) {
>         handle_flow_miss(ofproto, miss, flow_miss_ops, &n_ops);
> -        ofpbuf_list_delete(&miss->packets);
> -        hmap_remove(&todo, &miss->hmap_node);
> -        free(miss);
>     }
>     assert(n_ops <= ARRAY_SIZE(flow_miss_ops));
> -    hmap_destroy(&todo);
> 
>     /* Execute batch. */
>     for (i = 0; i < n_ops; i++) {
> @@ -2737,7 +2732,6 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, struct dpif_upcall *upcalls,
>             if (op->subfacet->actions != execute->actions) {
>                 free((struct nlattr *) execute->actions);
>             }
> -            ofpbuf_delete((struct ofpbuf *) execute->packet);
>             break;
> 
>         case DPIF_OP_FLOW_PUT:
> @@ -2748,6 +2742,13 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, struct dpif_upcall *upcalls,
>             break;
>         }
>     }
> +
> +    HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
> +        ofpbuf_list_delete(&miss->packets);
> +        hmap_remove(&todo, &miss->hmap_node);
> +        free(miss);
> +    }
> +    hmap_destroy(&todo);
> }
> 
> static void
> -- 
> 1.7.2.5
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list