[ofa-general] [PATCH 1/2] opensm: avoid LASH use-after-free when switch is deleted from fabric.
Jim Schutt
jaschut at sandia.gov
Fri Aug 28 12:08:15 PDT 2009
When LASH is run against ibsim, valgrind reports the following
(on x86_64) after a switch is removed from the fabric:
==15699== Invalid write of size 8
==15699== at 0x45FD8A: switch_delete (osm_ucast_lash.c:648)
==15699== by 0x461483: lash_cleanup (osm_ucast_lash.c:1123)
==15699== by 0x461848: lash_process (osm_ucast_lash.c:1230)
==15699== by 0x45C043: ucast_mgr_route (osm_ucast_mgr.c:1016)
==15699== by 0x45C1A0: osm_ucast_mgr_process (osm_ucast_mgr.c:1057)
==15699== by 0x44F11B: do_sweep (osm_state_mgr.c:1283)
==15699== by 0x44F539: osm_state_mgr_process (osm_state_mgr.c:1398)
==15699== by 0x447296: sm_process (osm_sm.c:90)
==15699== by 0x4473FE: sm_sweeper (osm_sm.c:130)
==15699== by 0x5023505: __cl_thread_wrapper (cl_thread.c:57)
==15699== by 0x37AC006366: start_thread (in /lib64/libpthread-2.5.so)
==15699== by 0x37AB4D30AC: clone (in /lib64/libc-2.5.so)
==15699== Address 0x9B28198 is 152 bytes inside a block of size 160 free'd
==15699== at 0x4A0541E: free (vg_replace_malloc.c:233)
==15699== by 0x453866: osm_switch_delete (osm_switch.c:97)
==15699== by 0x4116AA: drop_mgr_remove_switch (osm_drop_mgr.c:290)
==15699== by 0x411820: drop_mgr_process_node (osm_drop_mgr.c:339)
==15699== by 0x411D0C: osm_drop_mgr_process (osm_drop_mgr.c:465)
==15699== by 0x44EF97: do_sweep (osm_state_mgr.c:1231)
==15699== by 0x44F539: osm_state_mgr_process (osm_state_mgr.c:1398)
==15699== by 0x447296: sm_process (osm_sm.c:90)
==15699== by 0x4473FE: sm_sweeper (osm_sm.c:130)
==15699== by 0x5023505: __cl_thread_wrapper (cl_thread.c:57)
==15699== by 0x37AC006366: start_thread (in /lib64/libpthread-2.5.so)
==15699== by 0x37AB4D30AC: clone (in /lib64/libc-2.5.so)
The root cause is that in order to perform SL lookup for path record
queries, LASH needs to keep persistent data between calls to the
routing engine.
LASH uses the osm_switch_t:priv member to speed lookup of the LASH
switch_t objects it needs to perform SL lookup, and has a corresponding
switch_t:p_sw member to point to the corresponding osm_switch_t object.
When a switch is deleted from the fabric, the switch_t:p_sw value becomes
invalid, but LASH's switch_delete() uses it to clear the corresponding
osm_switch_t:priv value.
Solve this problem by adding a priv_release function pointer that
is set when osm_switch_t:priv is set. This allows the opensm core to
clean up after any routing engine that is using priv to access
persistent data (LASH seems to be the only one so far), without
knowing the details of how to do so.
When multiple routing engines are configured, it also allows a routing
engine using osm_switch_t:priv to clean up if some other routing engine
using priv fails in an unexpected way.
With this addition, the rules for using osm_switch_t:priv become:
1) Never assign to priv without also assigning to priv_release.
2) Always use priv_release() before assigning to priv; this
prevents memory issues due to unexpected errors in a
routing engine using priv.
3) Always use priv_release() to clean up after a use of priv.
Since updn uses osm_switch_t:priv, fix it up to follow the above
rules as well, for consistency.
Signed-off-by: Jim Schutt <jaschut at sandia.gov>
---
opensm/include/opensm/osm_switch.h | 1 +
opensm/opensm/osm_switch.c | 2 ++
opensm/opensm/osm_ucast_lash.c | 24 ++++++++++++++++++++----
opensm/opensm/osm_ucast_updn.c | 15 +++++++++++----
4 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/opensm/include/opensm/osm_switch.h b/opensm/include/opensm/osm_switch.h
index 7ce28c5..d48f8c6 100644
--- a/opensm/include/opensm/osm_switch.h
+++ b/opensm/include/opensm/osm_switch.h
@@ -106,6 +106,7 @@ typedef struct osm_switch {
unsigned endport_links;
unsigned need_update;
void *priv;
+ void (*priv_release)(struct osm_switch *p_sw);
} osm_switch_t;
/*
* FIELDS
diff --git a/opensm/opensm/osm_switch.c b/opensm/opensm/osm_switch.c
index ce1ca63..fbf3973 100644
--- a/opensm/opensm/osm_switch.c
+++ b/opensm/opensm/osm_switch.c
@@ -94,6 +94,8 @@ void osm_switch_delete(IN OUT osm_switch_t ** const pp_sw)
free(p_sw->hops[i]);
free(p_sw->hops);
}
+ if (p_sw->priv_release)
+ p_sw->priv_release(p_sw);
free(*pp_sw);
*pp_sw = NULL;
}
diff --git a/opensm/opensm/osm_ucast_lash.c b/opensm/opensm/osm_ucast_lash.c
index 0a567b3..ceae7d8 100644
--- a/opensm/opensm/osm_ucast_lash.c
+++ b/opensm/opensm/osm_ucast_lash.c
@@ -603,6 +603,17 @@ static int balance_virtual_lanes(lash_t * p_lash, unsigned lanes_needed)
return 0;
}
+static void lash_switch_priv_release(osm_switch_t *osm_sw)
+{
+ switch_t *sw = osm_sw->priv;
+
+ osm_sw->priv_release = NULL;
+ osm_sw->priv = NULL;
+
+ if (sw && sw->p_sw == osm_sw)
+ sw->p_sw = NULL;
+}
+
static switch_t *switch_create(lash_t * p_lash, unsigned id, osm_switch_t * p_sw)
{
unsigned num_switches = p_lash->num_switches;
@@ -628,8 +639,12 @@ static switch_t *switch_create(lash_t * p_lash, unsigned id, osm_switch_t * p_sw
}
sw->p_sw = p_sw;
- if (p_sw)
+ if (p_sw) {
+ if (p_sw->priv_release)
+ p_sw->priv_release(p_sw);
p_sw->priv = sw;
+ p_sw->priv_release = lash_switch_priv_release;
+ }
if (osm_mesh_node_create(p_lash, sw)) {
free(sw->dij_channels);
@@ -644,8 +659,8 @@ static void switch_delete(lash_t *p_lash, switch_t * sw)
{
if (sw->dij_channels)
free(sw->dij_channels);
- if (sw->p_sw)
- sw->p_sw->priv = NULL;
+ if (sw->p_sw && sw->p_sw->priv_release)
+ sw->p_sw->priv_release(sw->p_sw);
free(sw);
}
@@ -1113,7 +1128,8 @@ static void lash_cleanup(lash_t * p_lash)
while (p_next_sw != (osm_switch_t *) cl_qmap_end(&p_subn->sw_guid_tbl)) {
p_sw = p_next_sw;
p_next_sw = (osm_switch_t *) cl_qmap_next(&p_sw->map_item);
- p_sw->priv = NULL;
+ if (p_sw->priv_release)
+ p_sw->priv_release(p_sw);
}
if (p_lash->switches) {
diff --git a/opensm/opensm/osm_ucast_updn.c b/opensm/opensm/osm_ucast_updn.c
index bb9ccda..dc5f459 100644
--- a/opensm/opensm/osm_ucast_updn.c
+++ b/opensm/opensm/osm_ucast_updn.c
@@ -404,10 +404,13 @@ static struct updn_node *create_updn_node(osm_switch_t * sw)
return u;
}
-static void delete_updn_node(struct updn_node *u)
+static void updn_sw_priv_release(osm_switch_t *sw)
{
- u->sw->priv = NULL;
- free(u);
+ if (sw->priv)
+ free(sw->priv);
+
+ sw->priv_release = NULL;
+ sw->priv = NULL;
}
/**********************************************************************
@@ -589,6 +592,8 @@ static int updn_lid_matrices(void *ctx)
item != cl_qmap_end(&p_updn->p_osm->subn.sw_guid_tbl);
item = cl_qmap_next(item)) {
p_sw = (osm_switch_t *)item;
+ if (p_sw->priv_release)
+ p_sw->priv_release(p_sw);
p_sw->priv = create_updn_node(p_sw);
if (!p_sw->priv) {
OSM_LOG(&(p_updn->p_osm->log), OSM_LOG_ERROR, "ERR AA0C: "
@@ -596,6 +601,7 @@ static int updn_lid_matrices(void *ctx)
OSM_LOG_EXIT(&p_updn->p_osm->log);
return -1;
}
+ p_sw->priv_release = updn_sw_priv_release;
}
/* First setup root nodes */
@@ -653,7 +659,8 @@ static int updn_lid_matrices(void *ctx)
item != cl_qmap_end(&p_updn->p_osm->subn.sw_guid_tbl);
item = cl_qmap_next(item)) {
p_sw = (osm_switch_t *) item;
- delete_updn_node(p_sw->priv);
+ if (p_sw->priv_release)
+ p_sw->priv_release(p_sw);
}
OSM_LOG_EXIT(&p_updn->p_osm->log);
--
1.5.6.GIT
More information about the general
mailing list