[libvirt] [PATCH] virprocess: Extend list of platforms for setns wrapper

Tue Sep 16 06:53:05 UTC 2014

On Mon, Sep 15, 2014 at 04:50:23PM +0100, Daniel P. Berrange wrote:
>On Mon, Sep 15, 2014 at 05:44:17PM +0200, Martin Kletzander wrote:
>> On Mon, Sep 15, 2014 at 05:36:16PM +0200, Michal Privoznik wrote:
>> >On 15.09.2014 17:32, Martin Kletzander wrote:
>> >>On Mon, Sep 15, 2014 at 04:22:18PM +0100, Daniel P. Berrange wrote:
>> >>>On Mon, Sep 15, 2014 at 05:20:46PM +0200, Michal Privoznik wrote:
>> >>>>On 15.09.2014 17:15, Martin Kletzander wrote:
>> >>>>>On Mon, Sep 15, 2014 at 03:43:55PM +0200, Michal Privoznik wrote:
>> >>>>>>Currently, the setns() wrapper is supported only for x86_64 and i686
>> >>>>>>which leaves us failing to build on other platforms like arm, aarch64
>> >>>>>>and so on. This means, that the wrapper needs to be extended to those
>> >>>>>>platforms and make to fail on runtime not compile time.
>> >>>>>>
>> >>>>>>The syscall numbers for other platforms was fetched using this
>> >>>>>>command:
>> >>>>>>
>> >>>>>>kernel.git $ git grep "define.*__NR_setns" | grep -e arm -e powerpc -e
>> >>>>>>s390
>> >>>>>>arch/arm/include/uapi/asm/unistd.h:#define
>> >>>>>>__NR_setns                   (__NR_SYSCALL_BASE+375)
>> >>>>>>arch/arm64/include/asm/unistd32.h:#define __NR_setns 375
>> >>>>>>arch/powerpc/include/uapi/asm/unistd.h:#define
>> >>>>>>__NR_setns               350
>> >>>>>>arch/s390/include/uapi/asm/unistd.h:#define __NR_setns          339
>> >>>>>>
>> >>>>>>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> >>>>>>---
>> >>>>>>src/util/virprocess.c | 18 ++++++++++++------
>> >>>>>>1 file changed, 12 insertions(+), 6 deletions(-)
>> >>>>>>
>> >>>>>
>> >>>>>NACK, we shouldn't be duplicating syscall definitions.  There should
>> >>>>>be AC_CHECK_FUNCS([setns]) (instead of AC_CHECK_FUNCS_ONCE() for the
>> >>>>>syscall) and having with_lxc = "yes" and ac_cv_func_setns != "yes"
>> >>>>>should result in an error.
>> >>>>
>> >>>>The only problem with this might be that on systems with older glibc
>> >>>>(and
>> >>>>there is plenty of them) libvirt will fail to build / miss this
>> >>>>feature. And
>> >>>>it's not that the kernel doesn't support the namesapces. But let me
>> >>>>see if I
>> >>>>can get some ACKs on that approach you're suggesting.
>> >>>
>> >>>That's basically what the code did before we added the #define or
>> >>>NR_setns.
>> >>>We took the patch specifically to help Debian where the kernel has it but
>> >>>glibc is outdated.
>> >>>
>> >>
>> >>Either Debian should patch their glibc or we should at lease use
>> >>SYS_setns IMHO.
>> >
>> >That's not gonna fly either. On my system, the SYS_setns is declared in:
>> >
>> ># grep -r SYS_setns /usr/include/
>> >/usr/include/bits/syscall.h:#define SYS_setns __NR_setns
>> >
>> >And the syscall.h belongs to glibc, not kernel headers. So we are back
>> >at the start.
>> >
>>

BTW this only means that the glibc was compiled against kernel that is
new enough.  glibc doesn't keep SYS_syscall numbers around, for linux
the file bits/syscall.h is generated at compile time against kernel.
So if we do what I proposed and debian compiled that glibc with new
enough kernel, then it would all be unicorns spitting rainbows around
here.  Unless we're doing it for debian stable, which is a completely
different story (using circa 7 years old glibc with 2.5 years old
kernel).

Can't we at least include asm/unistd.h or something similar and user
the __NR_setns defined there?

>> Well, I'd argue that we're not :)  Distros could make our lives easier
>> by not requiring us to guess their kernel's syscall numbers :)
>
>Well we're not guessing - syscalls numbers are standardized ABI that
>does not change per distro.

Well, I looked at it from another point.  Syscall numbers are part of
kernel ABI and distros don't grant you binary compatibility (or at
least for now, unfortunately).

By guessing, I meant that there were numerous conditionals and if
somebody uses an architecture that is not considered by us (e.g. hppa)
or some new one comes along (e.g. ia64be), there's a slight chance
that in the future we might get a wrong number in there and find that
our pretty late.

>Realistically rebasing glibc in existing distros to pull in new
>functions isn't something distros are going todo.

Well, they don't have to rebase, just pull back some needed commits.
Or maybe just rebuild the package with kernel headers having same
versions as those being distributed.  But that depends on what
versions we're talking about here.  I missed the discussion about who
wanted this, but *even* if they are running debian stable, it should
be just a few-liner for glibc with a rebuild

>So unless we do a workaround in libvirt, users on that distro are
>facing a regression due us switching a bunch of LXC APIs to use
>setns() for a previous security fix.  So I think defining NR_setns in
>libvirt code is the least bad, from many unpleasant options.
>

From my understanding, making shipped versions match together is
distros' job.  Users on that distro may force the maintainers to fix
that.

But anyway, if you decide to go with an uneasy way of maintaining our
own syscall definitions in freshly baked versions of libvirt just for
rusty old glibc and laziness of some other people... I can't force you
not to do that.  So it's up to you guys now.

Have a nice day,
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140916/4fdb2607/attachment-0001.sig>