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