I forgot to note in this version that this applies to branch-1.4.
On Thu, Apr 25, 2013 at 11:23:00AM -0700, Ben Pfaff 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.k...@citrix.com>
> Signed-off-by: Ben Pfaff <b...@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@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
