Subject: Make accesses to spesh arg guard atomic Bug-Debian: https://bugs.debian.org/1086555 From: Timo Paulssen On architectures other than x86, other cores may see the pointer to the new arg guard tree before they see all the data inside it, causing them to jump out of bounds or otherwise misbehave. --- src/6model/reprs/MVMSpeshCandidate.c | 2 +- src/6model/reprs/MVMStaticFrameSpesh.c | 4 ++-- src/6model/reprs/MVMStaticFrameSpesh.h | 3 ++- src/core/frame.c | 4 ++-- src/spesh/arg_guard.c | 20 ++++++++++++++------ src/spesh/arg_guard.h | 4 ++-- src/spesh/optimize.c | 2 +- src/spesh/osr.c | 2 +- 8 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/6model/reprs/MVMSpeshCandidate.c b/src/6model/reprs/MVMSpeshCandidate.c index 03df05e..003dec7 100644 --- a/src/6model/reprs/MVMSpeshCandidate.c +++ b/src/6model/reprs/MVMSpeshCandidate.c @@ -377,7 +377,7 @@ void MVM_spesh_candidate_add(MVMThreadContext *tc, MVMSpeshPlanned *p) { /* If we're logging, dump the updated arg guards also. */ if (MVM_spesh_debug_enabled(tc)) { char *guard_dump = MVM_spesh_dump_arg_guard(tc, p->sf, - p->sf->body.spesh->body.spesh_arg_guard); + (MVMSpeshArgGuard *)MVM_load(&p->sf->body.spesh->body.spesh_arg_guard)); MVM_spesh_debug_printf(tc, "%s========\n\n", guard_dump); fflush(tc->instance->spesh_log_fh); MVM_free(guard_dump); diff --git a/src/6model/reprs/MVMStaticFrameSpesh.c b/src/6model/reprs/MVMStaticFrameSpesh.c index 7acc835..9fee30b 100644 --- a/src/6model/reprs/MVMStaticFrameSpesh.c +++ b/src/6model/reprs/MVMStaticFrameSpesh.c @@ -26,7 +26,7 @@ static void copy_to(MVMThreadContext *tc, MVMSTable *st, void *src, MVMObject *d static void gc_mark(MVMThreadContext *tc, MVMSTable *st, void *data, MVMGCWorklist *worklist) { MVMStaticFrameSpeshBody *body = (MVMStaticFrameSpeshBody *)data; MVM_spesh_stats_gc_mark(tc, body->spesh_stats, worklist); - MVM_spesh_arg_guard_gc_mark(tc, body->spesh_arg_guard, worklist); + MVM_spesh_arg_guard_gc_mark(tc, (MVMSpeshArgGuard *)MVM_load(&body->spesh_arg_guard), worklist); if (body->num_spesh_candidates) { MVMuint32 i; for (i = 0; i < body->num_spesh_candidates; i++) { @@ -40,7 +40,7 @@ static void gc_free(MVMThreadContext *tc, MVMObject *obj) { MVMStaticFrameSpesh *sfs = (MVMStaticFrameSpesh *)obj; MVM_spesh_stats_destroy(tc, sfs->body.spesh_stats); MVM_free(sfs->body.spesh_stats); - MVM_spesh_arg_guard_destroy(tc, sfs->body.spesh_arg_guard, 0); + MVM_spesh_arg_guard_destroy(tc, (MVMSpeshArgGuard *)MVM_load(&sfs->body.spesh_arg_guard), 0); MVM_free(sfs->body.spesh_candidates); } diff --git a/src/6model/reprs/MVMStaticFrameSpesh.h b/src/6model/reprs/MVMStaticFrameSpesh.h index d664c1c..f17809e 100644 --- a/src/6model/reprs/MVMStaticFrameSpesh.h +++ b/src/6model/reprs/MVMStaticFrameSpesh.h @@ -4,7 +4,8 @@ struct MVMStaticFrameSpeshBody { /* Specialization argument guard tree, for selecting a specialization. */ - MVMSpeshArgGuard *spesh_arg_guard; + /* This is a pointer to MVMSpeshArgGuard, but we access it atomically. */ + AO_t spesh_arg_guard; /* Specializations array, if there are any. Candidates themselves never * move in memory; the array of pointers to them is managed using the diff --git a/src/core/frame.c b/src/core/frame.c index 67067da..e44f811 100644 --- a/src/core/frame.c +++ b/src/core/frame.c @@ -468,14 +468,14 @@ void MVM_frame_dispatch(MVMThreadContext *tc, MVMCode *code, MVMArgs args, MVMin /* Run the specialization argument guard to see if we can use one. */ spesh = static_frame->body.spesh; - spesh_cand = MVM_spesh_arg_guard_run(tc, spesh->body.spesh_arg_guard, + spesh_cand = MVM_spesh_arg_guard_run(tc, (MVMSpeshArgGuard *)MVM_load(&spesh->body.spesh_arg_guard), args, NULL); } else { spesh = static_frame->body.spesh; #if MVM_SPESH_CHECK_PRESELECTION MVMint32 certain = -1; - MVMint32 correct = MVM_spesh_arg_guard_run(tc, spesh->body.spesh_arg_guard, + MVMint32 correct = MVM_spesh_arg_guard_run(tc, (MVMSpeshArgGuard *)MVM_load(&spesh->body.spesh_arg_guard), args, &certain); if (spesh_cand != correct && spesh_cand != certain) { fprintf(stderr, "Inconsistent spesh preselection of '%s' (%s): got %d, not %d\n", diff --git a/src/spesh/arg_guard.c b/src/spesh/arg_guard.c index b15572f..8c1edae 100644 --- a/src/spesh/arg_guard.c +++ b/src/spesh/arg_guard.c @@ -307,7 +307,7 @@ static void add_nodes_for_callsite(MVMThreadContext *tc, MVMSpeshArgGuard *tree, /* Produce and install an updated set of guards, incorporating the new * candidate. */ -void MVM_spesh_arg_guard_regenerate(MVMThreadContext *tc, MVMSpeshArgGuard **guard_ptr, +void MVM_spesh_arg_guard_regenerate(MVMThreadContext *tc, AO_t *guard_ptr, MVMSpeshCandidate **candidates, MVMuint32 num_spesh_candidates) { MVMSpeshArgGuard *tree; @@ -383,12 +383,18 @@ void MVM_spesh_arg_guard_regenerate(MVMThreadContext *tc, MVMSpeshArgGuard **gua /* Install the produced argument guard. */ if (*guard_ptr) { - MVMSpeshArgGuard *prev = *guard_ptr; - *guard_ptr = tree; + /* Ensure that the tree is already fully out of our cache. + * This is not necessary on x86_64, which has + * Total Store Ordering, but architectures like ARM are weaker + * and other cores can observe older contents of the */ + MVM_barrier(); + MVMSpeshArgGuard *prev = (MVMSpeshArgGuard *)MVM_load(guard_ptr); + MVM_store(guard_ptr, tree); MVM_spesh_arg_guard_destroy(tc, prev, 1); } else { - *guard_ptr = tree; + MVM_barrier(); + MVM_store(guard_ptr, tree); } /* Clean up working state. */ @@ -642,7 +648,9 @@ void MVM_spesh_arg_guard_destroy(MVMThreadContext *tc, MVMSpeshArgGuard *ag, MVM void MVM_spesh_arg_guard_discard(MVMThreadContext *tc, MVMStaticFrame *sf) { MVMStaticFrameSpesh *spesh = sf->body.spesh; if (spesh && spesh->body.spesh_arg_guard) { - MVM_spesh_arg_guard_destroy(tc, spesh->body.spesh_arg_guard, 1); - spesh->body.spesh_arg_guard = NULL; + /* No need to atomically exchange with NULL here, since actual freeing + * of data is postponed to the next safe point. */ + MVM_spesh_arg_guard_destroy(tc, (MVMSpeshArgGuard *)MVM_load(&spesh->body.spesh_arg_guard), 1); + MVM_store(&spesh->body.spesh_arg_guard, NULL); } } diff --git a/src/spesh/arg_guard.h b/src/spesh/arg_guard.h index f5f8046..b5518fa 100644 --- a/src/spesh/arg_guard.h +++ b/src/spesh/arg_guard.h @@ -73,8 +73,8 @@ struct MVMSpeshArgGuardNode { }; }; -void MVM_spesh_arg_guard_regenerate(MVMThreadContext *tc, MVMSpeshArgGuard **guard_ptr, - MVMSpeshCandidate **candidates, MVMuint32 num_spesh_candidates); +void MVM_spesh_arg_guard_regenerate(MVMThreadContext *tc, AO_t *guard_ptr, + MVMSpeshCandidate **candidates, MVMuint32 num_spesh_candidates); MVMint32 MVM_spesh_arg_guard_run_types(MVMThreadContext *tc, MVMSpeshArgGuard *ag, MVMCallsite *cs, MVMSpeshStatsType *types); MVMint32 MVM_spesh_arg_guard_run(MVMThreadContext *tc, MVMSpeshArgGuard *ag, diff --git a/src/spesh/optimize.c b/src/spesh/optimize.c index e29836f..a520b49 100644 --- a/src/spesh/optimize.c +++ b/src/spesh/optimize.c @@ -1539,7 +1539,7 @@ static void optimize_runbytecode(MVMThreadContext *tc, MVMSpeshGraph *g, MVMSpes } /* Try to find a specialization. */ - MVMSpeshArgGuard *ag = target_sf->body.spesh->body.spesh_arg_guard; + MVMSpeshArgGuard *ag = (MVMSpeshArgGuard *)MVM_load(&target_sf->body.spesh->body.spesh_arg_guard); MVMint16 spesh_cand = MVM_spesh_arg_guard_run_types(tc, ag, cs, stable_type_tuple); if (spesh_cand >= 0) { /* Found a candidate. Stack up any required guards. */ diff --git a/src/spesh/osr.c b/src/spesh/osr.c index 430b19b..8d23667 100644 --- a/src/spesh/osr.c +++ b/src/spesh/osr.c @@ -103,7 +103,7 @@ void MVM_spesh_osr_poll_for_result(MVMThreadContext *tc) { if (!tc->cur_frame->extra || !tc->cur_frame->extra->caller_pos_needed) { /* Check if there's a candidate available and install it if so. */ MVMint32 ag_result = MVM_spesh_arg_guard_run(tc, - spesh->body.spesh_arg_guard, + (MVMSpeshArgGuard *)MVM_load(&spesh->body.spesh_arg_guard), tc->cur_frame->params.arg_info, NULL); if (ag_result >= 0) { perform_osr(tc, spesh->body.spesh_candidates[ag_result]);