[ovs-dev] [PATCH] ovn-controller: Fix memory leak in recv_S_TLV_TABLE_REQUESTED().
Guru Shetty
guru at ovn.org
Fri Sep 9 16:38:27 UTC 2016
On 1 September 2016 at 10:02, Ben Pfaff <blp at ovn.org> wrote:
> Nothing freed 'reply'. This fixes the problem.
>
> Most of this patch is moving coding around. The essential change is that
> breaking the code that works with 'reply' out into a separate function
> makes it possible to catch all paths out of the function so that 'reply'
> can be freed in one place.
>
> Reported-by: Ryan Moats <rmoats at us.ibm.com>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
> ovn/controller/ofctrl.c | 124 ++++++++++++++++++++++++++----
> ------------------
> 1 file changed, 66 insertions(+), 58 deletions(-)
>
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index d8e111d..584e260 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -199,83 +199,91 @@ run_S_TLV_TABLE_REQUESTED(void)
> {
> }
>
> +static bool
> +process_tlv_table_reply(const struct ofputil_tlv_table_reply *reply)
> +{
> + const struct ofputil_tlv_map *map;
> + uint64_t md_free = UINT64_MAX;
> + BUILD_ASSERT(TUN_METADATA_NUM_OPTS == 64);
> +
> + LIST_FOR_EACH (map, list_node, &reply->mappings) {
> + if (map->option_class == OVN_GENEVE_CLASS
> + && map->option_type == OVN_GENEVE_TYPE
> + && map->option_len == OVN_GENEVE_LEN) {
> + if (map->index >= TUN_METADATA_NUM_OPTS) {
> + VLOG_ERR("desired Geneve tunnel option 0x%"PRIx16","
> + "%"PRIu8",%"PRIu8" already in use with "
> + "unsupported index %"PRIu16,
> + map->option_class, map->option_type,
> + map->option_len, map->index);
> + return false;
> + } else {
> + mff_ovn_geneve = MFF_TUN_METADATA0 + map->index;
> + state = S_CLEAR_FLOWS;
> + return false;
>
I am still trying to peel this, but what I see is that in my setup, the
above block gets executed and then we return 'false' even after setting
mff_ovn_geneve. Later in recv_S_TLV_TABLE_REQUESTED, we reset
mff_ovn_geneve to zero. This looks wrong to me.
> + }
> + }
> +
> + if (map->index < TUN_METADATA_NUM_OPTS) {
> + md_free &= ~(UINT64_C(1) << map->index);
> + }
> + }
> +
> + VLOG_DBG("OVN Geneve option not found");
> + if (!md_free) {
> + VLOG_ERR("no Geneve options free for use by OVN");
> + return false;
> + }
> +
> + unsigned int index = rightmost_1bit_idx(md_free);
> + mff_ovn_geneve = MFF_TUN_METADATA0 + index;
> + struct ofputil_tlv_map tm;
> + tm.option_class = OVN_GENEVE_CLASS;
> + tm.option_type = OVN_GENEVE_TYPE;
> + tm.option_len = OVN_GENEVE_LEN;
> + tm.index = index;
> +
> + struct ofputil_tlv_table_mod ttm;
> + ttm.command = NXTTMC_ADD;
> + ovs_list_init(&ttm.mappings);
> + ovs_list_push_back(&ttm.mappings, &tm.list_node);
> +
> + xid = queue_msg(ofputil_encode_tlv_table_mod(OFP13_VERSION, &ttm));
> + xid2 = queue_msg(ofputil_encode_barrier_request(OFP13_VERSION));
> + state = S_TLV_TABLE_MOD_SENT;
> +
> + return true;
> +}
> +
> static void
> recv_S_TLV_TABLE_REQUESTED(const struct ofp_header *oh, enum ofptype
> type)
> {
> if (oh->xid != xid) {
> ofctrl_recv(oh, type);
> + return;
> } else if (type == OFPTYPE_NXT_TLV_TABLE_REPLY) {
> struct ofputil_tlv_table_reply reply;
> enum ofperr error = ofputil_decode_tlv_table_reply(oh, &reply);
> - if (error) {
> + if (!error) {
> + bool ok = process_tlv_table_reply(&reply);
> + ofputil_uninit_tlv_table(&reply.mappings);
> + if (ok) {
> + return;
> + }
> + } else {
> VLOG_ERR("failed to decode TLV table request (%s)",
> ofperr_to_string(error));
> - goto error;
> - }
> -
> - const struct ofputil_tlv_map *map;
> - uint64_t md_free = UINT64_MAX;
> - BUILD_ASSERT(TUN_METADATA_NUM_OPTS == 64);
> -
> - LIST_FOR_EACH (map, list_node, &reply.mappings) {
> - if (map->option_class == OVN_GENEVE_CLASS
> - && map->option_type == OVN_GENEVE_TYPE
> - && map->option_len == OVN_GENEVE_LEN) {
> - if (map->index >= TUN_METADATA_NUM_OPTS) {
> - VLOG_ERR("desired Geneve tunnel option 0x%"PRIx16","
> - "%"PRIu8",%"PRIu8" already in use with "
> - "unsupported index %"PRIu16,
> - map->option_class, map->option_type,
> - map->option_len, map->index);
> - goto error;
> - } else {
> - mff_ovn_geneve = MFF_TUN_METADATA0 + map->index;
> - state = S_CLEAR_FLOWS;
> - return;
> - }
> - }
> -
> - if (map->index < TUN_METADATA_NUM_OPTS) {
> - md_free &= ~(UINT64_C(1) << map->index);
> - }
> - }
> -
> - VLOG_DBG("OVN Geneve option not found");
> - if (!md_free) {
> - VLOG_ERR("no Geneve options free for use by OVN");
> - goto error;
> }
> -
> - unsigned int index = rightmost_1bit_idx(md_free);
> - mff_ovn_geneve = MFF_TUN_METADATA0 + index;
> - struct ofputil_tlv_map tm;
> - tm.option_class = OVN_GENEVE_CLASS;
> - tm.option_type = OVN_GENEVE_TYPE;
> - tm.option_len = OVN_GENEVE_LEN;
> - tm.index = index;
> -
> - struct ofputil_tlv_table_mod ttm;
> - ttm.command = NXTTMC_ADD;
> - ovs_list_init(&ttm.mappings);
> - ovs_list_push_back(&ttm.mappings, &tm.list_node);
> -
> - xid = queue_msg(ofputil_encode_tlv_table_mod(OFP13_VERSION,
> &ttm));
> - xid2 = queue_msg(ofputil_encode_barrier_request(OFP13_VERSION));
> - state = S_TLV_TABLE_MOD_SENT;
> } else if (type == OFPTYPE_ERROR) {
> VLOG_ERR("switch refused to allocate Geneve option (%s)",
> ofperr_to_string(ofperr_decode_msg(oh, NULL)));
> - goto error;
> } else {
> char *s = ofp_to_string(oh, ntohs(oh->length), 1);
> - VLOG_ERR("unexpected reply to TLV table request (%s)",
> - s);
> + VLOG_ERR("unexpected reply to TLV table request (%s)", s);
> free(s);
> - goto error;
> }
> - return;
>
> -error:
> + /* Error path. */
> mff_ovn_geneve = 0;
> state = S_CLEAR_FLOWS;
> }
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list