Thanks so much for the review, Ansis!
Ansis Atteka <aatt...@nicira.com> writes:
> On Fri, Dec 18, 2015 at 10:27 AM, Aaron Conole <acon...@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 <acon...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
