diff mbox series

[ovs-dev,v2,1/5] ovs-kmod-ctl: introduce a kernel module load script

Message ID 20180504182818.24299-2-aconole@redhat.com
State Superseded
Headers show
Series selinux: introduce a transition domain for loading kmods | expand

Commit Message

Aaron Conole May 4, 2018, 6:28 p.m. UTC
Currently, Open vSwitch on linux embeds the logic of loading and unloading
kernel modules into the ovs-ctl and ovs-lib script files.  This works, but
it means that there is no way to leverage extended filesystem attributes
to grant fine grain permissions relating to module loading.

The split out utility 'ovs-kmod-ctl' will be used in an upcoming commit
for RHEL-based distributions to have a separate transition domain that
will allow module loading to be given to a separate selinux domain from
the openvswitch_t domain.

Acked-By: Timothy Redaelli <tredaelli@redhat.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 debian/openvswitch-switch.install  |   1 +
 debian/openvswitch-switch.manpages |   1 +
 rhel/openvswitch-fedora.spec.in    |   2 +
 rhel/openvswitch.spec.in           |   2 +
 utilities/.gitignore               |   1 +
 utilities/automake.mk              |   5 +
 utilities/ovs-ctl.in               |  32 +-----
 utilities/ovs-kmod-ctl.8           | 109 ++++++++++++++++++
 utilities/ovs-kmod-ctl.in          | 228 +++++++++++++++++++++++++++++++++++++
 utilities/ovs-lib.in               |  12 +-
 10 files changed, 356 insertions(+), 37 deletions(-)
 create mode 100644 utilities/ovs-kmod-ctl.8
 create mode 100644 utilities/ovs-kmod-ctl.in

Comments

Ansis May 11, 2018, 1:29 a.m. UTC | #1
On Fri, 4 May 2018 at 11:28, Aaron Conole <aconole@redhat.com> wrote:

> Currently, Open vSwitch on linux embeds the logic of loading and unloading
> kernel modules into the ovs-ctl and ovs-lib script files.  This works, but
> it means that there is no way to leverage extended filesystem attributes
> to grant fine grain permissions relating to module loading.

> The split out utility 'ovs-kmod-ctl' will be used in an upcoming commit
> for RHEL-based distributions to have a separate transition domain that
> will allow module loading to be given to a separate selinux domain from
> the openvswitch_t domain.

One thing I have been thinking about recently is how we could containerize
Open vSwitch (not sure if that is even possible in feasible way).

The idea would be that there would be, for example, Ubuntu based container
running OVS user space daemons installed from our deb packages. And the
"container host" would have Open vSwitch kernel module installed from our
dkms or kmod rpm packages. (or vice versa).

Such design, I think, inevitably would require sometihing like ovs-kmod-ctl
utility distributed with the dkms or kmod kernel module package.

This is something that does not requre action w.r.t. your series, but I
would be interested to hear your opinion if you have thought how to make
that happen...

> Acked-By: Timothy Redaelli <tredaelli@redhat.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   debian/openvswitch-switch.install  |   1 +
>   debian/openvswitch-switch.manpages |   1 +
>   rhel/openvswitch-fedora.spec.in    |   2 +
>   rhel/openvswitch.spec.in           |   2 +
>   utilities/.gitignore               |   1 +
>   utilities/automake.mk              |   5 +
>   utilities/ovs-ctl.in               |  32 +-----
>   utilities/ovs-kmod-ctl.8           | 109 ++++++++++++++++++
>   utilities/ovs-kmod-ctl.in          | 228
+++++++++++++++++++++++++++++++++++++
>   utilities/ovs-lib.in               |  12 +-
>   10 files changed, 356 insertions(+), 37 deletions(-)
>   create mode 100644 utilities/ovs-kmod-ctl.8
>   create mode 100644 utilities/ovs-kmod-ctl.in

> diff --git a/debian/openvswitch-switch.install
b/debian/openvswitch-switch.install
> index bfb391fe8..6a6e9a543 100644
> --- a/debian/openvswitch-switch.install
> +++ b/debian/openvswitch-switch.install
> @@ -12,5 +12,6 @@ usr/sbin/ovs-vswitchd
>   usr/sbin/ovsdb-server
>   usr/share/openvswitch/scripts/ovs-check-dead-ifs
>   usr/share/openvswitch/scripts/ovs-ctl
> +usr/share/openvswitch/scripts/ovs-kmod-ctl
>   usr/share/openvswitch/scripts/ovs-save
>   usr/share/openvswitch/vswitch.ovsschema
> diff --git a/debian/openvswitch-switch.manpages
b/debian/openvswitch-switch.manpages
> index c85cbfd30..1161cfda7 100644
> --- a/debian/openvswitch-switch.manpages
> +++ b/debian/openvswitch-switch.manpages
> @@ -3,6 +3,7 @@ ovsdb/ovsdb-server.5
>   utilities/ovs-ctl.8
>   utilities/ovs-dpctl-top.8
>   utilities/ovs-dpctl.8
> +utilities/ovs-kmod-ctl.8
>   utilities/ovs-pcap.1
>   utilities/ovs-tcpdump.8
>   utilities/ovs-tcpundump.1
> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/
openvswitch-fedora.spec.in
> index 3e5cf21e0..bf4526de2 100644
> --- a/rhel/openvswitch-fedora.spec.in
> +++ b/rhel/openvswitch-fedora.spec.in
> @@ -545,6 +545,7 @@ fi
>   %{_datadir}/openvswitch/scripts/ovs-save
>   %{_datadir}/openvswitch/scripts/ovs-vtep
>   %{_datadir}/openvswitch/scripts/ovs-ctl
> +%{_datadir}/openvswitch/scripts/ovs-kmod-ctl
>   %{_datadir}/openvswitch/scripts/ovs-systemd-reload
>   %config %{_datadir}/openvswitch/vswitch.ovsschema
>   %config %{_datadir}/openvswitch/vtep.ovsschema
> @@ -578,6 +579,7 @@ fi
>   %{_mandir}/man8/ovs-ctl.8*
>   %{_mandir}/man8/ovs-dpctl.8*
>   %{_mandir}/man8/ovs-dpctl-top.8*
> +%{_mandir}/man8/ovs-kmod-ctl.8*
>   %{_mandir}/man8/ovs-ofctl.8*
>   %{_mandir}/man8/ovs-pki.8*
>   %{_mandir}/man8/ovs-vsctl.8*
> diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
> index 2c5f0409a..883d25607 100644
> --- a/rhel/openvswitch.spec.in
> +++ b/rhel/openvswitch.spec.in
> @@ -237,6 +237,7 @@ exit 0
>   /usr/share/man/man8/ovs-ctl.8.gz
>   /usr/share/man/man8/ovs-dpctl.8.gz
>   /usr/share/man/man8/ovs-dpctl-top.8.gz
> +/usr/share/man/man8/ovs-kmod-ctl.8.gz
>   /usr/share/man/man8/ovs-ofctl.8.gz
>   /usr/share/man/man8/ovs-parse-backtrace.8.gz
>   /usr/share/man/man8/ovs-pki.8.gz
> @@ -250,6 +251,7 @@ exit 0
>   /usr/share/openvswitch/scripts/ovs-bugtool-*
>   /usr/share/openvswitch/scripts/ovs-check-dead-ifs
>   /usr/share/openvswitch/scripts/ovs-ctl
> +/usr/share/openvswitch/scripts/ovs-kmod-ctl
>   /usr/share/openvswitch/scripts/ovs-lib
>   /usr/share/openvswitch/scripts/ovs-save
>   /usr/share/openvswitch/scripts/ovs-vtep
> diff --git a/utilities/.gitignore b/utilities/.gitignore
> index 34c58f20f..eb2a69bf3 100644
> --- a/utilities/.gitignore
> +++ b/utilities/.gitignore
> @@ -13,6 +13,7 @@
>   /ovs-dpctl.8
>   /ovs-dpctl-top
>   /ovs-dpctl-top.8
> +/ovs-kmod-ctl
>   /ovs-l3ping
>   /ovs-l3ping.8
>   /ovs-lib
> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index 60cf1c5ed..d8f2374a3 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -20,6 +20,7 @@ endif
>   scripts_SCRIPTS += \
>          utilities/ovs-check-dead-ifs \
>          utilities/ovs-ctl \
> +       utilities/ovs-kmod-ctl \
>          utilities/ovs-save
>   scripts_DATA += utilities/ovs-lib

> @@ -44,6 +45,7 @@ EXTRA_DIST += \
>          utilities/ovs-dev.py \
>          utilities/ovs-docker \
>          utilities/ovs-dpctl-top.in \
> +       utilities/ovs-kmod-ctl.in \
>          utilities/ovs-l3ping.in \
>          utilities/ovs-lib.in \
>          utilities/ovs-parse-backtrace.in \
> @@ -63,6 +65,7 @@ MAN_ROOTS += \
>          utilities/ovs-ctl.8 \
>          utilities/ovs-dpctl.8.in \
>          utilities/ovs-dpctl-top.8.in \
> +       utilities/ovs-kmod-ctl.8 \
>          utilities/ovs-l3ping.8.in \
>          utilities/ovs-ofctl.8.in \
>          utilities/ovs-parse-backtrace.8 \
> @@ -81,6 +84,7 @@ CLEANFILES += \
>          utilities/ovs-dpctl.8 \
>          utilities/ovs-dpctl-top \
>          utilities/ovs-dpctl-top.8 \
> +       utilities/ovs-kmod-ctl \
>          utilities/ovs-l3ping \
>          utilities/ovs-l3ping.8 \
>          utilities/ovs-lib \
> @@ -107,6 +111,7 @@ man_MANS += \
>          utilities/ovs-testcontroller.8 \
>          utilities/ovs-dpctl.8 \
>          utilities/ovs-dpctl-top.8 \
> +       utilities/ovs-kmod-ctl.8 \
>          utilities/ovs-l3ping.8 \
>          utilities/ovs-ofctl.8 \
>          utilities/ovs-parse-backtrace.8 \
> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> index ef06dd967..8fdf8909a 100755
> --- a/utilities/ovs-ctl.in
> +++ b/utilities/ovs-ctl.in
> @@ -30,37 +30,9 @@ done
>   ## start ##
>   ## ----- ##

> -insert_mods () {
> -    # Try loading openvswitch again.
> -    action "Inserting openvswitch module" modprobe openvswitch
> -}
> -
>   insert_mod_if_required () {
> -    # If this kernel has no module support, expect we're done.
> -    if test ! -e /proc/modules
> -    then
> -        log_success_msg "Kernel has no loadable module support. Skipping
modprobe"
> -        return 0
> -    fi
> -
> -    # If openvswitch is already loaded then we're done.
> -    test -e /sys/module/openvswitch && return 0
> -
> -    # Load openvswitch.  If that's successful then we're done.
> -    insert_mods && return 0
> -
> -    # If the bridge module is loaded, then that might be blocking
> -    # openvswitch.  Try to unload it, if there are no bridges.
> -    test -e /sys/module/bridge || return 1
> -    bridges=`echo /sys/class/net/*/bridge | sed
's,/sys/class/net/,,g;s,/bridge,,g'`
> -    if test "$bridges" != "*"; then
> -        log_warning_msg "not removing bridge module because bridges
exist ($bridges)"
> -        return 1
> -    fi
> -    action "removing bridge module" rmmod bridge || return 1
> -
> -    # Try loading openvswitch again.
> -    insert_mods
> +    ## This takes care of inserting any required kernel modules
> +    ovs_kmod_ctl insert
>   }

