From e9067998b9dde14a937fa2c96106b8b439659430 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. closes-bug: #1572397 Change-Id: I216d8b6c55025a6053b808fd2807338265b307e0 --- 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 0a1f2f17d..1944c4fae 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; @@ -1586,11 +1589,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 bf2048c8b..f739cc2a3 100644 --- a/dp-core/vr_mirror.c +++ b/dp-core/vr_mirror.c @@ -298,12 +298,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) { @@ -313,29 +313,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); @@ -346,25 +348,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_flow_get_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, mirror_type_t mtype) @@ -422,8 +430,7 @@ vr_mirror(struct vrouter *router, uint8_t mirror_id, struct vr_packet *pkt, if (mtype == MIRROR_TYPE_ACL) { 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) { mirror_md_len = mme->mirror_md_len; mirror_md = mme->mirror_md; @@ -543,12 +550,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; @@ -561,7 +562,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) { @@ -572,27 +572,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 014645727..c0c6a031a 100644 --- a/include/vr_flow.h +++ b/include/vr_flow.h @@ -321,6 +321,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)) @@ -348,6 +349,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 c6402184a..951a7c7f0 100644 --- a/include/vr_mirror.h +++ b/include/vr_mirror.h @@ -53,10 +53,12 @@ extern int vr_mirror(struct vrouter *, uint8_t, struct vr_packet *, struct vr_forwarding_md *, mirror_type_t); 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 a3926435b..a84b3b2e4 100644 --- a/include/vrouter.h +++ b/include/vrouter.h @@ -283,7 +283,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;