[ovs-git] [openvswitch/ovs] 476d25: rconn: Introduce new invariant to fix assertion fa...

[GitHub]

  Branch: refs/heads/master
  Home:   https://github.com/openvswitch/ovs
  Commit: 476d2551abd2871696a64203f78d658ac2d7f32c
      https://github.com/openvswitch/ovs/commit/476d2551abd2871696a64203f78d658ac2d7f32c
  Author: Ben Pfaff <blp at ovn.org>
  Date:   2018-05-23 (Wed, 23 May 2018)

  Changed paths:
    M lib/learning-switch.c
    M lib/rconn.c
    M lib/vconn.c

  Log Message:
  -----------
  rconn: Introduce new invariant to fix assertion failure in corner case.

Until now, rconn_get_version() has only reported the OpenFlow version in
use when the rconn is actually connected.  This makes sense, but it has a
harsh consequence.  Consider code like this:

    if (rconn_is_connected(rconn) && rconn_get_version(rconn) >= 0) {
  for (int i = 0; i < 2; i++) {
      struct ofpbuf *b = ofputil_encode_echo_request(
          rconn_get_version(rconn));
      rconn_send(rconn, b, NULL);
  }
    }

Maybe not the smartest code in the world, and probably no one would write
this exact code in any case, but it doesn't look too risky or crazy.

But it is.  The second trip through the loop can assert-fail inside
ofputil_encode_echo_request() because rconn_get_version(rconn) returns -1
instead of a valid OpenFlow version.  That happens if the first call to
rconn_send() encounters an error while sending the message and therefore
destroys the underlying vconn and disconnects so that rconn_get_version()
doesn't have a vconn to query for its version.

In a case like this where all the code to send the messages is close by, we
could just check rconn_get_version() in each loop iteration.  We could even
go through the tree and convince ourselves that individual bits of code are
safe, or be conservative and check rconn_get_version() >= 0 in the iffy
cases.  But this seems to me like an ongoing source of risk and a way to
get things wrong in corner cases.

This commit takes a different approach.  It introduces a new invariant: if
an rconn has ever been connected, then it returns a valid OpenFlow version
from rconn_get_version().  In addition, if an rconn is currently connected,
then the OpenFlow version it returns is the correct one (that may be
obvious, but there were corner cases before where it returned -1 even
though rconn_is_connected() returned true).

With this commit, the code above would work OK.  If the first call to
rconn_send() encounters an error sending the message, then
rconn_get_version() in the second iteration will return the same value as
in the first iteration.  The message passed to rconn_send() will end up
being discarded, but that's much better than either an assertion failure or
having to carefully analyze a lot of our code to deal with one unusual
corner case.

Reported-by: Han Zhou <zhouhan at gmail.com>
Signed-off-by: Ben Pfaff <blp at ovn.org>
Acked-by: Han Zhou <hzhou8 at ebay.com>



      **NOTE:** This service been marked for deprecation: https://developer.github.com/changes/2018-04-25-github-services-deprecation/

      Functionality will be removed from GitHub.com on January 31st, 2019.