| Message ID | 1450463278-7931-6-git-send-email-aconole@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Aaron Conole <aconole@...> writes: > > 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. Hi Aaron, as I said I gave your patches a try, here the summarized review. I have seen that your old patches were based on your modifications to move dpdk config into the DB. So I rebased the two patches to set the socket ownership to the last master and on top of your most recent (v6) patches for that work. Overall I have these applied now: patches/ovs-dev-v6-1-4-netdev-dpdk-Restore-thread-affinity-after-DPDK- init.patch patches/ovs-dev-v6-2-4-netdev-dpdk-Convert-initialization-from-cmdline-to- db.patch patches/ovs-dev-v6-3-4-netdev-dpdk-Autofill-lcore-coremask-if-absent.patch patches/ovs-dev-v6-4-4-netdev-dpdk-Allow-arbitrary-eal-arguments.patch patches/ovs-dev-4-5-lib-daemon-Move-the-user-group-code-up-one-level.patch patches/ovs-dev-5-5-netdev-dpdk-Support-user-cfg-vhost-socket-perms.patch Testing followed more or less INSTALL.dpdk.md, trying to recreate my old setup with your code for the new dpdk config applied. On my first init with DPDK it failed at "EAL: Options -m and --socket-mem cannot be specified at the same time" But there is no socket-mem set ovs-vsctl --no-wait get Open_vSwitch . other_config {dpdk-alloc-mem="4096", dpdk-init="true"} This seems to conflict with the defaults set in get_dpdk_args. I haven't found a way to overwrite the default value for dpdk_socket_mem (tried setting NULL and 0). But it always ends up having either the value I set or the default one, and whatever it has it conflicts with alloc-mem. I think your patch series should have neither socket-mem nor alloc-mem as default, bail out if "no kind" of memory is specifed. Actually I think DPDK would bail out, just as it does now with both being set. So I went on with clearing and using socket-mem for now. ovs-vsctl --no-wait clear Open_vSwitch . other_config ovs-vsctl --no-wait set Open_vSwitch . other_config:dpdk- init=trueroot@horsea:/home/ubuntu/ovs-review-aaron-dpdkchanges# #ovs-vsctl --no-wait set Open_vSwitch . other_config:dpdk_socket_mem=4096,0 ovs-vsctl --no-wait set Open_vSwitch . other_config:dpdk_socket_mem=4096,0 With that it initilizes DPDK properly. This is the simple dpdk based config that I wanted to see if it brings up the socket for vhost-user-1 correctly: Bridge "ovsdpdkbr0" Port "ovsdpdkbr0" Interface "ovsdpdkbr0" type: internal Port "dpdk0" Interface "dpdk0" type: dpdk Port "vhost-user-1" Interface "vhost-user-1" type: dpdkvhostuser Note: I like the list in the log about what was specified/missing and what it defaulted to: dpdk|INFO|DPDK Enabled, initializing dpdk|INFO|No cuse-dev-name provided - defaulting to vhost-net dpdk|INFO|No vhost-sock-dir provided - defaulting to /usr/local/var/run/openvswitch dpdk|INFO|No vhost_sock_owners provided - defaulting to (null) dpdk|INFO|No vhost_sock_permissions provided - defaulting to 0600 Further down was the first glimpse of the control of socket permissions dpdk|INFO|Socket /usr/local/var/run/openvswitch/vhost-user-1 created for vhost-user port vhost-user-1 dpdk|INFO|Socket /usr/local/var/run/openvswitch/vhost-user-1 changed permissions to 0600 Here you changed the default, which formerly would have been 640. IMHO your patch should keep the old behavior until the user specified some value. Now I wanted to test path, owner and permission setting - so I set mkdir -p /usr/local/var/run/openvswitch-vhost ovs-vsctl --no-wait set Open_vSwitch . other_config:vhost-sock- dir=/usr/local/var/run/openvswitch-vhost ovs-vsctl --no-wait set Open_vSwitch . other_config:vhost_sock_owners=:kvm ovs-vsctl set Open_vSwitch . other_config:vhost_sock_permissions=660 So overall I was running with ovs-vsctl --no-wait get Open_vSwitch . other_config {dpdk-init="true", dpdk_socket_mem="4096,0", vhost-sock- dir="/usr/local/var/run/openvswitch-vhost", vhost_sock_owners=":kvm", vhost_sock_permissions="660"} Log looks good: dpdk|INFO|Socket /usr/local/var/run/openvswitch-vhost/vhost-user-1 created for vhost-user port vhost-user-1 dpdk|INFO|Socket /usr/local/var/run/openvswitch-vhost/vhost-user-1 changed permissions to 660 dpdk|INFO|Socket /usr/local/var/run/openvswitch-vhost/vhost-user-1 changed ownership to :kvm. But the actual socket was not 100% correct. It was in the right path and the kvm group was set, but permissions: s-w--w-r-T 1 root kvm 0 Jan 27 15:00 vhost-user-1= That is mode 224, which seemed very odd at first. But then I realized that we are passing that more or less directly to chmod (just through strtoul). So I set the four digit permissions and it worked fine: ovs-vsctl set Open_vSwitch . other_config:vhost_sock_permissions=0660 Still I think we should either reject the three digit config when setting it or at least verify the string. - <3 digits, bail out - 3 digits, add leading zero - >4 digits bail out and only then go on Also I think on that part + unsigned long int mode = strtoul(vhost_sock_perms, NULL, 0); + int err = chmod(vhost_sock_location, (mode_t)mode); We should set errno=0; and check after the strtoul. TL;DR - we need to make alloc-mem operable again - default permissions should be 640 as they were before - we should add some range checks and auto-extension to the socket permission Overall I like both patch series a lot! Once you create a new version of both series based on this and other feedback I'm eager to test it again if that helps you.
Christian Ehrhardt <christian.ehrhardt@canonical.com> writes: > Aaron Conole <aconole@...> writes: > >> >> 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. > > Hi Aaron, > as I said I gave your patches a try, here the summarized review. > > I have seen that your old patches were based on your modifications to move > dpdk config into the DB. > So I rebased the two patches to set the socket ownership to the last master > and on top of your most recent (v6) patches for that work. > > Overall I have these applied now: > patches/ovs-dev-v6-1-4-netdev-dpdk-Restore-thread-affinity-after-DPDK- > init.patch > patches/ovs-dev-v6-2-4-netdev-dpdk-Convert-initialization-from-cmdline-to- > db.patch > patches/ovs-dev-v6-3-4-netdev-dpdk-Autofill-lcore-coremask-if-absent.patch > patches/ovs-dev-v6-4-4-netdev-dpdk-Allow-arbitrary-eal-arguments.patch > patches/ovs-dev-4-5-lib-daemon-Move-the-user-group-code-up-one-level.patch > patches/ovs-dev-5-5-netdev-dpdk-Support-user-cfg-vhost-socket-perms.patch > > Testing followed more or less INSTALL.dpdk.md, trying to recreate my old > setup with your code for the new dpdk config applied. > > On my first init with DPDK it failed at > "EAL: Options -m and --socket-mem cannot be specified at the same time" > But there is no socket-mem set > ovs-vsctl --no-wait get Open_vSwitch . other_config > {dpdk-alloc-mem="4096", dpdk-init="true"} > > This seems to conflict with the defaults set in get_dpdk_args. > I haven't found a way to overwrite the default value for dpdk_socket_mem > (tried setting NULL and 0). > But it always ends up having either the value I set or the default one, and > whatever it has it conflicts with alloc-mem. > > I think your patch series should have neither socket-mem nor alloc-mem as > default, bail out if "no kind" of memory is specifed. > Actually I think DPDK would bail out, just as it does now with both being > set. > > So I went on with clearing and using socket-mem for now. > ovs-vsctl --no-wait clear Open_vSwitch . other_config > ovs-vsctl --no-wait set Open_vSwitch . other_config:dpdk- > init=trueroot@horsea:/home/ubuntu/ovs-review-aaron-dpdkchanges# #ovs-vsctl > --no-wait set Open_vSwitch . other_config:dpdk_socket_mem=4096,0 > ovs-vsctl --no-wait set Open_vSwitch . other_config:dpdk_socket_mem=4096,0 > With that it initilizes DPDK properly. > > This is the simple dpdk based config that I wanted to see if it brings up > the socket for vhost-user-1 correctly: > Bridge "ovsdpdkbr0" > Port "ovsdpdkbr0" > Interface "ovsdpdkbr0" > type: internal > Port "dpdk0" > Interface "dpdk0" > type: dpdk > Port "vhost-user-1" > Interface "vhost-user-1" > type: dpdkvhostuser > > Note: I like the list in the log about what was specified/missing and what > it defaulted to: > dpdk|INFO|DPDK Enabled, initializing > dpdk|INFO|No cuse-dev-name provided - defaulting to vhost-net > dpdk|INFO|No vhost-sock-dir provided - defaulting to > /usr/local/var/run/openvswitch > dpdk|INFO|No vhost_sock_owners provided - defaulting to (null) > dpdk|INFO|No vhost_sock_permissions provided - defaulting to 0600 > > Further down was the first glimpse of the control of socket permissions > dpdk|INFO|Socket /usr/local/var/run/openvswitch/vhost-user-1 created for > vhost-user port vhost-user-1 > dpdk|INFO|Socket /usr/local/var/run/openvswitch/vhost-user-1 changed > permissions to 0600 > > Here you changed the default, which formerly would have been 640. > IMHO your patch should keep the old behavior until the user specified some > value. > > Now I wanted to test path, owner and permission setting - so I set > mkdir -p /usr/local/var/run/openvswitch-vhost > ovs-vsctl --no-wait set Open_vSwitch . other_config:vhost-sock- > dir=/usr/local/var/run/openvswitch-vhost > ovs-vsctl --no-wait set Open_vSwitch . other_config:vhost_sock_owners=:kvm > ovs-vsctl set Open_vSwitch . other_config:vhost_sock_permissions=660 > So overall I was running with > ovs-vsctl --no-wait get Open_vSwitch . other_config > {dpdk-init="true", dpdk_socket_mem="4096,0", vhost-sock- > dir="/usr/local/var/run/openvswitch-vhost", vhost_sock_owners=":kvm", > vhost_sock_permissions="660"} > > Log looks good: > dpdk|INFO|Socket /usr/local/var/run/openvswitch-vhost/vhost-user-1 created > for vhost-user port vhost-user-1 > dpdk|INFO|Socket /usr/local/var/run/openvswitch-vhost/vhost-user-1 changed > permissions to 660 > dpdk|INFO|Socket /usr/local/var/run/openvswitch-vhost/vhost-user-1 changed > ownership to :kvm. > > But the actual socket was not 100% correct. > It was in the right path and the kvm group was set, but permissions: > s-w--w-r-T 1 root kvm 0 Jan 27 15:00 vhost-user-1= > That is mode 224, which seemed very odd at first. > But then I realized that we are passing that more or less directly to chmod > (just through strtoul). > > So I set the four digit permissions and it worked fine: > ovs-vsctl set Open_vSwitch . other_config:vhost_sock_permissions=0660 > > Still I think we should either reject the three digit config when setting > it or at least verify the string. > - <3 digits, bail out > - 3 digits, add leading zero > - >4 digits bail out > and only then go on > Also I think on that part > + unsigned long int mode = strtoul(vhost_sock_perms, NULL, 0); > + int err = chmod(vhost_sock_location, (mode_t)mode); > We should set errno=0; and check after the strtoul. > > > TL;DR > - we need to make alloc-mem operable again > - default permissions should be 640 as they were before > - we should add some range checks and auto-extension to the socket > permission > > Overall I like both patch series a lot! > Once you create a new version of both series based on this and other > feedback I'm eager to test it again if that helps you. Thanks for this feedback, Christian! I'll integrate and spin a series asap.
On Fri, Dec 18, 2015 at 10:27 AM, Aaron Conole <aconole@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@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. > > #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. > + 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 root@prmh-nsx-perf-server118:~/ovs# echo "I want this file to stay under root:root" > /r root@prmh-nsx-perf-server118:~/ovs# ls -la /r -rw-r--r-- 1 root root 10 Jan 29 13:29 /r root@prmh-nsx-perf-server118:~/ovs# ovs-vsctl add-port br0 "../../../../r" -- set interface "../../../../r" type=dpdkvhostuser root@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. > + 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@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Thanks so much for the review, Ansis! Ansis Atteka <aatteka@nicira.com> writes: > On Fri, Dec 18, 2015 at 10:27 AM, Aaron Conole <aconole@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@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@prmh-nsx-perf-server118:~/ovs# echo "I want this file to stay > under root:root" > /r > root@prmh-nsx-perf-server118:~/ovs# ls -la /r > -rw-r--r-- 1 root root 10 Jan 29 13:29 /r > root@prmh-nsx-perf-server118:~/ovs# ovs-vsctl add-port br0 > "../../../../r" -- set interface "../../../../r" type=dpdkvhostuser > root@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@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev
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" #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); + 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); + + 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);
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@redhat.com> --- INSTALL.DPDK.md | 17 +++++++++++++++ lib/netdev-dpdk.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 78 insertions(+), 2 deletions(-)