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

Tue Sep 16 07:54:19 UTC 2014

On 16.09.2014 08:53, Martin Kletzander wrote:
> 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

Okay, So now that I have green flag I'm pushing this. Maybe in the 
future we can drop this, once Debian rebase its glibc.

Michal