* Re: Setting monotonic time?
From: Thomas Gleixner @ 2018-10-02 20:06 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Eric W . Biederman, avagin, dima, Linux Kernel Mailing List,
0x7f454c46, adrian, Andy Lutomirski, Christian Brauner, gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov, xemul,
Shuah Khan, containers, criu, Linux API, the arch/x86 maintainers,
Alexey Dobriyan, linux-kselftest
In-Reply-To: <CAK8P3a2zTqrCGURE3XVpn3_w6C4R5O+cq8TdtbyKCMGyRk9_jw@mail.gmail.com>
On Tue, 2 Oct 2018, Arnd Bergmann wrote:
> On Mon, Oct 1, 2018 at 8:53 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > On Mon, 1 Oct 2018, Eric W. Biederman wrote:
> > > In the context of process migration there is a simpler subproblem that I
> > > think it is worth exploring if we can do something about.
> > >
> > > For a cluster of machines all running with synchronized
> > > clocks. CLOCK_REALTIME matches. CLOCK_MONOTNIC does not match between
> > > machines. Not having a matching CLOCK_MONOTONIC prevents successful
> > > process migration between nodes in that cluster.
> > >
> > > Would it be possible to allow setting CLOCK_MONOTONIC at the very
> > > beginning of time? So that all of the nodes in a cluster can be in
> > > sync?
> > >
> > > No change in skew just in offset for CLOCK_MONOTONIC.
> > >
> > > There are also dragons involved in coordinating things so that
> > > CLOCK_MONOTONIC gets set before CLOCK_MONOTONIC gets used. So I don't
> > > know if allowing CLOCK_MONOTONIC to be set would be practical but it
> > > seems work exploring all on it's own.
> >
> > It's used very early on in the kernel, so that would be a major surprise
> > for many things including user space which has expectations on clock
> > monotonic.
> >
> > It would be reasonably easy to add CLOCK_MONONOTIC_SYNC which can be set in
> > the way you described and then in name spaces make it possible to magically
> > map CLOCK_MONOTONIC to CLOCK_MONOTONIC_SYNC.
> >
> > It still wouldn't allow to have different NTP/PTP time domains, but might
> > be a good start to address the main migration headaches.
>
> If we make CLOCK_MONOTONIC settable this way in a namespace,
> do you think that should include device drivers that report timestamps
> in CLOCK_MONOTONIC base, or only the timekeeping clock and timer
> interfaces?
Uurgh. That gets messy very fast.
> Examples for drivers that can report timestamps are input, sound, v4l,
> and drm. I think most of these can report stamps in either monotonic
> or realtime base, while socket timestamps notably are always in
> realtime.
>
> We can probably get away with not setting the timebase for those
> device drivers as long as the checkpoint/restart and migration features
> are not expected to restore the state of an open character device
> in that way. I don't know if that is a reasonable assumption to make
> for the examples I listed.
No idea. I'm not a container migration wizard.
Thanks,
tglx
^ permalink raw reply
* Re: Setting monotonic time?
From: Arnd Bergmann @ 2018-10-02 20:00 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Eric W . Biederman, avagin, dima, Linux Kernel Mailing List,
0x7f454c46, adrian, Andy Lutomirski, Christian Brauner, gorcunov,
H. Peter Anvin, Ingo Molnar, Jeff Dike, Oleg Nesterov, xemul,
Shuah Khan, containers, criu, Linux API, the arch/x86 maintainers,
Alexey Dobriyan, linux-kselftest
In-Reply-To: <alpine.DEB.2.21.1810012047520.32062@nanos.tec.linutronix.de>
On Mon, Oct 1, 2018 at 8:53 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, 1 Oct 2018, Eric W. Biederman wrote:
> > In the context of process migration there is a simpler subproblem that I
> > think it is worth exploring if we can do something about.
> >
> > For a cluster of machines all running with synchronized
> > clocks. CLOCK_REALTIME matches. CLOCK_MONOTNIC does not match between
> > machines. Not having a matching CLOCK_MONOTONIC prevents successful
> > process migration between nodes in that cluster.
> >
> > Would it be possible to allow setting CLOCK_MONOTONIC at the very
> > beginning of time? So that all of the nodes in a cluster can be in
> > sync?
> >
> > No change in skew just in offset for CLOCK_MONOTONIC.
> >
> > There are also dragons involved in coordinating things so that
> > CLOCK_MONOTONIC gets set before CLOCK_MONOTONIC gets used. So I don't
> > know if allowing CLOCK_MONOTONIC to be set would be practical but it
> > seems work exploring all on it's own.
>
> It's used very early on in the kernel, so that would be a major surprise
> for many things including user space which has expectations on clock
> monotonic.
>
> It would be reasonably easy to add CLOCK_MONONOTIC_SYNC which can be set in
> the way you described and then in name spaces make it possible to magically
> map CLOCK_MONOTONIC to CLOCK_MONOTONIC_SYNC.
>
> It still wouldn't allow to have different NTP/PTP time domains, but might
> be a good start to address the main migration headaches.
If we make CLOCK_MONOTONIC settable this way in a namespace,
do you think that should include device drivers that report timestamps
in CLOCK_MONOTONIC base, or only the timekeeping clock and timer
interfaces?
Examples for drivers that can report timestamps are input, sound, v4l,
and drm. I think most of these can report stamps in either monotonic
or realtime base, while socket timestamps notably are always in
realtime.
We can probably get away with not setting the timebase for those
device drivers as long as the checkpoint/restart and migration features
are not expected to restore the state of an open character device
in that way. I don't know if that is a reasonable assumption to make
for the examples I listed.
Arnd
^ permalink raw reply
* Re: [PATCH 1/3] bpf: allow zero-initializing hash map seed
From: Jann Horn @ 2018-10-02 19:59 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, Network Development,
Linux API
In-Reply-To: <20181001104509.24211-2-lmb@cloudflare.com>
On Mon, Oct 1, 2018 at 12:47 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Add a new flag BPF_F_ZERO_SEED, which forces a hash map
> to initialize the seed to zero.
> ---
> include/uapi/linux/bpf.h | 2 ++
> kernel/bpf/hashtab.c | 8 ++++++--
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index aa5ccd2385ed..9d15c8f179ac 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -252,6 +252,8 @@ enum bpf_attach_type {
> #define BPF_F_NO_COMMON_LRU (1U << 1)
> /* Specify numa node during map creation */
> #define BPF_F_NUMA_NODE (1U << 2)
> +/* Zero-initialize hash function seed */
> +#define BPF_F_ZERO_SEED (1U << 6)
>
> /* flags for BPF_PROG_QUERY */
> #define BPF_F_QUERY_EFFECTIVE (1U << 0)
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 2c1790288138..a79e123dae62 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -23,7 +23,7 @@
>
> #define HTAB_CREATE_FLAG_MASK \
> (BPF_F_NO_PREALLOC | BPF_F_NO_COMMON_LRU | BPF_F_NUMA_NODE | \
> - BPF_F_RDONLY | BPF_F_WRONLY)
> + BPF_F_RDONLY | BPF_F_WRONLY | BPF_F_ZERO_SEED)
>
> struct bucket {
> struct hlist_nulls_head head;
> @@ -373,7 +373,11 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)
> if (!htab->buckets)
> goto free_htab;
>
> - htab->hashrnd = get_random_int();
> + if (htab->map.map_flags & BPF_F_ZERO_SEED)
> + htab->hashrnd = 0;
> + else
> + htab->hashrnd = get_random_int();
> +
If this is for testing only, you can slap a capable(CAP_SYS_ADMIN)
check in here, right? I doubt it matters, but I don't really like
seeing something like this exposed to unprivileged userspace just
because you need it for kernel testing.
^ permalink raw reply
* Re: [RFC PATCH v4 03/27] x86/fpu/xstate: Enable XSAVES system states
From: Borislav Petkov @ 2018-10-02 17:15 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-4-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:27AM -0700, Yu-cheng Yu wrote:
> XSAVES saves both system and user states. The Linux kernel
> currently does not save/restore any system states. This patch
> creates the framework for supporting system states.
... and needs a lot more text explaining *why* it is doing that.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/include/asm/fpu/internal.h | 3 +-
> arch/x86/include/asm/fpu/xstate.h | 9 ++-
> arch/x86/kernel/fpu/core.c | 7 +-
> arch/x86/kernel/fpu/init.c | 10 ---
> arch/x86/kernel/fpu/xstate.c | 112 +++++++++++++++++-----------
> 5 files changed, 80 insertions(+), 61 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> index f1f9bf91a0ab..1f447865db3a 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -45,7 +45,6 @@ extern void fpu__init_cpu_xstate(void);
> extern void fpu__init_system(struct cpuinfo_x86 *c);
> extern void fpu__init_check_bugs(void);
> extern void fpu__resume_cpu(void);
> -extern u64 fpu__get_supported_xfeatures_mask(void);
>
> /*
> * Debugging facility:
> @@ -94,7 +93,7 @@ static inline void fpstate_init_xstate(struct xregs_state *xsave)
> * trigger #GP:
> */
> xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
> - xfeatures_mask_user;
> + xfeatures_mask_all;
> }
>
> static inline void fpstate_init_fxstate(struct fxregs_state *fx)
> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
> index 9b382e5157ed..a32dc5f8c963 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -19,10 +19,10 @@
> #define XSAVE_YMM_SIZE 256
> #define XSAVE_YMM_OFFSET (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
>
> -/* System features */
> -#define XFEATURE_MASK_SYSTEM (XFEATURE_MASK_PT)
Previous patch renames it, this patch deletes it. Why do we need all
that unnecessary churn?
Also, this patch is trying to do a couple of things at once and
reviewing it is not trivial. Please split the changes logically.
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 19f8df54c72a..dd2c561c4544 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -51,13 +51,16 @@ static short xsave_cpuid_features[] __initdata = {
> };
>
> /*
> - * Mask of xstate features supported by the CPU and the kernel:
> + * Mask of xstate features supported by the CPU and the kernel.
> + * This is the result from CPUID query, SUPPORTED_XFEATURES_MASK,
> + * and boot_cpu_has().
> */
This needs to explain what both masks are - user and system. "CPU" and
"kernel" is not "user" and "all".
> u64 xfeatures_mask_user __read_mostly;
> +u64 xfeatures_mask_all __read_mostly;
> @@ -219,30 +222,31 @@ void fpstate_sanitize_xstate(struct fpu *fpu)
> */
> void fpu__init_cpu_xstate(void)
> {
> - if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask_user)
> + if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask_all)
> return;
> +
> + cr4_set_bits(X86_CR4_OSXSAVE);
> +
> /*
> - * Make it clear that XSAVES system states are not yet
> - * implemented should anyone expect it to work by changing
> - * bits in XFEATURE_MASK_* macros and XCR0.
> + * XCR_XFEATURE_ENABLED_MASK sets the features that are managed
> + * by XSAVE{C, OPT} and XRSTOR. Only XSAVE user states can be
> + * set here.
> */
> - WARN_ONCE((xfeatures_mask_user & XFEATURE_MASK_SYSTEM),
> - "x86/fpu: XSAVES system states are not yet implemented.\n");
> + xsetbv(XCR_XFEATURE_ENABLED_MASK,
> + xfeatures_mask_user);
No need to break the line here.
Also, you have a couple more places in your patches where you
unnecessarily break lines. Please don't do that, even if it exceeds 80
cols by a couple of chars.
>
> - xfeatures_mask_user &= ~XFEATURE_MASK_SYSTEM;
> -
> - cr4_set_bits(X86_CR4_OSXSAVE);
> - xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user);
> + /*
> + * MSR_IA32_XSS sets which XSAVES system states to be managed by
> + * XSAVES. Only XSAVES system states can be set here.
> + */
> + if (boot_cpu_has(X86_FEATURE_XSAVES))
> + wrmsrl(MSR_IA32_XSS,
> + xfeatures_mask_all & ~xfeatures_mask_user);
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [RFC v2 v2 0/1] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2018-10-02 16:47 UTC (permalink / raw)
To: James Bottomley, linux-kernel
Cc: Andrei Vagin, Dmitry Safonov, linux-api, containers,
Eric Biederman, linux-fsdevel, Alexander Viro
In-Reply-To: <1538496810.14607.5.camel@HansenPartnership.com>
Le 02/10/2018 à 18:13, James Bottomley a écrit :
> On Tue, 2018-10-02 at 12:20 +0200, Laurent Vivier wrote:
>> v2: no new namespace, binfmt_misc data are now part of
>> the mount namespace
>> I put this in mount namespace instead of user namespace
>> because the mount namespace is already needed and
>> I don't want to force to have the user namespace for that.
>> As this is a filesystem, it seems logic to have it here.
>>
>> This allows to define a new interpreter for each new container.
>>
>> But the main goal is to be able to chroot to a directory
>> using a binfmt_misc interpreter without being root.
>
> Reading all this, I don't quite understand why this works for me and
> not for you (I think I get from your explanation that it doesn't work
> for you, but I might have missed something):
>
> jejb@jarvis:~> uname -m
> x86_64
> jejb@jarvis:~> unshare -r -m
> root@jarvis:~# chroot /home/jejb/containers/aarch64
> jarvis:/ # uname -m
> aarch64
>
> Of course to get that to work I have an 'F' entry in
> /etc/binfmt.d/qemu-aarch64.conf
>
I'd like to configure the interpreter without being root.
As a simple user can run a VM and a full system inside, I'd like to be
able to start a container/chroot without having to configure something
at the host level.
For instance, I'd like to provide to "someone" (with no admin rights) a
tar file with inside an OS environment for a given target and the
interpreter, and allow him to run the binaries inside just by running a
simple command (like qemu-system-XXX -hda my.img)
It's also interesting for a test purpose: I can test concurrently
different interpreters for the same target without modifying the target
root filesystem (with the 'F' flag but on a per directory basis) or the
host configuration.
Another case is we can't configure qemu-mips/qemu-mipsel (old kernel
API) and qemu-mipsn32/qemu-mipsne32el (new kernel API) interpreters on
the same system because they share the same ELF signature (to be honest
qemu should have only one binary for the old and the new interface and
dynamically change it according to the ELF binary that is loaded, as it
is done for ARM).
But if no one thinks it's useful, I don't want to push this more than
that...
Thanks,
Laurent
^ permalink raw reply
* Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states
From: Yu-cheng Yu @ 2018-10-02 16:43 UTC (permalink / raw)
To: Dave Hansen, Borislav Petkov
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Florian Weimer,
H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit, Oleg Nesterov, Pavel Machek, Peter Zijlstra,
Randy Dunlap <rdu>
In-Reply-To: <6859a180-8973-e794-7ac4-1ac8f0e1c709@linux.intel.com>
On Tue, 2018-10-02 at 09:39 -0700, Dave Hansen wrote:
> On 10/02/2018 09:37 AM, Borislav Petkov wrote:
> > This patch's commit message is not even close. So I'd very much
> > appreciate a more verbose explanation, even if it repeats itself at
> > places.
>
> Yep, totally agree.
Ok, I will work on that.
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states
From: Dave Hansen @ 2018-10-02 16:39 UTC (permalink / raw)
To: Borislav Petkov, Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Florian Weimer,
H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit, Oleg Nesterov, Pavel Machek, Peter Zijlstra,
Randy Dunlap <rdu>
In-Reply-To: <20181002163736.GD29601@zn.tnic>
On 10/02/2018 09:37 AM, Borislav Petkov wrote:
> This patch's commit message is not even close. So I'd very much
> appreciate a more verbose explanation, even if it repeats itself at
> places.
Yep, totally agree.
^ permalink raw reply
* Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states
From: Borislav Petkov @ 2018-10-02 16:37 UTC (permalink / raw)
To: Dave Hansen, Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Florian Weimer,
H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit, Oleg Nesterov, Pavel Machek, Peter Zijlstra,
Randy Dunlap <rdu>
In-Reply-To: <498c8824-9255-96be-71c2-3ebfa684a9d3@linux.intel.com>
On Tue, Oct 02, 2018 at 09:30:52AM -0700, Dave Hansen wrote:
> > Good point. However, "system" is more indicative; CET states are per-task and
> > not "Supervisor". Do we want to go back to "Supervisor" or add comments?
>
> This is one of those things where the SDM language does not match what
> we use in the kernel. I think it's fine to call them "system" or
> "kernel" states to make it consistent with our existing in-kernel
> nomenclature.
>
> I say add comments to clarify what the SDM calls it vs. what we do.
So AFAIU, the difference is that XSAVES is a CPL0 insn. Thus the
supervisor thing, I'd guess.
Now it looks like CET uses XSAVES (from skimming the patchset forward)
but then what our nomenclature is and how it all gets tied together,
needs to be explained somewhere prominent so that we're all on the same
page.
This patch's commit message is not even close. So I'd very much
appreciate a more verbose explanation, even if it repeats itself at
places.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states
From: Dave Hansen @ 2018-10-02 16:30 UTC (permalink / raw)
To: Yu-cheng Yu, Borislav Petkov
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Florian Weimer,
H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit, Oleg Nesterov, Pavel Machek, Peter Zijlstra,
Randy Dunlap <rdu>
In-Reply-To: <ba13d643c21de8e1e01a8d528457fb5dd82c42aa.camel@intel.com>
On 10/02/2018 09:21 AM, Yu-cheng Yu wrote:
> On Tue, 2018-10-02 at 17:29 +0200, Borislav Petkov wrote:
>> On Fri, Sep 21, 2018 at 08:03:26AM -0700, Yu-cheng Yu wrote:
>>> To support XSAVES system states, change some names to distinguish
>>> user and system states.
>> I don't understand what the logic here is. SDM says:
>>
>> XSAVES—Save Processor Extended States Supervisor
>>
>> the stress being on "Supervisor" - why does it need to be renamed to
>> "system" now?
>>
> Good point. However, "system" is more indicative; CET states are per-task and
> not "Supervisor". Do we want to go back to "Supervisor" or add comments?
This is one of those things where the SDM language does not match what
we use in the kernel. I think it's fine to call them "system" or
"kernel" states to make it consistent with our existing in-kernel
nomenclature.
I say add comments to clarify what the SDM calls it vs. what we do.
^ permalink raw reply
* Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states
From: Yu-cheng Yu @ 2018-10-02 16:21 UTC (permalink / raw)
To: Borislav Petkov
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20181002152903.GB29601@zn.tnic>
On Tue, 2018-10-02 at 17:29 +0200, Borislav Petkov wrote:
> On Fri, Sep 21, 2018 at 08:03:26AM -0700, Yu-cheng Yu wrote:
> > To support XSAVES system states, change some names to distinguish
> > user and system states.
>
> I don't understand what the logic here is. SDM says:
>
> XSAVES—Save Processor Extended States Supervisor
>
> the stress being on "Supervisor" - why does it need to be renamed to
> "system" now?
>
Good point. However, "system" is more indicative; CET states are per-task and
not "Supervisor". Do we want to go back to "Supervisor" or add comments?
Yu-cheng
^ permalink raw reply
* Re: [RFC v2 v2 0/1] ns: introduce binfmt_misc namespace
From: James Bottomley @ 2018-10-02 16:13 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel
Cc: Andrei Vagin, Dmitry Safonov, linux-api, containers,
Eric Biederman, linux-fsdevel, Alexander Viro
In-Reply-To: <20181002102054.13245-1-laurent@vivier.eu>
On Tue, 2018-10-02 at 12:20 +0200, Laurent Vivier wrote:
> v2: no new namespace, binfmt_misc data are now part of
> the mount namespace
> I put this in mount namespace instead of user namespace
> because the mount namespace is already needed and
> I don't want to force to have the user namespace for that.
> As this is a filesystem, it seems logic to have it here.
>
> This allows to define a new interpreter for each new container.
>
> But the main goal is to be able to chroot to a directory
> using a binfmt_misc interpreter without being root.
Reading all this, I don't quite understand why this works for me and
not for you (I think I get from your explanation that it doesn't work
for you, but I might have missed something):
jejb@jarvis:~> uname -m
x86_64
jejb@jarvis:~> unshare -r -m
root@jarvis:~# chroot /home/jejb/containers/aarch64
jarvis:/ # uname -m
aarch64
Of course to get that to work I have an 'F' entry in
/etc/binfmt.d/qemu-aarch64.conf
Which means I'm running the host emulator in the container, which is
what I want to do. I think another goal of the patches might be to use
different emulators for different aarch64 containers? Do you have a
use case for this, because right at the moment for arch emulation
containers I think a single host wide entry per static emulator is the
right approach.
James
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Jan Kara @ 2018-10-02 15:31 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Jan Kara,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20181002145206.GA10903-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
On Tue 02-10-18 07:52:06, Christoph Hellwig wrote:
> On Tue, Oct 02, 2018 at 04:44:13PM +0200, Johannes Thumshirn wrote:
> > On Tue, Oct 02, 2018 at 07:37:13AM -0700, Christoph Hellwig wrote:
> > > No, it should not. DAX is an implementation detail thay may change
> > > or go away at any time.
> >
> > Well we had an issue with an application checking for dax, this is how
> > we landed here in the first place.
>
> So what exacty is that "DAX" they are querying about (and no, I'm not
> joking, nor being philosophical).
I believe the application we are speaking about is mostly concerned about
the memory overhead of the page cache. Think of a machine that has ~ 1TB of
DRAM, the database running on it is about that size as well and they want
database state stored somewhere persistently - which they may want to do by
modifying mmaped database files if they do small updates... So they really
want to be able to use close to all DRAM for the DB and not leave slack
space for the kernel page cache to cache 1TB of database files.
Honza
--
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR
^ permalink raw reply
* Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states
From: Borislav Petkov @ 2018-10-02 15:29 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-3-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:26AM -0700, Yu-cheng Yu wrote:
> To support XSAVES system states, change some names to distinguish
> user and system states.
I don't understand what the logic here is. SDM says:
XSAVES—Save Processor Extended States Supervisor
the stress being on "Supervisor" - why does it need to be renamed to
"system" now?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Jan Kara @ 2018-10-02 15:07 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Jan Kara,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20181002143713.GA19845-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
On Tue 02-10-18 07:37:13, Christoph Hellwig wrote:
> On Tue, Oct 02, 2018 at 04:29:59PM +0200, Jan Kara wrote:
> > > OK naive question from me, how do we want an application to be able to
> > > check if it is running on a DAX mapping?
> >
> > The question from me is: Should application really care?
>
> No, it should not. DAX is an implementation detail thay may change
> or go away at any time.
I agree that whether / how pagecache is used for filesystem access is an
implementation detail of the kernel. OTOH for some workloads it is about
whether kernel needs gigabytes of RAM to cache files or not, which is not a
detail anymore if you want to fully utilize the machine. So people will be
asking for this and will be finding odd ways to determine whether DAX is
used or not (such as poking in smaps). And once there is some widely enough
used application doing this, it is not "stupid application" problem anymore
but the kernel's problem of not maintaining backward compatibility.
So I think we would be better off providing *some* API which applications
can use to determine whether pagecache is used or not and make sure this
API will convey the right information even if we change DAX
implementation or remove it altogether.
Honza
--
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 04/11] UAPI: bcache: Fix use of embedded flexible array
From: Jan Engelhardt @ 2018-10-02 14:52 UTC (permalink / raw)
To: David Howells
Cc: linux-api, linux-kbuild, Coly Li, Kent Overstreet, linux-bcache,
linux-kernel
In-Reply-To: <153616290368.23468.7806230605345568524.stgit@warthog.procyon.org.uk>
On Wed, 05 Sep 2018 16:55:03 +0100, David Howells wrote:
>
>The bkey struct defined by bcache is embedded in the jset struct. However,
>this is illegal in C++ as there's a "flexible array" at the end of the struct.
>Change this to be a 0-length struct instead.
>
>- __u64 ptr[];
>+ __u64 ptr[0];
As per the C++ standard, it is _also_ illegal to declare an array of size zero.
"""it [the array size expression] shall be a converted constant expression of
type std::size_t and its value shall be greater than zero."""
—http://eel.is/c++draft/dcl.array
That makes both "__u64 ptr[]" and "__u64 ptr[0]" *implementation-specific
extensions*.
3rd party tooling (concerns both C and C++):
Coverity Scan (IIRC) treats "__u64 ptr[0]" as an array of "definitely-zero"
size. Writing to any element will outright flag an out-of-bounds violation.
That is sensible, since only "ptr[]" was standardized.
Conclusion:
So please, do never use __u64 ptr[0].
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Christoph Hellwig @ 2018-10-02 14:52 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: linux-xfs-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Jan Kara,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20181002144412.GC4963-qw2SdCWA0PpjqqEj2zc+bA@public.gmane.org>
On Tue, Oct 02, 2018 at 04:44:13PM +0200, Johannes Thumshirn wrote:
> On Tue, Oct 02, 2018 at 07:37:13AM -0700, Christoph Hellwig wrote:
> > No, it should not. DAX is an implementation detail thay may change
> > or go away at any time.
>
> Well we had an issue with an application checking for dax, this is how
> we landed here in the first place.
So what exacty is that "DAX" they are querying about (and no, I'm not
joking, nor being philosophical).
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Johannes Thumshirn @ 2018-10-02 14:44 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Jan Kara,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20181002143713.GA19845-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
On Tue, Oct 02, 2018 at 07:37:13AM -0700, Christoph Hellwig wrote:
> No, it should not. DAX is an implementation detail thay may change
> or go away at any time.
Well we had an issue with an application checking for dax, this is how
we landed here in the first place.
It's not that I want them to do it, it's more that they're actually
doing it in all kinds of interesting ways and then complaining when it
doesn't work anymore.
So it's less of an "API beauty price problem" but more of a "provide a
documented way which we won't break" way.
Byte,
Johannes
--
Johannes Thumshirn Storage
jthumshirn-l3A5Bk7waGM@public.gmane.org +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Christoph Hellwig @ 2018-10-02 14:37 UTC (permalink / raw)
To: Jan Kara
Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20181002142959.GD9127-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org>
On Tue, Oct 02, 2018 at 04:29:59PM +0200, Jan Kara wrote:
> > OK naive question from me, how do we want an application to be able to
> > check if it is running on a DAX mapping?
>
> The question from me is: Should application really care?
No, it should not. DAX is an implementation detail thay may change
or go away at any time.
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Jan Kara @ 2018-10-02 14:29 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Jan Kara,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20181002121039.GA3274-qw2SdCWA0PpjqqEj2zc+bA@public.gmane.org>
[Added ext4, xfs, and linux-api folks to CC for the interface discussion]
On Tue 02-10-18 14:10:39, Johannes Thumshirn wrote:
> On Tue, Oct 02, 2018 at 12:05:31PM +0200, Jan Kara wrote:
> > Hello,
> >
> > commit e1fb4a086495 "dax: remove VM_MIXEDMAP for fsdax and device dax" has
> > removed VM_MIXEDMAP flag from DAX VMAs. Now our testing shows that in the
> > mean time certain customer of ours started poking into /proc/<pid>/smaps
> > and looks at VMA flags there and if VM_MIXEDMAP is missing among the VMA
> > flags, the application just fails to start complaining that DAX support is
> > missing in the kernel. The question now is how do we go about this?
>
> OK naive question from me, how do we want an application to be able to
> check if it is running on a DAX mapping?
The question from me is: Should application really care? After all DAX is
just a caching decision. Sure it affects performance characteristics and
memory usage of the kernel but it is not a correctness issue (in particular
we took care for MAP_SYNC to return EOPNOTSUPP if the feature cannot be
supported for current mapping). And in the future the details of what we do
with DAX mapping can change - e.g. I could imagine we might decide to cache
writes in DRAM but do direct PMEM access on reads. And all this could be
auto-tuned based on media properties. And we don't want to tie our hands by
specifying too narrowly how the kernel is going to behave.
OTOH I understand that e.g. for a large database application the difference
between DAX and non-DAX mapping can be a difference between performs fine
and performs terribly / kills the machine so such application might want to
determine / force caching policy to save sysadmin from debugging why the
application is misbehaving.
> AFAIU DAX is always associated with a file descriptor of some kind (be
> it a real file with filesystem dax or the /dev/dax device file for
> device dax). So could a new fcntl() be of any help here? IS_DAX() only
> checks for the S_DAX flag in inode::i_flags, so this should be doable
> for both fsdax and devdax.
So fcntl() to query DAX usage is one option. Another option is the GETFLAGS
ioctl with which you can query the state of S_DAX flag (works only for XFS
currently). But that inode flag was meant more as a hint "use DAX if
available" AFAIK so that's probably not really suitable for querying
whether DAX is really in use or not. Since DAX is really about caching
policy, I was also thinking that we could use madvise / fadvise for this.
I.e., something like MADV_DIRECT_ACCESS which would return with success if
DAX is in use, with error if not. Later, kernel could use it as a hint to
really force DAX on a mapping and not try clever caching policies...
Thoughts?
Honza
--
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR
^ permalink raw reply
* [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-10-02 11:28 UTC (permalink / raw)
To: Andrew Morton
Cc: David Rientjes, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <20180926060624.GA18685@dhcp22.suse.cz>
On Wed 26-09-18 08:06:24, Michal Hocko wrote:
> On Tue 25-09-18 15:04:06, Andrew Morton wrote:
> > On Tue, 25 Sep 2018 14:45:19 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:
> >
> > > > > It is also used in
> > > > > automated testing to ensure that vmas get disabled for thp appropriately
> > > > > and we used "nh" since that is how PR_SET_THP_DISABLE previously enforced
> > > > > this, and those tests now break.
> > > >
> > > > This sounds like a bit of an abuse to me. It shows how an internal
> > > > implementation detail leaks out to the userspace which is something we
> > > > should try to avoid.
> > > >
> > >
> > > Well, it's already how this has worked for years before commit
> > > 1860033237d4 broke it. Changing the implementation in the kernel is fine
> > > as long as you don't break userspace who relies on what is exported to it
> > > and is the only way to determine if MADV_NOHUGEPAGE is preventing it from
> > > being backed by hugepages.
> >
> > 1860033237d4 was over a year ago so perhaps we don't need to be
> > too worried about restoring the old interface. In which case
> > we have an opportunity to make improvements such as that suggested
> > by Michal?
>
> Yeah, can we add a way to export PR_SET_THP_DISABLE to userspace
> somehow? E.g. /proc/<pid>/status. It is a process wide thing so
> reporting it per VMA sounds strange at best.
So how about this? (not tested yet but it should be pretty
straightforward)
---
>From 048b29102de326900b54cce78b614345cd77a230 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Tue, 2 Oct 2018 10:53:48 +0200
Subject: [PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
David Rientjes has reported that 1860033237d4 ("mm: make
PR_SET_THP_DISABLE immediately active") has changed the way how
we report THPable VMAs to the userspace. Their monitoring tool is
triggering false alarms on PR_SET_THP_DISABLE tasks because it considers
an insufficient THP usage as a memory fragmentation resp. memory
pressure issue.
Before the said commit each newly created VMA inherited VM_NOHUGEPAGE
flag and that got exposed to the userspace via /proc/<pid>/smaps file.
This implementation had its downsides as explained in the commit message
but it is true that the userspace doesn't have any means to query for
the process wide THP enabled/disabled status.
PR_SET_THP_DISABLE is a process wide flag so it makes a lot of sense
to export in the process wide context rather than per-vma. Introduce
a new field to /proc/<pid>/status which export this status. If
PR_SET_THP_DISABLE is used the it reports false same as when the THP is
not compiled in. It doesn't consider the global THP status because we
already export that information via sysfs
Fixes: 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
Documentation/filesystems/proc.txt | 3 +++
fs/proc/array.c | 10 ++++++++++
2 files changed, 13 insertions(+)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 22b4b00dee31..bafa5cb1685a 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -182,6 +182,7 @@ For example, to get the status information of a process, all you have to do is
VmSwap: 0 kB
HugetlbPages: 0 kB
CoreDumping: 0
+ THP_enabled: 1
Threads: 1
SigQ: 0/28578
SigPnd: 0000000000000000
@@ -256,6 +257,8 @@ Table 1-2: Contents of the status files (as of 4.8)
HugetlbPages size of hugetlb memory portions
CoreDumping process's memory is currently being dumped
(killing the process may lead to a corrupted core)
+ THP_enabled process is allowed to use THP (returns 0 when
+ PR_SET_THP_DISABLE is set on the process
Threads number of threads
SigQ number of signals queued/max. number for queue
SigPnd bitmap of pending signals for the thread
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 0ceb3b6b37e7..9d428d5a0ac8 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -392,6 +392,15 @@ static inline void task_core_dumping(struct seq_file *m, struct mm_struct *mm)
seq_putc(m, '\n');
}
+static inline void task_thp_status(struct seq_file *m, struct mm_struct *mm)
+{
+ bool thp_enabled = IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE);
+
+ if (thp_enabled)
+ thp_enabled = !test_bit(MMF_DISABLE_THP, &mm->flags);
+ seq_printf(m, "THP_enabled:\t%d\n", thp_enabled);
+}
+
int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
@@ -406,6 +415,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
if (mm) {
task_mem(m, mm);
task_core_dumping(m, mm);
+ task_thp_status(m, mm);
mmput(mm);
}
task_sig(m, task);
--
2.19.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply related
* [RFC v2 v2 1/1] ns: add binfmt_misc to the mount namespace
From: Laurent Vivier @ 2018-10-02 10:20 UTC (permalink / raw)
To: linux-kernel
Cc: Dmitry Safonov, Andrei Vagin, Eric Biederman, Alexander Viro,
James Bottomley, containers, linux-fsdevel, linux-api,
Laurent Vivier
In-Reply-To: <20181002102054.13245-1-laurent@vivier.eu>
This patch allows to have a different binftm_misc configuration
in each container we mount binfmt_misc filesystem with mount namespace
enabled.
A container started without the CLONE_NEWNS will use the host binfmt_misc
configuration, otherwise the container starts with an empty binfmt_misc
interpreters list.
For instance, using "unshare" we can start a chroot of an another
architecture and configure the binfmt_misc interpreted without being root
to run the binaries in this chroot.
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
fs/binfmt_misc.c | 50 +++++++++++++++++++++++++-----------------------
fs/mount.h | 8 ++++++++
fs/namespace.c | 6 ++++++
3 files changed, 40 insertions(+), 24 deletions(-)
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index aa4a7a23ff99..ecb14776c759 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -25,6 +25,7 @@
#include <linux/syscalls.h>
#include <linux/fs.h>
#include <linux/uaccess.h>
+#include <mount.h>
#include "internal.h"
@@ -38,9 +39,6 @@ enum {
VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
};
-static LIST_HEAD(entries);
-static int enabled = 1;
-
enum {Enabled, Magic};
#define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
#define MISC_FMT_OPEN_BINARY (1 << 30)
@@ -60,10 +58,7 @@ typedef struct {
struct file *interp_file;
} Node;
-static DEFINE_RWLOCK(entries_lock);
static struct file_system_type bm_fs_type;
-static struct vfsmount *bm_mnt;
-static int entry_count;
/*
* Max length of the register string. Determined by:
@@ -91,7 +86,7 @@ static Node *check_file(struct linux_binprm *bprm)
struct list_head *l;
/* Walk all the registered handlers. */
- list_for_each(l, &entries) {
+ list_for_each(l, &binfmt_ns(entries)) {
Node *e = list_entry(l, Node, list);
char *s;
int j;
@@ -135,15 +130,15 @@ static int load_misc_binary(struct linux_binprm *bprm)
int fd_binary = -1;
retval = -ENOEXEC;
- if (!enabled)
+ if (!binfmt_ns(enabled))
return retval;
/* to keep locking time low, we copy the interpreter string */
- read_lock(&entries_lock);
+ read_lock(&binfmt_ns(entries_lock));
fmt = check_file(bprm);
if (fmt)
dget(fmt->dentry);
- read_unlock(&entries_lock);
+ read_unlock(&binfmt_ns(entries_lock));
if (!fmt)
return retval;
@@ -613,15 +608,15 @@ static void kill_node(Node *e)
{
struct dentry *dentry;
- write_lock(&entries_lock);
+ write_lock(&binfmt_ns(entries_lock));
list_del_init(&e->list);
- write_unlock(&entries_lock);
+ write_unlock(&binfmt_ns(entries_lock));
dentry = e->dentry;
drop_nlink(d_inode(dentry));
d_drop(dentry);
dput(dentry);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&binfmt_ns(bm_mnt), &binfmt_ns(entry_count));
}
/* /<entry> */
@@ -716,7 +711,8 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (!inode)
goto out2;
- err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
+ err = simple_pin_fs(&bm_fs_type, &binfmt_ns(bm_mnt),
+ &binfmt_ns(entry_count));
if (err) {
iput(inode);
inode = NULL;
@@ -730,7 +726,8 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (IS_ERR(f)) {
err = PTR_ERR(f);
pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&binfmt_ns(bm_mnt),
+ &binfmt_ns(entry_count));
iput(inode);
inode = NULL;
goto out2;
@@ -743,9 +740,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
inode->i_fop = &bm_entry_operations;
d_instantiate(dentry, inode);
- write_lock(&entries_lock);
- list_add(&e->list, &entries);
- write_unlock(&entries_lock);
+ write_lock(&binfmt_ns(entries_lock));
+ list_add(&e->list, &binfmt_ns(entries));
+ write_unlock(&binfmt_ns(entries_lock));
err = 0;
out2:
@@ -770,7 +767,7 @@ static const struct file_operations bm_register_operations = {
static ssize_t
bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
{
- char *s = enabled ? "enabled\n" : "disabled\n";
+ char *s = binfmt_ns(enabled) ? "enabled\n" : "disabled\n";
return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
}
@@ -784,19 +781,20 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,
switch (res) {
case 1:
/* Disable all handlers. */
- enabled = 0;
+ binfmt_ns(enabled) = 0;
break;
case 2:
/* Enable all handlers. */
- enabled = 1;
+ binfmt_ns(enabled) = 1;
break;
case 3:
/* Delete all handlers. */
root = file_inode(file)->i_sb->s_root;
inode_lock(d_inode(root));
- while (!list_empty(&entries))
- kill_node(list_first_entry(&entries, Node, list));
+ while (!list_empty(&binfmt_ns(entries)))
+ kill_node(list_first_entry(&binfmt_ns(entries),
+ Node, list));
inode_unlock(d_inode(root));
break;
@@ -838,7 +836,10 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
static struct dentry *bm_mount(struct file_system_type *fs_type,
int flags, const char *dev_name, void *data)
{
- return mount_single(fs_type, flags, data, bm_fill_super);
+ struct mnt_namespace *mnt_ns = current->nsproxy->mnt_ns;
+
+ return mount_ns(fs_type, flags, data, mnt_ns, mnt_ns->user_ns,
+ bm_fill_super);
}
static struct linux_binfmt misc_format = {
@@ -849,6 +850,7 @@ static struct linux_binfmt misc_format = {
static struct file_system_type bm_fs_type = {
.owner = THIS_MODULE,
.name = "binfmt_misc",
+ .fs_flags = FS_USERNS_MOUNT,
.mount = bm_mount,
.kill_sb = kill_litter_super,
};
diff --git a/fs/mount.h b/fs/mount.h
index f39bc9da4d73..f03b35141440 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -17,6 +17,12 @@ struct mnt_namespace {
u64 event;
unsigned int mounts; /* # of mounts in the namespace */
unsigned int pending_mounts;
+ /* binfmt misc */
+ struct list_head entries;
+ rwlock_t entries_lock;
+ int enabled;
+ struct vfsmount *bm_mnt;
+ int entry_count;
} __randomize_layout;
struct mnt_pcp {
@@ -72,6 +78,8 @@ struct mount {
struct dentry *mnt_ex_mountpoint;
} __randomize_layout;
+#define binfmt_ns(a) (current->nsproxy->mnt_ns->a)
+
#define MNT_NS_INTERNAL ERR_PTR(-EINVAL) /* distinct from any mnt_namespace */
static inline struct mount *real_mount(struct vfsmount *mnt)
diff --git a/fs/namespace.c b/fs/namespace.c
index 99186556f8d3..f92b8371228d 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2850,6 +2850,12 @@ static struct mnt_namespace *alloc_mnt_ns(struct user_namespace *user_ns)
new_ns->ucounts = ucounts;
new_ns->mounts = 0;
new_ns->pending_mounts = 0;
+ /* binfmt_misc */
+ INIT_LIST_HEAD(&new_ns->entries);
+ new_ns->enabled = 1;
+ rwlock_init(&new_ns->entries_lock);
+ new_ns->bm_mnt = NULL;
+ new_ns->entry_count = 0;
return new_ns;
}
--
2.17.1
^ permalink raw reply related
* [RFC v2 v2 0/1] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2018-10-02 10:20 UTC (permalink / raw)
To: linux-kernel
Cc: Dmitry Safonov, Andrei Vagin, Eric Biederman, Alexander Viro,
James Bottomley, containers, linux-fsdevel, linux-api,
Laurent Vivier
v2: no new namespace, binfmt_misc data are now part of
the mount namespace
I put this in mount namespace instead of user namespace
because the mount namespace is already needed and
I don't want to force to have the user namespace for that.
As this is a filesystem, it seems logic to have it here.
This allows to define a new interpreter for each new container.
But the main goal is to be able to chroot to a directory
using a binfmt_misc interpreter without being root.
I have a modified version of unshare at:
git@github.com:vivier/util-linux.git branch unshare-chroot
with some new options to unshare binfmt_misc namespace and to chroot
to a directory.
If you have a directory /chroot/powerpc/jessie containing debian for powerpc
binaries and a qemu-ppc interpreter, you can do for instance:
$ uname -a
Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 x86_64 x86_64 x86_64 GNU/Linux
$ ./unshare --map-root-user --fork --pid \
--load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/qemu-ppc:OC" \
--root=/chroot/powerpc/jessie /bin/bash -l
# uname -a
Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 ppc GNU/Linux
# id
uid=0(root) gid=0(root) groups=0(root),65534(nogroup)
# ls -l
total 5940
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 bin
drwxr-xr-x. 2 nobody nogroup 4096 Jun 17 20:26 boot
drwxr-xr-x. 4 nobody nogroup 4096 Aug 12 00:08 dev
drwxr-xr-x. 42 nobody nogroup 4096 Sep 28 07:25 etc
drwxr-xr-x. 3 nobody nogroup 4096 Sep 28 07:25 home
drwxr-xr-x. 9 nobody nogroup 4096 Aug 12 00:58 lib
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 media
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 mnt
drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 13:09 opt
dr-xr-xr-x. 143 nobody nogroup 0 Sep 30 23:02 proc
-rwxr-xr-x. 1 nobody nogroup 6009712 Sep 28 07:22 qemu-ppc
drwx------. 3 nobody nogroup 4096 Aug 12 12:54 root
drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 00:08 run
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 sbin
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 srv
drwxr-xr-x. 2 nobody nogroup 4096 Apr 6 2015 sys
drwxrwxrwt. 2 nobody nogroup 4096 Sep 28 10:31 tmp
drwxr-xr-x. 10 nobody nogroup 4096 Aug 12 00:08 usr
drwxr-xr-x. 11 nobody nogroup 4096 Aug 12 00:08 var
If you want to use the qemu binary provided by your distro, you can use
--load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/bin/qemu-ppc-static:OCF"
With the 'F' flag, qemu-ppc-static will be then loaded from the main root
filesystem before switching to the chroot.
Laurent Vivier (1):
ns: add binfmt_misc to the mount namespace
fs/binfmt_misc.c | 50 +++++++++++++++++++++++++-----------------------
fs/mount.h | 8 ++++++++
fs/namespace.c | 6 ++++++
3 files changed, 40 insertions(+), 24 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Aleksa Sarai @ 2018-10-02 7:32 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jann Horn, Eric W. Biederman, jlayton, Bruce Fields, Al Viro,
Arnd Bergmann, shuah, David Howells, Andy Lutomirski, christian,
Tycho Andersen, kernel list, linux-fsdevel, linux-arch,
linux-kselftest, dev, containers, Linux API
In-Reply-To: <C89D720F-3CC4-4FA9-9CBB-E41A67360A6B@amacapital.net>
[-- Attachment #1: Type: text/plain, Size: 3183 bytes --]
On 2018-10-01, Andy Lutomirski <luto@amacapital.net> wrote:
> >>> Currently most container runtimes try to do this resolution in
> >>> userspace[1], causing many potential race conditions. In addition, the
> >>> "obvious" alternative (actually performing a {ch,pivot_}root(2))
> >>> requires a fork+exec which is *very* costly if necessary for every
> >>> filesystem operation involving a container.
> >>
> >> Wait. fork() I understand, but why exec? And actually, you don't need
> >> a full fork() either, clone() lets you do this with some process parts
> >> shared. And then you also shouldn't need to use SCM_RIGHTS, just keep
> >> the file descriptor table shared. And why chroot()/pivot_root(),
> >> wouldn't you want to use setns()?
> >
> > You're right about this -- for C runtimes. In Go we cannot do a raw
> > clone() or fork() (if you do it manually with RawSyscall you'll end with
> > broken runtime state). So you're forced to do fork+exec (which then
> > means that you can't use CLONE_FILES and must use SCM_RIGHTS). Same goes
> > for CLONE_VFORK.
>
> I must admit that I’m not very sympathetic to the argument that “Go’s
> runtime model is incompatible with the simpler solution.”
Multi-threaded programs have a similar issue (though with Go it's much
worse). If you fork a multi-threaded C program then you can only safely
use AS-Safe glibc functions (those that are safe within a signal
handler). But if you're just doing three syscalls this shouldn't be as
big of a problem as Go where you can't even do said syscalls.
> Anyway, it occurs to me that the real problem is that setns() and
> chroot() are both overkill for this use case.
I agree. My diversion to Go was to explain why it was particularly bad
for cri-o/rkt/runc/Docker/etc.
> What’s needed is to start your walk from /proc/pid-in-container/root,
> with two twists:
>
> 1. Do the walk as though rooted at a directory. This is basically just
> your AT_THIS_ROOT, but the footgun is avoided because the dirfd you
> use is from a foreign namespace, and, except for symlinks to absolute
> paths, no amount of .. racing is going to escape the *namespace*.
This is quite clever and I'll admit I hadn't thought of this. This
definitely fixes the ".." issue, but as you've said it won't handle
absolute symlinks (which means userspace has the same races that we
currently have even if you assume that you have a container process
already running -- CVE-2018-15664 is one of many examples of this).
(AT_THIS_ROOT using /proc/$container/root would in principle fix all of
the mentioned issues -- but as I said before I'd like to see whether
hardening ".." would be enough to solve the escape problem.)
> 2. Avoid /proc. It’s not just the *links* — you really don’t want to
> walk into /proc/self. *Maybe* procfs is already careful enough when
> mounted in a namespace?
I just tried it and /proc/self gives you -ENOENT. I believe this is
because it does a check against the pid namespace that the procfs mount
has pinned.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: Setting monotonic time?
From: Thomas Gleixner @ 2018-10-02 6:16 UTC (permalink / raw)
To: Andrey Vagin
Cc: Eric W. Biederman, Dmitry Safonov, linux-kernel@vger.kernel.org,
Dmitry Safonov, Adrian Reber, Andy Lutomirski, Christian Brauner,
Cyrill Gorcunov, H. Peter Anvin, Ingo Molnar, Jeff Dike,
Oleg Nesterov, Pavel Emelianov, Shuah Khan,
containers@lists.linux-foundation.org, criu@openvz.org,
linux-api@vger.kernel.org, x86@kernel.org, Alexey
In-Reply-To: <20181001205124.GA23620@outlook.office365.com>
On Mon, 1 Oct 2018, Andrey Vagin wrote:
> On Mon, Oct 01, 2018 at 11:15:32AM +0200, Eric W. Biederman wrote:
> >
> > In the context of process migration there is a simpler subproblem that I
> > think it is worth exploring if we can do something about.
> >
> > For a cluster of machines all running with synchronized
> > clocks. CLOCK_REALTIME matches. CLOCK_MONOTNIC does not match between
> > machines. Not having a matching CLOCK_MONOTONIC prevents successful
> > process migration between nodes in that cluster.
> >
> > Would it be possible to allow setting CLOCK_MONOTONIC at the very
> > beginning of time? So that all of the nodes in a cluster can be in
> > sync?
>
> Here is a question about how to synchronize clocks between nodes. It
> looks like we will need to have a working network for this, but a
> network configuration may be non-trivial and it can require to run a few
> processes which can use CLOCK_MONOTNIC...
>
> >
> > No change in skew just in offset for CLOCK_MONOTONIC.
> >
> > There are also dragons involved in coordinating things so that
> > CLOCK_MONOTONIC gets set before CLOCK_MONOTONIC gets used. So I don't
> > know if allowing CLOCK_MONOTONIC to be set would be practical but it
> > seems work exploring all on it's own.
> >
> > Dmitry would setting CLOCK_MONOTONIC exactly once at boot time solve
> > your problem that is you are looking at a time namespace to solve?
>
> Process migration is only one of use-cases. Another use-case is
> restoring from snapshots. It may be even more popular than process
> migration. We can't guarantee that all snapshots will be done in one
> cluster. For example, a user meets a bug, does a container snapshot and
> attaches it to a bug report.
Sure, but see my reply to Eric. That could be solved with that extra clock
id, which then gets mapped to monotonic for name spaces.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC 00/20] ns: Introduce Time Namespace
From: Thomas Gleixner @ 2018-10-02 6:15 UTC (permalink / raw)
To: Andrey Vagin
Cc: Eric W. Biederman, Dmitry Safonov, linux-kernel@vger.kernel.org,
Dmitry Safonov, Adrian Reber, Andy Lutomirski, Christian Brauner,
Cyrill Gorcunov, H. Peter Anvin, Ingo Molnar, Jeff Dike,
Oleg Nesterov, Pavel Emelianov, Shuah Khan,
containers@lists.linux-foundation.org, criu@openvz.org,
linux-api@vger.kernel.org, x86@kernel.org, Alexey
In-Reply-To: <20181001232033.GA31324@outlook.office365.com>
On Mon, 1 Oct 2018, Andrey Vagin wrote:
> On Thu, Sep 27, 2018 at 11:41:49PM +0200, Thomas Gleixner wrote:
> > On Thu, 27 Sep 2018, Thomas Gleixner wrote:
> > > Add time skew via NTP/PTP into the picture and you might have to adjust
> > > timers as well, because you need to guarantee that they are not expiring
> > > early.
> > >
> > > I haven't looked through Dimitry's patches yet, but I don't see how this
> > > can work at all without introducing subtle issues all over the place.
> >
> > And just a quick scan tells me that this is broken. Timers will expire
> > early or late. The latter is acceptible to some extent, but larger delays
> > might come with surprise. Expiring early is an absolute nono.
>
> Do you mean that we have to adjust all timers after changing offset for
> CLOCK_MONOTONIC or CLOCK_BOOTTIME? Our idea is that offsets for
> monotonic and boot times will be set immediately after creating a time
> namespace before using any timers.
I explained that in detail in this thread, but it's not about the initial
setting of clock mono/boot before any timers have been armed.
It's about setting the offset or clock realtime (via settimeofday) when
timers are already armed. Also having a entirely different time domain,
e.g. separate NTP adjustments, makes that necessary.
Thanks,
tglx
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox