Skip to content

Commit

Permalink
cmap, classifier: Avoid unsafe aliasing in iterators.
Browse files Browse the repository at this point in the history 
CMAP_FOR_EACH and CLS_FOR_EACH and their variants tried to use void ** as
a "pointer to any kind of pointer".  That is a violation of the aliasing
rules in ISO C which technically yields undefined behavior.  With GCC 4.1,
it causes both warnings and actual misbehavior.  One option would to add
-fno-strict-aliasing to the compiler flags, but that would only help with
GCC; who knows whether this can be worked around with other compilers.

Instead, this commit rewrites the iterators to avoid disallowed pointer
aliasing.

VMware-BZ: #1287651
Signed-off-by: Ben Pfaff <[email protected]>
Acked-by: Jarno Rajahalme <[email protected]>
  • Loading branch information
@blp
blp committed Jul 22, 2014
1 parent 480cda3 commit 78c8df1
Show file tree
Hide file tree
Showing 9 changed files with 115 additions and 161 deletions.
44 changes: 21 additions & 23 deletions lib/classifier.c
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -476,23 +476,21 @@ classifier_destroy(struct classifier *cls)
OVS_EXCLUDED(cls->mutex)
{
if (cls) {
struct cls_partition *partition, *next_partition;
struct cls_subtable *subtable, *next_subtable;
struct cls_partition *partition;
struct cls_subtable *subtable;
int i;

ovs_mutex_lock(&cls->mutex);
for (i = 0; i < cls->n_tries; i++) {
trie_destroy(&cls->tries[i].root);
}

CMAP_FOR_EACH_SAFE (subtable, next_subtable, cmap_node,
&cls->subtables_map) {
CMAP_FOR_EACH_SAFE (subtable, cmap_node, &cls->subtables_map) {
destroy_subtable(cls, subtable);
}
cmap_destroy(&cls->subtables_map);

CMAP_FOR_EACH_SAFE (partition, next_partition, cmap_node,
&cls->partitions) {
CMAP_FOR_EACH_SAFE (partition, cmap_node, &cls->partitions) {
ovsrcu_postpone(free, partition);
}
cmap_destroy(&cls->partitions);
Expand Down Expand Up @@ -1219,18 +1217,18 @@ search_subtable(const struct cls_subtable *subtable,
* such that cls_rule_is_loose_match(rule, target) returns true.
*
* Ignores target->priority. */
struct cls_cursor cls_cursor_init(const struct classifier *cls,
const struct cls_rule *target,
void **pnode, const void *offset, bool safe)
struct cls_cursor cls_cursor_start(const struct classifier *cls,
const struct cls_rule *target,
bool safe)
OVS_NO_THREAD_SAFETY_ANALYSIS
{
struct cls_cursor cursor;
struct cls_subtable *subtable;
struct cls_rule *cls_rule = NULL;

cursor.safe = safe;
cursor.cls = cls;
cursor.target = target && !cls_rule_is_catchall(target) ? target : NULL;
cursor.rule = NULL;

/* Find first rule. */
ovs_mutex_lock(&cursor.cls->mutex);
Expand All @@ -1240,14 +1238,13 @@ struct cls_cursor cls_cursor_init(const struct classifier *cls,

if (rule) {
cursor.subtable = subtable;
cls_rule = rule->cls_rule;
cursor.rule = rule->cls_rule;
break;
}
}
*pnode = (char *)cls_rule + (ptrdiff_t)offset;

/* Leave locked if requested and have a rule. */
if (safe || !cls_rule) {
if (safe || !cursor.rule) {
ovs_mutex_unlock(&cursor.cls->mutex);
}
return cursor;
Expand All @@ -1258,18 +1255,19 @@ cls_cursor_next_unlock(struct cls_cursor *cursor, struct cls_rule *rule)
OVS_NO_THREAD_SAFETY_ANALYSIS
{
/* Release the mutex if no rule, or 'safe' mode. */
cursor->rule = rule;
if (!rule || cursor->safe) {
ovs_mutex_unlock(&cursor->cls->mutex);
}
}

/* Returns the next matching cls_rule in 'cursor''s iteration, or a null
* pointer if there are no more matches. */
struct cls_rule *
cls_cursor_next(struct cls_cursor *cursor, const struct cls_rule *rule_)
/* Sets 'cursor->rule' to the next matching cls_rule in 'cursor''s iteration,
* or to null if all matching rules have been visited. */
void
cls_cursor_advance(struct cls_cursor *cursor)
OVS_NO_THREAD_SAFETY_ANALYSIS
{
struct cls_match *rule = CONST_CAST(struct cls_match *, rule_->cls_match);
struct cls_match *rule = cursor->rule->cls_match;
const struct cls_subtable *subtable;
struct cls_match *next;

Expand All @@ -1281,7 +1279,7 @@ cls_cursor_next(struct cls_cursor *cursor, const struct cls_rule *rule_)
next = next_rule_in_list__(rule);
if (next->priority < rule->priority) {
cls_cursor_next_unlock(cursor, next->cls_rule);
return next->cls_rule;
return;
}

/* 'next' is the head of the list, that is, the rule that is included in
Expand All @@ -1291,7 +1289,7 @@ cls_cursor_next(struct cls_cursor *cursor, const struct cls_rule *rule_)
CMAP_CURSOR_FOR_EACH_CONTINUE (rule, cmap_node, &cursor->rules) {
if (rule_matches(rule, cursor->target)) {
cls_cursor_next_unlock(cursor, rule->cls_rule);
return rule->cls_rule;
return;
}
}

Expand All @@ -1301,12 +1299,12 @@ cls_cursor_next(struct cls_cursor *cursor, const struct cls_rule *rule_)
if (rule) {
cursor->subtable = subtable;
cls_cursor_next_unlock(cursor, rule->cls_rule);
return rule->cls_rule;
return;
}
}

ovs_mutex_unlock(&cursor->cls->mutex);
return NULL;
cursor->rule = NULL;
}

static struct cls_subtable *
Expand Down
71 changes: 24 additions & 47 deletions lib/classifier.h
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -316,63 +316,40 @@ struct cls_cursor {
const struct cls_rule *target;
struct cmap_cursor subtables;
struct cmap_cursor rules;
struct cls_rule *rule;
bool safe;
};

/* Iteration requires mutual exclusion of writers. We do this by taking
* a mutex for the duration of the iteration, except for the
* 'SAFE' variant, where we release the mutex for the body of the loop. */
struct cls_cursor cls_cursor_init(const struct classifier *cls,
const struct cls_rule *target,
void **pnode, const void *offset, bool safe);
struct cls_cursor cls_cursor_start(const struct classifier *cls,
const struct cls_rule *target,
bool safe);

struct cls_rule *cls_cursor_next(struct cls_cursor *cursor,
const struct cls_rule *);

#define CLS_CURSOR_START(RULE, MEMBER, CLS, TARGET) \
cls_cursor_init(CLS, (TARGET), (void **)&(RULE), \
OBJECT_CONTAINING(NULL, RULE, MEMBER), false)

#define CLS_CURSOR_START_SAFE(RULE, MEMBER, CLS, TARGET) \
cls_cursor_init(CLS, (TARGET), (void **)&(RULE), \
OBJECT_CONTAINING(NULL, RULE, MEMBER), true)

#define CLS_FOR_EACH(RULE, MEMBER, CLS) \
for (struct cls_cursor cursor__ = CLS_CURSOR_START(RULE, MEMBER, CLS, \
NULL); \
RULE != OBJECT_CONTAINING(NULL, RULE, MEMBER); \
ASSIGN_CONTAINER(RULE, cls_cursor_next(&cursor__, &(RULE)->MEMBER), \
MEMBER))
void cls_cursor_advance(struct cls_cursor *);

#define CLS_FOR_EACH(RULE, MEMBER, CLS) \
CLS_FOR_EACH_TARGET(RULE, MEMBER, CLS, NULL)
#define CLS_FOR_EACH_TARGET(RULE, MEMBER, CLS, TARGET) \
for (struct cls_cursor cursor__ = CLS_CURSOR_START(RULE, MEMBER, CLS, \
TARGET); \
RULE != OBJECT_CONTAINING(NULL, RULE, MEMBER); \
ASSIGN_CONTAINER(RULE, cls_cursor_next(&cursor__, &(RULE)->MEMBER), \
MEMBER))

/* This form allows classifier_remove() to be called within the loop. */
#define CLS_FOR_EACH_SAFE(RULE, NEXT, MEMBER, CLS) \
for (struct cls_cursor cursor__ = CLS_CURSOR_START_SAFE(RULE, MEMBER, \
CLS, NULL); \
(RULE != OBJECT_CONTAINING(NULL, RULE, MEMBER) \
? ASSIGN_CONTAINER(NEXT, cls_cursor_next(&cursor__, \
&(RULE)->MEMBER), \
MEMBER), true \
for (struct cls_cursor cursor__ = cls_cursor_start(CLS, TARGET, false); \
(cursor__.rule \
? (ASSIGN_CONTAINER(RULE, cursor__.rule, MEMBER), \
true) \
: false); \
(RULE) = (NEXT))

/* This form allows classifier_remove() to be called within the loop. */
#define CLS_FOR_EACH_TARGET_SAFE(RULE, NEXT, MEMBER, CLS, TARGET) \
for (struct cls_cursor cursor__ = CLS_CURSOR_START_SAFE(RULE, MEMBER, \
CLS, TARGET); \
(RULE != OBJECT_CONTAINING(NULL, RULE, MEMBER) \
? ASSIGN_CONTAINER(NEXT, cls_cursor_next(&cursor__, \
&(RULE)->MEMBER), \
MEMBER), true \
cls_cursor_advance(&cursor__))

/* These forms allows classifier_remove() to be called within the loop. */
#define CLS_FOR_EACH_SAFE(RULE, MEMBER, CLS) \
CLS_FOR_EACH_TARGET_SAFE(RULE, MEMBER, CLS, NULL)
#define CLS_FOR_EACH_TARGET_SAFE(RULE, MEMBER, CLS, TARGET) \
for (struct cls_cursor cursor__ = cls_cursor_start(CLS, TARGET, true); \
(cursor__.rule \
? (ASSIGN_CONTAINER(RULE, cursor__.rule, MEMBER), \
cls_cursor_advance(&cursor__), \
true) \
: false); \
(RULE) = (NEXT))

) \

#ifdef __cplusplus
}
Expand Down
45 changes: 20 additions & 25 deletions lib/cmap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -784,50 +784,45 @@ cmap_rehash(struct cmap *cmap, uint32_t mask)
return new;
}

/* Initializes 'cursor' for iterating through 'cmap'.
*
* Use via CMAP_FOR_EACH. */
void
cmap_cursor_init(struct cmap_cursor *cursor, const struct cmap *cmap)
struct cmap_cursor
cmap_cursor_start(const struct cmap *cmap)
{
cursor->impl = cmap_get_impl(cmap);
cursor->bucket_idx = 0;
cursor->entry_idx = 0;
struct cmap_cursor cursor;

cursor.impl = cmap_get_impl(cmap);
cursor.bucket_idx = 0;
cursor.entry_idx = 0;
cursor.node = NULL;
cmap_cursor_advance(&cursor);

return cursor;
}

/* Returns the next node for 'cursor' to visit, following 'node', or NULL if
* the last node has been visited.
*
* Use via CMAP_FOR_EACH. */
struct cmap_node *
cmap_cursor_next(struct cmap_cursor *cursor, const struct cmap_node *node)
void
cmap_cursor_advance(struct cmap_cursor *cursor)
{
const struct cmap_impl *impl = cursor->impl;

if (node) {
struct cmap_node *next = cmap_node_next(node);

if (next) {
return next;
if (cursor->node) {
cursor->node = cmap_node_next(cursor->node);
if (cursor->node) {
return;
}
}

while (cursor->bucket_idx <= impl->mask) {
const struct cmap_bucket *b = &impl->buckets[cursor->bucket_idx];

while (cursor->entry_idx < CMAP_K) {
struct cmap_node *node = cmap_node_next(&b->nodes[cursor->entry_idx++]);

if (node) {
return node;
cursor->node = cmap_node_next(&b->nodes[cursor->entry_idx++]);
if (cursor->node) {
return;
}
}

cursor->bucket_idx++;
cursor->entry_idx = 0;
}

return NULL;
}

/* Returns the next node in 'cmap' in hash order, or NULL if no nodes remain in
Expand Down
Loading

0 comments on commit 78c8df1

Please sign in to comment.