From e937aa9b4cace0eac361439403af5fa01355bfe2 Mon Sep 17 00:00:00 2001 From: "Anand H. Krishnan" Date: Mon, 20 Apr 2015 12:41:11 +0530 Subject: [PATCH] Free the defer data in case of errors To make sure that we flush all the packets that are queued in a flow entry, we run a defer function. If for any reason this defer was not scheduled (because the function was called with no hold queue), the defer data has to be freed. Closes-BUG: #1436798 (cherry picked from commit 8c30ce9a3e254d45dc4de595cc066f2da21c18d6) Fix improper boundary checks and reference count leaks Boundary checks allow for one extra label than the maximum, causing memory corruption. Also, when a label is changed, reference to old nexthop has to be released. Two harmless boundary checks in nexthop subysystem is also addressed. Closes-BUG: #1446550 Change-Id: I9289265b8a843160fdfe6fffc3e52c131d9b2a4a --- dp-core/vr_flow.c | 30 ++++++++++++++++++++------ dp-core/vr_mpls.c | 50 ++++++++++++++++++++++++++++++-------------- dp-core/vr_nexthop.c | 4 ++-- 3 files changed, 60 insertions(+), 24 deletions(-) diff --git a/dp-core/vr_flow.c b/dp-core/vr_flow.c index 58c4acdd3..9a35db5f7 100644 --- a/dp-core/vr_flow.c +++ b/dp-core/vr_flow.c @@ -106,7 +106,7 @@ vr_valid_link_local_port(struct vrouter *router, int family, if (proto == VR_IP_PROTO_UDP) tmp += (router->vr_link_local_ports_size * 8 / 2); - data = router->vr_link_local_ports[(tmp /8)]; + data = router->vr_link_local_ports[(tmp / 8)]; if (data & (1 << (tmp % 8))) return true; @@ -117,7 +117,6 @@ static void vr_clear_link_local_port(struct vrouter *router, int family, int proto, int port) { - unsigned char *data; unsigned int tmp; @@ -135,15 +134,16 @@ vr_clear_link_local_port(struct vrouter *router, int family, if (proto == VR_IP_PROTO_UDP) tmp += (router->vr_link_local_ports_size * 8 / 2); - data = &router->vr_link_local_ports[(tmp /8)]; + data = &router->vr_link_local_ports[(tmp / 8)]; *data &= (~(1 << (tmp % 8))); + + return; } static void vr_set_link_local_port(struct vrouter *router, int family, int proto, int port) { - unsigned char *data; unsigned int tmp; @@ -161,8 +161,10 @@ vr_set_link_local_port(struct vrouter *router, int family, if (proto == VR_IP_PROTO_UDP) tmp += (router->vr_link_local_ports_size * 8 / 2); - data = &router->vr_link_local_ports[tmp /8]; + data = &router->vr_link_local_ports[tmp / 8]; *data |= (1 << (tmp % 8)); + + return; } static void @@ -200,7 +202,13 @@ vr_reset_flow_entry(struct vrouter *router, struct vr_flow_entry *fe, unsigned int index) { memset(&fe->fe_stats, 0, sizeof(fe->fe_stats)); - memset(&fe->fe_hold_list, 0, sizeof(fe->fe_hold_list));; + + if (fe->fe_hold_list) { + vr_printf("vrouter: Potential memory leak @ %s:%d\n", + __FILE__, __LINE__); + } + fe->fe_hold_list = NULL; + memset(&fe->fe_key, 0, sizeof(fe->fe_key)); vr_flow_reset_mirror(router, fe, index); @@ -358,6 +366,7 @@ vr_flow_queue_free_defer(struct vr_flow_md *flmd, struct vr_flow_queue *vfq) vdd->vdd_data = (void *)vfq; vr_defer(flmd->flmd_router, vr_flow_queue_free, (void *)vdd); + flmd->flmd_defer_data = NULL; return; } @@ -1330,6 +1339,11 @@ vr_flow_flush(void *arg) } exit_flush: + if (flmd->flmd_defer_data) { + vr_put_defer_data(flmd->flmd_defer_data); + flmd->flmd_defer_data = NULL; + } + vr_free(flmd); return; @@ -1896,6 +1910,8 @@ vr_link_local_ports_reset(struct vrouter *router) memset(router->vr_link_local_ports, 0, router->vr_link_local_ports_size); } + + return; } @@ -1908,6 +1924,8 @@ vr_link_local_ports_exit(struct vrouter *router) router->vr_link_local_ports = NULL; router->vr_link_local_ports_size = 0; } + + return; } int diff --git a/dp-core/vr_mpls.c b/dp-core/vr_mpls.c index 182627a99..23804a168 100644 --- a/dp-core/vr_mpls.c +++ b/dp-core/vr_mpls.c @@ -15,7 +15,7 @@ struct vr_nexthop * __vrouter_get_label(struct vrouter *router, unsigned int label) { - if (!router || label > router->vr_max_labels) + if (!router || label >= router->vr_max_labels) return NULL; return router->vr_ilm[label]; @@ -29,6 +29,17 @@ vrouter_get_label(unsigned int rid, unsigned int label) return __vrouter_get_label(router, label); } +static void +__vr_mpls_del(struct vrouter *router, unsigned int label) +{ + if (router->vr_ilm[label]) + vrouter_put_nexthop(router->vr_ilm[label]); + + router->vr_ilm[label] = NULL; + + return; +} + int vr_mpls_del(vr_mpls_req *req) { @@ -41,16 +52,12 @@ vr_mpls_del(vr_mpls_req *req) goto generate_resp; } - if (req->mr_label > (int)router->vr_max_labels) { + if ((unsigned int)req->mr_label >= router->vr_max_labels) { ret = -EINVAL; goto generate_resp; } - if (router->vr_ilm[req->mr_label]) - vrouter_put_nexthop(router->vr_ilm[req->mr_label]); - - router->vr_ilm[req->mr_label] = NULL; - + __vr_mpls_del(router, (unsigned int)req->mr_label); generate_resp: vr_send_response(ret); @@ -70,11 +77,13 @@ vr_mpls_add(vr_mpls_req *req) goto generate_resp; } - if ((unsigned int)req->mr_label > router->vr_max_labels) { + if ((unsigned int)req->mr_label >= router->vr_max_labels) { ret = -EINVAL; goto generate_resp; } + __vr_mpls_del(router, (unsigned int)req->mr_label); + nh = vrouter_get_nexthop(req->mr_rid, req->mr_nhid); if (!nh) { ret = -EINVAL; @@ -145,17 +154,26 @@ vr_mpls_get(vr_mpls_req *req) struct vrouter *router; router = vrouter_get(req->mr_rid); - if (!router || req->mr_label > (int)router->vr_max_labels) { + if (!router) { ret = -ENODEV; - } else { - nh = vrouter_get_label(req->mr_rid, req->mr_label); - if (!nh) - ret = -ENOENT; + goto generate_response; } - if (!ret) - vr_mpls_make_req(req, nh, req->mr_label); - else + if ((unsigned int)req->mr_label >= router->vr_max_labels) { + ret = -EINVAL; + goto generate_response; + } + + nh = vrouter_get_label(req->mr_rid, req->mr_label); + if (!nh) { + ret = -ENOENT; + goto generate_response; + } + + vr_mpls_make_req(req, nh, req->mr_label); + +generate_response: + if (ret) req = NULL; vr_message_response(VR_MPLS_OBJECT_ID, req, ret); diff --git a/dp-core/vr_nexthop.c b/dp-core/vr_nexthop.c index 833cef680..4d96b93a6 100644 --- a/dp-core/vr_nexthop.c +++ b/dp-core/vr_nexthop.c @@ -88,7 +88,7 @@ vrouter_add_nexthop(struct vr_nexthop *nh) { struct vrouter *router = vrouter_get(nh->nh_rid); - if (!router || nh->nh_id > router->vr_max_nexthops) + if (!router || nh->nh_id >= router->vr_max_nexthops) return -EINVAL; /* @@ -109,7 +109,7 @@ nh_del(struct vr_nexthop *nh) { struct vrouter *router = vrouter_get(nh->nh_rid); - if (!router || nh->nh_id > router->vr_max_nexthops) + if (!router || nh->nh_id >= router->vr_max_nexthops) return; if (router->vr_nexthops[nh->nh_id]) {