>   set_hostname () {
> diff --git a/utilities/ovs-kmod-ctl.8 b/utilities/ovs-kmod-ctl.8
> new file mode 100644
> index 000000000..c36638e79
> --- /dev/null
> +++ b/utilities/ovs-kmod-ctl.8
> @@ -0,0 +1,109 @@
> +.\" -*- nroff -*-
> +.de IQ
> +.  br
> +.  ns
> +.  IP "\\$1"
> +..
> +.de ST
> +.  PP
> +.  RS -0.15in
> +.  I "\\$1"
> +.  RE
> +..
> +.TH ovs\-ctl 8 "February 2018" "Open vSwitch" "Open vSwitch Manual"
> +.ds PN ovs\-ctl
> +.
> +.SH NAME
> +ovs\-kmod\-ctl \- OVS startup helper script for loading kernel modules
> +.
> +.SH SYNOPSIS
> +\fBovs\-kmod\-ctl\fR \fBinsert
> +.br
> +\fBovs\-kmod\-ctl \fBremove
> +.br
> +\fBovs\-kmod\-ctl help \fR| \fB\-h \fR| \fB\-\-help
> +.br
> +\fBovs\-kmod\-ctl \-\-version
> +.br
> +\fBovs\-kmod\-ctl version
> +.
> +.SH DESCRIPTION
> +.
> +.PP
> +The \fBovs\-kmod\-ctl\fR program is responsible for inserting and
> +removing Open vSwitch kernel modules.  It is not meant to be invoked
> +directly by system administrators but to be called internally by
> +system startup scripts.  The script is used as part of an SELinux
> +transition domain.
> +.
> +.PP
> +Each of \fBovs\-kmod\-ctl\fR's commands is described separately below.
> +.
> +.SH "The ``insert'' command"
> +.
> +.PP
> +The \fBinsert\fR command loads the Open vSwitch kernel modules, if
> +needed.  If this fails, and the Linux bridge module is loaded but no
> +bridges exist, it tries to unload the bridge module and tries loading
> +the Open vSwitch kernel module again.
> +.
> +.SH "The ``remove'' command"
> +.
> +.PP
> +The \fBremove\fR command unloads the Open vSwitch kernel module
(including
> +the bridge compatibility module, if loaded) and any associated vport
> +modules.
> +.
> +.SH "EXIT STATUS"
> +.
> +\fBovs\-kmod\-ctl\fR exits with status 0 on success and nonzero on
> +failure.  The \fBinsert\fR command is considered to succeed if kernel
> +modules are already loaded; the \fBremove\fR command is considered to
> +succeed if none of the kernel modules are loaded.
> +.
> +.SH "ENVIRONMENT"
> +.
> +The following environment variables affect \fBovs\-kmod\-ctl\fR:
> +.
> +.IP "\fBPATH\fR"
> +\fBovs\-kmod\-ctl\fR does not hardcode the location of any of the
programs
> +that it runs.  \fBovs\-kmod\-ctl\fR will add the \fIsbindir\fR and
> +\fIbindir\fR that were specified at \fBconfigure\fR time to
> +\fBPATH\fR, if they are not already present.
> +.
> +.IP "\fBOVS_LOGDIR\fR"
> +.IQ "\fBOVS_RUNDIR\fR"
> +.IQ "\fBOVS_DBDIR\fR"
> +.IQ "\fBOVS_SYSCONFDIR\fR"
> +.IQ "\fBOVS_PKGDATADIR\fR"
> +.IQ "\fBOVS_BINDIR\fR"
> +.IQ "\fBOVS_SBINDIR\fR"
> +Setting one of these variables in the environment overrides the
> +respective \fBconfigure\fR option, both for \fBovs\-kmod\-ctl\fR itself
> +and for the other Open vSwitch programs that it runs.
> +.
> +.SH "FILES"
> +.
> +\fBovs\-kmod\-ctl\fR uses the following files:
> +.
> +.IP "\fBovs\-lib"
> +Shell function library used internally by \fBovs\-kmod\-ctl\fR.  It must
> +be installed in the same directory as \fBovs\-kmod\-ctl\fR.
> +.
> +.SH "EXAMPLE"
> +.
> +.PP
> +\fBovs\-kmod\-ctl\fR isn't intended to be manually executed.  However,
the
> +following examples demonstrate loading the kernel modules.
> +.
> +.TP
> +\fBovs\-kmod\-ctl\fR insert
> +Attempts to insert the Open vSwitch kernel modules.
> +.
> +.TP
> +\fBovs\-kmod\-ctl\fR remove
> +Attempts to remove the Open vSwitch kernel modules.
> +.
> +.SH "SEE ALSO"
> +.
> +\fBREADME.rst\fR, \fBovs\-ctl\fR(8)
> diff --git a/utilities/ovs-kmod-ctl.in b/utilities/ovs-kmod-ctl.in
> new file mode 100644
> index 000000000..b85f6cd04
> --- /dev/null
> +++ b/utilities/ovs-kmod-ctl.in
> @@ -0,0 +1,228 @@
> +#! /bin/sh
> +# Copyright (C) 2018 Red Hat, Inc.
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License");
> +# you may not use this file except in compliance with the License.
> +# You may obtain a copy of the License at:
> +#
> +#     http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
> +# See the License for the specific language governing permissions and
> +# limitations under the License.
> +
> +case $0 in
> +    */*) dir0=`echo "$0" | sed 's,/[^/]*$,,'` ;;

I understand that you have borrowed this code from already existing OVS
code base so it is not like you are introducing anything new, but merely
refactoring. So I don't expect you to amend your patch unless you have
quick idea. However,

1. does the `sed` command above do the same thing as `dirname`? If yes,
then I am curious why we have not historically used `dirname`?

2. I don't like the approach how $dir0 is being used to source ovs-lib. Is
there a way to hardcode paths in these script? It would leave less
variables that attacker could try to leverage. For example, if one creates
a symlink with:

# ln -s  /usr/share/openvswitch/scripts/ovs-kmod-ctl /tmp/bogus-ovs-kmod-ctl

and then create malicous shell file:

/tmp/ovs-lib

Then /tmp/bogus-ovs-kmod-ctl will uncoditionally source the bogus
/tmp/ovs-lib. Of course, for something like this to be exploitable in real
life it implies that attacker must know other exploits, like:

1. create such symlink to ovs-ctl in first place; AND
2. create bogus ovs-lib file in the same directory (without +x is ok) or
create another symlink to shell file. AND
3. execute the symnlink (with or without any arguments. This is the main
ingredient making this attack possible);


> +    *) dir0=./ ;;
> +esac
> +. "$dir0/ovs-lib" || exit 1
> +
> +for dir in "$sbindir" "$bindir" /sbin /bin /usr/sbin /usr/bin; do
> +    case :$PATH: in
> +        *:$dir:*) ;;
> +        *) PATH=$PATH:$dir ;;
> +    esac
> +done
> +
> +insert_mods () {
> +    # Try loading openvswitch again.
I think the comment above is redundant, because only caller knows whether
this is second attempt to again load kernel module.

> +    action "Inserting openvswitch module" modprobe openvswitch
> +}
> +
> +insert_kmod_if_required() {
I would rename this function "insert_kmods_if_required" to make it
consistent with "remove_kmods" and "insert_kmods" that are in plural.

> +    # If this kernel has no module support, expect we're done.
> +    if test ! -e /proc/modules
> +    then
> +        log_success_msg "Kernel has no loadable module support. Skipping
modprobe"
> +        return 0
> +    fi
> +
> +    # If openvswitch is already loaded then we're done.
> +    test -e /sys/module/openvswitch && return 0
> +
> +    # Load openvswitch.  If that's successful then we're done.
> +    insert_mods && return 0
> +
> +    # If the bridge module is loaded, then that might be blocking
> +    # openvswitch.  Try to unload it, if there are no bridges.
> +    test -e /sys/module/bridge || return 1
> +    bridges=`echo /sys/class/net/*/bridge | sed
's,/sys/class/net/,,g;s,/bridge,,g'`
> +    if test "$bridges" != "*"; then
> +        log_warning_msg "not removing bridge module because bridges
exist ($bridges)"
> +        return 1
> +    fi
> +    action "removing bridge module" rmmod bridge || return 1
> +
> +    # Try loading openvswitch again.
> +    insert_mods
> +}
> +
> +remove_kmods() {
> +    for vport in `awk '/^vport_/ { print $1 }' /proc/modules`; do
> +        action "Removing $vport module" rmmod $vport
> +    done
> +
> +    if test -e /sys/module/openvswitch; then
> +        action "Removing openvswitch module" rmmod openvswitch
> +    fi
> +}
> +
> +set_defaults () {

I am confused about these defaults. Are they declared in two places now?

root@ubuntubuilder:/git/ovs# git grep -C4  "DELETE_BRIDGES=no"
utilities/ovs-ctl.in-
utilities/ovs-ctl.in-set_defaults () {
utilities/ovs-ctl.in-    SYSTEM_ID=
utilities/ovs-ctl.in-
utilities/ovs-ctl.in:    DELETE_BRIDGES=no
utilities/ovs-ctl.in-    DELETE_TRANSIENT_PORTS=no
utilities/ovs-ctl.in-
utilities/ovs-ctl.in-    DAEMON_CWD=/
utilities/ovs-ctl.in-    FORCE_COREFILES=yes
--
utilities/ovs-kmod-ctl.in-
utilities/ovs-kmod-ctl.in-set_defaults () {
utilities/ovs-kmod-ctl.in-    SYSTEM_ID=
utilities/ovs-kmod-ctl.in-
utilities/ovs-kmod-ctl.in:    DELETE_BRIDGES=no
utilities/ovs-kmod-ctl.in-    DELETE_TRANSIENT_PORTS=no
utilities/ovs-kmod-ctl.in-
utilities/ovs-kmod-ctl.in-    DAEMON_CWD=/
utilities/ovs-kmod-ctl.in-    FORCE_COREFILES=yes


> +    SYSTEM_ID=
> +
> +    DELETE_BRIDGES=no
> +    DELETE_TRANSIENT_PORTS=no
> +
> +    DAEMON_CWD=/
> +    FORCE_COREFILES=yes
> +    MLOCKALL=yes
> +    SELF_CONFINEMENT=yes
> +    MONITOR=yes
> +    OVS_USER=
> +    OVSDB_SERVER=yes
> +    OVS_VSWITCHD=yes
> +    OVSDB_SERVER_PRIORITY=-10
> +    OVS_VSWITCHD_PRIORITY=-10
> +    OVSDB_SERVER_WRAPPER=
> +    OVS_VSWITCHD_WRAPPER=
> +
> +    DB_FILE=$dbdir/conf.db
> +    DB_SOCK=$rundir/db.sock
> +    DB_SCHEMA=$datadir/vswitch.ovsschema
> +    EXTRA_DBS=
> +
> +    PROTOCOL=gre
> +    DPORT=
> +    SPORT=
> +
> +    type_file=$etcdir/system-type.conf
> +    version_file=$etcdir/system-version.conf
> +
> +    if test -e "$type_file" ; then
> +        SYSTEM_TYPE=`cat $type_file`
> +        SYSTEM_VERSION=`cat $version_file`
> +    elif test -e "@sysconfdir@/os-release"; then
> +        SYSTEM_TYPE=`. '@sysconfdir@/os-release' && echo "$ID"`
> +        SYSTEM_VERSION=`. '@sysconfdir@/os-release' && echo
"$VERSION_ID"`
> +    elif (lsb_release --id) >/dev/null 2>&1; then
> +        SYSTEM_TYPE=`lsb_release --id -s`
> +        system_release=`lsb_release --release -s`
> +        system_codename=`lsb_release --codename -s`
> +        SYSTEM_VERSION="${system_release}-${system_codename}"
> +    else
> +        SYSTEM_TYPE=unknown
> +        SYSTEM_VERSION=unknown
> +    fi
> +}
> +
> +usage () {
> +    set_defaults
> +    cat <<EOF
> +$0: controls Open vSwitch kernel modules
> +usage: $0 [OPTIONS] COMMAND
> +
> +This program is intended to be invoked internally by Open vSwitch startup
> +scripts.  System administrators should not normally invoke it directly.
> +
> +Commands:
> +  insert                  insert the Open vSwitch kernel modules
> +  remove                  remove the Open vSwitch kernel modules
> +
> +Options:
> +  -h, --help              display this help message
> +  -V, --version           display version information
> +
> +Default directories with "configure" option and environment variable
override:
> +  logs: @LOGDIR@ (--with-logdir, OVS_LOGDIR)
> +  pidfiles and sockets: @RUNDIR@ (--with-rundir, OVS_RUNDIR)
> +  conf.db: @DBDIR@ (--with-dbdir, OVS_DBDIR)
> +  system configuration: @sysconfdir@ (--sysconfdir, OVS_SYSCONFDIR)
> +  data files: @pkgdatadir@ (--pkgdatadir, OVS_PKGDATADIR)
> +  user binaries: @bindir@ (--bindir, OVS_BINDIR)
> +  system binaries: @sbindir@ (--sbindir, OVS_SBINDIR)
> +
> +Please report bugs to bugs@openvswitch.org (see REPORTING-BUGS for
details).
> +EOF
> +
> +    exit 0
> +}
> +
> +set_option () {
> +    var=`echo "$option" | tr abcdefghijklmnopqrstuvwxyz-
ABCDEFGHIJKLMNOPQRSTUVWXYZ_`
> +    eval set=\${$var+yes}
> +    eval old_value=\$$var
> +    if test X$set = X || \
> +        (test $type = bool && \
> +        test X"$old_value" != Xno && test X"$old_value" != Xyes); then
> +        echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
> +        return
> +    fi
> +    eval $var=\$value
> +}
> +
> +set_defaults
> +extra_ids=
> +command=
> +for arg
> +do
> +    case $arg in
> +        -h | --help)
> +            usage
> +            ;;
> +        -V | --version)
> +            echo "$0 (Open vSwitch) $VERSION"
> +            exit 0
> +            ;;
> +        --[a-z]*=*)
> +            option=`expr X"$arg" : 'X--\([^=]*\)'`
> +            value=`expr X"$arg" : 'X[^=]*=\(.*\)'`
> +            type=string
> +            set_option
> +            ;;
> +        --no-[a-z]*)
> +            option=`expr X"$arg" : 'X--no-\(.*\)'`
> +            value=no
> +            type=bool
> +            set_option
> +            ;;
> +        --[a-z]*)
> +            option=`expr X"$arg" : 'X--\(.*\)'`
> +            value=yes
> +            type=bool
> +            set_option
> +            ;;
> +        -*)
> +            echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
> +            exit 1
> +            ;;
> +        *)
> +            if test X"$command" = X; then
> +                command=$arg
> +            else
> +                echo >&2 "$0: exactly one non-option argument required
(use --help for help)"
> +                exit 1
> +            fi
> +            ;;
> +    esac
> +done
> +case $command in
> +    remove)
> +        remove_kmods
> +        ;;
> +    insert)
> +        insert_kmod_if_required
> +        ;;
> +    help)
> +        usage
> +        ;;
> +    '')
> +        echo >&2 "$0: missing command name (use --help for help)"
> +        exit 1
> +        ;;
> +    *)
> +        echo >&2 "$0: unknown command \"$command\" (use --help for help)"
> +        exit 1
> +        ;;
> +esac
> diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
> index 4c3ad0f0b..6a958cbdf 100644
> --- a/utilities/ovs-lib.in
> +++ b/utilities/ovs-lib.in
> @@ -503,6 +503,10 @@ ovs_vsctl () {
>   ## force-reload-kmod ##
>   ## ----------------- ##

> +ovs_kmod_ctl () {
> +    "$dir0/ovs-kmod-ctl" "$@"
> +}
> +
>   internal_interfaces () {
>       # Outputs a list of internal interfaces:
>       #
> @@ -618,13 +622,7 @@ force_reload_kmod () {
>       done
>       action "ovs-appctl dpctl/flush-conntrack"

> -    for vport in `awk '/^vport_/ { print $1 }' /proc/modules`; do
> -        action "Removing $vport module" rmmod $vport
> -    done
> -
> -    if test -e /sys/module/openvswitch; then
> -        action "Removing openvswitch module" rmmod openvswitch
> -    fi
> +    ovs_kmod_ctl remove

>       # Start vswitchd by asking it to wait till flow restore is finished.
>       flow_restore_wait
> --
> 2.14.3
Aaron Conole May 11, 2018, 2:21 p.m. UTC | #2
Thanks for the review, Ansis!

Ansis Atteka <ansisatteka@gmail.com> writes:

> On Fri, 4 May 2018 at 11:28, Aaron Conole <aconole@redhat.com> wrote:
>
>> Currently, Open vSwitch on linux embeds the logic of loading and unloading
>> kernel modules into the ovs-ctl and ovs-lib script files.  This works, but
>> it means that there is no way to leverage extended filesystem attributes
>> to grant fine grain permissions relating to module loading.
>
>> The split out utility 'ovs-kmod-ctl' will be used in an upcoming commit
>> for RHEL-based distributions to have a separate transition domain that
>> will allow module loading to be given to a separate selinux domain from
>> the openvswitch_t domain.
>
> One thing I have been thinking about recently is how we could containerize
> Open vSwitch (not sure if that is even possible in feasible way).

Seems it's the new hotness.  My understanding is if you want all the
benefits of containerization, OvS still has some things to adapt.

> The idea would be that there would be, for example, Ubuntu based container
> running OVS user space daemons installed from our deb packages. And the
> "container host" would have Open vSwitch kernel module installed from our
> dkms or kmod rpm packages. (or vice versa).
>
> Such design, I think, inevitably would require sometihing like ovs-kmod-ctl
> utility distributed with the dkms or kmod kernel module package.

Most likely - and the corresponding selinux policies applied, and
probably will need to refactor 2/5 to use an interface definition
instead of hard coding the openvswitch_t as the only allowed domain
(unless you're thinking that ovs-ctl and an openvswitch labeled service
will be installed in the host as well - in which case, it would likely
be okay to keep as is).

There's definitely some work there (but it sounds beneficial).  I
haven't done too much thinking about it, though.

> This is something that does not requre action w.r.t. your series, but I
> would be interested to hear your opinion if you have thought how to make
> that happen...

I haven't thought too much about it, but maybe someone with more
knowledge of containers and orchestration would have some ideas.  I'm
interested in the purpose of containerizing the openvswitch suite (for
instance, is it to gain some of the isolation/separation benefits?)

>> Acked-By: Timothy Redaelli <tredaelli@redhat.com>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>   debian/openvswitch-switch.install  |   1 +
>>   debian/openvswitch-switch.manpages |   1 +
>>   rhel/openvswitch-fedora.spec.in    |   2 +
>>   rhel/openvswitch.spec.in           |   2 +
>>   utilities/.gitignore               |   1 +
>>   utilities/automake.mk              |   5 +
>>   utilities/ovs-ctl.in               |  32 +-----
>>   utilities/ovs-kmod-ctl.8           | 109 ++++++++++++++++++
>>   utilities/ovs-kmod-ctl.in          | 228
> +++++++++++++++++++++++++++++++++++++
>>   utilities/ovs-lib.in               |  12 +-
>>   10 files changed, 356 insertions(+), 37 deletions(-)
>>   create mode 100644 utilities/ovs-kmod-ctl.8
>>   create mode 100644 utilities/ovs-kmod-ctl.in
>
>> diff --git a/debian/openvswitch-switch.install
> b/debian/openvswitch-switch.install
>> index bfb391fe8..6a6e9a543 100644
>> --- a/debian/openvswitch-switch.install
>> +++ b/debian/openvswitch-switch.install
>> @@ -12,5 +12,6 @@ usr/sbin/ovs-vswitchd
>>   usr/sbin/ovsdb-server
>>   usr/share/openvswitch/scripts/ovs-check-dead-ifs
>>   usr/share/openvswitch/scripts/ovs-ctl
>> +usr/share/openvswitch/scripts/ovs-kmod-ctl
>>   usr/share/openvswitch/scripts/ovs-save
>>   usr/share/openvswitch/vswitch.ovsschema
>> diff --git a/debian/openvswitch-switch.manpages
> b/debian/openvswitch-switch.manpages
>> index c85cbfd30..1161cfda7 100644
>> --- a/debian/openvswitch-switch.manpages
>> +++ b/debian/openvswitch-switch.manpages
>> @@ -3,6 +3,7 @@ ovsdb/ovsdb-server.5
>>   utilities/ovs-ctl.8
>>   utilities/ovs-dpctl-top.8
>>   utilities/ovs-dpctl.8
>> +utilities/ovs-kmod-ctl.8
>>   utilities/ovs-pcap.1
>>   utilities/ovs-tcpdump.8
>>   utilities/ovs-tcpundump.1
>> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/
> openvswitch-fedora.spec.in
>> index 3e5cf21e0..bf4526de2 100644
>> --- a/rhel/openvswitch-fedora.spec.in
>> +++ b/rhel/openvswitch-fedora.spec.in
>> @@ -545,6 +545,7 @@ fi
>>   %{_datadir}/openvswitch/scripts/ovs-save
>>   %{_datadir}/openvswitch/scripts/ovs-vtep
>>   %{_datadir}/openvswitch/scripts/ovs-ctl
>> +%{_datadir}/openvswitch/scripts/ovs-kmod-ctl
>>   %{_datadir}/openvswitch/scripts/ovs-systemd-reload
>>   %config %{_datadir}/openvswitch/vswitch.ovsschema
>>   %config %{_datadir}/openvswitch/vtep.ovsschema
>> @@ -578,6 +579,7 @@ fi
>>   %{_mandir}/man8/ovs-ctl.8*
>>   %{_mandir}/man8/ovs-dpctl.8*
>>   %{_mandir}/man8/ovs-dpctl-top.8*
>> +%{_mandir}/man8/ovs-kmod-ctl.8*
>>   %{_mandir}/man8/ovs-ofctl.8*
>>   %{_mandir}/man8/ovs-pki.8*
>>   %{_mandir}/man8/ovs-vsctl.8*
>> diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
>> index 2c5f0409a..883d25607 100644
>> --- a/rhel/openvswitch.spec.in
>> +++ b/rhel/openvswitch.spec.in
>> @@ -237,6 +237,7 @@ exit 0
>>   /usr/share/man/man8/ovs-ctl.8.gz
>>   /usr/share/man/man8/ovs-dpctl.8.gz
>>   /usr/share/man/man8/ovs-dpctl-top.8.gz
>> +/usr/share/man/man8/ovs-kmod-ctl.8.gz
>>   /usr/share/man/man8/ovs-ofctl.8.gz
>>   /usr/share/man/man8/ovs-parse-backtrace.8.gz
>>   /usr/share/man/man8/ovs-pki.8.gz
>> @@ -250,6 +251,7 @@ exit 0
>>   /usr/share/openvswitch/scripts/ovs-bugtool-*
>>   /usr/share/openvswitch/scripts/ovs-check-dead-ifs
>>   /usr/share/openvswitch/scripts/ovs-ctl
>> +/usr/share/openvswitch/scripts/ovs-kmod-ctl
>>   /usr/share/openvswitch/scripts/ovs-lib
>>   /usr/share/openvswitch/scripts/ovs-save
>>   /usr/share/openvswitch/scripts/ovs-vtep
>> diff --git a/utilities/.gitignore b/utilities/.gitignore
>> index 34c58f20f..eb2a69bf3 100644
>> --- a/utilities/.gitignore
>> +++ b/utilities/.gitignore
>> @@ -13,6 +13,7 @@
>>   /ovs-dpctl.8
>>   /ovs-dpctl-top
>>   /ovs-dpctl-top.8
>> +/ovs-kmod-ctl
>>   /ovs-l3ping
>>   /ovs-l3ping.8
>>   /ovs-lib
>> diff --git a/utilities/automake.mk b/utilities/automake.mk
>> index 60cf1c5ed..d8f2374a3 100644
>> --- a/utilities/automake.mk
>> +++ b/utilities/automake.mk
>> @@ -20,6 +20,7 @@ endif
>>   scripts_SCRIPTS += \
>>          utilities/ovs-check-dead-ifs \
>>          utilities/ovs-ctl \
>> +       utilities/ovs-kmod-ctl \
>>          utilities/ovs-save
>>   scripts_DATA += utilities/ovs-lib
>
>> @@ -44,6 +45,7 @@ EXTRA_DIST += \
>>          utilities/ovs-dev.py \
>>          utilities/ovs-docker \
>>          utilities/ovs-dpctl-top.in \
>> +       utilities/ovs-kmod-ctl.in \
>>          utilities/ovs-l3ping.in \
>>          utilities/ovs-lib.in \
>>          utilities/ovs-parse-backtrace.in \
>> @@ -63,6 +65,7 @@ MAN_ROOTS += \
>>          utilities/ovs-ctl.8 \
>>          utilities/ovs-dpctl.8.in \
>>          utilities/ovs-dpctl-top.8.in \
>> +       utilities/ovs-kmod-ctl.8 \
>>          utilities/ovs-l3ping.8.in \
>>          utilities/ovs-ofctl.8.in \
>>          utilities/ovs-parse-backtrace.8 \
>> @@ -81,6 +84,7 @@ CLEANFILES += \
>>          utilities/ovs-dpctl.8 \
>>          utilities/ovs-dpctl-top \
>>          utilities/ovs-dpctl-top.8 \
>> +       utilities/ovs-kmod-ctl \
>>          utilities/ovs-l3ping \
>>          utilities/ovs-l3ping.8 \
>>          utilities/ovs-lib \
>> @@ -107,6 +111,7 @@ man_MANS += \
>>          utilities/ovs-testcontroller.8 \
>>          utilities/ovs-dpctl.8 \
>>          utilities/ovs-dpctl-top.8 \
>> +       utilities/ovs-kmod-ctl.8 \
>>          utilities/ovs-l3ping.8 \
>>          utilities/ovs-ofctl.8 \
>>          utilities/ovs-parse-backtrace.8 \
>> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
>> index ef06dd967..8fdf8909a 100755
>> --- a/utilities/ovs-ctl.in
>> +++ b/utilities/ovs-ctl.in
>> @@ -30,37 +30,9 @@ done
>>   ## start ##
>>   ## ----- ##
>
>> -insert_mods () {
>> -    # Try loading openvswitch again.
>> -    action "Inserting openvswitch module" modprobe openvswitch
>> -}
>> -
>>   insert_mod_if_required () {
>> -    # If this kernel has no module support, expect we're done.
>> -    if test ! -e /proc/modules
>> -    then
>> -        log_success_msg "Kernel has no loadable module support. Skipping
> modprobe"
>> -        return 0
>> -    fi
>> -
>> -    # If openvswitch is already loaded then we're done.
>> -    test -e /sys/module/openvswitch && return 0
>> -
>> -    # Load openvswitch.  If that's successful then we're done.
>> -    insert_mods && return 0
>> -
>> -    # If the bridge module is loaded, then that might be blocking
>> -    # openvswitch.  Try to unload it, if there are no bridges.
>> -    test -e /sys/module/bridge || return 1
>> -    bridges=`echo /sys/class/net/*/bridge | sed
> 's,/sys/class/net/,,g;s,/bridge,,g'`
>> -    if test "$bridges" != "*"; then
>> -        log_warning_msg "not removing bridge module because bridges
> exist ($bridges)"
>> -        return 1
>> -    fi
>> -    action "removing bridge module" rmmod bridge || return 1
>> -
>> -    # Try loading openvswitch again.
>> -    insert_mods
>> +    ## This takes care of inserting any required kernel modules
>> +    ovs_kmod_ctl insert
>>   }
>
>>   set_hostname () {
>> diff --git a/utilities/ovs-kmod-ctl.8 b/utilities/ovs-kmod-ctl.8
>> new file mode 100644
>> index 000000000..c36638e79
>> --- /dev/null
>> +++ b/utilities/ovs-kmod-ctl.8
>> @@ -0,0 +1,109 @@
>> +.\" -*- nroff -*-
>> +.de IQ
>> +.  br
>> +.  ns
>> +.  IP "\\$1"
>> +..
>> +.de ST
>> +.  PP
>> +.  RS -0.15in
>> +.  I "\\$1"
>> +.  RE
>> +..
>> +.TH ovs\-ctl 8 "February 2018" "Open vSwitch" "Open vSwitch Manual"
>> +.ds PN ovs\-ctl
>> +.
>> +.SH NAME
>> +ovs\-kmod\-ctl \- OVS startup helper script for loading kernel modules
>> +.
>> +.SH SYNOPSIS
>> +\fBovs\-kmod\-ctl\fR \fBinsert
>> +.br
>> +\fBovs\-kmod\-ctl \fBremove
>> +.br
>> +\fBovs\-kmod\-ctl help \fR| \fB\-h \fR| \fB\-\-help
>> +.br
>> +\fBovs\-kmod\-ctl \-\-version
>> +.br
>> +\fBovs\-kmod\-ctl version
>> +.
>> +.SH DESCRIPTION
>> +.
>> +.PP
>> +The \fBovs\-kmod\-ctl\fR program is responsible for inserting and
>> +removing Open vSwitch kernel modules.  It is not meant to be invoked
>> +directly by system administrators but to be called internally by
>> +system startup scripts.  The script is used as part of an SELinux
>> +transition domain.
>> +.
>> +.PP
>> +Each of \fBovs\-kmod\-ctl\fR's commands is described separately below.
>> +.
>> +.SH "The ``insert'' command"
>> +.
>> +.PP
>> +The \fBinsert\fR command loads the Open vSwitch kernel modules, if
>> +needed.  If this fails, and the Linux bridge module is loaded but no
>> +bridges exist, it tries to unload the bridge module and tries loading
>> +the Open vSwitch kernel module again.
>> +.
>> +.SH "The ``remove'' command"
>> +.
>> +.PP
>> +The \fBremove\fR command unloads the Open vSwitch kernel module
> (including
>> +the bridge compatibility module, if loaded) and any associated vport
>> +modules.
>> +.
>> +.SH "EXIT STATUS"
>> +.
>> +\fBovs\-kmod\-ctl\fR exits with status 0 on success and nonzero on
>> +failure.  The \fBinsert\fR command is considered to succeed if kernel
>> +modules are already loaded; the \fBremove\fR command is considered to
>> +succeed if none of the kernel modules are loaded.
>> +.
>> +.SH "ENVIRONMENT"
>> +.
>> +The following environment variables affect \fBovs\-kmod\-ctl\fR:
>> +.
>> +.IP "\fBPATH\fR"
>> +\fBovs\-kmod\-ctl\fR does not hardcode the location of any of the
> programs
>> +that it runs.  \fBovs\-kmod\-ctl\fR will add the \fIsbindir\fR and
>> +\fIbindir\fR that were specified at \fBconfigure\fR time to
>> +\fBPATH\fR, if they are not already present.
>> +.
>> +.IP "\fBOVS_LOGDIR\fR"
>> +.IQ "\fBOVS_RUNDIR\fR"
>> +.IQ "\fBOVS_DBDIR\fR"
>> +.IQ "\fBOVS_SYSCONFDIR\fR"
>> +.IQ "\fBOVS_PKGDATADIR\fR"
>> +.IQ "\fBOVS_BINDIR\fR"
>> +.IQ "\fBOVS_SBINDIR\fR"
>> +Setting one of these variables in the environment overrides the
>> +respective \fBconfigure\fR option, both for \fBovs\-kmod\-ctl\fR itself
>> +and for the other Open vSwitch programs that it runs.
>> +.
>> +.SH "FILES"
>> +.
>> +\fBovs\-kmod\-ctl\fR uses the following files:
>> +.
>> +.IP "\fBovs\-lib"
>> +Shell function library used internally by \fBovs\-kmod\-ctl\fR.  It must
>> +be installed in the same directory as \fBovs\-kmod\-ctl\fR.
>> +.
>> +.SH "EXAMPLE"
>> +.
>> +.PP
>> +\fBovs\-kmod\-ctl\fR isn't intended to be manually executed.  However,
> the
>> +following examples demonstrate loading the kernel modules.
>> +.
>> +.TP
>> +\fBovs\-kmod\-ctl\fR insert
>> +Attempts to insert the Open vSwitch kernel modules.
>> +.
>> +.TP
>> +\fBovs\-kmod\-ctl\fR remove
>> +Attempts to remove the Open vSwitch kernel modules.
>> +.
>> +.SH "SEE ALSO"
>> +.
>> +\fBREADME.rst\fR, \fBovs\-ctl\fR(8)
>> diff --git a/utilities/ovs-kmod-ctl.in b/utilities/ovs-kmod-ctl.in
>> new file mode 100644
>> index 000000000..b85f6cd04
>> --- /dev/null
>> +++ b/utilities/ovs-kmod-ctl.in
>> @@ -0,0 +1,228 @@
>> +#! /bin/sh
>> +# Copyright (C) 2018 Red Hat, Inc.
>> +#
>> +# Licensed under the Apache License, Version 2.0 (the "License");
>> +# you may not use this file except in compliance with the License.
>> +# You may obtain a copy of the License at:
>> +#
>> +#     http://www.apache.org/licenses/LICENSE-2.0
>> +#
>> +# Unless required by applicable law or agreed to in writing, software
>> +# distributed under the License is distributed on an "AS IS" BASIS,
>> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
>> +# See the License for the specific language governing permissions and
>> +# limitations under the License.
>> +
>> +case $0 in
>> +    */*) dir0=`echo "$0" | sed 's,/[^/]*$,,'` ;;
>
> I understand that you have borrowed this code from already existing OVS
> code base so it is not like you are introducing anything new, but merely
> refactoring. So I don't expect you to amend your patch unless you have
> quick idea. However,
>
> 1. does the `sed` command above do the same thing as `dirname`? If yes,
> then I am curious why we have not historically used `dirname`?

