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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
