Acked-by: Ethan Jackson <et...@nicira.com>
On Wed, Jan 2, 2013 at 5:10 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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 <pre...@nicira.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> 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 7a2bcf5..1ea022b 100644
> --- a/lib/rconn.c
> +++ b/lib/rconn.c
> @@ -674,14 +674,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 a26d9c5..c92eb63 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 a3792ec..6f318a9 100644
> --- a/lib/vconn.c
> +++ b/lib/vconn.c
> @@ -395,6 +395,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)
> {
> @@ -601,7 +622,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
> --- 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 bc2773e..96c4741 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);
> }
>
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
