Commit a7a43b43 authored by Ben Pfaff's avatar Ben Pfaff

ofproto: Fix memory leak and memory exhaustion bugs in group_mod.

In handle_group_mod() cases where adding a group failed, nothing freed the
list of buckets, causing a leak.  The same was true in every case of
modifying a group.  This commit fixes the problem by changing add_group()
to never steal or free the buckets (modify_group() already acted this way)
and then making handle_group_mod() always free the buckets when it's done.

This approach might at first raise objections, because it makes add_group()
copy the buckets instead of just take the existing ones.  On branch-2.5
and master, there's a good reason for that--please see the original commit
for explanation.  On this backport to branch-2.4, though, we just use this
approach to avoid having to carefully write a new version for the backport.

Found by pain and suffering.
Signed-off-by: default avatarBen Pfaff <blp@ovn.org>
Acked-by: default avatarJarno Rajahalme <jarno@ovn.org>
parent 74f55f6d
/*
* Copyright (c) 2009-2015 Nicira, Inc.
* Copyright (c) 2009-2016 Nicira, Inc.
* Copyright (c) 2010 Jean Tourrilhes - HP-Labs.
*
* Licensed under the Apache License, Version 2.0 (the "License");
......@@ -291,7 +291,8 @@ static bool ofproto_group_exists__(const struct ofproto *ofproto,
static bool ofproto_group_exists(const struct ofproto *ofproto,
uint32_t group_id)
OVS_EXCLUDED(ofproto->groups_rwlock);
static enum ofperr add_group(struct ofproto *, struct ofputil_group_mod *);
static enum ofperr add_group(struct ofproto *,
const struct ofputil_group_mod *);
static void handle_openflow(struct ofconn *, const struct ofpbuf *);
static enum ofperr do_bundle_flow_mod_start(struct ofproto *,
struct ofputil_flow_mod *,
......@@ -6197,7 +6198,7 @@ handle_queue_get_config_request(struct ofconn *ofconn,
}
static enum ofperr
init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm,
init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm,
struct ofgroup **ofgroup)
{
enum ofperr error;
......@@ -6223,7 +6224,9 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm,
*CONST_CAST(long long int *, &((*ofgroup)->modified)) = now;
ovs_refcount_init(&(*ofgroup)->ref_count);
list_move(&(*ofgroup)->buckets, &gm->buckets);
list_init(&(*ofgroup)->buckets);
ofputil_bucket_clone_list(&(*ofgroup)->buckets, &gm->buckets, NULL);
*CONST_CAST(uint32_t *, &(*ofgroup)->n_buckets) =
list_size(&(*ofgroup)->buckets);
......@@ -6243,7 +6246,7 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm,
* 'ofproto''s group table. Returns 0 on success or an OpenFlow error code on
* failure. */
static enum ofperr
add_group(struct ofproto *ofproto, struct ofputil_group_mod *gm)
add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
{
struct ofgroup *ofgroup;
enum ofperr error;
......@@ -6391,7 +6394,7 @@ copy_buckets_for_remove_bucket(const struct ofgroup *ofgroup,
* ofproto's ofgroup hash map. Thus, the group is never altered while users of
* the xlate module hold a pointer to the group. */
static enum ofperr
modify_group(struct ofproto *ofproto, struct ofputil_group_mod *gm)
modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
{
struct ofgroup *ofgroup, *new_ofgroup, *retiring;
enum ofperr error;
......@@ -6535,28 +6538,37 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh)
switch (gm.command) {
case OFPGC11_ADD:
return add_group(ofproto, &gm);
error = add_group(ofproto, &gm);
break;
case OFPGC11_MODIFY:
return modify_group(ofproto, &gm);
error = modify_group(ofproto, &gm);
break;
case OFPGC11_DELETE:
delete_group(ofproto, gm.group_id);
return 0;
error = 0;
break;
case OFPGC15_INSERT_BUCKET:
return modify_group(ofproto, &gm);
error = modify_group(ofproto, &gm);
break;
case OFPGC15_REMOVE_BUCKET:
return modify_group(ofproto, &gm);
error = modify_group(ofproto, &gm);
break;
default:
if (gm.command > OFPGC11_DELETE) {
VLOG_WARN_RL(&rl, "%s: Invalid group_mod command type %d",
ofproto->name, gm.command);
}
return OFPERR_OFPGMFC_BAD_COMMAND;
error = OFPERR_OFPGMFC_BAD_COMMAND;
break;
}
ofputil_bucket_list_destroy(&gm.buckets);
return error;
}
enum ofputil_table_miss
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment