Skip to content

Commit

Permalink
tun-metadata: Provide error messages during auto-allocation.
Browse files Browse the repository at this point in the history 
In cases where we don't have a map of tunnel metadata options (such
as with ovs-ofctl) we dynamically allocate them as part of the match.
However, dynamic allocation brings the possibility of errors such as
duplicate entries or running out of space. Up until now, anything that
would cause an error was silently ignored. Since that is not very user
friendly, this adds a mechanism for reporting these types of errors.

Signed-off-by: Jesse Gross <[email protected]>
Acked-by: Ben Pfaff <[email protected]>
  • Loading branch information
@jessegross
jessegross committed Sep 9, 2015
1 parent 0eede6b commit 4f7b100
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 30 deletions.
52 changes: 38 additions & 14 deletions lib/meta-flow.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -796,11 +796,19 @@ mf_get_value(const struct mf_field *mf, const struct flow *flow,

/* Makes 'match' match field 'mf' exactly, with the value matched taken from
* 'value'. The caller is responsible for ensuring that 'match' meets 'mf''s
* prerequisites. */
* prerequisites.
*
* If non-NULL, 'err_str' returns a malloc'ed string describing any errors
* with the request or NULL if there is no error. The caller is reponsible
* for freeing the string. */
void
mf_set_value(const struct mf_field *mf,
const union mf_value *value, struct match *match)
const union mf_value *value, struct match *match, char **err_str)
{
if (err_str) {
*err_str = NULL;
}

switch (mf->id) {
case MFF_DP_HASH:
match_set_dp_hash(match, ntohl(value->be32));
Expand Down Expand Up @@ -836,7 +844,7 @@ mf_set_value(const struct mf_field *mf,
match_set_tun_ttl(match, value->u8);
break;
CASE_MFF_TUN_METADATA:
tun_metadata_set_match(mf, value, NULL, match);
tun_metadata_set_match(mf, value, NULL, match, err_str);
break;

case MFF_METADATA:
Expand Down Expand Up @@ -1364,10 +1372,18 @@ mf_is_set(const struct mf_field *mf, const struct flow *flow)
/* Makes 'match' wildcard field 'mf'.
*
* The caller is responsible for ensuring that 'match' meets 'mf''s
* prerequisites. */
* prerequisites.
*
* If non-NULL, 'err_str' returns a malloc'ed string describing any errors
* with the request or NULL if there is no error. The caller is reponsible
* for freeing the string. */
void
mf_set_wild(const struct mf_field *mf, struct match *match)
mf_set_wild(const struct mf_field *mf, struct match *match, char **err_str)
{
if (err_str) {
*err_str = NULL;
}

switch (mf->id) {
case MFF_DP_HASH:
match->flow.dp_hash = 0;
Expand Down Expand Up @@ -1406,7 +1422,7 @@ mf_set_wild(const struct mf_field *mf, struct match *match)
match_set_tun_ttl_masked(match, 0, 0);
break;
CASE_MFF_TUN_METADATA:
tun_metadata_set_match(mf, NULL, NULL, match);
tun_metadata_set_match(mf, NULL, NULL, match, err_str);
break;

case MFF_METADATA:
Expand Down Expand Up @@ -1595,22 +1611,30 @@ mf_set_wild(const struct mf_field *mf, struct match *match)
* call is equivalent to mf_set_wild(mf, match).
*
* 'mask' must be a valid mask for 'mf' (see mf_is_mask_valid()). The caller
* is responsible for ensuring that 'match' meets 'mf''s prerequisites. */
* is responsible for ensuring that 'match' meets 'mf''s prerequisites.
*
* If non-NULL, 'err_str' returns a malloc'ed string describing any errors
* with the request or NULL if there is no error. The caller is reponsible
* for freeing the string.*/
enum ofputil_protocol
mf_set(const struct mf_field *mf,
const union mf_value *value, const union mf_value *mask,
struct match *match)
struct match *match, char **err_str)
{
if (!mask || is_all_ones(mask, mf->n_bytes)) {
mf_set_value(mf, value, match);
mf_set_value(mf, value, match, err_str);
return mf->usable_protocols_exact;
} else if (is_all_zeros(mask, mf->n_bytes) && !mf_is_tun_metadata(mf)) {
/* Tunnel metadata matches on the existence of the field itself, so
* it still needs to be encoded even if the value is wildcarded. */
mf_set_wild(mf, match);
mf_set_wild(mf, match, err_str);
return OFPUTIL_P_ANY;
}

if (err_str) {
*err_str = NULL;
}

switch (mf->id) {
case MFF_RECIRC_ID:
case MFF_CONJ_ID:
Expand Down Expand Up @@ -1665,7 +1689,7 @@ mf_set(const struct mf_field *mf,
match_set_tun_tos_masked(match, value->u8, mask->u8);
break;
CASE_MFF_TUN_METADATA:
tun_metadata_set_match(mf, value, mask, match);
tun_metadata_set_match(mf, value, mask, match, err_str);
break;

case MFF_METADATA:
Expand Down Expand Up @@ -1731,7 +1755,7 @@ mf_set(const struct mf_field *mf,

case MFF_IPV6_LABEL:
if ((mask->be32 & htonl(IPV6_LABEL_MASK)) == htonl(IPV6_LABEL_MASK)) {
mf_set_value(mf, value, match);
mf_set_value(mf, value, match, err_str);
} else {
match_set_ipv6_label_masked(match, value->be32, mask->be32);
}
Expand Down Expand Up @@ -2316,7 +2340,7 @@ mf_write_subfield(const struct mf_subfield *sf, const union mf_subvalue *x,
mf_get(field, match, &value, &mask);
bitwise_copy(x, sizeof *x, 0, &value, field->n_bytes, sf->ofs, sf->n_bits);
bitwise_one ( &mask, field->n_bytes, sf->ofs, sf->n_bits);
mf_set(field, &value, &mask, match);
mf_set(field, &value, &mask, match, NULL);
}

/* 'v' and 'm' correspond to values of 'field'. This function copies them into
Expand All @@ -2332,7 +2356,7 @@ mf_mask_subfield(const struct mf_field *field,
mf_get(field, match, &value, &mask);
bitwise_copy(v, sizeof *v, 0, &value, field->n_bytes, 0, field->n_bits);
bitwise_copy(m, sizeof *m, 0, &mask, field->n_bytes, 0, field->n_bits);
mf_set(field, &value, &mask, match);
mf_set(field, &value, &mask, match, NULL);
}

/* Initializes 'x' to the value of 'sf' within 'flow'. 'sf' must be valid for
Expand Down
6 changes: 3 additions & 3 deletions lib/meta-flow.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1844,7 +1844,7 @@ bool mf_is_value_valid(const struct mf_field *, const union mf_value *value);
void mf_get_value(const struct mf_field *, const struct flow *,
union mf_value *value);
void mf_set_value(const struct mf_field *, const union mf_value *value,
struct match *);
struct match *, char **err_str);
void mf_set_flow_value(const struct mf_field *, const union mf_value *value,
struct flow *);
void mf_set_flow_value_masked(const struct mf_field *,
Expand All @@ -1864,9 +1864,9 @@ void mf_get(const struct mf_field *, const struct match *,
enum ofputil_protocol mf_set(const struct mf_field *,
const union mf_value *value,
const union mf_value *mask,
struct match *);
struct match *, char **err_str);

void mf_set_wild(const struct mf_field *, struct match *);
void mf_set_wild(const struct mf_field *, struct match *, char **err_str);

/* Subfields. */
void mf_write_subfield_flow(const struct mf_subfield *,
Expand Down
10 changes: 9 additions & 1 deletion lib/nx-match.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,15 @@ nx_pull_raw(const uint8_t *p, unsigned int match_len, bool strict,
} else if (!mf_is_all_wild(field, &match->wc)) {
error = OFPERR_OFPBMC_DUP_FIELD;
} else {
mf_set(field, &value, &mask, match);
char *err_str;

mf_set(field, &value, &mask, match, &err_str);
if (err_str) {
VLOG_DBG_RL(&rl, "error parsing OXM at offset %"PRIdPTR" "
"within match (%s)", pos - p, err_str);
free(err_str);
return OFPERR_OFPBMC_BAD_VALUE;
}
}

if (error) {
Expand Down
2 changes: 1 addition & 1 deletion lib/ofp-parse.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ parse_field(const struct mf_field *mf, const char *s, struct match *match,

error = mf_parse(mf, s, &value, &mask);
if (!error) {
*usable_protocols &= mf_set(mf, &value, &mask, match);
*usable_protocols &= mf_set(mf, &value, &mask, match, &error);
}
return error;
}
Expand Down
39 changes: 32 additions & 7 deletions lib/tun-metadata.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,10 +280,15 @@ tun_metadata_write(struct flow_tnl *tnl,

static const struct tun_metadata_loc *
metadata_loc_from_match(struct tun_table *map, struct match *match,
unsigned int idx, unsigned int field_len, bool masked)
const char *name, unsigned int idx,
unsigned int field_len, bool masked, char **err_str)
{
ovs_assert(idx < TUN_METADATA_NUM_OPTS);

if (err_str) {
*err_str = NULL;
}

if (map) {
if (map->entries[idx].valid) {
return &map->entries[idx].loc;
Expand All @@ -292,8 +297,22 @@ metadata_loc_from_match(struct tun_table *map, struct match *match,
}
}

if (match->tun_md.alloc_offset + field_len >= TUN_METADATA_TOT_OPT_SIZE ||
ULLONG_GET(match->wc.masks.tunnel.metadata.present.map, idx)) {
if (match->tun_md.alloc_offset + field_len > TUN_METADATA_TOT_OPT_SIZE) {
if (err_str) {
*err_str = xasprintf("field %s exceeds maximum size for tunnel "
"metadata (used %d, max %d)", name,
match->tun_md.alloc_offset + field_len,
TUN_METADATA_TOT_OPT_SIZE);
}

return NULL;
}

if (ULLONG_GET(match->wc.masks.tunnel.metadata.present.map, idx)) {
if (err_str) {
*err_str = xasprintf("field %s set multiple times", name);
}

return NULL;
}

Expand Down Expand Up @@ -322,10 +341,15 @@ metadata_loc_from_match(struct tun_table *map, struct match *match,
* value.
*
* 'mask' may be NULL; if so, then 'mf' is made exact-match.
*
* If non-NULL, 'err_str' returns a malloc'ed string describing any errors
* with the request or NULL if there is no error. The caller is reponsible
* for freeing the string.
*/
void
tun_metadata_set_match(const struct mf_field *mf, const union mf_value *value,
const union mf_value *mask, struct match *match)
const union mf_value *mask, struct match *match,
char **err_str)
{
struct tun_table *map = ovsrcu_get(struct tun_table *, &metadata_tab);
const struct tun_metadata_loc *loc;
Expand All @@ -338,7 +362,8 @@ tun_metadata_set_match(const struct mf_field *mf, const union mf_value *value,
ovs_assert(!(match->flow.tunnel.flags & FLOW_TNL_F_UDPIF));

field_len = mf_field_len(mf, value, mask, &is_masked);
loc = metadata_loc_from_match(map, match, idx, field_len, is_masked);
loc = metadata_loc_from_match(map, match, mf->name, idx, field_len,
is_masked, err_str);
if (!loc) {
return;
}
Expand Down Expand Up @@ -424,8 +449,8 @@ tun_metadata_get_fmd(const struct flow_tnl *tnl, struct match *flow_metadata)
const struct tun_metadata_loc *old_loc = &flow.metadata.tab->entries[i].loc;
const struct tun_metadata_loc *new_loc;

new_loc = metadata_loc_from_match(NULL, flow_metadata, i,
old_loc->len, false);
new_loc = metadata_loc_from_match(NULL, flow_metadata, NULL, i,
old_loc->len, false, NULL);

memcpy_from_metadata(opts.tun_metadata, &flow.metadata, old_loc);
memcpy_to_metadata(&flow_metadata->flow.tunnel.metadata,
Expand Down
9 changes: 5 additions & 4 deletions lib/tun-metadata.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ BUILD_ASSERT_DECL(sizeof(((struct tun_metadata *)0)->present.map) * 8 >=
* linked list of these blocks. */
struct tun_metadata_loc_chain {
struct tun_metadata_loc_chain *next;
uint8_t offset; /* In bytes, from start of 'opts', multiple of 4. */
uint8_t len; /* In bytes, multiple of 4. */
int offset; /* In bytes, from start of 'opts', multiple of 4. */
int len; /* In bytes, multiple of 4. */
};

struct tun_metadata_loc {
Expand All @@ -102,7 +102,7 @@ struct tun_metadata_match_entry {
* allocated memory because the address space is never fragmented. */
struct tun_metadata_allocation {
struct tun_metadata_match_entry entry[TUN_METADATA_NUM_OPTS];
uint8_t alloc_offset; /* Byte offset into 'opts', multiple of 4. */
int alloc_offset; /* Byte offset into 'opts', multiple of 4. */
bool valid; /* Set to true after any allocation occurs. */
};

Expand All @@ -117,7 +117,8 @@ void tun_metadata_write(struct flow_tnl *,
const struct mf_field *, const union mf_value *);
void tun_metadata_set_match(const struct mf_field *,
const union mf_value *value,
const union mf_value *mask, struct match *);
const union mf_value *mask, struct match *,
char **err_str);
void tun_metadata_get_fmd(const struct flow_tnl *, struct match *flow_metadata);

int tun_metadata_from_geneve_nlattr(const struct nlattr *attr,
Expand Down
8 changes: 8 additions & 0 deletions tests/tunnel.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,14 @@ NXT_GENEVE_TABLE_MOD (xid=0x4):
0xffff 0x3 124 tun_metadata3
])

AT_CHECK([ovs-ofctl add-flow br0 "tun_metadata0,tun_metadata0,actions=drop"], [1], [ignore],
[ovs-ofctl: field tun_metadata0 set multiple times
])

AT_CHECK([ovs-ofctl add-flow br0 "tun_metadata0=0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef,tun_metadata1=0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef,tun_metadata2=0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef,tun_metadata3=0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef,tun_metadata4=0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef,actions=drop"], [1], [ignore],
[ovs-ofctl: field tun_metadata4 exceeds maximum size for tunnel metadata (used 320, max 256)
])

dnl Allocation and match with fragmented address space
AT_CHECK([ovs-ofctl add-geneve-map br0 "{class=0xffff,type=2,len=124}->tun_metadata2"])
AT_CHECK([ovs-ofctl add-geneve-map br0 "{class=0xffff,type=3,len=4}->tun_metadata3"])
Expand Down

0 comments on commit 4f7b100

Please sign in to comment.