The userspace connection tracker treats Neighbor Discovery packets
as invalid, because they're not checked against any connection.
This in inconsistent with the kernel connection tracker which always
returns 'CS_NEW'.
Therefore, this commit makes the userspace connection tracker conforming
with the kernel. ND packets still do not create or read any state, but
they're treated as NEW.
To support this, the key extraction functions can now return
KEY_NO_TRACK, meaning that the packet is ok, but it should be treated
statelessly.
We also have to remove a test that explicitly checked that neighbor
discovery was treated as invalid.
Reported-by: Sridhar Gaddam <sgad...@redhat.com>
Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com>
---
lib/conntrack.c | 134 ++++++++++++++++++++++++++++++++----------------
tests/ofproto-dpif.at | 32 ------------
tests/system-traffic.at | 35 +++++++++++++
3 files changed, 125 insertions(+), 76 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index d459321..30f1ed0 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -52,9 +52,17 @@ struct conn_lookup_ctx {
bool related;
};
-static bool conn_key_extract(struct conntrack *, struct dp_packet *,
- ovs_be16 dl_type, struct conn_lookup_ctx *,
- uint16_t zone);
+enum key_status {
+ KEY_INVALID, /* Could not extract the connection key: invalid. */
+ KEY_OK, /* Connection key is ok. */
+ KEY_NO_TRACK, /* Connection key is ok, but it should not be tracked. */
+};
+
+static enum key_status conn_key_extract(struct conntrack *,
+ struct dp_packet *,
+ ovs_be16 dl_type,
+ struct conn_lookup_ctx *,
+ uint16_t zone);
static uint32_t conn_key_hash(const struct conn_key *, uint32_t basis);
static void conn_key_reverse(struct conn_key *);
static void conn_key_lookup(struct conntrack_bucket *ctb,
@@ -157,6 +165,20 @@ static unsigned hash_to_bucket(uint32_t hash)
return (hash >> (32 - CONNTRACK_BUCKETS_SHIFT)) % CONNTRACK_BUCKETS;
}
+static uint16_t
+key_status_to_cs(enum key_status s)
+{
+ switch (s) {
+ case KEY_INVALID:
+ return CS_INVALID;
+ case KEY_OK:
+ case KEY_NO_TRACK:
+ return CS_NEW;
+ default:
+ OVS_NOT_REACHED();
+ }
+}
+
static void
write_ct_md(struct dp_packet *pkt, uint16_t state, uint16_t zone,
uint32_t mark, ovs_u128 label)
@@ -301,10 +323,13 @@ conntrack_execute(struct conntrack *ct, struct
dp_packet_batch *pkt_batch,
memset(bucket_list, INT8_C(-1), sizeof bucket_list);
for (i = 0; i < cnt; i++) {
+ enum key_status extract_res;
unsigned bucket;
- if (!conn_key_extract(ct, pkts[i], dl_type, &ctxs[i], zone)) {
- write_ct_md(pkts[i], CS_INVALID, zone, 0, OVS_U128_ZERO);
+ extract_res = conn_key_extract(ct, pkts[i], dl_type, &ctxs[i], zone);
+ if (extract_res != KEY_OK) {
+ write_ct_md(pkts[i], key_status_to_cs(extract_res), zone, 0,
+ OVS_U128_ZERO);
continue;
}
@@ -691,8 +716,11 @@ extract_l4_udp(struct conn_key *key, const void *data,
size_t size)
return key->src.port && key->dst.port;
}
-static inline bool extract_l4(struct conn_key *key, const void *data,
- size_t size, bool *related, const void *l3);
+static inline enum key_status extract_l4(struct conn_key *key,
+ const void *data,
+ size_t size,
+ bool *related,
+ const void *l3);
static uint8_t
reverse_icmp_type(uint8_t type)
@@ -722,14 +750,14 @@ reverse_icmp_type(uint8_t type)
* instead and set *related to true. If 'related' is NULL we're
* already processing a nested header and no such recursion is
* possible */
-static inline int
+static inline enum key_status
extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
bool *related)
{
const struct icmp_header *icmp = data;
if (OVS_UNLIKELY(size < ICMP_HEADER_LEN)) {
- return false;
+ return KEY_INVALID;
}
switch (icmp->icmp_type) {
@@ -740,13 +768,15 @@ extract_l4_icmp(struct conn_key *key, const void *data,
size_t size,
case ICMP4_INFOREQUEST:
case ICMP4_INFOREPLY:
if (icmp->icmp_code != 0) {
- return false;
+ return KEY_INVALID;
}
/* Separate ICMP connection: identified using id */
key->src.icmp_id = key->dst.icmp_id = icmp->icmp_fields.echo.id;
key->src.icmp_type = icmp->icmp_type;
key->dst.icmp_type = reverse_icmp_type(icmp->icmp_type);
- break;
+
+ return KEY_OK;
+
case ICMP4_DST_UNREACH:
case ICMP4_TIME_EXCEEDED:
case ICMP4_PARAM_PROB:
@@ -758,41 +788,40 @@ extract_l4_icmp(struct conn_key *key, const void *data,
size_t size,
const char *l3 = (const char *) (icmp + 1);
const char *tail = (const char *) data + size;
const char *l4;
+ enum key_status res;
bool ok;
if (!related) {
- return false;
+ return KEY_INVALID;
}
memset(&inner_key, 0, sizeof inner_key);
inner_key.dl_type = htons(ETH_TYPE_IP);
ok = extract_l3_ipv4(&inner_key, l3, tail - l3, &l4, false);
if (!ok) {
- return false;
+ return KEY_INVALID;
}
/* pf doesn't do this, but it seems a good idea */
if (inner_key.src.addr.ipv4_aligned != key->dst.addr.ipv4_aligned
|| inner_key.dst.addr.ipv4_aligned != key->src.addr.ipv4_aligned) {
- return false;
+ return KEY_INVALID;
}
key->src = inner_key.src;
key->dst = inner_key.dst;
key->nw_proto = inner_key.nw_proto;
- ok = extract_l4(key, l4, tail - l4, NULL, l3);
- if (ok) {
+ res = extract_l4(key, l4, tail - l4, NULL, l3);
+ if (res != KEY_INVALID) {
conn_key_reverse(key);
*related = true;
}
- return ok;
+ return res;
}
default:
- return false;
+ return KEY_INVALID;
}
-
- return true;
}
static uint8_t
@@ -813,7 +842,7 @@ reverse_icmp6_type(uint8_t type)
* instead and set *related to true. If 'related' is NULL we're
* already processing a nested header and no such recursion is
* possible */
-static inline bool
+static inline enum key_status
extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
bool *related)
{
@@ -822,20 +851,22 @@ extract_l4_icmp6(struct conn_key *key, const void *data,
size_t size,
/* All the messages that we support need at least 4 bytes after
* the header */
if (size < sizeof *icmp6 + 4) {
- return false;
+ return KEY_INVALID;
}
switch (icmp6->icmp6_type) {
case ICMP6_ECHO_REQUEST:
case ICMP6_ECHO_REPLY:
if (icmp6->icmp6_code != 0) {
- return false;
+ return KEY_INVALID;
}
/* Separate ICMP connection: identified using id */
key->src.icmp_id = key->dst.icmp_id = *(ovs_be16 *) (icmp6 + 1);
key->src.icmp_type = icmp6->icmp6_type;
key->dst.icmp_type = reverse_icmp6_type(icmp6->icmp6_type);
- break;
+
+ return KEY_OK;
+
case ICMP6_DST_UNREACH:
case ICMP6_PACKET_TOO_BIG:
case ICMP6_TIME_EXCEEDED:
@@ -846,17 +877,18 @@ extract_l4_icmp6(struct conn_key *key, const void *data,
size_t size,
const char *l3 = (const char *) icmp6 + 8;
const char *tail = (const char *) data + size;
const char *l4 = NULL;
+ enum key_status res;
bool ok;
if (!related) {
- return false;
+ return KEY_INVALID;
}
memset(&inner_key, 0, sizeof inner_key);
inner_key.dl_type = htons(ETH_TYPE_IPV6);
ok = extract_l3_ipv6(&inner_key, l3, tail - l3, &l4);
if (!ok) {
- return false;
+ return KEY_INVALID;
}
/* pf doesn't do this, but it seems a good idea */
@@ -864,25 +896,32 @@ extract_l4_icmp6(struct conn_key *key, const void *data,
size_t size,
&key->dst.addr.ipv6_aligned)
|| !ipv6_addr_equals(&inner_key.dst.addr.ipv6_aligned,
&key->src.addr.ipv6_aligned)) {
- return false;
+ return KEY_INVALID;
}
key->src = inner_key.src;
key->dst = inner_key.dst;
key->nw_proto = inner_key.nw_proto;
- ok = extract_l4(key, l4, tail - l4, NULL, l3);
- if (ok) {
+ res = extract_l4(key, l4, tail - l4, NULL, l3);
+ if (res != KEY_INVALID) {
conn_key_reverse(key);
*related = true;
}
- return ok;
+ return res;
}
+ case ND_NEIGHBOR_SOLICIT:
+ case ND_NEIGHBOR_ADVERT:
+ case ND_ROUTER_SOLICIT:
+ case ND_ROUTER_ADVERT:
+ if (icmp6->icmp6_code != 0) {
+ return KEY_INVALID;
+ }
+ /* The packet is valid, but it's not going to be tracked. */
+ return KEY_NO_TRACK;
default:
- return false;
+ return KEY_INVALID;
}
-
- return true;
}
/* Extract l4 fields into 'key', which must already contain valid l3
@@ -894,30 +933,34 @@ extract_l4_icmp6(struct conn_key *key, const void *data,
size_t size,
*
* If 'related' is NULL, it means that we're already parsing a header nested
* in an ICMP error. In this case, we skip checksum and length validation. */
-static inline bool
+static inline enum key_status
extract_l4(struct conn_key *key, const void *data, size_t size, bool *related,
const void *l3)
{
if (key->nw_proto == IPPROTO_TCP) {
return (!related || check_l4_tcp(key, data, size, l3))
- && extract_l4_tcp(key, data, size);
+ ? extract_l4_tcp(key, data, size)
+ : KEY_INVALID;
} else if (key->nw_proto == IPPROTO_UDP) {
return (!related || check_l4_udp(key, data, size, l3))
- && extract_l4_udp(key, data, size);
+ ? extract_l4_udp(key, data, size)
+ : KEY_INVALID;
} else if (key->dl_type == htons(ETH_TYPE_IP)
&& key->nw_proto == IPPROTO_ICMP) {
return (!related || check_l4_icmp(data, size))
- && extract_l4_icmp(key, data, size, related);
+ ? extract_l4_icmp(key, data, size, related)
+ : KEY_INVALID;
} else if (key->dl_type == htons(ETH_TYPE_IPV6)
&& key->nw_proto == IPPROTO_ICMPV6) {
return (!related || check_l4_icmp6(key, data, size, l3))
- && extract_l4_icmp6(key, data, size, related);
+ ? extract_l4_icmp6(key, data, size, related)
+ : KEY_INVALID;
} else {
- return false;
+ return KEY_INVALID;
}
}
-static bool
+static enum key_status
conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
struct conn_lookup_ctx *ctx, uint16_t zone)
{
@@ -930,7 +973,7 @@ conn_key_extract(struct conntrack *ct, struct dp_packet
*pkt, ovs_be16 dl_type,
memset(ctx, 0, sizeof *ctx);
if (!l2 || !l3 || !l4) {
- return false;
+ return KEY_INVALID;
}
ctx->key.zone = zone;
@@ -977,13 +1020,16 @@ conn_key_extract(struct conntrack *ct, struct dp_packet
*pkt, ovs_be16 dl_type,
}
if (ok) {
- if (extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3)) {
+ enum key_status res;
+
+ res = extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3);
+ if (res == KEY_OK) {
ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
- return true;
}
+ return res;
}
- return false;
+ return KEY_INVALID;
}
/* Symmetric */
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 07a1675..a63f031 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -8608,38 +8608,6 @@ OFPT_ECHO_REQUEST (xid=0x0): 0 bytes of payload
OVS_VSWITCHD_STOP
AT_CLEANUP
-AT_SETUP([ofproto-dpif - conntrack - untrackable traffic])
-OVS_VSWITCHD_START
-
-add_of_ports br0 1 2
-
-AT_CHECK([ovs-appctl vlog/set dpif_netdev:dbg vconn:info ofproto_dpif:info])
-
-AT_DATA([flows.txt], [dnl
-ipv6,ct_state=-trk,action=ct(table=0,zone=0)
-ct_state=+trk,action=controller
-])
-
-AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
-
-AT_CAPTURE_FILE([ofctl_monitor.log])
-AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach
--no-chdir --pidfile 2> ofctl_monitor.log])
-
-AT_CHECK([ovs-appctl time/stop])
-
-AT_CHECK([ovs-appctl netdev-dummy/receive p2
'0060970769ea0000860580da86dd6000000000203afffe80000000000000020086fffe0580dafe80000000000000026097fffe0769ea870068bd00000000fe80000000000000026097fffe0769ea01010000860580da'])
-
-OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 1])
-OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
-
-AT_CHECK([cat ofctl_monitor.log], [0], [dnl
-NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=86 ct_state=inv|trk,in_port=2
(via action) data_len=86 (unbuffered)
-icmp6,vlan_tci=0x0000,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src=fe80::200:86ff:fe05:80da,ipv6_dst=fe80::260:97ff:fe07:69ea,ipv6_label=0x00000,nw_tos=0,nw_ecn=0,nw_ttl=255,icmp_type=135,icmp_code=0,nd_target=fe80::260:97ff:fe07:69ea,nd_sll=00:00:86:05:80:da,nd_tll=00:00:00:00:00:00
icmp6_csum:68bd
-])
-
-OVS_VSWITCHD_STOP
-AT_CLEANUP
-
AT_SETUP([ofproto-dpif - conntrack - zones])
OVS_VSWITCHD_START
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 2a7575c..fba5f1f 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -785,6 +785,41 @@
icmpv6,orig=(src=fc00::1,dst=fc00::2,id=<cleared>,type=128,code=0),reply=(src=fc
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([conntrack - neighbor discovery])
+CHECK_CONNTRACK()
+OVS_TRAFFIC_VSWITCHD_START()
+
+dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from
ns1->ns0.
+AT_DATA([flows.txt], [dnl
+table=0,priority=1,action=drop
+table=0,priority=100,ip6,action=ct(table=1)
+dnl
+table=1,priority=100,ip6,action=controller
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl --detach --no-chdir
--pidfile 2> ofctl_monitor.log])
+
+dnl e6:6f:38:26:34:c4 > 33:33:ff:00:00:02, ethertype IPv6 (0x86dd), length 86:
(hlim 255, next-header ICMPv6 (58) payload length: 32) fc00::1 >
ff02::1:ff00:2: [icmp6 sum ok] ICMP6, neighbor solicitation, length 32, who has
fc00::2
+dnl source link-address option (1), length 8 (1): e6:6f:38:26:34:c4
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 LOCAL 'resubmit(,0)'
'3333ff000002e66f382634c486dd6000000000203afffc000000000000000000000000000001ff0200000000000000000001ff00000287002e3e00000000fc0000000000000000000000000000020101e66f382634c4'])
+
+dnl 56:5b:b3:68:8f:09 > e6:6f:38:26:34:c4, ethertype IPv6 (0x86dd), length 86:
(hlim 255, next-header ICMPv6 (58) payload length: 32) fc00::2 > fc00::1:
[icmp6 sum ok] ICMP6, neighbor advertisement, length 32, tgt is fc00::2, Flags
[solicited, override]
+dnl destination link-address option (2), length 8 (1): 56:5b:b3:68:8f:09
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 LOCAL 'resubmit(,0)'
'e66f382634c4565bb3688f0986dd6000000000203afffc000000000000000000000000000002fc000000000000000000000000000001880088ce60000000fc0000000000000000000000000000020201565bb3688f09'])
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN2 (xid=0x0): table_id=1 cookie=0x0 total_len=86
ct_state=new|trk,in_port=LOCAL (via action) data_len=86 (unbuffered)
+icmp6,vlan_tci=0x0000,dl_src=e6:6f:38:26:34:c4,dl_dst=33:33:ff:00:00:02,ipv6_src=fc00::1,ipv6_dst=ff02::1:ff00:2,ipv6_label=0x00000,nw_tos=0,nw_ecn=0,nw_ttl=255,icmp_type=135,icmp_code=0,nd_target=fc00::2,nd_sll=e6:6f:38:26:34:c4,nd_tll=00:00:00:00:00:00
icmp6_csum:2e3e
+NXT_PACKET_IN2 (xid=0x0): table_id=1 cookie=0x0 total_len=86
ct_state=new|trk,in_port=LOCAL (via action) data_len=86 (unbuffered)
+icmp6,vlan_tci=0x0000,dl_src=56:5b:b3:68:8f:09,dl_dst=e6:6f:38:26:34:c4,ipv6_src=fc00::2,ipv6_dst=fc00::1,ipv6_label=0x00000,nw_tos=0,nw_ecn=0,nw_ttl=255,icmp_type=136,icmp_code=0,nd_target=fc00::2,nd_sll=00:00:00:00:00:00,nd_tll=56:5b:b3:68:8f:09
icmp6_csum:88ce
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([conntrack - commit, recirc])
CHECK_CONNTRACK()
OVS_TRAFFIC_VSWITCHD_START()
--
2.10.2
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
