From 4766ce7a6438c11743c354fddad9d1164c76c467 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 4 Jan 2013 12:41:01 -0800 Subject: [PATCH] rconn: Fix null pointer dereference in rconn_add_monitor(). rconn_add_monitor() tries to check the version of the controller connection being monitored, so that it can decide what OpenFlow version to tell the monitor connection to negotiate. But at any given time an rconn may not have a controller connection (e.g. during backoff), so rc->vconn may be null and thus vconn_get_version(rc->vconn) dereferences a null pointer. Fixing the problem in a local way would require the rconn to remember the previous version negotiated, and that fails if the rconn hasn't yet connected or if the next connection negotiates a new version. This commit instead adds the ability to a vconn to accept any OpenFlow message version and modifies "ovs-ofctl snoop" to use that feature, thus removing the need to negotiate the "correct" version on snoops. Bug #14265. Reported-by: Pratap Reddy Signed-off-by: Ben Pfaff Acked-by: Ethan Jackson --- lib/rconn.c | 8 -------- lib/vconn-provider.h | 13 +++++++++---- lib/vconn.c | 23 ++++++++++++++++++++++- lib/vconn.h | 8 ++++++-- utilities/ovs-ofctl.c | 18 ++++++++++++------ 5 files changed, 49 insertions(+), 21 deletions(-) diff --git a/lib/rconn.c b/lib/rconn.c index 67ea86c0e..9b6cd8638 100644 --- a/lib/rconn.c +++ b/lib/rconn.c @@ -672,14 +672,6 @@ void rconn_add_monitor(struct rconn *rc, struct vconn *vconn) { if (rc->n_monitors < ARRAY_SIZE(rc->monitors)) { - int version = vconn_get_version(rc->vconn); - - /* Override the allowed versions of the snoop vconn so that - * only the version of the controller connection is allowed. - * This is because the snoop will see the same messages as the - * controller */ - vconn_set_allowed_versions(vconn, 1u << version); - VLOG_INFO("new monitor connection from %s", vconn_get_name(vconn)); rc->monitors[rc->n_monitors++] = vconn; } else { diff --git a/lib/vconn-provider.h b/lib/vconn-provider.h index f4e9a9e4d..e6058f335 100644 --- a/lib/vconn-provider.h +++ b/lib/vconn-provider.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2012 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2012, 2013 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -33,13 +33,18 @@ struct vconn { struct vconn_class *class; int state; int error; - uint32_t allowed_versions; - uint32_t peer_versions; - enum ofp_version version; + + /* OpenFlow versions. */ + uint32_t allowed_versions; /* Bitmap of versions we will accept. */ + uint32_t peer_versions; /* Peer's bitmap of versions it will accept. */ + enum ofp_version version; /* Negotiated version (or 0). */ + bool recv_any_version; /* True to receive a message of any version. */ + ovs_be32 remote_ip; ovs_be16 remote_port; ovs_be32 local_ip; ovs_be16 local_port; + char *name; }; diff --git a/lib/vconn.c b/lib/vconn.c index e0223cd5e..ea1e13012 100644 --- a/lib/vconn.c +++ b/lib/vconn.c @@ -394,6 +394,27 @@ vconn_get_version(const struct vconn *vconn) return vconn->version ? vconn->version : -1; } +/* By default, a vconn accepts only OpenFlow messages whose version matches the + * one negotiated for the connection. A message received with a different + * version is an error that causes the vconn to drop the connection. + * + * This functions allows 'vconn' to accept messages with any OpenFlow version. + * This is useful in the special case where 'vconn' is used as an rconn + * "monitor" connection (see rconn_add_monitor()), that is, where 'vconn' is + * used as a target for mirroring OpenFlow messages for debugging and + * troubleshooting. + * + * This function should be called after a successful vconn_open() or + * pvconn_accept() but before the connection completes, that is, before + * vconn_connect() returns success. Otherwise, messages that arrive on 'vconn' + * beforehand with an unexpected version will the vconn to drop the + * connection. */ +void +vconn_set_recv_any_version(struct vconn *vconn) +{ + vconn->recv_any_version = true; +} + static void vcs_connecting(struct vconn *vconn) { @@ -600,7 +621,7 @@ vconn_recv(struct vconn *vconn, struct ofpbuf **msgp) if (!retval) { retval = do_recv(vconn, &msg); } - if (!retval) { + if (!retval && !vconn->recv_any_version) { const struct ofp_header *oh = msg->data; if (oh->version != vconn->version) { enum ofptype type; diff --git a/lib/vconn.h b/lib/vconn.h index 81bdcc976..b15388c0e 100644 --- a/lib/vconn.h +++ b/lib/vconn.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -38,14 +38,18 @@ int vconn_open(const char *name, uint32_t allowed_versions, uint8_t dscp, struct vconn **vconnp); void vconn_close(struct vconn *); const char *vconn_get_name(const struct vconn *); + uint32_t vconn_get_allowed_versions(const struct vconn *vconn); void vconn_set_allowed_versions(struct vconn *vconn, uint32_t allowed_versions); +int vconn_get_version(const struct vconn *); +void vconn_set_recv_any_version(struct vconn *); + ovs_be32 vconn_get_remote_ip(const struct vconn *); ovs_be16 vconn_get_remote_port(const struct vconn *); ovs_be32 vconn_get_local_ip(const struct vconn *); ovs_be16 vconn_get_local_port(const struct vconn *); -int vconn_get_version(const struct vconn *); + int vconn_connect(struct vconn *); int vconn_recv(struct vconn *, struct ofpbuf **); int vconn_send(struct vconn *, struct ofpbuf *); diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index 779404319..4d8c39f2c 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -368,21 +368,23 @@ open_vconn_socket(const char *name, struct vconn **vconnp) return error; } +enum open_target { MGMT, SNOOP }; + static enum ofputil_protocol -open_vconn__(const char *name, const char *default_suffix, +open_vconn__(const char *name, enum open_target target, struct vconn **vconnp) { + const char *suffix = target == MGMT ? "mgmt" : "snoop"; char *datapath_name, *datapath_type, *socket_name; enum ofputil_protocol protocol; char *bridge_path; int ofp_version; int error; - bridge_path = xasprintf("%s/%s.%s", ovs_rundir(), name, default_suffix); + bridge_path = xasprintf("%s/%s.%s", ovs_rundir(), name, suffix); ofproto_parse_name(name, &datapath_name, &datapath_type); - socket_name = xasprintf("%s/%s.%s", - ovs_rundir(), datapath_name, default_suffix); + socket_name = xasprintf("%s/%s.%s", ovs_rundir(), datapath_name, suffix); free(datapath_name); free(datapath_type); @@ -399,6 +401,10 @@ open_vconn__(const char *name, const char *default_suffix, ovs_fatal(0, "%s is not a bridge or a socket", name); } + if (target == SNOOP) { + vconn_set_recv_any_version(*vconnp); + } + free(bridge_path); free(socket_name); @@ -421,7 +427,7 @@ open_vconn__(const char *name, const char *default_suffix, static enum ofputil_protocol open_vconn(const char *name, struct vconn **vconnp) { - return open_vconn__(name, "mgmt", vconnp); + return open_vconn__(name, MGMT, vconnp); } static void @@ -1456,7 +1462,7 @@ ofctl_snoop(int argc OVS_UNUSED, char *argv[]) { struct vconn *vconn; - open_vconn__(argv[1], "snoop", &vconn); + open_vconn__(argv[1], SNOOP, &vconn); monitor_vconn(vconn); } -- 2.45.2