That's a good question.  They do behave somewhat differently (for
instance, GNU dirname will substitute a '.' if it doesn't find any path
separator, whereas the sed command above will not).  I'm not sure that
the distinction is that important, since the case requires a '/', so
perhaps it's a historical thing.

> 2. I don't like the approach how $dir0 is being used to source ovs-lib. Is
> there a way to hardcode paths in these script? It would leave less
> variables that attacker could try to leverage. For example, if one creates
> a symlink with:
>
> # ln -s  /usr/share/openvswitch/scripts/ovs-kmod-ctl /tmp/bogus-ovs-kmod-ctl
>
> and then create malicous shell file:
>
> /tmp/ovs-lib
>
> Then /tmp/bogus-ovs-kmod-ctl will uncoditionally source the bogus
> /tmp/ovs-lib. Of course, for something like this to be exploitable in real
> life it implies that attacker must know other exploits, like:
>
> 1. create such symlink to ovs-ctl in first place; AND
> 2. create bogus ovs-lib file in the same directory (without +x is ok) or
> create another symlink to shell file. AND
> 3. execute the symnlink (with or without any arguments. This is the main
> ingredient making this attack possible);

don't forget:
4. Set their source domain to openvswitch_t to force the load.

But yes, I'll spin a correction here and hardcode the path from the
result of --configure.  It makes sense to me.

