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

Ben Pfaff blp at nicira.com
Fri Jan 4 20:41:37 UTC 2013


Thanks for the reviews, I'll do another round of unit tests and push
this.

On Fri, Jan 04, 2013 at 12:39:19PM -0800, Ethan Jackson wrote:
> 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
> >



More information about the dev mailing list