[ovs-dev] [PATCH] ofproto: Fix crash on flow monitor request with tun_metadata.
Daniele Di Proietto
diproiettod at vmware.com
Wed Dec 28 03:39:52 UTC 2016
nx_put_match() needs a non-NULL tunnel metadata table, otherwise it will
crash if a flow matches on tunnel metadata.
This wasn't handled in ofputil_append_flow_update(), causing a crash
when the controller sent a flow monitor request.
To fix the problem, this commit changes ofputil_append_flow_update() to
behave like ofputil_append_flow_stats_reply().
Since ofputil_append_flow_update() now needs to temporarily modify the
match, this commits also embeds 'struct match' into 'struct
ofputil_flow_update', to be safer. This is more similar to
'struct ofputil_flow_stats'.
A regression test is added and a comment is updated in ovs-ofctl.c
#0 0x000055699bd82fa0 in memcpy_from_metadata (dst=0x7ffc770930d0, src=0x7ffc77093698, loc=0x18) at ../lib/tun-metadata.c:451
#1 0x000055699bd83c2e in metadata_loc_from_match_read (map=0x0, match=0x7ffc77093410, idx=0, mask=0x7ffc77093658, is_masked=0x7ffc77093287) at ../lib/tun-metadata.c:848
#2 0x000055699bd83d9b in tun_metadata_to_nx_match (b=0x55699d3f0300, oxm=0, match=0x7ffc77093410) at ../lib/tun-metadata.c:871
#3 0x000055699bce523d in nx_put_raw (b=0x55699d3f0300, oxm=0, match=0x7ffc77093410, cookie=0, cookie_mask=0) at ../lib/nx-match.c:1052
#4 0x000055699bce5580 in nx_put_match (b=0x55699d3f0300, match=0x7ffc77093410, cookie=0, cookie_mask=0) at ../lib/nx-match.c:1116
#5 0x000055699bd3926f in ofputil_append_flow_update (update=0x7ffc770940b0, replies=0x7ffc77094e00) at ../lib/ofp-util.c:6805
#6 0x000055699bc4b5a9 in ofproto_compose_flow_refresh_update (rule=0x55699d405b40, flags=(NXFMF_INITIAL | NXFMF_ACTIONS), msgs=0x7ffc77094e00) at ../ofproto/ofproto.c:5915
#7 0x000055699bc4b5f6 in ofmonitor_compose_refresh_updates (rules=0x7ffc77094e10, msgs=0x7ffc77094e00) at ../ofproto/ofproto.c:5929
#8 0x000055699bc4bafc in handle_flow_monitor_request (ofconn=0x55699d404090, oh=0x55699d404220) at ../ofproto/ofproto.c:6082
#9 0x000055699bc4f46d in handle_openflow__ (ofconn=0x55699d404090, msg=0x55699d404910) at ../ofproto/ofproto.c:7912
#10 0x000055699bc4f5df in handle_openflow (ofconn=0x55699d404090, ofp_msg=0x55699d404910) at ../ofproto/ofproto.c:8002
#11 0x000055699bc88154 in ofconn_run (ofconn=0x55699d404090, handle_openflow=0x55699bc4f5bc <handle_openflow>) at ../ofproto/connmgr.c:1427
#12 0x000055699bc85934 in connmgr_run (mgr=0x55699d3adb90, handle_openflow=0x55699bc4f5bc <handle_openflow>) at ../ofproto/connmgr.c:363
#13 0x000055699bc422c9 in ofproto_run (p=0x55699d3c85e0) at ../ofproto/ofproto.c:1798
#14 0x000055699bc31ec6 in bridge_run__ () at ../vswitchd/bridge.c:2881
#15 0x000055699bc320a6 in bridge_run () at ../vswitchd/bridge.c:2938
#16 0x000055699bc3784e in main (argc=10, argv=0x7ffc770952c8) at ../vswitchd/ovs-vswitchd.c:111
Fixes: 8d8ab6c2d574 ("tun-metadata: Manage tunnel TLV mapping table on a
per-bridge basis.")
Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
---
include/openvswitch/ofp-util.h | 5 +++--
lib/ofp-print.c | 4 +---
lib/ofp-util.c | 14 ++++++++++---
ofproto/connmgr.c | 10 +++++-----
ofproto/ofproto.c | 12 +++++------
tests/ofproto.at | 45 ++++++++++++++++++++++++++++++++++++++++++
utilities/ovs-ofctl.c | 2 +-
7 files changed, 72 insertions(+), 20 deletions(-)
diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
index 91ff0c2..b197a9a 100644
--- a/include/openvswitch/ofp-util.h
+++ b/include/openvswitch/ofp-util.h
@@ -1083,7 +1083,7 @@ struct ofputil_flow_update {
uint8_t table_id;
uint16_t priority;
ovs_be64 cookie;
- struct match *match;
+ struct match match;
const struct ofpact *ofpacts;
size_t ofpacts_len;
@@ -1095,7 +1095,8 @@ int ofputil_decode_flow_update(struct ofputil_flow_update *,
struct ofpbuf *msg, struct ofpbuf *ofpacts);
void ofputil_start_flow_update(struct ovs_list *replies);
void ofputil_append_flow_update(const struct ofputil_flow_update *,
- struct ovs_list *replies);
+ struct ovs_list *replies,
+ const struct tun_table *);
/* Abstract nx_flow_monitor_cancel. */
uint32_t ofputil_decode_flow_monitor_cancel(const struct ofp_header *);
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 7b7c430..dacaa07 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2371,10 +2371,8 @@ ofp_print_nxst_flow_monitor_reply(struct ds *string,
for (;;) {
char reasonbuf[OFP_FLOW_REMOVED_REASON_BUFSIZE];
struct ofputil_flow_update update;
- struct match match;
int retval;
- update.match = &match;
retval = ofputil_decode_flow_update(&update, &b, &ofpacts);
if (retval) {
if (retval != EOF) {
@@ -2418,7 +2416,7 @@ ofp_print_nxst_flow_monitor_reply(struct ds *string,
ds_put_format(string, " cookie=%#"PRIx64, ntohll(update.cookie));
ds_put_char(string, ' ');
- match_format(update.match, string, OFP_DEFAULT_PRIORITY);
+ match_format(&update.match, string, OFP_DEFAULT_PRIORITY);
if (update.ofpacts_len) {
if (string->string[string->length - 1] != ' ') {
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index b9efd32..156d8d2 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -6720,7 +6720,7 @@ ofputil_decode_flow_update(struct ofputil_flow_update *update,
update->cookie = nfuf->cookie;
update->priority = ntohs(nfuf->priority);
- error = nx_pull_match(msg, match_len, update->match, NULL, NULL, NULL);
+ error = nx_pull_match(msg, match_len, &update->match, NULL, NULL, NULL);
if (error) {
return error;
}
@@ -6782,13 +6782,20 @@ ofputil_start_flow_update(struct ovs_list *replies)
void
ofputil_append_flow_update(const struct ofputil_flow_update *update,
- struct ovs_list *replies)
+ struct ovs_list *replies,
+ const struct tun_table *tun_table)
{
+ struct ofputil_flow_update *update_ =
+ CONST_CAST(struct ofputil_flow_update *, update);
+ const struct tun_table *orig_tun_table;
enum ofp_version version = ofpmp_version(replies);
struct nx_flow_update_header *nfuh;
struct ofpbuf *msg;
size_t start_ofs;
+ orig_tun_table = update->match.flow.tunnel.metadata.tab;
+ update_->match.flow.tunnel.metadata.tab = tun_table;
+
msg = ofpbuf_from_list(ovs_list_back(replies));
start_ofs = msg->size;
@@ -6802,7 +6809,7 @@ ofputil_append_flow_update(const struct ofputil_flow_update *update,
int match_len;
ofpbuf_put_zeros(msg, sizeof *nfuf);
- match_len = nx_put_match(msg, update->match, htonll(0), htonll(0));
+ match_len = nx_put_match(msg, &update->match, htonll(0), htonll(0));
ofpacts_put_openflow_actions(update->ofpacts, update->ofpacts_len, msg,
version);
nfuf = ofpbuf_at_assert(msg, start_ofs, sizeof *nfuf);
@@ -6820,6 +6827,7 @@ ofputil_append_flow_update(const struct ofputil_flow_update *update,
nfuh->event = htons(update->event);
ofpmp_postappend(replies, start_ofs);
+ update_->match.flow.tunnel.metadata.tab = orig_tun_table;
}
struct ofpbuf *
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 1f135a4..854868e 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -2183,14 +2183,12 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule,
if (flags & NXFMF_OWN || ofconn != abbrev_ofconn
|| ofconn->monitor_paused) {
struct ofputil_flow_update fu;
- struct match match;
fu.event = event;
fu.reason = event == NXFME_DELETED ? reason : 0;
fu.table_id = rule->table_id;
fu.cookie = rule->flow_cookie;
- minimatch_expand(&rule->cr.match, &match);
- fu.match = &match;
+ minimatch_expand(&rule->cr.match, &fu.match);
fu.priority = rule->cr.priority;
ovs_mutex_lock(&rule->mutex);
@@ -2206,13 +2204,15 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule,
fu.ofpacts = NULL;
fu.ofpacts_len = 0;
}
- ofputil_append_flow_update(&fu, &ofconn->updates);
+ ofputil_append_flow_update(&fu, &ofconn->updates,
+ ofproto_get_tun_tab(rule->ofproto));
} else if (!ofconn->sent_abbrev_update) {
struct ofputil_flow_update fu;
fu.event = NXFME_ABBREV;
fu.xid = abbrev_xid;
- ofputil_append_flow_update(&fu, &ofconn->updates);
+ ofputil_append_flow_update(&fu, &ofconn->updates,
+ ofproto_get_tun_tab(rule->ofproto));
ofconn->sent_abbrev_update = true;
}
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 58295a1..5f21efc 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5885,12 +5885,12 @@ handle_barrier_request(struct ofconn *ofconn, const struct ofp_header *oh)
static void
ofproto_compose_flow_refresh_update(const struct rule *rule,
enum nx_flow_monitor_flags flags,
- struct ovs_list *msgs)
+ struct ovs_list *msgs,
+ const struct tun_table *tun_table)
OVS_REQUIRES(ofproto_mutex)
{
const struct rule_actions *actions;
struct ofputil_flow_update fu;
- struct match match;
fu.event = (flags & (NXFMF_INITIAL | NXFMF_ADD)
? NXFME_ADDED : NXFME_MODIFIED);
@@ -5901,8 +5901,7 @@ ofproto_compose_flow_refresh_update(const struct rule *rule,
ovs_mutex_unlock(&rule->mutex);
fu.table_id = rule->table_id;
fu.cookie = rule->flow_cookie;
- minimatch_expand(&rule->cr.match, &match);
- fu.match = &match;
+ minimatch_expand(&rule->cr.match, &fu.match);
fu.priority = rule->cr.priority;
actions = flags & NXFMF_ACTIONS ? rule_get_actions(rule) : NULL;
@@ -5912,7 +5911,7 @@ ofproto_compose_flow_refresh_update(const struct rule *rule,
if (ovs_list_is_empty(msgs)) {
ofputil_start_flow_update(msgs);
}
- ofputil_append_flow_update(&fu, msgs);
+ ofputil_append_flow_update(&fu, msgs, tun_table);
}
void
@@ -5926,7 +5925,8 @@ ofmonitor_compose_refresh_updates(struct rule_collection *rules,
enum nx_flow_monitor_flags flags = rule->monitor_flags;
rule->monitor_flags = 0;
- ofproto_compose_flow_refresh_update(rule, flags, msgs);
+ ofproto_compose_flow_refresh_update(rule, flags, msgs,
+ ofproto_get_tun_tab(rule->ofproto));
}
}
diff --git a/tests/ofproto.at b/tests/ofproto.at
index d360b7a..ad8df28 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -5640,3 +5640,48 @@ NXST_FLOW reply:
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+AT_SETUP([ofproto - monitor flows with tun_md])
+OVS_VSWITCHD_START
+
+AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0,len=4}->tun_metadata0"])
+AT_CHECK([ovs-ofctl add-flow br0 tun_metadata0=0x1,actions=drop])
+
+AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip], [0], [dnl
+NXST_FLOW reply:
+ tun_metadata0=0x1 actions=drop
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+dnl Usually ovs-ofctl monitor outputs on stderr, but the first message here
+dnl is put on stdout, because it is handled by ofctl in dump_transaction()
+dnl and not in monitor_vconn().
+AT_CHECK([ovs-ofctl monitor br0 65534 watch: --detach --no-chdir --pidfile >ofctl_monitor.log 2>&1])
+
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 2])
+
+AT_CHECK([cat ofctl_monitor.log | ofctl_strip], [0], [dnl
+NXST_FLOW_MONITOR reply:
+ event=ADDED table=0 cookie=0 tun_metadata0=0x1
+])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 4])
+
+AT_CHECK([cat ofctl_monitor.log | ofctl_strip], [0], [dnl
+NXST_FLOW_MONITOR reply:
+ event=ADDED table=0 cookie=0 tun_metadata0=0x1
+NXST_FLOW_MONITOR reply:
+ event=DELETED reason=delete table=0 cookie=0 tun_metadata0=0x1
+])
+
+OVS_APP_EXIT_AND_WAIT([ovs-ofctl])
+
+dnl Check that vswitchd hasn't crashed
+AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip], [0], [dnl
+NXST_FLOW reply:
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 7f5bb61..25e2b30 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -1811,7 +1811,7 @@ ofctl_unblock(struct unixctl_conn *conn, int argc OVS_UNUSED,
}
}
-/* Prints to stdout all of the messages received on 'vconn'.
+/* Prints to stderr all of the messages received on 'vconn'.
*
* Iff 'reply_to_echo_requests' is true, sends a reply to any echo request
* received on 'vconn'.
--
2.10.2
More information about the dev
mailing list