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 <preddy@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>

diff --git a/lib/rconn.c b/lib/rconn.c
index 67ea86c..9b6cd86 100644 (file)
--- 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 f4e9a9e..e6058f3 100644 (file)
--- 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 e0223cd..ea1e130 100644 (file)
--- 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 81bdcc9..b15388c 100644 (file)
--- 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 7794043..4d8c39f 100644 (file)
--- 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);
 }