[ovs-dev] [PATCH 5/5] netdev-dpdk: Support user cfg vhost socket perms

Aaron Conole aconole at redhat.com
Sat Jan 30 17:32:50 UTC 2016


Thanks so much for the review, Ansis!

Ansis Atteka <aatteka at nicira.com> writes:
> On Fri, Dec 18, 2015 at 10:27 AM, Aaron Conole <aconole at redhat.com> wrote:
>> The current DPDK vhost socket user and group permissions are derived
>> during creation from the DPDK library. This patch adds an action, post
>> socket creation, to change the socket permissions and ownership to
>> support multi-user systems.
>>
>> This is especially useful when using vhost-user sockets from qemu, which
>> currently requires manual intervention to change the vhost-user sockets
>> allowing the qemu user access.
>>
>> Signed-off-by: Aaron Conole <aconole at redhat.com>
>> ---
>>  INSTALL.DPDK.md   | 17 +++++++++++++++
>>  lib/netdev-dpdk.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 78 insertions(+), 2 deletions(-)
>>
>> diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md
>> index b9d92d0..86f934c 100644
>> --- a/INSTALL.DPDK.md
>> +++ b/INSTALL.DPDK.md
>> @@ -554,6 +554,23 @@ have arbitrary names.
>>
>>      `./vswitchd/ovs-vsctl set Open_vSwitch . other_config:vhost_sock_dir=path`
>>
>> +  - You may specify optional user:group and permissions for the vhost-user
>> +    sockets. If these are not present, the default ownership is the executing
>> +    user and group for ovs-vswitchd, and the default permissions are `0600`.
>> +
>> +    To change the vhost-user ownership, you may modify the `vhost_sock_owners`
>> +    entry in the ovsdb like:
>> +
>> +    `./vswitchd/ovs-vsctl set Open_vSwitch . \
>> +                          other_config:vhost_sock_owners=[user][:group]`
>> +
>> +    To change the vhost-user permissions, you may modify the octal value
>> +    `vhost_sock_permissions` entry in the ovsdb like:
>> +
>> +    `./vswitchd/ovs-vsctl set Open_vSwitch . \
>> +                          other_config:vhost_sock_permissions=permissions`
>> +
>> +
>>  DPDK vhost-user VM configuration:
>>  ---------------------------------
>>  Follow the steps below to attach vhost-user port(s) to a VM.
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 696430f..ee59250 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -52,6 +52,7 @@
>>  #include "openvswitch/vlog.h"
>>  #include "smap.h"
>>  #include "vswitch-idl.h"
>> +#include "daemon.h"
>
> I would try to keep #includes in alphabetical order. The reason for
> this is that it reduces possibility of GIT conflicts if someone else
> pushed patch after you sent yours for review, but before revewer
> applied your patch in his repository.

Roger that!

