Jesse Gross [Wed, 2 Mar 2011 01:56:01 +0000 (17:56 -0800)]
flow: Avoid assertion failure on invalid IPv6 packet.
We compute the length of the IPv6 header by parsing all of the
extension headers that we know about. However, the final result
is checked using ofpbuf_pull(), which checks the size with an
assertion. Since the length of the final header is not checked
in any other way an invalid packet can trigger this assertion.
Ben Pfaff [Tue, 1 Mar 2011 23:33:40 +0000 (15:33 -0800)]
datapath: Avoid duplicate test in tnl_free_linked_skbs().
I don't see the value in testing 'skb' for nonnull twice. This is only
called on error paths, so it seems really doubtful to me that there is any
value in the "unlikely", either. (It could be put into the "while"
expression if it is valuable.)
Signed-off-by: Ben Pfaff <blp@nicira.com> Acked-by: Jesse Gross <jesse@nicira.com>
Ben Pfaff [Fri, 28 Jan 2011 23:32:57 +0000 (15:32 -0800)]
stream-ssl: Only set SSL client session once.
Attempting to call SSL_set_session() on every trip through the SSL
connection state machine seems like it could cause the session to be
re-set to the cached one even after the server has told us which session
is actually to be used.
In testing, this change didn't make any difference, but it seems seems like
the right thing to do.
Ben Pfaff [Tue, 22 Feb 2011 21:17:51 +0000 (13:17 -0800)]
ofp-util: Make NXM required for 64-bit cookies in is_nxm_required().
Only NXM supports 64-bit cookies, but this code didn't properly check
for that. This commit fixes the problem and makes the code much more
explicit about what it is checking.
This will hide bug #4566, but the following commit actually fixes it.
Andrew Evans [Tue, 1 Mar 2011 02:26:04 +0000 (18:26 -0800)]
xenserver: Add support for disabling in-band management via XAPI.
Allow users or applications to enable or disable in-band management of
individual bridges by setting the 'vswitch-disable-in-band' key in the
'other_config' attribute of the corresponding network to 'true' or
'false'.
Valient Gough [Fri, 25 Feb 2011 05:45:18 +0000 (21:45 -0800)]
datapath: call tnl_ops only after setting options
Calling methods in tnl_ops should be done only after initializing the tunnel
configuration options. This patch moves the hdr_len call after the output key
setup.
Jesse Gross [Thu, 24 Feb 2011 22:07:29 +0000 (14:07 -0800)]
datapath: Destroy internal devices before freeing datapath.
When destroying vports we account for two types of synchronization
mechanisms: RTNL and RCU. However, it is possible to call into
network device methods with just a device reference without either
of these. These device methods can use the datapath data structures
but we don't wait for all of the references to go away before freeing
the datapath. The actual wait happens in rtnl_unlock(), so by moving
up that call we can avoid the possibility of use after free with
internal devices.
Signed-off-by: Jesse Gross <jesse@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Jesse Gross [Thu, 24 Feb 2011 22:16:24 +0000 (14:16 -0800)]
datapath: Don't free vport until all references are gone.
We currently call vport_free() for internal devices after the
device is unregistered. This takes care of callers that use
either RTNL or RCU but not ones that have only a device reference.
In particular, if stats are requested while a datapath is being
unregistered we can try to use the vport data structures which
have already been freed.
Bug #4736
Reported-by: Brad Hall <brad@nicira.com> Signed-off-by: Jesse Gross <jesse@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Ethan Jackson [Thu, 24 Feb 2011 20:38:30 +0000 (12:38 -0800)]
ofproto: Reset facet's rs_used at rule changes.
When a facet changes rules it's 'used' timer is set to the new
rule's created time. This is possibly before the time stored in
'rs_used' which could cause an assertion failure in
facet_push_stats().
Jesse Gross [Thu, 24 Feb 2011 04:04:31 +0000 (20:04 -0800)]
datapath: Memset executed packet's fields using memset.
Currently we explicitly zero out each of the fields in the OVS_CB for
executed packets. However, it seems simpler and more robust to just
memset the whole thing to zero.
Signed-off-by: Jesse Gross <jesse@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Ben Pfaff [Wed, 23 Feb 2011 23:43:34 +0000 (15:43 -0800)]
util: Make out_of_memory() call abort() instead of exit(EXIT_FAILURE).
exit(EXIT_FAILURE) will make a monitoring process (the one created by
--monitor) think that it should exit. But the most likely reason for
out_of_memory() to be called is a bug: probably, the process is trying
to allocate more memory than there is available address space, e.g.
something like malloc(-1). So it's better, in my opinion, to call abort()
instead, so that the monitor process restarts the daemon and we are more
likely to stay alive and, in addition, get a core dump and a useful bug
report.
I decided to implement a new general-purpose function for this purpose in
case we run into other similar situations in the future.
(I haven't actually run into this problem in practice. This commit is
just speculation about what is better behavior.)
Ben Pfaff [Wed, 23 Feb 2011 20:20:38 +0000 (12:20 -0800)]
ovs-vsctl: Simplify parse_column_key_value().
Coverity pointed out some inconsistencies on tests for whether columnp and
keyp were nonnull. These tests were, at best, confusing, but in fact every
caller always passed nonnull for both parameters, so this commit drops all
of those tests.
Ben Pfaff [Wed, 23 Feb 2011 20:42:44 +0000 (12:42 -0800)]
ovsdb-idl: JSONRPC_REPLY message always has nonnull 'id'.
A JSONRPC_REPLY message always have a nonnull 'id' member, as ensured by
jsonrpc_msg_is_valid(). Checking for NULL here confused Coverity into
believing that the call to ovsdb_idl_txn_process_reply() just below could
cause a null pointer dereference, since ovsdb_idl_txn_process_reply() uses
the 'id' member without checking it for null.
Ben Pfaff [Wed, 23 Feb 2011 20:35:32 +0000 (12:35 -0800)]
rconn: Clarify rconn_run_wait().
At first glance the vconn_wait() call looks risky because this function
checked whether rc->vconn is nonnull at the top. In fact it's OK because
rc->state will be S_ACTIVE or S_IDLE only if rc->vconn is nonnull, but
there's no harm in putting that check inside the block that only runs if
rc->vconn is nonnull.
Ben Pfaff [Wed, 23 Feb 2011 19:29:37 +0000 (11:29 -0800)]
stream: Make stream_report_content() tolerate negative size.
A negative size probably means that a system call failed. The caller could
set that to 0 but we might as well just tolerate it in
stream_report_content() by making the parameter type signed.
Justin Pettit [Wed, 23 Feb 2011 18:39:21 +0000 (10:39 -0800)]
ovsdb: Warn when result of ovsdb_txn_commit() is ignored and fix caller.
ovsdb_txn_commit() may return a ovsdb_error structure, which should be
freed by the caller. The only remaining caller that discards the result
is in ovsdb_file_open__(), which this fixes.
Justin Pettit [Wed, 23 Feb 2011 03:07:05 +0000 (19:07 -0800)]
nx-match: Don't fall through case when handling dest mcast mac addresses.
When an NXM wildcard entry that includes a multicast address is parsed,
it would fall through to the next case statement, which would also set
an inappropriate source mac address match.
Justin Pettit [Wed, 23 Feb 2011 01:50:19 +0000 (17:50 -0800)]
netdev-linux: Zero-out "sin" in netdev_linux_arp_lookup().
Coverity complains that we're copying the unitialized "sin_zero" member
from "sin" into "r". I don't think this is an actual problem, but
there's no harm in zeroing out the structure, either.
Ben Pfaff [Wed, 23 Feb 2011 00:24:19 +0000 (16:24 -0800)]
multipath: Validate multipath actions more thoroughly in multipath_parse().
The stricter validation requires updates to the calls to test-multipath
to supply a valid n_links value. test-multipath doesn't actually use
that value (it runs over different values in an internal "for" loop), so
this doesn't change any behavior.
Also adds a test to exercise each possible multipath_parse() error message.
Ben Pfaff [Tue, 22 Feb 2011 22:55:39 +0000 (14:55 -0800)]
ofp-parse: Don't segfault when an OpenFlow action's argument is missing.
Some actions checked that 'arg' was nonnull before attempting to parse it
but a lot of them didn't. This commit avoids the segfault by substituting
an empty string when no argument is given. It also updates a few of the
action implementations to correspond.
Ben Pfaff [Tue, 22 Feb 2011 18:58:36 +0000 (10:58 -0800)]
util: New function ovs_strzcpy().
Static analyzers hate strncpy(). This new function shares its property of
initializing an entire buffer, without its nasty habit of failing to
null-terminate long strings.
Ben Pfaff [Tue, 22 Feb 2011 18:41:15 +0000 (10:41 -0800)]
util: Don't read over 'size - 1' bytes of source string in ovs_strlcpy().
The blind replacement of strncpy() by ovs_strlcpy() is risky because
strncpy() never reads more bytes from its source string than necessary to
write its destination string, but ovs_strlcpy() and the OpenBSD function
that inspired it both read the entire source string. This avoids that
problem.
Given that change, we can use ovs_strlcpy() in a few more places, and
this commit does that too.
Justin Pettit [Tue, 22 Feb 2011 01:02:35 +0000 (17:02 -0800)]
ovsdb: Don't check "date" before assignment in ovsdb_file_txn_from_json().
There's no indication that "date" is optional in the description of
ovsdb_file_txn_from_json(), and the one caller always passes it in, so
don't bother checking whether it exists.
Andrew Evans [Thu, 17 Feb 2011 06:40:46 +0000 (22:40 -0800)]
xenserver: Split kernel/userspace into separate RPMs for supplemental packs.
XenServer has the concept of a "supplemental pack", which is just a collection
of RPMs rolled into an ISO image for installation as a unit, either during
XenServer installation or later as an update.
The script used to build supplemental packs requires that a single RPM not mix
kernel and userspace components. It also enforces a particular naming scheme
for kernel module RPMs. This commit modifies the OVS RPM spec file to create
separate RPMs in accordance with these conventions.
In an attempt to ensure that the userspace and kernel module packages are both
present and compatible, the kernel package provides a fictitious capability,
openvswitch_mod.ko.0, and the userspace package requires that capability. If a
future change breaks backward compatibility between userspace and kernel, we'll
increment the numeric suffix, preventing mismatched installs.
Ben Pfaff [Sat, 19 Feb 2011 04:53:51 +0000 (20:53 -0800)]
datapath: Properly initialize ovs_skb_cb of packet from userspace.
The ovs_skb_cb in 'packet' in this function is initially a clone of the
corresponding area in 'skb', which came from the Netlink layer and thus
isn't necessarily all-zeros. This commit initializes it properly before
passing it along to execute_actions().
The most common problem caused by failing to initialize the ovs_skb_cb
properly was that on Linux 2.6.26 and earlier, where Open vSwitch keeps
its own vlan_tci field inside ovs_skb_cb, the first packet of a flow would
get sent out tagged with a random VLAN (usually 0x0001 or 0xffff in our
testing). This commit should fix that problem.
Another likely problem would be for turning on sFlow to randomly panic the
kernel. That problem would not be kernel version dependent. We haven't
been testing sFlow so we haven't noticed this problem.
Andrew Evans [Thu, 17 Feb 2011 20:51:34 +0000 (12:51 -0800)]
xenserver: Revert only the XenServer scripts no longer replaced by OVS.
Commit d66880ee (xenserver: Clean up /usr/sbin/brctl dangling symlink.)
attempted to revert the original XenServer-shipped versions of scripts replaced
by OVS during an RPM upgrade, but the logic was incorrect. It assumed that
%postun of the package being replaced was run before the %post of the new
version replacing it. The reverse is actually true.
Make upgrade and erase cases both work correctly by simply checking whether any
of the files ever replaced in any OVS version are dangling symlinks, and if so,
attempt to copy the saved XenServer original back to its rightful place. In the
upgrade case, if the newly-installed version of OVS lacks any of the scripts in
the previous version, those will be reverted. In the erase case, none of the
OVS replacements will exist, so they will all be dangling symlinks and will all
be reverted.
Furthermore, replace any dangling symlink from /usr/sbin/xen-bugtool to the
now-nonexistent OVS replacement (caused by commit 92dbd5c9 (xenserver: Replace
customized xen-bugtool with plugin to collect qdisc info.)).
Lastly, prevent accidental reversion of files replaced by OVS during rpm -U
(also caused by commit 92dbd5c9).
Ethan Jackson [Fri, 11 Feb 2011 00:35:48 +0000 (16:35 -0800)]
ofproto: Resubmit Statistics.
This patch causes statistics to be updated for rules which are
resubmitted into. Once per second statistics are queried from the
datapath and pushed along the resubmit graph (calculated on demand
from the action list). This approach is simple, easy to understand,
and in most cases accurate. However, when the resubmit graph
changes, it is possible that some statistics will be accounted to
the wrong rule for a short period of time.
Ethan Jackson [Wed, 9 Feb 2011 21:18:51 +0000 (13:18 -0800)]
ofproto: Batch statistics updates.
Facet statistics are updated once per second during
ofproto_expire() instead of upon request. This will greatly
simplify implementation of future patches. This commit also changes
each facet's packet and byte counters to include the statistics
stored in the datapath.
Andrew Evans [Fri, 18 Feb 2011 17:50:12 +0000 (09:50 -0800)]
ovsdb: Remove 'managers' column from 'Open vSwitch' table.
We had retained but deprecated the use of the older 'managers' column in the
'Open vSwitch' table for compatibility with applications that might still use
it, but that created more problems than it solved. This commit removes the
'managers' column from the schema, and removes all references to it from the
code, init scripts, documentation, and tests.
Ben Pfaff [Thu, 17 Feb 2011 23:04:44 +0000 (15:04 -0800)]
ofproto: Fix behavior when special_cb not present (as with ovs-openflowd).
The special_cb hook allows ofproto's client to intercept packets. When
it is not present (as in ovs-openflowd), no packets need to be intercepted,
but ofproto implemented the opposite behavior. This fixes the problem.
Ben Pfaff [Fri, 4 Feb 2011 23:10:27 +0000 (15:10 -0800)]
vconn-stream: Make definitions of tcp_vconn_class, etc., easier to find.
When object definitions are entirely hidden inside macros with token
pasting tricks, tools like "tags" and even "grep" cannot find them. This
change makes the definitions of tcp_vconn_class and other stream-based
vconns easier to find, by exposing their definitions to these tools.
Ben Pfaff [Tue, 15 Feb 2011 18:17:44 +0000 (10:17 -0800)]
Avoid unneeded database compaction at startup, and improve backups.
Until now, Open vSwitch "start" has always converted the database to the
current database schema. This compacts the database, which as a side
effect throws away useful information about the transactions that were
executed to bring the database into its current state. This can make
debugging database-related problems more difficult.
This commit changes the "start" command to only convert the database if
the database schema has changed. It also adds the database checksum to
the backup file name, to avoid overwriting backups in the case where the
checksum changed but the developer neglected to update the version number.
I tested an earlier version of the xenserver changes but not any version
of the Debian changes.
Ben Pfaff [Tue, 8 Feb 2011 23:36:21 +0000 (15:36 -0800)]
ovsdb: New function ovsdb_file_read_schema() for reading schema from db.
This new function saves reading the whole database when only the schema is
of interest. This commit adapts ovsdb-tool to use it for the "db-version"
command. Upcoming commits will introduce another caller.
Ethan Jackson [Tue, 15 Feb 2011 01:28:56 +0000 (17:28 -0800)]
ovs-bugtool: Ignore deprecation warnings.
Since we have to support many versions of Python, this commit
forces ovs-bugtool to ignore deprecation warnings. They were
complaining about use of the md5 module whose replacement does not
appear until Python 2.5. In Python 2.7 deprecation warnings are
ignored by default.
Andrew Evans [Fri, 11 Feb 2011 19:29:33 +0000 (11:29 -0800)]
xenserver: Replace customized xen-bugtool with plugin to collect qdisc info.
XenServer RPMs have installed a modified /usr/sbin/xen-bugtool that is
identical to upstream except for an additional command to collect tc qdisc
class configuration for each network interface. Since xen-bugtool has an
extension mechanism, use that instead of maintaining a parallel patched
xen-bugtool.
This does change the content of the bug report slightly. Instead of one file
per network interface, qdisc class info for all interfaces is written to one
file. This is an artifact of the xen-bugtool extension mechanism: a single
extension command's output must go to one file, and it is impractical to create
individual <command> tags for each interface dynamically.
Andrew Evans [Mon, 14 Feb 2011 21:02:43 +0000 (13:02 -0800)]
xenserver: Clean up /usr/sbin/brctl dangling symlink.
Commit 54f16a10 removed Open vSwitch's /usr/sbin/brctl replacement for
XenServer, but because the RPM doesn't restore the original files on upgrade,
an upgrade leaves /usr/sbin/brctl a broken symlink to the removed
/usr/share/openvswitch/scripts/brctl.
This commit adds a one-time cleanup to restore the original XenServer-supplied
/usr/sbin/brctl. It also changes the RPM to restore all the replaced files with
their XenServer originals at post-uninstall time during an upgrade transaction,
just as uninstall has done up to now. This should prevent the same problem in
the future.
Ben Pfaff [Fri, 11 Feb 2011 21:16:28 +0000 (13:16 -0800)]
Remove /proc/net compatibility support.
This feature was included only to allow Citrix QA to run some tests that
interacted directly with the bridge. This feature hasn't been turned on
for some time, so it should not be necessary any longer.
Bob Ball [Fri, 11 Feb 2011 20:35:38 +0000 (12:35 -0800)]
xenserver: Don't create XAPI cache at install.
There are two mechanisms of installing a supplemental pack – one is
after installation (using the xe-install-supplemental-pack command) and
one is during installation by either creating a new install ISO with the
supplemental pack bundled or creating an HTTP directory which serves the
install files and the new supplemental pack contents.
The RPM should support both methods, but currently attempts to backup
data that doesn't exist yet. With this commit, the XAPI cache is not
created at install.
Signed-off-by: Bob Ball <bob.ball@citrix.com>
[removed sourcing /etc/xensource-inventory since it is no longer needed] Signed-off-by: Justin Pettit <jpettit@nicira.com>
Justin Pettit [Fri, 11 Feb 2011 20:35:37 +0000 (12:35 -0800)]
xenserver: Rename ovs-external-ids to ovs-xapi-sync.
With commit 5692e3 (xenserver: Set fail_mode on internal bridges.), the
responsibilities of ovs-external-ids got expanded beyond just monitoring
external-ids. This commit renames the script to more accurately
describe its job.
Justin Pettit [Fri, 11 Feb 2011 20:35:35 +0000 (12:35 -0800)]
xenserver: Remove support for XenServer versions older than 5.6 FP1.
Commit 404c169247b1c3ac2ebad887f0421478a6cef924 breaks compatibility with
XenServers older than 5.6 FP1. This commit removes the last vestiges of
support for those older XenServer versions.
Justin Pettit [Fri, 11 Feb 2011 20:35:36 +0000 (12:35 -0800)]
xenserver: Remove brctl wrapper script.
The OVS "brctl" wrapper script was added to aid Citrix's QA and help
XenCenter display link status and statistics of bridges. Since the Citrix
integrated OVS ships without this wrapper script, it's clearly no longer
needed in the versions of XenServer we now support.
Andrew Evans [Fri, 11 Feb 2011 00:50:49 +0000 (16:50 -0800)]
xenserver: Merge upstream changes and drop pre-5.6.100 support.
Citrix have made modifications to the various interface configuration scripts
that OVS replaces, so at present those modifications are lost when OVS is
installed on e.g. XenServer 5.6.100. This commit applies those changes while
preserving OVS-specific modifications that have been made in the interim.
One major change introduced by these updates is the removal of support for
XenServer releases prior to 5.6.100.
This commit also updates the OVS-supplied xen-bugtool in similar fashion. We
will soon remove xen-bugtool from OVS altogether and move the added OVS
functionality to an XML extension as described in the Supplemental Pack DDK
guide.
Ben Pfaff [Thu, 10 Feb 2011 21:20:27 +0000 (13:20 -0800)]
docs: Consistently suggest /etc/openvswitch/conf.db as location for db.
Our Debian and XenServer packages use /etc/openvswitch/conf.db as the
location for the database but some of the docs suggested
/etc/ovs-vswitchd.conf.db. This settles on the former consistently.
The manpage is still ovs-vswitchd.conf.db but I don't know what to do
about that.
Ben Pfaff [Wed, 9 Feb 2011 00:43:40 +0000 (16:43 -0800)]
xenserver: Introduce "force-reload-kmod" to reload kernel module.
Running "service openvswitch force-reload-kmod" will now save the kernel
configuration state of Open vSwitch interfaces, stop the vswitch, unload
the kernel module, reload the kernel module, restart the vswitch, and
restore kernel configuration state. It is a reasonably safe way to upgrade
or downgrade the Open vSwitch kernel module on a running system.
Ben Pfaff [Wed, 9 Feb 2011 00:10:34 +0000 (16:10 -0800)]
ovs-vsctl: Add formatting options for the "list" and "find" commands.
The default format is the same as before (which the testsuite verifies).
The most important use for the new formatting options is --bare, which
outputs a format that is easy to parse from a shell script. An upcoming
patch will start using that.