Hi Marcos, Marcos Felipe Schwarz <marcos.schw...@rnp.br> writes:
> Thanks for the suggestion Aaron. > > Follows below the revised patch for the current master using Aaron and > Timothy contributions. May I submit the patch as is or are there any further > suggestions? > I've tested it in the following conditions: > 1) Fedora 27, ovs_user root:root, vfio-uio driver: Fixed by this patch > 2) Fedora 27, ovs_user root:root, uio-pci-generic driver: Fixed by this patch > 3) Fedora 27, ovs_user openvswitch:hugetlbfs, vfio-uio driver: Continues > working > 4) Fedora 27, ovs_user openvswitch:hugetlbfs, uio-pci-generic driver: > Continues broken, for kernel 4.0 and newer since the user is missing the > CAP_SYS_ADMIN capability. Ref: > https://www.kernel.org/doc/Documentation/vm/pagemap.txt Thanks for following up with this! Please submit it as a patch. I will set aside time to review it (if Timothy doesn't beat me to it). > diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c > index adb549c98..06528e9ab 100644 > --- a/lib/daemon-unix.c > +++ b/lib/daemon-unix.c > @@ -1047,5 +1047,6 @@ daemon_set_new_user(const char *user_spec) > } > }kernel > > - switch_user = true; > + if (!uid_verify(uid) || !gid_verify(gid)) > + switch_user = true; > } > diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in > b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in > index c6d9aa1b8..9b01c9271 100644 > --- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in > +++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in > @@ -14,7 +14,7 @@ Environment=HOME=/var/run/openvswitch > EnvironmentFile=/etc/openvswitch/default.conf > EnvironmentFile=-/etc/sysconfig/openvswitch > @begin_dpdk@ > -ExecStartPre=-/usr/bin/chown :hugetlbfs /dev/hugepages > +ExecStartPre=-/bin/sh -c '/usr/bin/chown :${OVS_USER_ID##*:} /dev/hugepages' > ExecStartPre=-/usr/bin/chmod 0775 /dev/hugepages > @end_dpdk@ > ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \ > > Regards, > > Marcos Schwarz > > ----- Original Message ----- > From: "Aaron Conole" <acon...@redhat.com> > To: "Marcos Felipe Schwarz" <marcos.schw...@rnp.br> > Cc: "Timothy Redaelli" <tredae...@redhat.com>, ovs-discuss@openvswitch.org > Sent: Wednesday, January 10, 2018 6:54:24 PM > Subject: Re: [ovs-discuss] DPDK with UIO drivers is broken on Fedora since > OVS 2.8.0 > > Marcos Felipe Schwarz <marcos.schw...@rnp.br> writes: > >> Thanks for the suggestion Timothy, didn't knew that worked. Just >> fixing some little things, it should be: >> ExecStartPre=-/usr/bin/chown :${OVS_USER_ID##*:} /dev/hugepages >> >> Regarding the daemon-unix.c patch, any suggestions on how to improve >> it? I tested it is working, but currently, I'm not aware if the new >> capability should be set separeted as I did or using any of the >> current blocks of code. > > One thing that might work is to not attempt switching users and > capabilities if the current user is the target user. > > ex: > > @@ -1047,5 +1047,6 @@ daemon_set_new_user(const char *user_spec) > } > } > > - switch_user = true; > + if (!uid_verify(uid) || !gid_verify(gid)) > + switch_user = true; > } > > NOTE: this isn't compile or runtime tested, just a thought. > >> Regards, >> >> Marcos Schwarz >> >> ----- Original Message ----- >> From: "Timothy Redaelli" <tredae...@redhat.com> >> To: "Marcos Felipe Schwarz" <marcos.schw...@rnp.br> >> Cc: "Ben Pfaff" <b...@ovn.org>, ovs-discuss@openvswitch.org >> Sent: Monday, January 8, 2018 9:20:17 AM >> Subject: Re: [ovs-discuss] DPDK with UIO drivers is broken on Fedora >> since OVS 2.8.0 >> >> On Sat, Jan 6, 2018 at 2:41 AM, Marcos Felipe Schwarz >> <marcos.schw...@rnp.br> wrote: >>> Got it fixed. >>> >>> The problem was related to not setting the CAP_SYS_ADMIN capability at >>> daemon-unix.c. Follows the patch bellow to set the capability and >>> dynamically extract the group from OVS_USER_ID instead of forcing it to >>> :hugetlbfs. >>> >>> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c >>> index 839114f3e..3b94164ea 100644 >>> --- a/lib/daemon-unix.c >>> +++ b/lib/daemon-unix.c >>> @@ -818,6 +818,9 @@ daemon_become_new_user_linux(bool access_datapath >>> OVS_UNUSED) >>> ret = capng_update(CAPNG_ADD, cap_sets, CAP_NET_ADMIN) >>> || capng_update(CAPNG_ADD, cap_sets, CAP_NET_RAW); >>> } >>> + if (!ret) { >>> + ret = capng_update(CAPNG_ADD, cap_sets, CAP_SYS_ADMIN); >>> + } >>> } else { >>> ret = -1; >>> } >>> diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in >>> b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in >>> index c6d9aa1b8..94290a847 100644 >>> --- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in >>> +++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in >>> @@ -14,7 +14,7 @@ Environment=HOME=/var/run/openvswitch >>> EnvironmentFile=/etc/openvswitch/default.conf >>> EnvironmentFile=-/etc/sysconfig/openvswitch >>> @begin_dpdk@ >>> -ExecStartPre=-/usr/bin/chown :hugetlbfs /dev/hugepages >>> +ExecStartPre=-/bin/sh -c 'chown :$(echo $OVS_USER_ID | tr ":" "\n" | tail >>> -1) /dev/hugepages' >> >> I think it's better to avoid using multiple useless forks, shell >> script parameter expansion are better in this case: >> >> ExecStartPre=-/bin/sh -c '/usr/bin/chown $${OVS_USER_ID##*:} /dev/hugepages' >> >>> ExecStartPre=-/usr/bin/chmod 0775 /dev/hugepages >>> @end_dpdk@ >>> ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \ >>> >>> Regards, >>> >>> Marcos Schwarz >> _______________________________________________ >> discuss mailing list >> disc...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-discuss _______________________________________________ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