>>
>>  #include "rte_config.h"
>>  #include "rte_mbuf.h"
>> @@ -106,6 +107,9 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>>
>>  static char *cuse_dev_name = NULL;    /* Character device cuse_dev_name. */
>>  static char *vhost_sock_dir = NULL;   /* Location of vhost-user sockets */
>> +static char *vhost_sock_owner = NULL; /* user:group notation for ownership
>> +                                         of vhost-user sockets */
>> +static char *vhost_sock_perms = NULL; /* OCTAL string of permission bits */
>>
>>  /*
>>   * Maximum amount of time in micro seconds to try and enqueue to vhost.
>> @@ -682,6 +686,43 @@ netdev_dpdk_vhost_cuse_construct(struct netdev *netdev_)
>>      return err;
>>  }
>>
>> +static void
>> +vhost_set_permissions(const char *vhost_sock_location)
>> +{
>> +    unsigned long int mode = strtoul(vhost_sock_perms, NULL, 0);
>> +    int err = chmod(vhost_sock_location, (mode_t)mode);
>> +    if (err) {
>> +        VLOG_ERR("vhost-user socket cannot set permissions to %s (%s).\n",
>> +                 vhost_sock_perms, ovs_strerror(err));
>> +        return;
>> +    }
>> +    VLOG_INFO("Socket %s changed permissions to %s\n", vhost_sock_location,
>> +              vhost_sock_perms);
>> +}
>> +
>> +static void
>> +vhost_set_ownership(const char *vhost_sock_location)
>> +{
>> +    uid_t vhuid;
>> +    gid_t vhgid;
>> +
>> +    if (get_owners_from_str(vhost_sock_owner, &vhuid, NULL, &vhgid, false)) {
>> +        VLOG_ERR("vhost-user socket unable to set ownership: %s\n",
>> +                 vhost_sock_owner);
>> +        return;
>> +    }
>> +
>> +    int err = chown(vhost_sock_location, vhuid, vhgid);
> FYI:
>
> chown and chmod need to be white listed in openvswitch SELinux policy
> - https://github.com/TresysTechnology/refpolicy-contrib.git.
> Otherwise, SELinux for OVS must be disabled on default Fedora, CentOS
> and RHEL distributions for this to work in the first place.
>
> However, ball for this is in my hands here, because I am working on a
> patch where tailored SELinux policy will be distributed from our own
> packages.

I'll help out here if I can, as well.

>> +    if (err) {
>> +        VLOG_ERR("vhost-user socket unable to set ownership to %s (%s).\n",
>> +                 vhost_sock_owner, ovs_strerror(err));
>> +        return;
>> +    }
>> +
>> +    VLOG_INFO("Socket %s changed ownership to %s.\n", vhost_sock_location,
>> +              vhost_sock_owner);
>> +}
>> +
>>  static int
>>  netdev_dpdk_vhost_user_construct(struct netdev *netdev_)
>>  {
>> @@ -700,6 +741,15 @@ netdev_dpdk_vhost_user_construct(struct netdev *netdev_)
>>                   netdev->vhost_id);
>>      }
>>      VLOG_INFO("Socket %s created for vhost-user port %s\n", netdev->vhost_id, netdev_->name);
>> +
>
>
> Try:
> ovs-vsctl set Open_vSwitch . other_config:vhost_sock_owners=:nobody
> <--- The first bug is that if group did not exist, then your code
> picks up the last group (ssh in my case)
> ovs-vsctl set Open_vSwitch . other_config:vhost_sock_permissions=0777

Heh, I actuall caught this, as well; I need to incorporate the chmod
values change suggested by Christian, and I can post an RFC if you want.

> root at prmh-nsx-perf-server118:~/ovs# echo "I want this file to stay
> under root:root" > /r
> root at prmh-nsx-perf-server118:~/ovs# ls -la /r
> -rw-r--r-- 1 root root 10 Jan 29 13:29 /r
> root at prmh-nsx-perf-server118:~/ovs# ovs-vsctl add-port br0
> "../../../../r" -- set interface "../../../../r" type=dpdkvhostuser
> root at prmh-nsx-perf-server118:~/ovs# ls -la /r
> -rwxrwxrwx 1 root ssh 10 Jan 29 13:29 /r   <---- The second bug is
> that you just gave away this file to ssh group.
>
>
> Note, that running Open vSwitch under confined user does not fully
> solve this issue, because you could still change user:group of
> /etc/openvswitch/conf.db.
>
>
> Here is log snippet:
>
> 2016-01-29T21:30:36.837Z|00078|dpdk|ERR|vhost-user socket device setup
> failure for socket //../../../../r
> 2016-01-29T21:30:36.837Z|00079|dpdk|INFO|Socket //../../../../r
> created for vhost-user port ../../../../r
> 2016-01-29T21:30:36.837Z|00080|dpdk|INFO|Socket //../../../../r
> changed permissions to 0777
> 2016-01-29T21:30:36.837Z|00081|dpdk|INFO|Socket //../../../../r
> changed ownership to :nobody.
>
> In your patch I would change owner:group and permissions of a file
> only and only if you are 100% sure that you were the one who just
> created it.

Yikes, I hadn't done that test. I'll put checks around it.

>> +    if (vhost_sock_perms) {
>> +        vhost_set_permissions(netdev->vhost_id);
>> +    }
>> +
>> +    if (vhost_sock_owner) {
>> +        vhost_set_ownership(netdev->vhost_id);
>> +    }
>> +
>>      err = vhost_construct_helper(netdev_);
>>      ovs_mutex_unlock(&dpdk_mutex);
>>      return err;
>> @@ -2217,12 +2267,16 @@ __dpdk_init(const struct ovsrec_open_vswitch *ovs_cfg)
>>
>>      VLOG_INFO("DPDK Enabled, initializing");
>>
>> -#ifdef VHOST_CUSE
>>      if (process_vhost_flags("cuse_dev_name", strdup("vhost-net"),
>>                              PATH_MAX, ovs_cfg, &cuse_dev_name)) {
>> -#else
>> +        /**
>> +         * This is ignored when vhost_user anyway...
>> +         */
>> +    }
>> +
>>      if (process_vhost_flags("vhost_sock_dir", strdup(ovs_rundir()),
>>                              NAME_MAX, ovs_cfg, &vhost_sock_dir)) {
>> +#ifndef VHOST_CUSE
>>          struct stat s;
>>
>>          err = stat(vhost_sock_dir, &s);
>> @@ -2233,6 +2287,11 @@ __dpdk_init(const struct ovsrec_open_vswitch *ovs_cfg)
>>  #endif
>>      }
>>
>> +    (void)process_vhost_flags("vhost_sock_owners", NULL, NAME_MAX, ovs_cfg,
>> +                              &vhost_sock_owner);
>> +    (void)process_vhost_flags("vhost_sock_permissions", "0600", NAME_MAX,
>> +                              ovs_cfg, &vhost_sock_perms);
>> +
>>      /* Get the main thread affinity */
>>      CPU_ZERO(&cpuset);
>>      err = pthread_getaffinity_np(pthread_self(), sizeof(cpu_set_t), &cpuset);
>> --
>> 2.6.1.133.gf5b6079
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list