Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Jan 18, 2016, at 11:26 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> 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. But it actually
> fixes a worse problem too: when OF1.4+ REQUESTFORWARD is enabled, the
> group_mod is reused for the request forwarding. Until now, for a group_mod
> that adds a new group and that has some buckets, the previous stealing of
> buckets in add_group() meant that the group_mod's buckets were no longer
> valid; in practice, the list of buckets became linked in a way that
> iteration never terminated, which caused memory to be exhausted while
> composing the requestforward message. By making add_group() no longer
> modify the group_mod, we also fix this problem.
>
> The requestforward test in the testsuite did not find the latter problem
> because it only added a group without any buckets. This commit also
> updates the testsuite to include a bucket in its group_mod, which would
> have found the problem.
>
> Found by pain and suffering.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---
> ofproto/ofproto.c | 17 +++++++++++------
> tests/ofproto.at | 16 ++++++++--------
> 2 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 528d5ac..386009e 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -297,7 +297,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 ofproto_flow_mod_start(struct ofproto *,
> struct ofproto_flow_mod *)
> @@ -6280,7 +6281,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;
> @@ -6306,7 +6307,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);
>
> @@ -6326,7 +6329,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;
> @@ -6474,7 +6477,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;
> @@ -6643,7 +6646,7 @@ handle_group_mod(struct ofconn *ofconn, const struct
> ofp_header *oh)
> VLOG_INFO_RL(&rl, "%s: Invalid group_mod command type %d",
> ofproto->name, gm.command);
> }
> - return OFPERR_OFPGMFC_BAD_COMMAND;
> + error = OFPERR_OFPGMFC_BAD_COMMAND;
> }
>
> if (!error) {
> @@ -6653,6 +6656,8 @@ handle_group_mod(struct ofconn *ofconn, const struct
> ofp_header *oh)
> rf.group_mod = &gm;
> connmgr_send_requestforward(ofproto->connmgr, ofconn, &rf);
> }
> + ofputil_bucket_list_destroy(&gm.buckets);
> +
> return error;
> }
>
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index b3b8a0f..787def1 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -3144,25 +3144,25 @@ check_async () {
> shift
>
> # OFPGC_ADD
> - ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020000000000000001
> + ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020000000000000001
> 00100000 ffffffffffffffff 00000000"
> if test X"$1" = X"OFPGC_ADD"; then shift;
> echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4):
> - ADD group_id=1,type=all"
> + ADD group_id=1,type=all,bucket=actions=drop"
> echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod
> - ADD group_id=1,type=all"
> + ADD group_id=1,type=all,bucket=bucket_id:0,actions=drop"
> echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod
> - ADD group_id=1,type=all"
> + ADD group_id=1,type=all,bucket=actions=drop"
> fi
>
> # OFPGC_MODIFY
> - ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020001010000000001
> + ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020001010000000001
> 00100000 ffffffffffffffff 00000000"
> if test X"$1" = X"OFPGC_MODIFY"; then shift;
> echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4):
> - MOD group_id=1,type=select"
> + MOD group_id=1,type=select,bucket=weight:0,actions=drop"
> echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod
> - MOD group_id=1,type=select"
> + MOD group_id=1,type=select,bucket=bucket_id:0,weight:0,actions=drop"
> echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod
> - MOD group_id=1,type=select"
> + MOD group_id=1,type=select,bucket=weight:0,actions=drop"
> fi
>
> ovs-appctl -t `pwd`/c1 ofctl/barrier
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