>> +    *) dir0=./ ;;
>> +esac
>> +. "$dir0/ovs-lib" || exit 1
>> +
>> +for dir in "$sbindir" "$bindir" /sbin /bin /usr/sbin /usr/bin; do
>> +    case :$PATH: in
>> +        *:$dir:*) ;;
>> +        *) PATH=$PATH:$dir ;;
>> +    esac
>> +done
>> +
>> +insert_mods () {
>> +    # Try loading openvswitch again.
> I think the comment above is redundant, because only caller knows whether
> this is second attempt to again load kernel module.

True, I'll remove it.

>> +    action "Inserting openvswitch module" modprobe openvswitch
>> +}
>> +
>> +insert_kmod_if_required() {
> I would rename this function "insert_kmods_if_required" to make it
> consistent with "remove_kmods" and "insert_kmods" that are in plural.

Ack.

>> +    # If this kernel has no module support, expect we're done.
>> +    if test ! -e /proc/modules
>> +    then
>> +        log_success_msg "Kernel has no loadable module support. Skipping
> modprobe"
>> +        return 0
>> +    fi
>> +
>> +    # If openvswitch is already loaded then we're done.
>> +    test -e /sys/module/openvswitch && return 0
>> +
>> +    # Load openvswitch.  If that's successful then we're done.
>> +    insert_mods && return 0
>> +
>> +    # If the bridge module is loaded, then that might be blocking
>> +    # openvswitch.  Try to unload it, if there are no bridges.
>> +    test -e /sys/module/bridge || return 1
>> +    bridges=`echo /sys/class/net/*/bridge | sed
> 's,/sys/class/net/,,g;s,/bridge,,g'`
>> +    if test "$bridges" != "*"; then
>> +        log_warning_msg "not removing bridge module because bridges
> exist ($bridges)"
>> +        return 1
>> +    fi
>> +    action "removing bridge module" rmmod bridge || return 1
>> +
>> +    # Try loading openvswitch again.
>> +    insert_mods
>> +}
>> +
>> +remove_kmods() {
>> +    for vport in `awk '/^vport_/ { print $1 }' /proc/modules`; do
>> +        action "Removing $vport module" rmmod $vport
>> +    done
>> +
>> +    if test -e /sys/module/openvswitch; then
>> +        action "Removing openvswitch module" rmmod openvswitch
>> +    fi
>> +}
>> +
>> +set_defaults () {
>
> I am confused about these defaults. Are they declared in two places now?

Seems like, and I think they aren't needed even.  I'll trim the list
completely.

> root@ubuntubuilder:/git/ovs# git grep -C4  "DELETE_BRIDGES=no"
> utilities/ovs-ctl.in-
> utilities/ovs-ctl.in-set_defaults () {
> utilities/ovs-ctl.in-    SYSTEM_ID=
> utilities/ovs-ctl.in-
> utilities/ovs-ctl.in:    DELETE_BRIDGES=no
> utilities/ovs-ctl.in-    DELETE_TRANSIENT_PORTS=no
> utilities/ovs-ctl.in-
> utilities/ovs-ctl.in-    DAEMON_CWD=/
> utilities/ovs-ctl.in-    FORCE_COREFILES=yes
> --
> utilities/ovs-kmod-ctl.in-
> utilities/ovs-kmod-ctl.in-set_defaults () {
> utilities/ovs-kmod-ctl.in-    SYSTEM_ID=
> utilities/ovs-kmod-ctl.in-
> utilities/ovs-kmod-ctl.in:    DELETE_BRIDGES=no
> utilities/ovs-kmod-ctl.in-    DELETE_TRANSIENT_PORTS=no
> utilities/ovs-kmod-ctl.in-
> utilities/ovs-kmod-ctl.in-    DAEMON_CWD=/
> utilities/ovs-kmod-ctl.in-    FORCE_COREFILES=yes
>
>
>> +    SYSTEM_ID=
>> +
>> +    DELETE_BRIDGES=no
>> +    DELETE_TRANSIENT_PORTS=no
>> +
>> +    DAEMON_CWD=/
>> +    FORCE_COREFILES=yes
>> +    MLOCKALL=yes
>> +    SELF_CONFINEMENT=yes
>> +    MONITOR=yes
>> +    OVS_USER=
>> +    OVSDB_SERVER=yes
>> +    OVS_VSWITCHD=yes
>> +    OVSDB_SERVER_PRIORITY=-10
>> +    OVS_VSWITCHD_PRIORITY=-10
>> +    OVSDB_SERVER_WRAPPER=
>> +    OVS_VSWITCHD_WRAPPER=
>> +
>> +    DB_FILE=$dbdir/conf.db
>> +    DB_SOCK=$rundir/db.sock
>> +    DB_SCHEMA=$datadir/vswitch.ovsschema
>> +    EXTRA_DBS=
>> +
>> +    PROTOCOL=gre
>> +    DPORT=
>> +    SPORT=
>> +
>> +    type_file=$etcdir/system-type.conf
>> +    version_file=$etcdir/system-version.conf
>> +
>> +    if test -e "$type_file" ; then
>> +        SYSTEM_TYPE=`cat $type_file`
>> +        SYSTEM_VERSION=`cat $version_file`
>> +    elif test -e "@sysconfdir@/os-release"; then
>> +        SYSTEM_TYPE=`. '@sysconfdir@/os-release' && echo "$ID"`
>> +        SYSTEM_VERSION=`. '@sysconfdir@/os-release' && echo
> "$VERSION_ID"`
>> +    elif (lsb_release --id) >/dev/null 2>&1; then
>> +        SYSTEM_TYPE=`lsb_release --id -s`
>> +        system_release=`lsb_release --release -s`
>> +        system_codename=`lsb_release --codename -s`
>> +        SYSTEM_VERSION="${system_release}-${system_codename}"
>> +    else
>> +        SYSTEM_TYPE=unknown
>> +        SYSTEM_VERSION=unknown
>> +    fi
>> +}
>> +
>> +usage () {
>> +    set_defaults
>> +    cat <<EOF
>> +$0: controls Open vSwitch kernel modules
>> +usage: $0 [OPTIONS] COMMAND
>> +
>> +This program is intended to be invoked internally by Open vSwitch startup
>> +scripts.  System administrators should not normally invoke it directly.
>> +
>> +Commands:
>> +  insert                  insert the Open vSwitch kernel modules
>> +  remove                  remove the Open vSwitch kernel modules
>> +
>> +Options:
>> +  -h, --help              display this help message
>> +  -V, --version           display version information
>> +
>> +Default directories with "configure" option and environment variable
> override:
>> +  logs: @LOGDIR@ (--with-logdir, OVS_LOGDIR)
>> +  pidfiles and sockets: @RUNDIR@ (--with-rundir, OVS_RUNDIR)
>> +  conf.db: @DBDIR@ (--with-dbdir, OVS_DBDIR)
>> +  system configuration: @sysconfdir@ (--sysconfdir, OVS_SYSCONFDIR)
>> +  data files: @pkgdatadir@ (--pkgdatadir, OVS_PKGDATADIR)
>> +  user binaries: @bindir@ (--bindir, OVS_BINDIR)
>> +  system binaries: @sbindir@ (--sbindir, OVS_SBINDIR)
>> +
>> +Please report bugs to bugs@openvswitch.org (see REPORTING-BUGS for
> details).
>> +EOF
>> +
>> +    exit 0
>> +}
>> +
>> +set_option () {
>> +    var=`echo "$option" | tr abcdefghijklmnopqrstuvwxyz-
> ABCDEFGHIJKLMNOPQRSTUVWXYZ_`
>> +    eval set=\${$var+yes}
>> +    eval old_value=\$$var
>> +    if test X$set = X || \
>> +        (test $type = bool && \
>> +        test X"$old_value" != Xno && test X"$old_value" != Xyes); then
>> +        echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
>> +        return
>> +    fi
>> +    eval $var=\$value
>> +}
>> +
>> +set_defaults
>> +extra_ids=
>> +command=
>> +for arg
>> +do
>> +    case $arg in
>> +        -h | --help)
>> +            usage
>> +            ;;
>> +        -V | --version)
>> +            echo "$0 (Open vSwitch) $VERSION"
>> +            exit 0
>> +            ;;
>> +        --[a-z]*=*)
>> +            option=`expr X"$arg" : 'X--\([^=]*\)'`
>> +            value=`expr X"$arg" : 'X[^=]*=\(.*\)'`
>> +            type=string
>> +            set_option
>> +            ;;
>> +        --no-[a-z]*)
>> +            option=`expr X"$arg" : 'X--no-\(.*\)'`
>> +            value=no
>> +            type=bool
>> +            set_option
>> +            ;;
>> +        --[a-z]*)
>> +            option=`expr X"$arg" : 'X--\(.*\)'`
>> +            value=yes
>> +            type=bool
>> +            set_option
>> +            ;;
>> +        -*)
>> +            echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
>> +            exit 1
>> +            ;;
>> +        *)
>> +            if test X"$command" = X; then
>> +                command=$arg
>> +            else
>> +                echo >&2 "$0: exactly one non-option argument required
> (use --help for help)"
>> +                exit 1
>> +            fi
>> +            ;;
>> +    esac
>> +done
>> +case $command in
>> +    remove)
>> +        remove_kmods
>> +        ;;
>> +    insert)
>> +        insert_kmod_if_required
>> +        ;;
>> +    help)
>> +        usage
>> +        ;;
>> +    '')
>> +        echo >&2 "$0: missing command name (use --help for help)"
>> +        exit 1
>> +        ;;
>> +    *)
>> +        echo >&2 "$0: unknown command \"$command\" (use --help for help)"
>> +        exit 1
>> +        ;;
>> +esac
>> diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
>> index 4c3ad0f0b..6a958cbdf 100644
>> --- a/utilities/ovs-lib.in
>> +++ b/utilities/ovs-lib.in
>> @@ -503,6 +503,10 @@ ovs_vsctl () {
>>   ## force-reload-kmod ##
>>   ## ----------------- ##
>
>> +ovs_kmod_ctl () {
>> +    "$dir0/ovs-kmod-ctl" "$@"
>> +}
>> +
>>   internal_interfaces () {
>>       # Outputs a list of internal interfaces:
>>       #
>> @@ -618,13 +622,7 @@ force_reload_kmod () {
>>       done
>>       action "ovs-appctl dpctl/flush-conntrack"
>
>> -    for vport in `awk '/^vport_/ { print $1 }' /proc/modules`; do
>> -        action "Removing $vport module" rmmod $vport
>> -    done
>> -
>> -    if test -e /sys/module/openvswitch; then
>> -        action "Removing openvswitch module" rmmod openvswitch
>> -    fi
>> +    ovs_kmod_ctl remove
>
>>       # Start vswitchd by asking it to wait till flow restore is finished.
>>       flow_restore_wait
>> --
>> 2.14.3
Ben Pfaff May 11, 2018, 5:49 p.m. UTC | #3
On Fri, May 11, 2018 at 01:29:09AM +0000, Ansis Atteka wrote:
> On Fri, 4 May 2018 at 11:28, Aaron Conole <aconole@redhat.com> wrote:
> 
> > Currently, Open vSwitch on linux embeds the logic of loading and unloading
> > kernel modules into the ovs-ctl and ovs-lib script files.  This works, but
> > it means that there is no way to leverage extended filesystem attributes
> > to grant fine grain permissions relating to module loading.
> 
> > The split out utility 'ovs-kmod-ctl' will be used in an upcoming commit
> > for RHEL-based distributions to have a separate transition domain that
> > will allow module loading to be given to a separate selinux domain from
> > the openvswitch_t domain.
> 
> One thing I have been thinking about recently is how we could containerize
> Open vSwitch (not sure if that is even possible in feasible way).
> 
> The idea would be that there would be, for example, Ubuntu based container
> running OVS user space daemons installed from our deb packages. And the
> "container host" would have Open vSwitch kernel module installed from our
> dkms or kmod rpm packages. (or vice versa).
> 
> Such design, I think, inevitably would require sometihing like ovs-kmod-ctl
> utility distributed with the dkms or kmod kernel module package.
> 
> This is something that does not requre action w.r.t. your series, but I
> would be interested to hear your opinion if you have thought how to make
> that happen...

I'd like to see this happen too.
Ansis May 11, 2018, 6:43 p.m. UTC | #4
On Fri, 11 May 2018 at 07:21, Aaron Conole <aconole@redhat.com> wrote:

> Thanks for the review, Ansis!

> Ansis Atteka <ansisatteka@gmail.com> writes:

> > On Fri, 4 May 2018 at 11:28, Aaron Conole <aconole@redhat.com> wrote:
> >
> >> Currently, Open vSwitch on linux embeds the logic of loading and
unloading
> >> kernel modules into the ovs-ctl and ovs-lib script files.  This works,
but
> >> it means that there is no way to leverage extended filesystem
attributes
> >> to grant fine grain permissions relating to module loading.
> >
> >> The split out utility 'ovs-kmod-ctl' will be used in an upcoming commit
> >> for RHEL-based distributions to have a separate transition domain that
> >> will allow module loading to be given to a separate selinux domain from
> >> the openvswitch_t domain.
> >
> > One thing I have been thinking about recently is how we could
containerize
> > Open vSwitch (not sure if that is even possible in feasible way).

> Seems it's the new hotness.  My understanding is if you want all the
> benefits of containerization, OvS still has some things to adapt.

> > The idea would be that there would be, for example, Ubuntu based
container
> > running OVS user space daemons installed from our deb packages. And the
> > "container host" would have Open vSwitch kernel module installed from
our
> > dkms or kmod rpm packages. (or vice versa).
> >
> > Such design, I think, inevitably would require sometihing like
ovs-kmod-ctl
> > utility distributed with the dkms or kmod kernel module package.

> Most likely - and the corresponding selinux policies applied, and
> probably will need to refactor 2/5 to use an interface definition
> instead of hard coding the openvswitch_t as the only allowed domain
> (unless you're thinking that ovs-ctl and an openvswitch labeled service
> will be installed in the host as well - in which case, it would likely
> be okay to keep as is).

> There's definitely some work there (but it sounds beneficial).  I
> haven't done too much thinking about it, though.

> > This is something that does not requre action w.r.t. your series, but I
> > would be interested to hear your opinion if you have thought how to make
> > that happen...

> I haven't thought too much about it, but maybe someone with more
> knowledge of containers and orchestration would have some ideas.  I'm
> interested in the purpose of containerizing the openvswitch suite (for
> instance, is it to gain some of the isolation/separation benefits?)

> >> Acked-By: Timothy Redaelli <tredaelli@redhat.com>
> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> ---
> >>   debian/openvswitch-switch.install  |   1 +
> >>   debian/openvswitch-switch.manpages |   1 +
> >>   rhel/openvswitch-fedora.spec.in    |   2 +
> >>   rhel/openvswitch.spec.in           |   2 +
> >>   utilities/.gitignore               |   1 +
> >>   utilities/automake.mk              |   5 +
> >>   utilities/ovs-ctl.in               |  32 +-----
> >>   utilities/ovs-kmod-ctl.8           | 109 ++++++++++++++++++
> >>   utilities/ovs-kmod-ctl.in          | 228
> > +++++++++++++++++++++++++++++++++++++
> >>   utilities/ovs-lib.in               |  12 +-
> >>   10 files changed, 356 insertions(+), 37 deletions(-)
> >>   create mode 100644 utilities/ovs-kmod-ctl.8
> >>   create mode 100644 utilities/ovs-kmod-ctl.in
> >
> >> diff --git a/debian/openvswitch-switch.install
> > b/debian/openvswitch-switch.install
> >> index bfb391fe8..6a6e9a543 100644
> >> --- a/debian/openvswitch-switch.install
> >> +++ b/debian/openvswitch-switch.install
> >> @@ -12,5 +12,6 @@ usr/sbin/ovs-vswitchd
> >>   usr/sbin/ovsdb-server
> >>   usr/share/openvswitch/scripts/ovs-check-dead-ifs
> >>   usr/share/openvswitch/scripts/ovs-ctl
> >> +usr/share/openvswitch/scripts/ovs-kmod-ctl
> >>   usr/share/openvswitch/scripts/ovs-save
> >>   usr/share/openvswitch/vswitch.ovsschema
> >> diff --git a/debian/openvswitch-switch.manpages
> > b/debian/openvswitch-switch.manpages
> >> index c85cbfd30..1161cfda7 100644
> >> --- a/debian/openvswitch-switch.manpages
> >> +++ b/debian/openvswitch-switch.manpages
> >> @@ -3,6 +3,7 @@ ovsdb/ovsdb-server.5
> >>   utilities/ovs-ctl.8
> >>   utilities/ovs-dpctl-top.8
> >>   utilities/ovs-dpctl.8
> >> +utilities/ovs-kmod-ctl.8
> >>   utilities/ovs-pcap.1
> >>   utilities/ovs-tcpdump.8
> >>   utilities/ovs-tcpundump.1
> >> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/
> > openvswitch-fedora.spec.in
> >> index 3e5cf21e0..bf4526de2 100644
> >> --- a/rhel/openvswitch-fedora.spec.in
> >> +++ b/rhel/openvswitch-fedora.spec.in
> >> @@ -545,6 +545,7 @@ fi
> >>   %{_datadir}/openvswitch/scripts/ovs-save
> >>   %{_datadir}/openvswitch/scripts/ovs-vtep
> >>   %{_datadir}/openvswitch/scripts/ovs-ctl
> >> +%{_datadir}/openvswitch/scripts/ovs-kmod-ctl
> >>   %{_datadir}/openvswitch/scripts/ovs-systemd-reload
> >>   %config %{_datadir}/openvswitch/vswitch.ovsschema
> >>   %config %{_datadir}/openvswitch/vtep.ovsschema
> >> @@ -578,6 +579,7 @@ fi
> >>   %{_mandir}/man8/ovs-ctl.8*
> >>   %{_mandir}/man8/ovs-dpctl.8*
> >>   %{_mandir}/man8/ovs-dpctl-top.8*
> >> +%{_mandir}/man8/ovs-kmod-ctl.8*
> >>   %{_mandir}/man8/ovs-ofctl.8*
> >>   %{_mandir}/man8/ovs-pki.8*
> >>   %{_mandir}/man8/ovs-vsctl.8*
> >> diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
> >> index 2c5f0409a..883d25607 100644
> >> --- a/rhel/openvswitch.spec.in
> >> +++ b/rhel/openvswitch.spec.in
> >> @@ -237,6 +237,7 @@ exit 0
> >>   /usr/share/man/man8/ovs-ctl.8.gz
> >>   /usr/share/man/man8/ovs-dpctl.8.gz
> >>   /usr/share/man/man8/ovs-dpctl-top.8.gz
> >> +/usr/share/man/man8/ovs-kmod-ctl.8.gz
> >>   /usr/share/man/man8/ovs-ofctl.8.gz
> >>   /usr/share/man/man8/ovs-parse-backtrace.8.gz
> >>   /usr/share/man/man8/ovs-pki.8.gz
> >> @@ -250,6 +251,7 @@ exit 0
> >>   /usr/share/openvswitch/scripts/ovs-bugtool-*
> >>   /usr/share/openvswitch/scripts/ovs-check-dead-ifs
> >>   /usr/share/openvswitch/scripts/ovs-ctl
> >> +/usr/share/openvswitch/scripts/ovs-kmod-ctl
> >>   /usr/share/openvswitch/scripts/ovs-lib
> >>   /usr/share/openvswitch/scripts/ovs-save
> >>   /usr/share/openvswitch/scripts/ovs-vtep
> >> diff --git a/utilities/.gitignore b/utilities/.gitignore
> >> index 34c58f20f..eb2a69bf3 100644
> >> --- a/utilities/.gitignore
> >> +++ b/utilities/.gitignore
> >> @@ -13,6 +13,7 @@
> >>   /ovs-dpctl.8
> >>   /ovs-dpctl-top
> >>   /ovs-dpctl-top.8
> >> +/ovs-kmod-ctl
> >>   /ovs-l3ping
> >>   /ovs-l3ping.8
> >>   /ovs-lib
> >> diff --git a/utilities/automake.mk b/utilities/automake.mk
> >> index 60cf1c5ed..d8f2374a3 100644
> >> --- a/utilities/automake.mk
> >> +++ b/utilities/automake.mk
> >> @@ -20,6 +20,7 @@ endif
> >>   scripts_SCRIPTS += \
> >>          utilities/ovs-check-dead-ifs \
> >>          utilities/ovs-ctl \
> >> +       utilities/ovs-kmod-ctl \
> >>          utilities/ovs-save
> >>   scripts_DATA += utilities/ovs-lib
> >
> >> @@ -44,6 +45,7 @@ EXTRA_DIST += \
> >>          utilities/ovs-dev.py \
> >>          utilities/ovs-docker \
> >>          utilities/ovs-dpctl-top.in \
> >> +       utilities/ovs-kmod-ctl.in \
> >>          utilities/ovs-l3ping.in \
> >>          utilities/ovs-lib.in \
> >>          utilities/ovs-parse-backtrace.in \
> >> @@ -63,6 +65,7 @@ MAN_ROOTS += \
> >>          utilities/ovs-ctl.8 \
> >>          utilities/ovs-dpctl.8.in \
> >>          utilities/ovs-dpctl-top.8.in \
> >> +       utilities/ovs-kmod-ctl.8 \
> >>          utilities/ovs-l3ping.8.in \
> >>          utilities/ovs-ofctl.8.in \
> >>          utilities/ovs-parse-backtrace.8 \
> >> @@ -81,6 +84,7 @@ CLEANFILES += \
> >>          utilities/ovs-dpctl.8 \
> >>          utilities/ovs-dpctl-top \
> >>          utilities/ovs-dpctl-top.8 \
> >> +       utilities/ovs-kmod-ctl \
> >>          utilities/ovs-l3ping \
> >>          utilities/ovs-l3ping.8 \
> >>          utilities/ovs-lib \
> >> @@ -107,6 +111,7 @@ man_MANS += \
> >>          utilities/ovs-testcontroller.8 \
> >>          utilities/ovs-dpctl.8 \
> >>          utilities/ovs-dpctl-top.8 \
> >> +       utilities/ovs-kmod-ctl.8 \
> >>          utilities/ovs-l3ping.8 \
> >>          utilities/ovs-ofctl.8 \
> >>          utilities/ovs-parse-backtrace.8 \
> >> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> >> index ef06dd967..8fdf8909a 100755
> >> --- a/utilities/ovs-ctl.in
> >> +++ b/utilities/ovs-ctl.in
> >> @@ -30,37 +30,9 @@ done
> >>   ## start ##
> >>   ## ----- ##
> >
> >> -insert_mods () {
> >> -    # Try loading openvswitch again.
> >> -    action "Inserting openvswitch module" modprobe openvswitch
> >> -}
> >> -
> >>   insert_mod_if_required () {
> >> -    # If this kernel has no module support, expect we're done.
> >> -    if test ! -e /proc/modules
> >> -    then
> >> -        log_success_msg "Kernel has no loadable module support.
Skipping
> > modprobe"
> >> -        return 0
> >> -    fi
> >> -
> >> -    # If openvswitch is already loaded then we're done.
> >> -    test -e /sys/module/openvswitch && return 0
> >> -
> >> -    # Load openvswitch.  If that's successful then we're done.
> >> -    insert_mods && return 0
> >> -
> >> -    # If the bridge module is loaded, then that might be blocking
> >> -    # openvswitch.  Try to unload it, if there are no bridges.
> >> -    test -e /sys/module/bridge || return 1
> >> -    bridges=`echo /sys/class/net/*/bridge | sed
> > 's,/sys/class/net/,,g;s,/bridge,,g'`
> >> -    if test "$bridges" != "*"; then
> >> -        log_warning_msg "not removing bridge module because bridges
> > exist ($bridges)"
> >> -        return 1
> >> -    fi
> >> -    action "removing bridge module" rmmod bridge || return 1
> >> -
> >> -    # Try loading openvswitch again.
> >> -    insert_mods
> >> +    ## This takes care of inserting any required kernel modules
> >> +    ovs_kmod_ctl insert
> >>   }
> >
> >>   set_hostname () {
> >> diff --git a/utilities/ovs-kmod-ctl.8 b/utilities/ovs-kmod-ctl.8
> >> new file mode 100644
> >> index 000000000..c36638e79
> >> --- /dev/null
> >> +++ b/utilities/ovs-kmod-ctl.8
> >> @@ -0,0 +1,109 @@
> >> +.\" -*- nroff -*-
> >> +.de IQ
> >> +.  br
> >> +.  ns
> >> +.  IP "\\$1"
> >> +..
> >> +.de ST
> >> +.  PP
> >> +.  RS -0.15in
> >> +.  I "\\$1"
> >> +.  RE
> >> +..
> >> +.TH ovs\-ctl 8 "February 2018" "Open vSwitch" "Open vSwitch Manual"
> >> +.ds PN ovs\-ctl
> >> +.
> >> +.SH NAME
> >> +ovs\-kmod\-ctl \- OVS startup helper script for loading kernel modules
> >> +.
> >> +.SH SYNOPSIS
> >> +\fBovs\-kmod\-ctl\fR \fBinsert
> >> +.br
> >> +\fBovs\-kmod\-ctl \fBremove
> >> +.br
> >> +\fBovs\-kmod\-ctl help \fR| \fB\-h \fR| \fB\-\-help
> >> +.br
> >> +\fBovs\-kmod\-ctl \-\-version
> >> +.br
> >> +\fBovs\-kmod\-ctl version
> >> +.
> >> +.SH DESCRIPTION
> >> +.
> >> +.PP
> >> +The \fBovs\-kmod\-ctl\fR program is responsible for inserting and
> >> +removing Open vSwitch kernel modules.  It is not meant to be invoked
> >> +directly by system administrators but to be called internally by
> >> +system startup scripts.  The script is used as part of an SELinux
> >> +transition domain.
> >> +.
> >> +.PP
> >> +Each of \fBovs\-kmod\-ctl\fR's commands is described separately below.
> >> +.
> >> +.SH "The ``insert'' command"
> >> +.
> >> +.PP
> >> +The \fBinsert\fR command loads the Open vSwitch kernel modules, if
> >> +needed.  If this fails, and the Linux bridge module is loaded but no
> >> +bridges exist, it tries to unload the bridge module and tries loading
> >> +the Open vSwitch kernel module again.
> >> +.
> >> +.SH "The ``remove'' command"
> >> +.
> >> +.PP
> >> +The \fBremove\fR command unloads the Open vSwitch kernel module
> > (including
> >> +the bridge compatibility module, if loaded) and any associated vport
> >> +modules.
> >> +.
> >> +.SH "EXIT STATUS"
> >> +.
> >> +\fBovs\-kmod\-ctl\fR exits with status 0 on success and nonzero on
> >> +failure.  The \fBinsert\fR command is considered to succeed if kernel
> >> +modules are already loaded; the \fBremove\fR command is considered to
> >> +succeed if none of the kernel modules are loaded.
> >> +.
> >> +.SH "ENVIRONMENT"
> >> +.
> >> +The following environment variables affect \fBovs\-kmod\-ctl\fR:
> >> +.
> >> +.IP "\fBPATH\fR"
> >> +\fBovs\-kmod\-ctl\fR does not hardcode the location of any of the
> > programs
> >> +that it runs.  \fBovs\-kmod\-ctl\fR will add the \fIsbindir\fR and
> >> +\fIbindir\fR that were specified at \fBconfigure\fR time to
> >> +\fBPATH\fR, if they are not already present.
> >> +.
> >> +.IP "\fBOVS_LOGDIR\fR"
> >> +.IQ "\fBOVS_RUNDIR\fR"
> >> +.IQ "\fBOVS_DBDIR\fR"
> >> +.IQ "\fBOVS_SYSCONFDIR\fR"
> >> +.IQ "\fBOVS_PKGDATADIR\fR"
> >> +.IQ "\fBOVS_BINDIR\fR"
> >> +.IQ "\fBOVS_SBINDIR\fR"
> >> +Setting one of these variables in the environment overrides the
> >> +respective \fBconfigure\fR option, both for \fBovs\-kmod\-ctl\fR
itself
> >> +and for the other Open vSwitch programs that it runs.
> >> +.
> >> +.SH "FILES"
> >> +.
> >> +\fBovs\-kmod\-ctl\fR uses the following files:
> >> +.
> >> +.IP "\fBovs\-lib"
> >> +Shell function library used internally by \fBovs\-kmod\-ctl\fR.  It
must
> >> +be installed in the same directory as \fBovs\-kmod\-ctl\fR.
> >> +.
> >> +.SH "EXAMPLE"
> >> +.
> >> +.PP
> >> +\fBovs\-kmod\-ctl\fR isn't intended to be manually executed.  However,
> > the
> >> +following examples demonstrate loading the kernel modules.
> >> +.
> >> +.TP
> >> +\fBovs\-kmod\-ctl\fR insert
> >> +Attempts to insert the Open vSwitch kernel modules.
> >> +.
> >> +.TP
> >> +\fBovs\-kmod\-ctl\fR remove
> >> +Attempts to remove the Open vSwitch kernel modules.
> >> +.
> >> +.SH "SEE ALSO"
> >> +.
> >> +\fBREADME.rst\fR, \fBovs\-ctl\fR(8)
> >> diff --git a/utilities/ovs-kmod-ctl.in b/utilities/ovs-kmod-ctl.in
> >> new file mode 100644
> >> index 000000000..b85f6cd04
> >> --- /dev/null
> >> +++ b/utilities/ovs-kmod-ctl.in
> >> @@ -0,0 +1,228 @@
> >> +#! /bin/sh
> >> +# Copyright (C) 2018 Red Hat, Inc.
> >> +#
> >> +# Licensed under the Apache License, Version 2.0 (the "License");
> >> +# you may not use this file except in compliance with the License.
> >> +# You may obtain a copy of the License at:
> >> +#
> >> +#     http://www.apache.org/licenses/LICENSE-2.0
> >> +#
> >> +# Unless required by applicable law or agreed to in writing, software
> >> +# distributed under the License is distributed on an "AS IS" BASIS,
> >> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > implied.
> >> +# See the License for the specific language governing permissions and
> >> +# limitations under the License.
> >> +
> >> +case $0 in
> >> +    */*) dir0=`echo "$0" | sed 's,/[^/]*$,,'` ;;
> >
> > I understand that you have borrowed this code from already existing OVS
> > code base so it is not like you are introducing anything new, but merely
> > refactoring. So I don't expect you to amend your patch unless you have
> > quick idea. However,
> >
> > 1. does the `sed` command above do the same thing as `dirname`? If yes,
> > then I am curious why we have not historically used `dirname`?

> That's a good question.  They do behave somewhat differently (for
> instance, GNU dirname will substitute a '.' if it doesn't find any path
> separator, whereas the sed command above will not).  I'm not sure that
> the distinction is that important, since the case requires a '/', so
> perhaps it's a historical thing.

> > 2. I don't like the approach how $dir0 is being used to source ovs-lib.
Is
> > there a way to hardcode paths in these script? It would leave less
> > variables that attacker could try to leverage. For example, if one
creates
> > a symlink with:
> >
> > # ln -s  /usr/share/openvswitch/scripts/ovs-kmod-ctl
/tmp/bogus-ovs-kmod-ctl
> >
> > and then create malicous shell file:
> >
> > /tmp/ovs-lib
> >
> > Then /tmp/bogus-ovs-kmod-ctl will uncoditionally source the bogus
> > /tmp/ovs-lib. Of course, for something like this to be exploitable in
real
> > life it implies that attacker must know other exploits, like:
> >
> > 1. create such symlink to ovs-ctl in first place; AND
> > 2. create bogus ovs-lib file in the same directory (without +x is ok) or
> > create another symlink to shell file. AND
> > 3. execute the symnlink (with or without any arguments. This is the main
> > ingredient making this attack possible);

> don't forget:
> 4. Set their source domain to openvswitch_t to force the load.

Yes, SElinux for sure makes things more secure in this case. However, we
can't assume that SElinux is available on all platforms and that all
daemons are confined through SElinux on the given host (i.e. if there is a
buggy daemon using only Discretionary Access Control then it will ignore
SElinux labels; and just by having privileges to symlink stuff it may be
capable to execute arbitrary shell scripts on the host). But I don't see
this as something critical just yet because as precondition it implies a
bug somewhere outside OVS too and we would just "level it up".

> But yes, I'll spin a correction here and hardcode the path from the
> result of --configure.  It makes sense to me.

That would be great, but to avoiod scope creep I am also completely ok if
you decide to do that outside this series, because that code historically
was already using $dir0 to source ovs-lib.

Thanks for making OVS more secure! I will wrap review of remaining patches
today.


> >> +    *) dir0=./ ;;
> >> +esac
> >> +. "$dir0/ovs-lib" || exit 1
> >> +
> >> +for dir in "$sbindir" "$bindir" /sbin /bin /usr/sbin /usr/bin; do
> >> +    case :$PATH: in
> >> +        *:$dir:*) ;;
> >> +        *) PATH=$PATH:$dir ;;
> >> +    esac
> >> +done
> >> +
> >> +insert_mods () {
> >> +    # Try loading openvswitch again.
> > I think the comment above is redundant, because only caller knows
whether
> > this is second attempt to again load kernel module.

> True, I'll remove it.

> >> +    action "Inserting openvswitch module" modprobe openvswitch
> >> +}
> >> +
> >> +insert_kmod_if_required() {
> > I would rename this function "insert_kmods_if_required" to make it
> > consistent with "remove_kmods" and "insert_kmods" that are in plural.

> Ack.

> >> +    # If this kernel has no module support, expect we're done.
> >> +    if test ! -e /proc/modules
> >> +    then
> >> +        log_success_msg "Kernel has no loadable module support.
Skipping
> > modprobe"
> >> +        return 0
> >> +    fi
> >> +
> >> +    # If openvswitch is already loaded then we're done.
> >> +    test -e /sys/module/openvswitch && return 0
> >> +
> >> +    # Load openvswitch.  If that's successful then we're done.
> >> +    insert_mods && return 0
> >> +
> >> +    # If the bridge module is loaded, then that might be blocking
> >> +    # openvswitch.  Try to unload it, if there are no bridges.
> >> +    test -e /sys/module/bridge || return 1
> >> +    bridges=`echo /sys/class/net/*/bridge | sed
> > 's,/sys/class/net/,,g;s,/bridge,,g'`
> >> +    if test "$bridges" != "*"; then
> >> +        log_warning_msg "not removing bridge module because bridges
> > exist ($bridges)"
> >> +        return 1
> >> +    fi
> >> +    action "removing bridge module" rmmod bridge || return 1
> >> +
> >> +    # Try loading openvswitch again.
> >> +    insert_mods
> >> +}
> >> +
> >> +remove_kmods() {
> >> +    for vport in `awk '/^vport_/ { print $1 }' /proc/modules`; do
> >> +        action "Removing $vport module" rmmod $vport
> >> +    done
> >> +
> >> +    if test -e /sys/module/openvswitch; then
> >> +        action "Removing openvswitch module" rmmod openvswitch
> >> +    fi
> >> +}
> >> +
> >> +set_defaults () {
> >
> > I am confused about these defaults. Are they declared in two places now?

> Seems like, and I think they aren't needed even.  I'll trim the list
> completely.

> > root@ubuntubuilder:/git/ovs# git grep -C4  "DELETE_BRIDGES=no"
> > utilities/ovs-ctl.in-
> > utilities/ovs-ctl.in-set_defaults () {
> > utilities/ovs-ctl.in-    SYSTEM_ID=
> > utilities/ovs-ctl.in-
> > utilities/ovs-ctl.in:    DELETE_BRIDGES=no
> > utilities/ovs-ctl.in-    DELETE_TRANSIENT_PORTS=no
> > utilities/ovs-ctl.in-
> > utilities/ovs-ctl.in-    DAEMON_CWD=/
> > utilities/ovs-ctl.in-    FORCE_COREFILES=yes
> > --
> > utilities/ovs-kmod-ctl.in-
> > utilities/ovs-kmod-ctl.in-set_defaults () {
> > utilities/ovs-kmod-ctl.in-    SYSTEM_ID=
> > utilities/ovs-kmod-ctl.in-
> > utilities/ovs-kmod-ctl.in:    DELETE_BRIDGES=no
> > utilities/ovs-kmod-ctl.in-    DELETE_TRANSIENT_PORTS=no
> > utilities/ovs-kmod-ctl.in-
> > utilities/ovs-kmod-ctl.in-    DAEMON_CWD=/
> > utilities/ovs-kmod-ctl.in-    FORCE_COREFILES=yes
> >
> >
> >> +    SYSTEM_ID=
> >> +
> >> +    DELETE_BRIDGES=no
> >> +    DELETE_TRANSIENT_PORTS=no
> >> +
> >> +    DAEMON_CWD=/
> >> +    FORCE_COREFILES=yes
> >> +    MLOCKALL=yes
> >> +    SELF_CONFINEMENT=yes
> >> +    MONITOR=yes
> >> +    OVS_USER=
> >> +    OVSDB_SERVER=yes
> >> +    OVS_VSWITCHD=yes
> >> +    OVSDB_SERVER_PRIORITY=-10
> >> +    OVS_VSWITCHD_PRIORITY=-10
> >> +    OVSDB_SERVER_WRAPPER=
> >> +    OVS_VSWITCHD_WRAPPER=
> >> +
> >> +    DB_FILE=$dbdir/conf.db
> >> +    DB_SOCK=$rundir/db.sock
> >> +    DB_SCHEMA=$datadir/vswitch.ovsschema
> >> +    EXTRA_DBS=
> >> +
> >> +    PROTOCOL=gre
> >> +    DPORT=
> >> +    SPORT=
> >> +
> >> +    type_file=$etcdir/system-type.conf
> >> +    version_file=$etcdir/system-version.conf
> >> +
> >> +    if test -e "$type_file" ; then
> >> +        SYSTEM_TYPE=`cat $type_file`
> >> +        SYSTEM_VERSION=`cat $version_file`
> >> +    elif test -e "@sysconfdir@/os-release"; then
> >> +        SYSTEM_TYPE=`. '@sysconfdir@/os-release' && echo "$ID"`
> >> +        SYSTEM_VERSION=`. '@sysconfdir@/os-release' && echo
> > "$VERSION_ID"`
> >> +    elif (lsb_release --id) >/dev/null 2>&1; then
> >> +        SYSTEM_TYPE=`lsb_release --id -s`
> >> +        system_release=`lsb_release --release -s`
> >> +        system_codename=`lsb_release --codename -s`
> >> +        SYSTEM_VERSION="${system_release}-${system_codename}"
> >> +    else
> >> +        SYSTEM_TYPE=unknown
> >> +        SYSTEM_VERSION=unknown
> >> +    fi
> >> +}
> >> +
> >> +usage () {
> >> +    set_defaults
> >> +    cat <<EOF
> >> +$0: controls Open vSwitch kernel modules
> >> +usage: $0 [OPTIONS] COMMAND
> >> +
> >> +This program is intended to be invoked internally by Open vSwitch
startup
> >> +scripts.  System administrators should not normally invoke it
directly.
> >> +
> >> +Commands:
> >> +  insert                  insert the Open vSwitch kernel modules
> >> +  remove                  remove the Open vSwitch kernel modules
> >> +
> >> +Options:
> >> +  -h, --help              display this help message
> >> +  -V, --version           display version information
> >> +
> >> +Default directories with "configure" option and environment variable
> > override:
> >> +  logs: @LOGDIR@ (--with-logdir, OVS_LOGDIR)
> >> +  pidfiles and sockets: @RUNDIR@ (--with-rundir, OVS_RUNDIR)
> >> +  conf.db: @DBDIR@ (--with-dbdir, OVS_DBDIR)
> >> +  system configuration: @sysconfdir@ (--sysconfdir, OVS_SYSCONFDIR)
> >> +  data files: @pkgdatadir@ (--pkgdatadir, OVS_PKGDATADIR)
> >> +  user binaries: @bindir@ (--bindir, OVS_BINDIR)
> >> +  system binaries: @sbindir@ (--sbindir, OVS_SBINDIR)
> >> +
> >> +Please report bugs to bugs@openvswitch.org (see REPORTING-BUGS for
> > details).
> >> +EOF
> >> +
> >> +    exit 0
> >> +}
> >> +
> >> +set_option () {
> >> +    var=`echo "$option" | tr abcdefghijklmnopqrstuvwxyz-
> > ABCDEFGHIJKLMNOPQRSTUVWXYZ_`
> >> +    eval set=\${$var+yes}
> >> +    eval old_value=\$$var
> >> +    if test X$set = X || \
> >> +        (test $type = bool && \
> >> +        test X"$old_value" != Xno && test X"$old_value" != Xyes); then
> >> +        echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
> >> +        return
> >> +    fi
> >> +    eval $var=\$value
> >> +}
> >> +
> >> +set_defaults
> >> +extra_ids=
> >> +command=
> >> +for arg
> >> +do
> >> +    case $arg in
> >> +        -h | --help)
> >> +            usage
> >> +            ;;
> >> +        -V | --version)
> >> +            echo "$0 (Open vSwitch) $VERSION"
> >> +            exit 0
> >> +            ;;
> >> +        --[a-z]*=*)
> >> +            option=`expr X"$arg" : 'X--\([^=]*\)'`
> >> +            value=`expr X"$arg" : 'X[^=]*=\(.*\)'`
> >> +            type=string
> >> +            set_option
> >> +            ;;
> >> +        --no-[a-z]*)
> >> +            option=`expr X"$arg" : 'X--no-\(.*\)'`
> >> +            value=no
> >> +            type=bool
> >> +            set_option
> >> +            ;;
> >> +        --[a-z]*)
> >> +            option=`expr X"$arg" : 'X--\(.*\)'`
> >> +            value=yes
> >> +            type=bool
> >> +            set_option
> >> +            ;;
> >> +        -*)
> >> +            echo >&2 "$0: unknown option \"$arg\" (use --help for
help)"
> >> +            exit 1
> >> +            ;;
> >> +        *)
> >> +            if test X"$command" = X; then
> >> +                command=$arg
> >> +            else
> >> +                echo >&2 "$0: exactly one non-option argument required
> > (use --help for help)"
> >> +                exit 1
> >> +            fi
> >> +            ;;
> >> +    esac
> >> +done
> >> +case $command in
> >> +    remove)
> >> +        remove_kmods
> >> +        ;;
> >> +    insert)
> >> +        insert_kmod_if_required
> >> +        ;;
> >> +    help)
> >> +        usage
> >> +        ;;
> >> +    '')
> >> +        echo >&2 "$0: missing command name (use --help for help)"
> >> +        exit 1
> >> +        ;;
> >> +    *)
> >> +        echo >&2 "$0: unknown command \"$command\" (use --help for
help)"
> >> +        exit 1
> >> +        ;;
> >> +esac
> >> diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
> >> index 4c3ad0f0b..6a958cbdf 100644
> >> --- a/utilities/ovs-lib.in
> >> +++ b/utilities/ovs-lib.in
> >> @@ -503,6 +503,10 @@ ovs_vsctl () {
> >>   ## force-reload-kmod ##
> >>   ## ----------------- ##
> >
> >> +ovs_kmod_ctl () {
> >> +    "$dir0/ovs-kmod-ctl" "$@"
> >> +}
> >> +
> >>   internal_interfaces () {
> >>       # Outputs a list of internal interfaces:
> >>       #
> >> @@ -618,13 +622,7 @@ force_reload_kmod () {
> >>       done
> >>       action "ovs-appctl dpctl/flush-conntrack"
> >
> >> -    for vport in `awk '/^vport_/ { print $1 }' /proc/modules`; do
> >> -        action "Removing $vport module" rmmod $vport
> >> -    done
> >> -
> >> -    if test -e /sys/module/openvswitch; then
> >> -        action "Removing openvswitch module" rmmod openvswitch
> >> -    fi
> >> +    ovs_kmod_ctl remove
> >
> >>       # Start vswitchd by asking it to wait till flow restore is
finished.
> >>       flow_restore_wait
> >> --
> >> 2.14.3
diff mbox series

Patch

diff --git a/debian/openvswitch-switch.install b/debian/openvswitch-switch.install
index bfb391fe8..6a6e9a543 100644
--- a/debian/openvswitch-switch.install
+++ b/debian/openvswitch-switch.install
@@ -12,5 +12,6 @@  usr/sbin/ovs-vswitchd
 usr/sbin/ovsdb-server
 usr/share/openvswitch/scripts/ovs-check-dead-ifs
 usr/share/openvswitch/scripts/ovs-ctl
+usr/share/openvswitch/scripts/ovs-kmod-ctl
 usr/share/openvswitch/scripts/ovs-save
 usr/share/openvswitch/vswitch.ovsschema
diff --git a/debian/openvswitch-switch.manpages b/debian/openvswitch-switch.manpages
index c85cbfd30..1161cfda7 100644
--- a/debian/openvswitch-switch.manpages
+++ b/debian/openvswitch-switch.manpages
@@ -3,6 +3,7 @@  ovsdb/ovsdb-server.5
 utilities/ovs-ctl.8
 utilities/ovs-dpctl-top.8
 utilities/ovs-dpctl.8
+utilities/ovs-kmod-ctl.8
 utilities/ovs-pcap.1
 utilities/ovs-tcpdump.8
 utilities/ovs-tcpundump.1
diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 3e5cf21e0..bf4526de2 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -545,6 +545,7 @@  fi
 %{_datadir}/openvswitch/scripts/ovs-save
 %{_datadir}/openvswitch/scripts/ovs-vtep
 %{_datadir}/openvswitch/scripts/ovs-ctl
+%{_datadir}/openvswitch/scripts/ovs-kmod-ctl
 %{_datadir}/openvswitch/scripts/ovs-systemd-reload
 %config %{_datadir}/openvswitch/vswitch.ovsschema
 %config %{_datadir}/openvswitch/vtep.ovsschema
@@ -578,6 +579,7 @@  fi
 %{_mandir}/man8/ovs-ctl.8*
 %{_mandir}/man8/ovs-dpctl.8*
 %{_mandir}/man8/ovs-dpctl-top.8*
+%{_mandir}/man8/ovs-kmod-ctl.8*
 %{_mandir}/man8/ovs-ofctl.8*
 %{_mandir}/man8/ovs-pki.8*
 %{_mandir}/man8/ovs-vsctl.8*
diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
index 2c5f0409a..883d25607 100644
--- a/rhel/openvswitch.spec.in
+++ b/rhel/openvswitch.spec.in
@@ -237,6 +237,7 @@  exit 0
 /usr/share/man/man8/ovs-ctl.8.gz
 /usr/share/man/man8/ovs-dpctl.8.gz
 /usr/share/man/man8/ovs-dpctl-top.8.gz
+/usr/share/man/man8/ovs-kmod-ctl.8.gz
 /usr/share/man/man8/ovs-ofctl.8.gz
 /usr/share/man/man8/ovs-parse-backtrace.8.gz
 /usr/share/man/man8/ovs-pki.8.gz
@@ -250,6 +251,7 @@  exit 0
 /usr/share/openvswitch/scripts/ovs-bugtool-*
 /usr/share/openvswitch/scripts/ovs-check-dead-ifs
 /usr/share/openvswitch/scripts/ovs-ctl
+/usr/share/openvswitch/scripts/ovs-kmod-ctl
 /usr/share/openvswitch/scripts/ovs-lib
 /usr/share/openvswitch/scripts/ovs-save
 /usr/share/openvswitch/scripts/ovs-vtep
diff --git a/utilities/.gitignore b/utilities/.gitignore
index 34c58f20f..eb2a69bf3 100644
--- a/utilities/.gitignore
+++ b/utilities/.gitignore
@@ -13,6 +13,7 @@ 
 /ovs-dpctl.8
 /ovs-dpctl-top
 /ovs-dpctl-top.8
+/ovs-kmod-ctl
 /ovs-l3ping
 /ovs-l3ping.8
 /ovs-lib
diff --git a/utilities/automake.mk b/utilities/automake.mk
index 60cf1c5ed..d8f2374a3 100644
--- a/utilities/automake.mk
+++ b/utilities/automake.mk
@@ -20,6 +20,7 @@  endif
 scripts_SCRIPTS += \
 	utilities/ovs-check-dead-ifs \
 	utilities/ovs-ctl \
+	utilities/ovs-kmod-ctl \
 	utilities/ovs-save
 scripts_DATA += utilities/ovs-lib
 
@@ -44,6 +45,7 @@  EXTRA_DIST += \
 	utilities/ovs-dev.py \
 	utilities/ovs-docker \
 	utilities/ovs-dpctl-top.in \
+	utilities/ovs-kmod-ctl.in \
 	utilities/ovs-l3ping.in \
 	utilities/ovs-lib.in \
 	utilities/ovs-parse-backtrace.in \
@@ -63,6 +65,7 @@  MAN_ROOTS += \
 	utilities/ovs-ctl.8 \
 	utilities/ovs-dpctl.8.in \
 	utilities/ovs-dpctl-top.8.in \
+	utilities/ovs-kmod-ctl.8 \
 	utilities/ovs-l3ping.8.in \
 	utilities/ovs-ofctl.8.in \
 	utilities/ovs-parse-backtrace.8 \
@@ -81,6 +84,7 @@  CLEANFILES += \
 	utilities/ovs-dpctl.8 \
 	utilities/ovs-dpctl-top \
 	utilities/ovs-dpctl-top.8 \
+	utilities/ovs-kmod-ctl \
 	utilities/ovs-l3ping \
 	utilities/ovs-l3ping.8 \
 	utilities/ovs-lib \
@@ -107,6 +111,7 @@  man_MANS += \
 	utilities/ovs-testcontroller.8 \
 	utilities/ovs-dpctl.8 \
 	utilities/ovs-dpctl-top.8 \
+	utilities/ovs-kmod-ctl.8 \
 	utilities/ovs-l3ping.8 \
 	utilities/ovs-ofctl.8 \
 	utilities/ovs-parse-backtrace.8 \
diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
index ef06dd967..8fdf8909a 100755
--- a/utilities/ovs-ctl.in
+++ b/utilities/ovs-ctl.in
@@ -30,37 +30,9 @@  done
 ## start ##
 ## ----- ##
 
-insert_mods () {
-    # Try loading openvswitch again.
-    action "Inserting openvswitch module" modprobe openvswitch
-}
-
 insert_mod_if_required () {
-    # If this kernel has no module support, expect we're done.
-    if test ! -e /proc/modules
-    then
-        log_success_msg "Kernel has no loadable module support. Skipping modprobe"
-        return 0
-    fi
-
-    # If openvswitch is already loaded then we're done.
-    test -e /sys/module/openvswitch && return 0
-
-    # Load openvswitch.  If that's successful then we're done.
-    insert_mods && return 0
-
-    # If the bridge module is loaded, then that might be blocking
-    # openvswitch.  Try to unload it, if there are no bridges.
-    test -e /sys/module/bridge || return 1
-    bridges=`echo /sys/class/net/*/bridge | sed 's,/sys/class/net/,,g;s,/bridge,,g'`
-    if test "$bridges" != "*"; then
-        log_warning_msg "not removing bridge module because bridges exist ($bridges)"
-        return 1
-    fi
-    action "removing bridge module" rmmod bridge || return 1
-
-    # Try loading openvswitch again.
-    insert_mods
+    ## This takes care of inserting any required kernel modules
+    ovs_kmod_ctl insert
 }
 
 set_hostname () {
diff --git a/utilities/ovs-kmod-ctl.8 b/utilities/ovs-kmod-ctl.8
new file mode 100644
index 000000000..c36638e79
--- /dev/null
+++ b/utilities/ovs-kmod-ctl.8
@@ -0,0 +1,109 @@ 
+.\" -*- nroff -*-
+.de IQ
+.  br
+.  ns
+.  IP "\\$1"
+..
+.de ST
+.  PP
+.  RS -0.15in
+.  I "\\$1"
+.  RE
+..
+.TH ovs\-ctl 8 "February 2018" "Open vSwitch" "Open vSwitch Manual"
+.ds PN ovs\-ctl
+.
+.SH NAME
+ovs\-kmod\-ctl \- OVS startup helper script for loading kernel modules
+.
+.SH SYNOPSIS
+\fBovs\-kmod\-ctl\fR \fBinsert
+.br
+\fBovs\-kmod\-ctl \fBremove
+.br
+\fBovs\-kmod\-ctl help \fR| \fB\-h \fR| \fB\-\-help
+.br
+\fBovs\-kmod\-ctl \-\-version
+.br
+\fBovs\-kmod\-ctl version
+.
+.SH DESCRIPTION
+.
+.PP
+The \fBovs\-kmod\-ctl\fR program is responsible for inserting and
+removing Open vSwitch kernel modules.  It is not meant to be invoked
+directly by system administrators but to be called internally by
+system startup scripts.  The script is used as part of an SELinux
+transition domain.
+.
+.PP
+Each of \fBovs\-kmod\-ctl\fR's commands is described separately below.
+.
+.SH "The ``insert'' command"
+.
+.PP
+The \fBinsert\fR command loads the Open vSwitch kernel modules, if
+needed.  If this fails, and the Linux bridge module is loaded but no
+bridges exist, it tries to unload the bridge module and tries loading
+the Open vSwitch kernel module again.
+.
+.SH "The ``remove'' command"
+.
+.PP
+The \fBremove\fR command unloads the Open vSwitch kernel module (including
+the bridge compatibility module, if loaded) and any associated vport
+modules.
+.
+.SH "EXIT STATUS"
+.
+\fBovs\-kmod\-ctl\fR exits with status 0 on success and nonzero on
+failure.  The \fBinsert\fR command is considered to succeed if kernel
+modules are already loaded; the \fBremove\fR command is considered to
+succeed if none of the kernel modules are loaded.
+.
+.SH "ENVIRONMENT"
+.
+The following environment variables affect \fBovs\-kmod\-ctl\fR:
+.
+.IP "\fBPATH\fR"
+\fBovs\-kmod\-ctl\fR does not hardcode the location of any of the programs
+that it runs.  \fBovs\-kmod\-ctl\fR will add the \fIsbindir\fR and
+\fIbindir\fR that were specified at \fBconfigure\fR time to
+\fBPATH\fR, if they are not already present.
+.
+.IP "\fBOVS_LOGDIR\fR"
+.IQ "\fBOVS_RUNDIR\fR"
+.IQ "\fBOVS_DBDIR\fR"
+.IQ "\fBOVS_SYSCONFDIR\fR"
+.IQ "\fBOVS_PKGDATADIR\fR"
+.IQ "\fBOVS_BINDIR\fR"
+.IQ "\fBOVS_SBINDIR\fR"
+Setting one of these variables in the environment overrides the
+respective \fBconfigure\fR option, both for \fBovs\-kmod\-ctl\fR itself
+and for the other Open vSwitch programs that it runs.
+.
+.SH "FILES"
+.
+\fBovs\-kmod\-ctl\fR uses the following files:
+.
+.IP "\fBovs\-lib"
+Shell function library used internally by \fBovs\-kmod\-ctl\fR.  It must
+be installed in the same directory as \fBovs\-kmod\-ctl\fR.
+.
+.SH "EXAMPLE"
+.
+.PP
+\fBovs\-kmod\-ctl\fR isn't intended to be manually executed.  However, the
+following examples demonstrate loading the kernel modules.
+.
+.TP
+\fBovs\-kmod\-ctl\fR insert
+Attempts to insert the Open vSwitch kernel modules.
+.
+.TP
+\fBovs\-kmod\-ctl\fR remove
+Attempts to remove the Open vSwitch kernel modules.
+.
+.SH "SEE ALSO"
+.
+\fBREADME.rst\fR, \fBovs\-ctl\fR(8)
diff --git a/utilities/ovs-kmod-ctl.in b/utilities/ovs-kmod-ctl.in
new file mode 100644
index 000000000..b85f6cd04
--- /dev/null
+++ b/utilities/ovs-kmod-ctl.in
@@ -0,0 +1,228 @@ 
+#! /bin/sh
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at:
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+case $0 in
+    */*) dir0=`echo "$0" | sed 's,/[^/]*$,,'` ;;
+    *) dir0=./ ;;
+esac
+. "$dir0/ovs-lib" || exit 1
+
+for dir in "$sbindir" "$bindir" /sbin /bin /usr/sbin /usr/bin; do
+    case :$PATH: in
+        *:$dir:*) ;;
+        *) PATH=$PATH:$dir ;;
+    esac
+done
+
+insert_mods () {
+    # Try loading openvswitch again.
+    action "Inserting openvswitch module" modprobe openvswitch
+}
+
+insert_kmod_if_required() {
+    # If this kernel has no module support, expect we're done.
+    if test ! -e /proc/modules
+    then
+        log_success_msg "Kernel has no loadable module support. Skipping modprobe"
+        return 0
+    fi
+
+    # If openvswitch is already loaded then we're done.
+    test -e /sys/module/openvswitch && return 0
+
+    # Load openvswitch.  If that's successful then we're done.
+    insert_mods && return 0
+
+    # If the bridge module is loaded, then that might be blocking
+    # openvswitch.  Try to unload it, if there are no bridges.
+    test -e /sys/module/bridge || return 1
+    bridges=`echo /sys/class/net/*/bridge | sed 's,/sys/class/net/,,g;s,/bridge,,g'`
+    if test "$bridges" != "*"; then
+        log_warning_msg "not removing bridge module because bridges exist ($bridges)"
+        return 1
+    fi
+    action "removing bridge module" rmmod bridge || return 1
+
+    # Try loading openvswitch again.
+    insert_mods
+}
+
+remove_kmods() {
+    for vport in `awk '/^vport_/ { print $1 }' /proc/modules`; do
+        action "Removing $vport module" rmmod $vport
+    done
+
+    if test -e /sys/module/openvswitch; then
+        action "Removing openvswitch module" rmmod openvswitch
+    fi
+}
+
+set_defaults () {
+    SYSTEM_ID=
+
+    DELETE_BRIDGES=no
+    DELETE_TRANSIENT_PORTS=no
+
+    DAEMON_CWD=/
+    FORCE_COREFILES=yes
+    MLOCKALL=yes
+    SELF_CONFINEMENT=yes
+    MONITOR=yes
+    OVS_USER=
+    OVSDB_SERVER=yes
+    OVS_VSWITCHD=yes
+    OVSDB_SERVER_PRIORITY=-10
+    OVS_VSWITCHD_PRIORITY=-10
+    OVSDB_SERVER_WRAPPER=
+    OVS_VSWITCHD_WRAPPER=
+
+    DB_FILE=$dbdir/conf.db
+    DB_SOCK=$rundir/db.sock
+    DB_SCHEMA=$datadir/vswitch.ovsschema
+    EXTRA_DBS=
+
+    PROTOCOL=gre
+    DPORT=
+    SPORT=
+
+    type_file=$etcdir/system-type.conf
+    version_file=$etcdir/system-version.conf
+
+    if test -e "$type_file" ; then
+        SYSTEM_TYPE=`cat $type_file`
+        SYSTEM_VERSION=`cat $version_file`
+    elif test -e "@sysconfdir@/os-release"; then
+        SYSTEM_TYPE=`. '@sysconfdir@/os-release' && echo "$ID"`
+        SYSTEM_VERSION=`. '@sysconfdir@/os-release' && echo "$VERSION_ID"`
+    elif (lsb_release --id) >/dev/null 2>&1; then
+        SYSTEM_TYPE=`lsb_release --id -s`
+        system_release=`lsb_release --release -s`
+        system_codename=`lsb_release --codename -s`
+        SYSTEM_VERSION="${system_release}-${system_codename}"
+    else
+        SYSTEM_TYPE=unknown
+        SYSTEM_VERSION=unknown
+    fi
+}
+
+usage () {
+    set_defaults
+    cat <<EOF
+$0: controls Open vSwitch kernel modules
+usage: $0 [OPTIONS] COMMAND
+
+This program is intended to be invoked internally by Open vSwitch startup
+scripts.  System administrators should not normally invoke it directly.
+
+Commands:
+  insert                  insert the Open vSwitch kernel modules
+  remove                  remove the Open vSwitch kernel modules
+
+Options:
+  -h, --help              display this help message
+  -V, --version           display version information
+
+Default directories with "configure" option and environment variable override:
+  logs: @LOGDIR@ (--with-logdir, OVS_LOGDIR)
+  pidfiles and sockets: @RUNDIR@ (--with-rundir, OVS_RUNDIR)
+  conf.db: @DBDIR@ (--with-dbdir, OVS_DBDIR)
+  system configuration: @sysconfdir@ (--sysconfdir, OVS_SYSCONFDIR)
+  data files: @pkgdatadir@ (--pkgdatadir, OVS_PKGDATADIR)
+  user binaries: @bindir@ (--bindir, OVS_BINDIR)
+  system binaries: @sbindir@ (--sbindir, OVS_SBINDIR)
+
+Please report bugs to bugs@openvswitch.org (see REPORTING-BUGS for details).
+EOF
+
+    exit 0
+}
+
+set_option () {
+    var=`echo "$option" | tr abcdefghijklmnopqrstuvwxyz- ABCDEFGHIJKLMNOPQRSTUVWXYZ_`
+    eval set=\${$var+yes}
+    eval old_value=\$$var
+    if test X$set = X || \
+        (test $type = bool && \
+        test X"$old_value" != Xno && test X"$old_value" != Xyes); then
+        echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
+        return
+    fi
+    eval $var=\$value
+}
+
+set_defaults
+extra_ids=
+command=
+for arg
+do
+    case $arg in
+        -h | --help)
+            usage
+            ;;
+        -V | --version)
+            echo "$0 (Open vSwitch) $VERSION"
+            exit 0
+            ;;
+        --[a-z]*=*)
+            option=`expr X"$arg" : 'X--\([^=]*\)'`
+            value=`expr X"$arg" : 'X[^=]*=\(.*\)'`
+            type=string
+            set_option
+            ;;
+        --no-[a-z]*)
+            option=`expr X"$arg" : 'X--no-\(.*\)'`
+            value=no
+            type=bool
+            set_option
+            ;;
+        --[a-z]*)
+            option=`expr X"$arg" : 'X--\(.*\)'`
+            value=yes
+            type=bool
+            set_option
+            ;;
+        -*)
+            echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
+            exit 1
+            ;;
+        *)
+            if test X"$command" = X; then
+                command=$arg
+            else
+                echo >&2 "$0: exactly one non-option argument required (use --help for help)"
+                exit 1
+            fi
+            ;;
+    esac
+done
+case $command in
+    remove)
+        remove_kmods
+        ;;
+    insert)
+        insert_kmod_if_required
+        ;;
+    help)
+        usage
+        ;;
+    '')
+        echo >&2 "$0: missing command name (use --help for help)"
+        exit 1
+        ;;
+    *)
+        echo >&2 "$0: unknown command \"$command\" (use --help for help)"
+        exit 1
+        ;;
+esac
diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
index 4c3ad0f0b..6a958cbdf 100644
--- a/utilities/ovs-lib.in
+++ b/utilities/ovs-lib.in
@@ -503,6 +503,10 @@  ovs_vsctl () {
 ## force-reload-kmod ##
 ## ----------------- ##
 
+ovs_kmod_ctl () {
+    "$dir0/ovs-kmod-ctl" "$@"
+}
+
 internal_interfaces () {
     # Outputs a list of internal interfaces:
     #
@@ -618,13 +622,7 @@  force_reload_kmod () {
     done
     action "ovs-appctl dpctl/flush-conntrack"
 
-    for vport in `awk '/^vport_/ { print $1 }' /proc/modules`; do
-        action "Removing $vport module" rmmod $vport
-    done
-
-    if test -e /sys/module/openvswitch; then
-        action "Removing openvswitch module" rmmod openvswitch
-    fi
+    ovs_kmod_ctl remove
 
     # Start vswitchd by asking it to wait till flow restore is finished.
     flow_restore_wait