[ovs-dev] [bug 14265 3/3] rconn: Fix null pointer dereference in rconn_add_monitor().

Ethan Jackson ethan at nicira.com
Fri Jan 4 20:39:19 UTC 2013


Acked-by: Ethan Jackson <ethan at nicira.com>


On Wed, Jan 2, 2013 at 5:10 PM, Ben Pfaff <blp at 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 <preddy at nicira.com>
> Signed-off-by: Ben Pfaff <blp at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130104/c463332f/attachment-0003.html>


More information about the dev mailing list