Currently classifier tries and n_tries can be updated not atomically, there is a race condition which can lead to NULL dereference. The race can happen when main thread updates a classifier tries and n_tries in classifier_set_prefix_fields() and at the same time revalidator or handler thread try to lookup them in classifier_lookup__(). Such race can be triggered when user changes prefixes of flow_table.
Race(user changes flow_table prefixes: ip_dst,ip_src => none): [main thread] [revalidator/handler thread] =========================================================== /* cls->n_tries == 2 */ for (int i = 0; i < cls->n_tries; i++) { trie_init(cls, i, NULL); /* n_tries == 0 */ cls->n_tries = n_tries; /* cls->tries[i]->feild is NULL */ trie_ctx_init(&trie_ctx[i],&cls->tries[i]); /* trie->field is NULL */ ctx->be32ofs = trie->field->flow_be32ofs; To prevent the race, instead of re-introducing internal mutex implemented in the commit fccd7c092e09 ("classifier: Remove internal mutex."), this patch uses trie field after it checked subtable trie_plen is synchronized with field. This leaves classifier lookup lockless and prevent the NULL deref. Fixes: fccd7c092e09 ("classifier: Remove internal mutex.") Signed-off-by: Eiichi Tsukata <eiichi.tsuk...@nutanix.com> --- lib/classifier.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/classifier.c b/lib/classifier.c index 0fad953..9f70180 100644 --- a/lib/classifier.c +++ b/lib/classifier.c @@ -839,7 +839,6 @@ classifier_remove_assert(struct classifier *cls, struct trie_ctx { const struct cls_trie *trie; bool lookup_done; /* Status of the lookup. */ - uint8_t be32ofs; /* U32 offset of the field in question. */ unsigned int maskbits; /* Prefix length needed to avoid false matches. */ union trie_prefix match_plens; /* Bitmask of prefix lengths with possible * matches. */ @@ -849,7 +848,6 @@ static void trie_ctx_init(struct trie_ctx *ctx, const struct cls_trie *trie) { ctx->trie = trie; - ctx->be32ofs = trie->field->flow_be32ofs; ctx->lookup_done = false; } @@ -1575,11 +1573,16 @@ check_tries(struct trie_ctx trie_ctx[CLS_MAX_TRIES], unsigned int n_tries, * fields using the prefix tries. The trie checks are done only as * needed to avoid folding in additional bits to the wildcards mask. */ for (j = 0; j < n_tries; j++) { - /* Is the trie field relevant for this subtable, and - is the trie field within the current range of fields? */ - if (field_plen[j] && - flowmap_is_set(&range_map, trie_ctx[j].be32ofs / 2)) { - struct trie_ctx *ctx = &trie_ctx[j]; + struct trie_ctx *ctx = &trie_ctx[j]; + /* Check if trie field is relevant for this subtable and + * synchronized with field_plen. */ + if (!field_plen[j] || !ctx->trie->field) { + continue; + } + + uint8_t be32ofs = ctx->trie->field->flow_be32ofs; + /* Is the trie field within the current range of fields? */ + if (flowmap_is_set(&range_map, be32ofs / 2)) { /* On-demand trie lookup. */ if (!ctx->lookup_done) { @@ -1601,14 +1604,14 @@ check_tries(struct trie_ctx trie_ctx[CLS_MAX_TRIES], unsigned int n_tries, * than this subtable would otherwise. */ if (ctx->maskbits <= field_plen[j]) { /* Unwildcard the bits and skip the rest. */ - mask_set_prefix_bits(wc, ctx->be32ofs, ctx->maskbits); + mask_set_prefix_bits(wc, be32ofs, ctx->maskbits); /* Note: Prerequisite already unwildcarded, as the only * prerequisite of the supported trie lookup fields is * the ethertype, which is always unwildcarded. */ return true; } /* Can skip if the field is already unwildcarded. */ - if (mask_prefix_bits_set(wc, ctx->be32ofs, ctx->maskbits)) { + if (mask_prefix_bits_set(wc, be32ofs, ctx->maskbits)) { return true; } } -- 1.8.3.1 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev