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 <[email protected]>
---
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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
