From 063ab3dfea324e526894bdf41908ea0a13b37ab4 Mon Sep 17 00:00:00 2001 From: Divakar Date: Fri, 27 May 2016 15:58:31 +0530 Subject: [PATCH] Keeping the mirror metadata entry in Flow entry Currently mirror metadata is stored in index table using flow index as key. Before the Flow eviction, the addition and deletion of this meta data to Index table is always from Agent and hence no parallel manipulation of Index table. After eviction, the metadata deletion from index table can happen in parallel and hence leading to memory corruption/leak in index table manipulation. The right fix is making the index table SMP ready. As a stop gap fix, the mirror metada data pointer is stored in the flow entry itself directly. Change-Id: I4c98af36646c3389996575af8eac3e98b3ffdca2 closes-bug: #1572397 --- dp-core/vr_flow.c | 15 ++++++++-- dp-core/vr_mirror.c | 72 ++++++++++++++++----------------------------- include/vr_flow.h | 2 ++ include/vr_mirror.h | 12 ++++---- include/vrouter.h | 1 - 5 files changed, 47 insertions(+), 55 deletions(-) diff --git a/dp-core/vr_flow.c b/dp-core/vr_flow.c index dce99c430..35670e9f5 100644 --- a/dp-core/vr_flow.c +++ b/dp-core/vr_flow.c @@ -180,7 +180,10 @@ vr_flow_reset_mirror(struct vrouter *router, struct vr_flow_entry *fe, fe->fe_mirror_id = VR_MAX_MIRROR_INDICES; vrouter_put_mirror(router, fe->fe_sec_mirror_id); fe->fe_sec_mirror_id = VR_MAX_MIRROR_INDICES; - vr_mirror_meta_entry_del(router, index); + if (fe->fe_mme) { + vr_mirror_meta_entry_del(router, fe->fe_mme); + fe->fe_mme = NULL; + } } fe->fe_flags &= ~VR_FLOW_FLAG_MIRROR; fe->fe_mirror_id = VR_MAX_MIRROR_INDICES; @@ -1512,11 +1515,17 @@ vr_flow_set_mirror(struct vrouter *router, vr_flow_req *req, } } - if (req->fr_pcap_meta_data_size && req->fr_pcap_meta_data) - vr_mirror_meta_entry_set(router, req->fr_index, + if (req->fr_pcap_meta_data_size && req->fr_pcap_meta_data) { + if (fe->fe_mme) { + vr_mirror_meta_entry_del(router, fe->fe_mme); + fe->fe_mme = NULL; + } + + fe->fe_mme = vr_mirror_meta_entry_set(router, req->fr_index, req->fr_mir_sip, req->fr_mir_sport, req->fr_pcap_meta_data, req->fr_pcap_meta_data_size, req->fr_mir_vrf); + } return; } diff --git a/dp-core/vr_mirror.c b/dp-core/vr_mirror.c index b99b649db..10d4de8be 100644 --- a/dp-core/vr_mirror.c +++ b/dp-core/vr_mirror.c @@ -297,12 +297,12 @@ vr_mirror_meta_destructor(struct vrouter *router, void *arg) } static void -vr_mirror_meta_entry_destroy(unsigned int index, void *arg) +vr_mirror_meta_entry_destroy(struct vrouter *router, + struct vr_mirror_meta_entry *me) { - struct vr_mirror_meta_entry *me = (struct vr_mirror_meta_entry *)arg; struct vr_defer_data *defer; - if (me && me != VR_ITABLE_ERR_PTR) { + if (me) { if (!vr_not_ready) { defer = vr_get_defer_data(sizeof(*defer)); if (!defer) { @@ -312,29 +312,31 @@ vr_mirror_meta_entry_destroy(unsigned int index, void *arg) } defer->vdd_data = (void *)me; vr_defer(me->mirror_router, vr_mirror_meta_destructor, (void *)defer); + } else { + vr_mirror_meta_destroy(me); } } return; } -int +struct vr_mirror_meta_entry * vr_mirror_meta_entry_set(struct vrouter *router, unsigned int index, unsigned int mir_sip, unsigned short mir_sport, void *meta_data, unsigned int meta_data_len, unsigned short mirror_vrf) { char *buf; - struct vr_mirror_meta_entry *me, *me_old; + struct vr_mirror_meta_entry *me; me = vr_malloc(sizeof(*me), VR_MIRROR_META_OBJECT); if (!me) - return -ENOMEM; + return NULL; buf = vr_malloc(meta_data_len, VR_MIRROR_META_OBJECT); if (!buf) { vr_free(me, VR_MIRROR_META_OBJECT); - return -ENOMEM; + return NULL; } memcpy(buf, meta_data, meta_data_len); @@ -345,25 +347,31 @@ vr_mirror_meta_entry_set(struct vrouter *router, unsigned int index, me->mirror_sport = mir_sport; me->mirror_vrf = mirror_vrf; - me_old = vr_itable_set(router->vr_mirror_md, index, me); - if (me_old && me_old != VR_ITABLE_ERR_PTR) - vr_mirror_meta_entry_destroy(index, (void *)me_old); - - return 0; + return me; } void -vr_mirror_meta_entry_del(struct vrouter *router, unsigned int index) +vr_mirror_meta_entry_del(struct vrouter *router, + struct vr_mirror_meta_entry *me) { - struct vr_mirror_meta_entry *me; - - me = vr_itable_del(router->vr_mirror_md, index); if (me) - vr_mirror_meta_entry_destroy(index, (void *)me); + vr_mirror_meta_entry_destroy(router, (void *)me); return; } +static struct vr_mirror_meta_entry * +vr_mirror_meta_entry_get(struct vrouter *router, unsigned int flow_index) +{ + struct vr_flow_entry *fe; + + fe = vr_get_flow_entry(router, flow_index); + if (fe) + return fe->fe_mme; + + return NULL; +} + int vr_mirror(struct vrouter *router, uint8_t mirror_id, struct vr_packet *pkt, struct vr_forwarding_md *fmd) @@ -397,8 +405,7 @@ vr_mirror(struct vrouter *router, uint8_t mirror_id, } if (fmd->fmd_flow_index >= 0) { - mme = (struct vr_mirror_meta_entry *)vr_itable_get(router->vr_mirror_md, - fmd->fmd_flow_index); + mme = vr_mirror_meta_entry_get(router, fmd->fmd_flow_index); if (!mme) return 0; mirror_md_len = mme->mirror_md_len; @@ -502,12 +509,6 @@ vr_mirror_exit(struct vrouter *router, bool soft_reset) if (router->vr_mirrors[i]) vrouter_put_mirror(router, i); - if (router->vr_mirror_md) { - vr_itable_delete(router->vr_mirror_md, - vr_mirror_meta_entry_destroy); - router->vr_mirror_md = NULL; - } - if (!soft_reset) { vr_free(router->vr_mirrors, VR_MIRROR_TABLE_OBJECT); router->vr_mirrors = NULL; @@ -520,7 +521,6 @@ vr_mirror_exit(struct vrouter *router, bool soft_reset) int vr_mirror_init(struct vrouter *router) { - int ret = 0; unsigned int size; if (!router->vr_mirrors) { @@ -531,27 +531,7 @@ vr_mirror_init(struct vrouter *router) return vr_module_error(-ENOMEM, __FUNCTION__, __LINE__, size); } - if (!router->vr_mirror_md) { - router->vr_mirror_md = vr_itable_create(32, 4, 8, 8, 8, 8); - if (!router->vr_mirror_md && (ret = -ENOMEM)) { - vr_module_error(ret, __FUNCTION__, __LINE__, 0); - goto cleanup; - } - } - return 0; -cleanup: - if (router->vr_mirrors) { - vr_free(router->vr_mirrors, VR_MIRROR_TABLE_OBJECT); - router->vr_mirrors = NULL; - } - - if (router->vr_mirror_md) { - vr_itable_delete(router->vr_mirror_md, vr_mirror_meta_entry_destroy); - router->vr_mirror_md = NULL; - } - - return ret; } diff --git a/include/vr_flow.h b/include/vr_flow.h index d52089846..d860fffff 100644 --- a/include/vr_flow.h +++ b/include/vr_flow.h @@ -314,6 +314,7 @@ struct vr_dummy_flow_entry { uint8_t fe_drop_reason; uint8_t fe_type; unsigned short fe_udp_src_port; + struct vr_mirror_meta_entry *fe_mme;; } __attribute__((packed)); #define VR_FLOW_ENTRY_PACK (128 - sizeof(struct vr_dummy_flow_entry)) @@ -340,6 +341,7 @@ struct vr_flow_entry { uint8_t fe_drop_reason; uint8_t fe_type; unsigned short fe_udp_src_port; + struct vr_mirror_meta_entry *fe_mme; unsigned char fe_pack[VR_FLOW_ENTRY_PACK]; } __attribute__((packed)); diff --git a/include/vr_mirror.h b/include/vr_mirror.h index c011bcc9d..a00366ea1 100644 --- a/include/vr_mirror.h +++ b/include/vr_mirror.h @@ -37,10 +37,12 @@ extern int vr_mirror(struct vrouter *, uint8_t, struct vr_packet *, struct vr_forwarding_md *); extern struct vr_mirror_entry *vrouter_get_mirror(unsigned int, unsigned int); extern int vrouter_put_mirror(struct vrouter *, unsigned int); -extern int vr_mirror_meta_entry_set(struct vrouter *, unsigned int, - unsigned int, unsigned short, - void *, unsigned int, - unsigned short); -extern void vr_mirror_meta_entry_del(struct vrouter *, unsigned int); +extern struct vr_mirror_meta_entry * + vr_mirror_meta_entry_set(struct vrouter *, unsigned int, + unsigned int, unsigned short, + void *, unsigned int, + unsigned short); +extern void vr_mirror_meta_entry_del(struct vrouter *, + struct vr_mirror_meta_entry *); #endif /* __VR_MIRROR_H__ */ diff --git a/include/vrouter.h b/include/vrouter.h index 9ab619758..70bb75e5a 100644 --- a/include/vrouter.h +++ b/include/vrouter.h @@ -280,7 +280,6 @@ struct vrouter { unsigned int vr_max_vrfs; unsigned int vr_max_mirror_indices; struct vr_mirror_entry **vr_mirrors; - vr_itable_t vr_mirror_md; vr_itable_t vr_vxlan_table; struct vr_btable *vr_fragment_table;