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

Christian Ehrhardt christian.ehrhardt at canonical.com
Wed Jan 27 15:32:25 UTC 2016


Aaron Conole <aconole at ...> 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 at 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.




More information about the dev mailing list