Skip to content

Commit

Permalink
tun-metadata: Always set option present when copying data.
Browse files Browse the repository at this point in the history 
Whenever we write into a tunnel option field, we also need to mark
it as significant. If we don't, then the data will later be ignored.

We currently do this in every case except for flow metadata. This causes
us to not correctly serialize the tunnel metadata for Packet Ins to the
controller.

Rather than separately writing the data and marking the options as present,
it is better to combine the two steps to ensure that one can never be
done without the other.

Signed-off-by: Jesse Gross <[email protected]>
Acked-by: Ben Pfaff <[email protected]>
  • Loading branch information
@jessegross
jessegross committed Aug 21, 2015
1 parent f0b12cd commit 2234a7d
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 16 deletions.
27 changes: 14 additions & 13 deletions lib/tun-metadata.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ static enum ofperr tun_metadata_add_entry(struct tun_table *map, uint8_t idx,
static void tun_metadata_del_entry(struct tun_table *map, uint8_t idx)
OVS_REQUIRES(tab_mutex);
static void memcpy_to_metadata(struct tun_metadata *dst, const void *src,
const struct tun_metadata_loc *);
const struct tun_metadata_loc *,
unsigned int idx);
static void memcpy_from_metadata(void *dst, const struct tun_metadata *src,
const struct tun_metadata_loc *);

Expand Down Expand Up @@ -273,10 +274,8 @@ tun_metadata_write(struct flow_tnl *tnl,
}

loc = &map->entries[idx].loc;

ULLONG_SET1(tnl->metadata.present.map, idx);
memcpy_to_metadata(&tnl->metadata,
value->tun_metadata + mf->n_bytes - loc->len, loc);
value->tun_metadata + mf->n_bytes - loc->len, loc, idx);
}

static const struct tun_metadata_loc *
Expand Down Expand Up @@ -355,8 +354,8 @@ tun_metadata_set_match(const struct mf_field *mf, const union mf_value *value,
mask->tun_metadata[data_offset + i];
}
}
ULLONG_SET1(match->flow.tunnel.metadata.present.map, idx);
memcpy_to_metadata(&match->flow.tunnel.metadata, data.tun_metadata, loc);
memcpy_to_metadata(&match->flow.tunnel.metadata, data.tun_metadata,
loc, idx);

if (!value) {
memset(data.tun_metadata, 0, loc->len);
Expand All @@ -365,8 +364,8 @@ tun_metadata_set_match(const struct mf_field *mf, const union mf_value *value,
} else {
memcpy(data.tun_metadata, mask->tun_metadata + data_offset, loc->len);
}
ULLONG_SET1(match->wc.masks.tunnel.metadata.present.map, idx);
memcpy_to_metadata(&match->wc.masks.tunnel.metadata, data.tun_metadata, loc);
memcpy_to_metadata(&match->wc.masks.tunnel.metadata, data.tun_metadata,
loc, idx);
}

static bool
Expand Down Expand Up @@ -427,11 +426,11 @@ tun_metadata_get_fmd(const struct flow_tnl *tnl, struct match *flow_metadata)

memcpy_from_metadata(opts.tun_metadata, &flow.metadata, old_loc);
memcpy_to_metadata(&flow_metadata->flow.tunnel.metadata,
opts.tun_metadata, new_loc);
opts.tun_metadata, new_loc, i);

memset(opts.tun_metadata, 0xff, old_loc->len);
memcpy_to_metadata(&flow_metadata->wc.masks.tunnel.metadata,
opts.tun_metadata, new_loc);
opts.tun_metadata, new_loc, i);
}
}

Expand All @@ -456,7 +455,7 @@ tun_meta_find_key(const struct hmap *hmap, uint32_t key)

static void
memcpy_to_metadata(struct tun_metadata *dst, const void *src,
const struct tun_metadata_loc *loc)
const struct tun_metadata_loc *loc, unsigned int idx)
{
const struct tun_metadata_loc_chain *chain = &loc->c;
int addr = 0;
Expand All @@ -467,6 +466,8 @@ memcpy_to_metadata(struct tun_metadata *dst, const void *src,
addr += chain->len;
chain = chain->next;
}

ULLONG_SET1(dst->present.map, idx);
}

static void
Expand Down Expand Up @@ -654,8 +655,8 @@ tun_metadata_from_geneve__(const struct tun_metadata *flow_metadata,
flow_opt->type));
if (entry) {
if (entry->loc.len == flow_opt->length * 4) {
memcpy_to_metadata(metadata, opt + 1, &entry->loc);
ULLONG_SET1(metadata->present.map, entry - map->entries);
memcpy_to_metadata(metadata, opt + 1, &entry->loc,
entry - map->entries);
} else {
return EINVAL;
}
Expand Down
16 changes: 13 additions & 3 deletions tests/tunnel-push-pop.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,26 @@ AT_CHECK([ovs-ofctl dump-ports int-br | grep 'port 3'], [0], [dnl
])

dnl Check decapsulation of Geneve packet with options
AT_CAPTURE_FILE([ofctl_monitor.log])
AT_CHECK([ovs-ofctl monitor int-br 65534 --detach --no-chdir --pidfile 2> ofctl_monitor.log])

AT_CHECK([ovs-ofctl del-flows int-br])
AT_CHECK([ovs-ofctl add-flow int-br "tun_metadata0=0xa/0xf,actions=5"])
AT_CHECK([ovs-ofctl add-flow int-br "tun_metadata0=0xa/0xf,actions=5,controller"])
AT_CHECK([ovs-appctl netdev-dummy/receive p0 '001b213cac30001b213cab64080045000096794640004011ba630101025c01010258308817c1008200000400655800007b00ffff80010000000affff00010000000bfe71d883724fbeb6f4e1494a080045000054ba200000400184861e0000011e00000200004227e75400030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637'])

ovs-appctl time/warp 1000
OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 2])
OVS_APP_EXIT_AND_WAIT(ovs-ofctl)

AT_CHECK([cat ofctl_monitor.log], [0], [dnl
NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=98 tun_id=0x7b,tun_src=1.1.2.92,tun_dst=1.1.2.88,tun_metadata0=0xa,in_port=5 (via action) data_len=98 (unbuffered)
icmp,vlan_tci=0x0000,dl_src=be:b6:f4:e1:49:4a,dl_dst=fe:71:d8:83:72:4f,nw_src=30.0.0.1,nw_dst=30.0.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,icmp_type=0,icmp_code=0 icmp_csum:4227
])

AT_CHECK([ovs-ofctl dump-ports int-br | grep 'port 5'], [0], [dnl
port 5: rx pkts=1, bytes=98, drop=0, errs=0, frame=0, over=0, crc=0
])
AT_CHECK([ovs-appctl dpif/dump-flows int-br], [0], [dnl
tunnel(tun_id=0x7b,src=1.1.2.92,dst=1.1.2.88,ttl=64,geneve({class=0xffff,type=0x80,len=4,0xa/0xf}{class=0xffff,type=0,len=4}),flags(-df-csum+key)),skb_mark(0),recirc_id(0),in_port(6081),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:drop
tunnel(tun_id=0x7b,src=1.1.2.92,dst=1.1.2.88,ttl=64,geneve({class=0xffff,type=0x80,len=4,0xa/0xf}{class=0xffff,type=0,len=4}),flags(-df-csum+key)),skb_mark(0),recirc_id(0),in_port(6081),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:userspace(pid=0,slow_path(controller))
])

OVS_VSWITCHD_STOP
Expand Down

0 comments on commit 2234a7d

Please sign in to comment.