* Re: [PATCH 2/6] x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit
From: Andy Lutomirski @ 2015-03-13 22:01 UTC (permalink / raw)
To: Josh Triplett
Cc: Al Viro, Andrew Morton, Ingo Molnar, Kees Cook, Oleg Nesterov,
Paul E. McKenney, H. Peter Anvin, Rik van Riel, Thomas Gleixner,
Thiago Macieira, Michael Kerrisk,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API,
Linux FS Devel, X86 ML
In-Reply-To: <cf79b9f0c40314e6bfda7c634e378015bd7ba037.1426180120.git.josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>
On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> For 32-bit userspace on a 64-bit kernel, this requires modifying
> stub32_clone to actually swap the appropriate arguments to match
> CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls
> broken.
>
> Signed-off-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>
> Signed-off-by: Thiago Macieira <thiago.macieira-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> arch/x86/Kconfig | 1 +
> arch/x86/ia32/ia32entry.S | 2 +-
> arch/x86/kernel/process_32.c | 6 +++---
> arch/x86/kernel/process_64.c | 8 ++++----
> 4 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index b7d31ca..4960b0d 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -124,6 +124,7 @@ config X86
> select MODULES_USE_ELF_REL if X86_32
> select MODULES_USE_ELF_RELA if X86_64
> select CLONE_BACKWARDS if X86_32
> + select HAVE_COPY_THREAD_TLS
> select ARCH_USE_BUILTIN_BSWAP
> select ARCH_USE_QUEUE_RWLOCK
> select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION
> diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
> index 156ebca..0286735 100644
> --- a/arch/x86/ia32/ia32entry.S
> +++ b/arch/x86/ia32/ia32entry.S
> @@ -487,7 +487,7 @@ GLOBAL(\label)
> ALIGN
> GLOBAL(stub32_clone)
> leaq sys_clone(%rip),%rax
> - mov %r8, %rcx
> + xchg %r8, %rcx
> jmp ia32_ptregs_common
Do I understand correct that whatever function this is a stub for just
takes its arguments in the wrong order? If so, can we just fix it
instead of using xchg here?
In general, I much prefer C code to new asm where it makes sense to
make this tradeoff.
Other than that, this is a huge improvement. You'll have minor
conflicts against -tip, though.
--Andy
^ permalink raw reply
* Re: [PATCH 6/6] clone4: Introduce new CLONE_FD flag to get task exit notification via fd
From: josh @ 2015-03-13 22:20 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Oleg Nesterov, Al Viro, Andrew Morton, Ingo Molnar, Kees Cook,
Paul E. McKenney, H. Peter Anvin, Rik van Riel, Thomas Gleixner,
Thiago Macieira, Michael Kerrisk, linux-kernel@vger.kernel.org,
Linux API, Linux FS Devel, X86 ML
In-Reply-To: <CALCETrVkZVa6zsd6gpW_AnQSO2cC___U8M3Dc7c6=PJdEiT9Pg@mail.gmail.com>
On Fri, Mar 13, 2015 at 02:34:58PM -0700, Andy Lutomirski wrote:
> On Fri, Mar 13, 2015 at 12:57 PM, <josh@joshtriplett.org> wrote:
> > A process launching a new process with CLONE_FD is explicitly requesting
> > that the process be automatically reaped without any other process
> > having to wait on it. The task needs to not become a zombie, because
> > otherwise, it'll show up in waitpid(-1, ...) calls in the parent
> > process, which would break the ability to use this to completely
> > encapsulate process management within a library and not interfere with
> > the parent's process handling via SIGCHLD and wait{pid,3,4}.
>
> Wouldn't the correct behavior be to keep it alive as a zombie but
> *not* show it in waitpid, etc?
That's a significant change to the semantics of waitpid. And then
someone would still need to wait on the process, which we'd like to
avoid. (We don't want to have magic "reap on read(2)" semantics,
because among other things, what if we add a means in the future to get
an additional file descriptor corresponding to an existing process?)
- Josh Triplett
^ permalink raw reply
* Re: [PATCH 6/6] clone4: Introduce new CLONE_FD flag to get task exit notification via fd
From: Andy Lutomirski @ 2015-03-13 22:28 UTC (permalink / raw)
To: Josh Triplett
Cc: Oleg Nesterov, Al Viro, Andrew Morton, Ingo Molnar, Kees Cook,
Paul E. McKenney, H. Peter Anvin, Rik van Riel, Thomas Gleixner,
Thiago Macieira, Michael Kerrisk,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API,
Linux FS Devel, X86 ML
In-Reply-To: <20150313222052.GC10954@cloud>
On Fri, Mar 13, 2015 at 3:20 PM, <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> On Fri, Mar 13, 2015 at 02:34:58PM -0700, Andy Lutomirski wrote:
>> On Fri, Mar 13, 2015 at 12:57 PM, <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
>> > A process launching a new process with CLONE_FD is explicitly requesting
>> > that the process be automatically reaped without any other process
>> > having to wait on it. The task needs to not become a zombie, because
>> > otherwise, it'll show up in waitpid(-1, ...) calls in the parent
>> > process, which would break the ability to use this to completely
>> > encapsulate process management within a library and not interfere with
>> > the parent's process handling via SIGCHLD and wait{pid,3,4}.
>>
>> Wouldn't the correct behavior be to keep it alive as a zombie but
>> *not* show it in waitpid, etc?
>
> That's a significant change to the semantics of waitpid. And then
> someone would still need to wait on the process, which we'd like to
> avoid. (We don't want to have magic "reap on read(2)" semantics,
> because among other things, what if we add a means in the future to get
> an additional file descriptor corresponding to an existing process?)
>
Do we not already have a state "dead, successfully waited on by
parent, but still around because ptraced"? If not, shouldn't we?
Isn't that what PTRACE_SEIZE does? Or am I just confused?
--Andy
^ permalink raw reply
* Re: [PATCH 2/6] x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit
From: josh @ 2015-03-13 22:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Al Viro, Andrew Morton, Ingo Molnar, Kees Cook, Oleg Nesterov,
Paul E. McKenney, H. Peter Anvin, Rik van Riel, Thomas Gleixner,
Thiago Macieira, Michael Kerrisk, linux-kernel@vger.kernel.org,
Linux API, Linux FS Devel, X86 ML
In-Reply-To: <CALCETrUbQJhE8VCTt5yQ+KBZOJnNC5e3yUneKjhwhKxgvfU2Xg@mail.gmail.com>
On Fri, Mar 13, 2015 at 03:01:16PM -0700, Andy Lutomirski wrote:
> On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > For 32-bit userspace on a 64-bit kernel, this requires modifying
> > stub32_clone to actually swap the appropriate arguments to match
> > CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls
> > broken.
> >
> > Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> > Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
> > ---
> > arch/x86/Kconfig | 1 +
> > arch/x86/ia32/ia32entry.S | 2 +-
> > arch/x86/kernel/process_32.c | 6 +++---
> > arch/x86/kernel/process_64.c | 8 ++++----
> > 4 files changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index b7d31ca..4960b0d 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -124,6 +124,7 @@ config X86
> > select MODULES_USE_ELF_REL if X86_32
> > select MODULES_USE_ELF_RELA if X86_64
> > select CLONE_BACKWARDS if X86_32
> > + select HAVE_COPY_THREAD_TLS
> > select ARCH_USE_BUILTIN_BSWAP
> > select ARCH_USE_QUEUE_RWLOCK
> > select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION
> > diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
> > index 156ebca..0286735 100644
> > --- a/arch/x86/ia32/ia32entry.S
> > +++ b/arch/x86/ia32/ia32entry.S
> > @@ -487,7 +487,7 @@ GLOBAL(\label)
> > ALIGN
> > GLOBAL(stub32_clone)
> > leaq sys_clone(%rip),%rax
> > - mov %r8, %rcx
> > + xchg %r8, %rcx
> > jmp ia32_ptregs_common
>
> Do I understand correct that whatever function this is a stub for just
> takes its arguments in the wrong order? If so, can we just fix it
> instead of using xchg here?
32-bit x86 and 64-bit x86 take the arguments to clone in a different
order, and stub32_clone fixes up the argument order then calls the
64-bit sys_clone.
I'd love to see *all* the 32-on-64 compat stubs for clone rewritten in C
under CONFIG_COMPAT. However, doing so would require encoding the
knowledge for each 64-bit architecture for how its corresponding 32-bit
architecture accepts arguments to clone, which is information that the
current CONFIG_CLONE_BACKWARDS{1,2,3} don't include; it would then
require cleaning up all the architecture-specific assembly stubs for
32-bit clone entry points.
In the meantime, doing that *just* for 32-bit x86 on 64-bit x86 doesn't
seem worth it, since it would require adding a new C entry point for
compat_sys_clone under arch/x86 somewhere.
One cleanup at a time. :)
> In general, I much prefer C code to new asm where it makes sense to
> make this tradeoff.
Agreed completely. However, this is at least conservation-of-asm, or
reduction if you consider the pt_regs argument-grabbing hack to be
asm-esque code.
> Other than that, this is a huge improvement. You'll have minor
> conflicts against -tip, though.
Right, I've seen your current changes there. Should be a trivial merge
though.
Would you mind providing an ack for the series, or at least for the
first two patches?
(I'm wondering whose tree this series ought to go through, for that
matter.)
- Josh Triplett
^ permalink raw reply
* Re: [PATCH 6/6] clone4: Introduce new CLONE_FD flag to get task exit notification via fd
From: josh-iaAMLnmF4UmaiuxdJuQwMA @ 2015-03-13 22:34 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Oleg Nesterov, Al Viro, Andrew Morton, Ingo Molnar, Kees Cook,
Paul E. McKenney, H. Peter Anvin, Rik van Riel, Thomas Gleixner,
Thiago Macieira, Michael Kerrisk,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API,
Linux FS Devel, X86 ML
In-Reply-To: <CALCETrVRqgsbpi9pPRwy42cuXiDzyPgWRJmfSRSQM7eGFfsZYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Mar 13, 2015 at 03:28:26PM -0700, Andy Lutomirski wrote:
> On Fri, Mar 13, 2015 at 3:20 PM, <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> > On Fri, Mar 13, 2015 at 02:34:58PM -0700, Andy Lutomirski wrote:
> >> On Fri, Mar 13, 2015 at 12:57 PM, <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> >> > A process launching a new process with CLONE_FD is explicitly requesting
> >> > that the process be automatically reaped without any other process
> >> > having to wait on it. The task needs to not become a zombie, because
> >> > otherwise, it'll show up in waitpid(-1, ...) calls in the parent
> >> > process, which would break the ability to use this to completely
> >> > encapsulate process management within a library and not interfere with
> >> > the parent's process handling via SIGCHLD and wait{pid,3,4}.
> >>
> >> Wouldn't the correct behavior be to keep it alive as a zombie but
> >> *not* show it in waitpid, etc?
> >
> > That's a significant change to the semantics of waitpid. And then
> > someone would still need to wait on the process, which we'd like to
> > avoid. (We don't want to have magic "reap on read(2)" semantics,
> > because among other things, what if we add a means in the future to get
> > an additional file descriptor corresponding to an existing process?)
>
> Do we not already have a state "dead, successfully waited on by
> parent, but still around because ptraced"? If not, shouldn't we?
> Isn't that what PTRACE_SEIZE does? Or am I just confused?
I don't think that affects the task's exit_state though.
- Josh Triplett
^ permalink raw reply
* Re: [PATCH 2/6] x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit
From: Andy Lutomirski @ 2015-03-13 22:38 UTC (permalink / raw)
To: Josh Triplett
Cc: Al Viro, Andrew Morton, Ingo Molnar, Kees Cook, Oleg Nesterov,
Paul E. McKenney, H. Peter Anvin, Rik van Riel, Thomas Gleixner,
Thiago Macieira, Michael Kerrisk, linux-kernel@vger.kernel.org,
Linux API, Linux FS Devel, X86 ML
In-Reply-To: <20150313223139.GD10954@cloud>
On Fri, Mar 13, 2015 at 3:31 PM, <josh@joshtriplett.org> wrote:
> On Fri, Mar 13, 2015 at 03:01:16PM -0700, Andy Lutomirski wrote:
>> On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh@joshtriplett.org> wrote:
>> > For 32-bit userspace on a 64-bit kernel, this requires modifying
>> > stub32_clone to actually swap the appropriate arguments to match
>> > CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls
>> > broken.
>> >
>> > Signed-off-by: Josh Triplett <josh@joshtriplett.org>
>> > Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
>> > ---
>> > arch/x86/Kconfig | 1 +
>> > arch/x86/ia32/ia32entry.S | 2 +-
>> > arch/x86/kernel/process_32.c | 6 +++---
>> > arch/x86/kernel/process_64.c | 8 ++++----
>> > 4 files changed, 9 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> > index b7d31ca..4960b0d 100644
>> > --- a/arch/x86/Kconfig
>> > +++ b/arch/x86/Kconfig
>> > @@ -124,6 +124,7 @@ config X86
>> > select MODULES_USE_ELF_REL if X86_32
>> > select MODULES_USE_ELF_RELA if X86_64
>> > select CLONE_BACKWARDS if X86_32
>> > + select HAVE_COPY_THREAD_TLS
>> > select ARCH_USE_BUILTIN_BSWAP
>> > select ARCH_USE_QUEUE_RWLOCK
>> > select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION
>> > diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
>> > index 156ebca..0286735 100644
>> > --- a/arch/x86/ia32/ia32entry.S
>> > +++ b/arch/x86/ia32/ia32entry.S
>> > @@ -487,7 +487,7 @@ GLOBAL(\label)
>> > ALIGN
>> > GLOBAL(stub32_clone)
>> > leaq sys_clone(%rip),%rax
>> > - mov %r8, %rcx
>> > + xchg %r8, %rcx
>> > jmp ia32_ptregs_common
>>
>> Do I understand correct that whatever function this is a stub for just
>> takes its arguments in the wrong order? If so, can we just fix it
>> instead of using xchg here?
>
> 32-bit x86 and 64-bit x86 take the arguments to clone in a different
> order, and stub32_clone fixes up the argument order then calls the
> 64-bit sys_clone.
>
> I'd love to see *all* the 32-on-64 compat stubs for clone rewritten in C
> under CONFIG_COMPAT. However, doing so would require encoding the
> knowledge for each 64-bit architecture for how its corresponding 32-bit
> architecture accepts arguments to clone, which is information that the
> current CONFIG_CLONE_BACKWARDS{1,2,3} don't include; it would then
> require cleaning up all the architecture-specific assembly stubs for
> 32-bit clone entry points.
>
> In the meantime, doing that *just* for 32-bit x86 on 64-bit x86 doesn't
> seem worth it, since it would require adding a new C entry point for
> compat_sys_clone under arch/x86 somewhere.
>
> One cleanup at a time. :)
Fine w/ me.
>
>> In general, I much prefer C code to new asm where it makes sense to
>> make this tradeoff.
>
> Agreed completely. However, this is at least conservation-of-asm, or
> reduction if you consider the pt_regs argument-grabbing hack to be
> asm-esque code.
>
>> Other than that, this is a huge improvement. You'll have minor
>> conflicts against -tip, though.
>
> Right, I've seen your current changes there. Should be a trivial merge
> though.
>
> Would you mind providing an ack for the series, or at least for the
> first two patches?
I can give you an ok-in-principle on the first two. I'd need to stare
at the awful code for a bit to understand the @!*&! clone variants to
really ack them convincingly.
OTOH, it would be rather surprising if you messed it up in a way that
still boots on all three variants (native 32-bit, native 64-bit, and
compat).
So, for the first two patches:
Acked-by: Andy Lutomirski <luto@kernel.org> # assuming all bitnesses boot
--Andy
>
> (I'm wondering whose tree this series ought to go through, for that
> matter.)
>
> - Josh Triplett
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH 6/6] clone4: Introduce new CLONE_FD flag to get task exit notification via fd
From: Andy Lutomirski @ 2015-03-13 22:38 UTC (permalink / raw)
To: Josh Triplett
Cc: Oleg Nesterov, Al Viro, Andrew Morton, Ingo Molnar, Kees Cook,
Paul E. McKenney, H. Peter Anvin, Rik van Riel, Thomas Gleixner,
Thiago Macieira, Michael Kerrisk, linux-kernel@vger.kernel.org,
Linux API, Linux FS Devel, X86 ML
In-Reply-To: <20150313223409.GE10954@cloud>
On Fri, Mar 13, 2015 at 3:34 PM, <josh@joshtriplett.org> wrote:
> On Fri, Mar 13, 2015 at 03:28:26PM -0700, Andy Lutomirski wrote:
>> On Fri, Mar 13, 2015 at 3:20 PM, <josh@joshtriplett.org> wrote:
>> > On Fri, Mar 13, 2015 at 02:34:58PM -0700, Andy Lutomirski wrote:
>> >> On Fri, Mar 13, 2015 at 12:57 PM, <josh@joshtriplett.org> wrote:
>> >> > A process launching a new process with CLONE_FD is explicitly requesting
>> >> > that the process be automatically reaped without any other process
>> >> > having to wait on it. The task needs to not become a zombie, because
>> >> > otherwise, it'll show up in waitpid(-1, ...) calls in the parent
>> >> > process, which would break the ability to use this to completely
>> >> > encapsulate process management within a library and not interfere with
>> >> > the parent's process handling via SIGCHLD and wait{pid,3,4}.
>> >>
>> >> Wouldn't the correct behavior be to keep it alive as a zombie but
>> >> *not* show it in waitpid, etc?
>> >
>> > That's a significant change to the semantics of waitpid. And then
>> > someone would still need to wait on the process, which we'd like to
>> > avoid. (We don't want to have magic "reap on read(2)" semantics,
>> > because among other things, what if we add a means in the future to get
>> > an additional file descriptor corresponding to an existing process?)
>>
>> Do we not already have a state "dead, successfully waited on by
>> parent, but still around because ptraced"? If not, shouldn't we?
>> Isn't that what PTRACE_SEIZE does? Or am I just confused?
>
> I don't think that affects the task's exit_state though.
>
That's a question for Oleg. I have no idea how ptrace is actually implemented.
--Andy
> - Josh Triplett
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH 2/6] x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit
From: josh-iaAMLnmF4UmaiuxdJuQwMA @ 2015-03-13 22:43 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Al Viro, Andrew Morton, Ingo Molnar, Kees Cook, Oleg Nesterov,
Paul E. McKenney, H. Peter Anvin, Rik van Riel, Thomas Gleixner,
Thiago Macieira, Michael Kerrisk,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API,
Linux FS Devel, X86 ML
In-Reply-To: <CALCETrWspvNcEYxwbo1+ifXSj7Qj7YdcRgmNvZ1RaBrUAK12Zw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Mar 13, 2015 at 03:38:31PM -0700, Andy Lutomirski wrote:
> On Fri, Mar 13, 2015 at 3:31 PM, <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> > On Fri, Mar 13, 2015 at 03:01:16PM -0700, Andy Lutomirski wrote:
> >> On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> >> > For 32-bit userspace on a 64-bit kernel, this requires modifying
> >> > stub32_clone to actually swap the appropriate arguments to match
> >> > CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls
> >> > broken.
> >> >
> >> > Signed-off-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>
> >> > Signed-off-by: Thiago Macieira <thiago.macieira-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >> > ---
> >> > arch/x86/Kconfig | 1 +
> >> > arch/x86/ia32/ia32entry.S | 2 +-
> >> > arch/x86/kernel/process_32.c | 6 +++---
> >> > arch/x86/kernel/process_64.c | 8 ++++----
> >> > 4 files changed, 9 insertions(+), 8 deletions(-)
> >> >
> >> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >> > index b7d31ca..4960b0d 100644
> >> > --- a/arch/x86/Kconfig
> >> > +++ b/arch/x86/Kconfig
> >> > @@ -124,6 +124,7 @@ config X86
> >> > select MODULES_USE_ELF_REL if X86_32
> >> > select MODULES_USE_ELF_RELA if X86_64
> >> > select CLONE_BACKWARDS if X86_32
> >> > + select HAVE_COPY_THREAD_TLS
> >> > select ARCH_USE_BUILTIN_BSWAP
> >> > select ARCH_USE_QUEUE_RWLOCK
> >> > select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION
> >> > diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
> >> > index 156ebca..0286735 100644
> >> > --- a/arch/x86/ia32/ia32entry.S
> >> > +++ b/arch/x86/ia32/ia32entry.S
> >> > @@ -487,7 +487,7 @@ GLOBAL(\label)
> >> > ALIGN
> >> > GLOBAL(stub32_clone)
> >> > leaq sys_clone(%rip),%rax
> >> > - mov %r8, %rcx
> >> > + xchg %r8, %rcx
> >> > jmp ia32_ptregs_common
> >>
> >> Do I understand correct that whatever function this is a stub for just
> >> takes its arguments in the wrong order? If so, can we just fix it
> >> instead of using xchg here?
> >
> > 32-bit x86 and 64-bit x86 take the arguments to clone in a different
> > order, and stub32_clone fixes up the argument order then calls the
> > 64-bit sys_clone.
> >
> > I'd love to see *all* the 32-on-64 compat stubs for clone rewritten in C
> > under CONFIG_COMPAT. However, doing so would require encoding the
> > knowledge for each 64-bit architecture for how its corresponding 32-bit
> > architecture accepts arguments to clone, which is information that the
> > current CONFIG_CLONE_BACKWARDS{1,2,3} don't include; it would then
> > require cleaning up all the architecture-specific assembly stubs for
> > 32-bit clone entry points.
> >
> > In the meantime, doing that *just* for 32-bit x86 on 64-bit x86 doesn't
> > seem worth it, since it would require adding a new C entry point for
> > compat_sys_clone under arch/x86 somewhere.
> >
> > One cleanup at a time. :)
>
> Fine w/ me.
Thanks.
> >
> >> In general, I much prefer C code to new asm where it makes sense to
> >> make this tradeoff.
> >
> > Agreed completely. However, this is at least conservation-of-asm, or
> > reduction if you consider the pt_regs argument-grabbing hack to be
> > asm-esque code.
> >
> >> Other than that, this is a huge improvement. You'll have minor
> >> conflicts against -tip, though.
> >
> > Right, I've seen your current changes there. Should be a trivial merge
> > though.
> >
> > Would you mind providing an ack for the series, or at least for the
> > first two patches?
>
> I can give you an ok-in-principle on the first two. I'd need to stare
> at the awful code for a bit to understand the @!*&! clone variants to
> really ack them convincingly.
I'd definitely appreciate the staring. :)
> OTOH, it would be rather surprising if you messed it up in a way that
> still boots on all three variants (native 32-bit, native 64-bit, and
> compat).
>
> So, for the first two patches:
>
> Acked-by: Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> # assuming all bitnesses boot
I did test all three, not just with booting but with a thread-local
storage test.
- Josh Triplett
^ permalink raw reply
* Re: [PATCH 2/6] x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit
From: Andy Lutomirski @ 2015-03-13 22:45 UTC (permalink / raw)
To: Josh Triplett
Cc: Al Viro, Andrew Morton, Ingo Molnar, Kees Cook, Oleg Nesterov,
Paul E. McKenney, H. Peter Anvin, Rik van Riel, Thomas Gleixner,
Thiago Macieira, Michael Kerrisk,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API,
Linux FS Devel, X86 ML
In-Reply-To: <20150313224319.GA11282@cloud>
On Fri, Mar 13, 2015 at 3:43 PM, <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> On Fri, Mar 13, 2015 at 03:38:31PM -0700, Andy Lutomirski wrote:
>> On Fri, Mar 13, 2015 at 3:31 PM, <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
>> > On Fri, Mar 13, 2015 at 03:01:16PM -0700, Andy Lutomirski wrote:
>> >> On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
>> >> > For 32-bit userspace on a 64-bit kernel, this requires modifying
>> >> > stub32_clone to actually swap the appropriate arguments to match
>> >> > CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls
>> >> > broken.
>> >> >
>> >> > Signed-off-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>
>> >> > Signed-off-by: Thiago Macieira <thiago.macieira-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> >> > ---
>> >> > arch/x86/Kconfig | 1 +
>> >> > arch/x86/ia32/ia32entry.S | 2 +-
>> >> > arch/x86/kernel/process_32.c | 6 +++---
>> >> > arch/x86/kernel/process_64.c | 8 ++++----
>> >> > 4 files changed, 9 insertions(+), 8 deletions(-)
>> >> >
>> >> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> >> > index b7d31ca..4960b0d 100644
>> >> > --- a/arch/x86/Kconfig
>> >> > +++ b/arch/x86/Kconfig
>> >> > @@ -124,6 +124,7 @@ config X86
>> >> > select MODULES_USE_ELF_REL if X86_32
>> >> > select MODULES_USE_ELF_RELA if X86_64
>> >> > select CLONE_BACKWARDS if X86_32
>> >> > + select HAVE_COPY_THREAD_TLS
>> >> > select ARCH_USE_BUILTIN_BSWAP
>> >> > select ARCH_USE_QUEUE_RWLOCK
>> >> > select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION
>> >> > diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
>> >> > index 156ebca..0286735 100644
>> >> > --- a/arch/x86/ia32/ia32entry.S
>> >> > +++ b/arch/x86/ia32/ia32entry.S
>> >> > @@ -487,7 +487,7 @@ GLOBAL(\label)
>> >> > ALIGN
>> >> > GLOBAL(stub32_clone)
>> >> > leaq sys_clone(%rip),%rax
>> >> > - mov %r8, %rcx
>> >> > + xchg %r8, %rcx
>> >> > jmp ia32_ptregs_common
>> >>
>> >> Do I understand correct that whatever function this is a stub for just
>> >> takes its arguments in the wrong order? If so, can we just fix it
>> >> instead of using xchg here?
>> >
>> > 32-bit x86 and 64-bit x86 take the arguments to clone in a different
>> > order, and stub32_clone fixes up the argument order then calls the
>> > 64-bit sys_clone.
>> >
>> > I'd love to see *all* the 32-on-64 compat stubs for clone rewritten in C
>> > under CONFIG_COMPAT. However, doing so would require encoding the
>> > knowledge for each 64-bit architecture for how its corresponding 32-bit
>> > architecture accepts arguments to clone, which is information that the
>> > current CONFIG_CLONE_BACKWARDS{1,2,3} don't include; it would then
>> > require cleaning up all the architecture-specific assembly stubs for
>> > 32-bit clone entry points.
>> >
>> > In the meantime, doing that *just* for 32-bit x86 on 64-bit x86 doesn't
>> > seem worth it, since it would require adding a new C entry point for
>> > compat_sys_clone under arch/x86 somewhere.
>> >
>> > One cleanup at a time. :)
>>
>> Fine w/ me.
>
> Thanks.
>
>> >
>> >> In general, I much prefer C code to new asm where it makes sense to
>> >> make this tradeoff.
>> >
>> > Agreed completely. However, this is at least conservation-of-asm, or
>> > reduction if you consider the pt_regs argument-grabbing hack to be
>> > asm-esque code.
>> >
>> >> Other than that, this is a huge improvement. You'll have minor
>> >> conflicts against -tip, though.
>> >
>> > Right, I've seen your current changes there. Should be a trivial merge
>> > though.
>> >
>> > Would you mind providing an ack for the series, or at least for the
>> > first two patches?
>>
>> I can give you an ok-in-principle on the first two. I'd need to stare
>> at the awful code for a bit to understand the @!*&! clone variants to
>> really ack them convincingly.
>
> I'd definitely appreciate the staring. :)
>
>> OTOH, it would be rather surprising if you messed it up in a way that
>> still boots on all three variants (native 32-bit, native 64-bit, and
>> compat).
>>
>> So, for the first two patches:
>>
>> Acked-by: Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> # assuming all bitnesses boot
>
> I did test all three, not just with booting but with a thread-local
> storage test.
And it's fairly clear that no one ever tested clone-based TLS in 32
bits from a 64-bit ELF binary, because it was broken until very
recently :-/
This stuff is too magical and too poorly documented for my tastes.
--Andy
^ permalink raw reply
* [PATCH 0/2] Changes to timers Maefile to use shared logic for run_tests and install
From: Shuah Khan @ 2015-03-13 22:57 UTC (permalink / raw)
To: john.stultz-QSEj5FYQhm4dnm+yROfE0A, tglx-hfZtesqFncYOwBW4kG4KsQ,
mpe-Gsx/Oe8HsFggBc27wqDAHg
Cc: Shuah Khan, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
This patch set:
adds ability to run and emit script for tests args to lib.mk.
lib.mk provides a way to override the default RUN_TESTS and
EMIT_TESTS, however in the cases where a test has several
executables that don't require any special args and/or handling
and some that do, overrding will be cumbersome. Add variables
RUN_TESTS_WITH_ARGS, and EMIT_TESTS_WITH_ARGS to enable a test to
run and exmit script for such special executables.
Timers test Makefile is changed to use the shared logic for running
and installing tests.
Shuah Khan (2):
selftests: add ability to run and emit script for tests args to lib.mk
selftests/timers: change to use shared logic to run and install tests
tools/testing/selftests/lib.mk | 6 ++++++
tools/testing/selftests/timers/Makefile | 20 +++++++++++---------
2 files changed, 17 insertions(+), 9 deletions(-)
--
2.1.0
^ permalink raw reply
* [PATCH 1/2] selftests: add ability to run and emit script for tests args to lib.mk
From: Shuah Khan @ 2015-03-13 22:57 UTC (permalink / raw)
To: john.stultz-QSEj5FYQhm4dnm+yROfE0A, tglx-hfZtesqFncYOwBW4kG4KsQ,
mpe-Gsx/Oe8HsFggBc27wqDAHg
Cc: Shuah Khan, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1426286799.git.shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
Add ability to run, emit, install tests take arguments to the
shared logic in lib.mk. lib.mk provides a way to override the
default RUN_TESTS and EMIT_TESTS, however in the cases where a
test has several executables that don't require any special args
and/or handling and some that do, overrding will be cumbersome.
Add variables RUN_TESTS_WITH_ARGS, and EMIT_TESTS_WITH_ARGS to
enable a test to run and exmit script for such special executables.
Signed-off-by: Shuah Khan <shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
---
tools/testing/selftests/lib.mk | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 7bd3dab..78cdf91 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -6,6 +6,9 @@ endef
run_tests: all
$(RUN_TESTS)
+ifdef RUN_TESTS_WITH_ARGS
+ $(RUN_TESTS_WITH_ARGS)
+endif
define INSTALL_RULE
mkdir -p $(INSTALL_PATH)
@@ -27,5 +30,8 @@ endef
emit_tests:
$(EMIT_TESTS)
+ifdef EMIT_TESTS_WITH_ARGS
+ $(EMIT_TESTS_WITH_ARGS)
+endif
.PHONY: run_tests all clean install emit_tests
--
2.1.0
^ permalink raw reply related
* [PATCH 2/2] selftests/timers: change to use shared logic to run and install tests
From: Shuah Khan @ 2015-03-13 22:57 UTC (permalink / raw)
To: john.stultz-QSEj5FYQhm4dnm+yROfE0A, tglx-hfZtesqFncYOwBW4kG4KsQ,
mpe-Gsx/Oe8HsFggBc27wqDAHg
Cc: Shuah Khan, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1426286799.git.shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
Change the timers Makefile to make use of shared run and install
logic in lib.mk. Destructive tests are installed. Regular tests
are emited to run_kselftest script to match the run_tests behavior.
Signed-off-by: Shuah Khan <shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
---
tools/testing/selftests/timers/Makefile | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/timers/Makefile b/tools/testing/selftests/timers/Makefile
index 9da3498..61e7284 100644
--- a/tools/testing/selftests/timers/Makefile
+++ b/tools/testing/selftests/timers/Makefile
@@ -7,19 +7,21 @@ bins = posix_timers nanosleep inconsistency-check nsleep-lat raw_skew \
alarmtimer-suspend change_skew skew_consistency clocksource-switch \
leap-a-day leapcrash set-tai set-2038
+TEST_PROGS = posix_timers nanosleep nsleep-lat set-timer-lat mqueue-lat \
+ inconsistency-check raw_skew
+TEST_FILES = threadtest alarmtimer-suspend valid-adjtimex change_skew \
+ skew_consistency clocksource-switch leap-a-day leapcrash \
+ set-tai set-2038
+
+RUN_TESTS_WITH_ARGS := ./threadtest -t 30 -n 8 || echo "selftests: threadtest [FAIL]"
+
+EMIT_TESTS_WITH_ARGS := echo "$(RUN_TESTS_WITH_ARGS)"
+
all: ${bins}
# these are all "safe" tests that don't modify
# system time or require escalated privledges
-run_tests: all
- ./posix_timers
- ./nanosleep
- ./nsleep-lat
- ./set-timer-lat
- ./mqueue-lat
- ./inconsistency-check
- ./raw_skew
- ./threadtest -t 30 -n 8
+include ../lib.mk
# these tests require escalated privledges
# and may modify the system time or trigger
--
2.1.0
^ permalink raw reply related
* Re: [PATCH 2/6] x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit
From: josh-iaAMLnmF4UmaiuxdJuQwMA @ 2015-03-13 23:01 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Al Viro, Andrew Morton, Ingo Molnar, Kees Cook, Oleg Nesterov,
Paul E. McKenney, H. Peter Anvin, Rik van Riel, Thomas Gleixner,
Thiago Macieira, Michael Kerrisk,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API,
Linux FS Devel, X86 ML
In-Reply-To: <CALCETrW3kJSVz4ffVC6YdB+ELukhOHNgPFKZSziMq5nn_Nq3Zg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Mar 13, 2015 at 03:45:16PM -0700, Andy Lutomirski wrote:
> On Fri, Mar 13, 2015 at 3:43 PM, <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> > On Fri, Mar 13, 2015 at 03:38:31PM -0700, Andy Lutomirski wrote:
> >> On Fri, Mar 13, 2015 at 3:31 PM, <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> >> > On Fri, Mar 13, 2015 at 03:01:16PM -0700, Andy Lutomirski wrote:
> >> >> On Thu, Mar 12, 2015 at 6:40 PM, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> >> >> > For 32-bit userspace on a 64-bit kernel, this requires modifying
> >> >> > stub32_clone to actually swap the appropriate arguments to match
> >> >> > CONFIG_CLONE_BACKWARDS, rather than just leaving the C argument for tls
> >> >> > broken.
> >> >> >
> >> >> > Signed-off-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>
> >> >> > Signed-off-by: Thiago Macieira <thiago.macieira-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >> >> > ---
> >> >> > arch/x86/Kconfig | 1 +
> >> >> > arch/x86/ia32/ia32entry.S | 2 +-
> >> >> > arch/x86/kernel/process_32.c | 6 +++---
> >> >> > arch/x86/kernel/process_64.c | 8 ++++----
> >> >> > 4 files changed, 9 insertions(+), 8 deletions(-)
> >> >> >
> >> >> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >> >> > index b7d31ca..4960b0d 100644
> >> >> > --- a/arch/x86/Kconfig
> >> >> > +++ b/arch/x86/Kconfig
> >> >> > @@ -124,6 +124,7 @@ config X86
> >> >> > select MODULES_USE_ELF_REL if X86_32
> >> >> > select MODULES_USE_ELF_RELA if X86_64
> >> >> > select CLONE_BACKWARDS if X86_32
> >> >> > + select HAVE_COPY_THREAD_TLS
> >> >> > select ARCH_USE_BUILTIN_BSWAP
> >> >> > select ARCH_USE_QUEUE_RWLOCK
> >> >> > select OLD_SIGSUSPEND3 if X86_32 || IA32_EMULATION
> >> >> > diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
> >> >> > index 156ebca..0286735 100644
> >> >> > --- a/arch/x86/ia32/ia32entry.S
> >> >> > +++ b/arch/x86/ia32/ia32entry.S
> >> >> > @@ -487,7 +487,7 @@ GLOBAL(\label)
> >> >> > ALIGN
> >> >> > GLOBAL(stub32_clone)
> >> >> > leaq sys_clone(%rip),%rax
> >> >> > - mov %r8, %rcx
> >> >> > + xchg %r8, %rcx
> >> >> > jmp ia32_ptregs_common
> >> >>
> >> >> Do I understand correct that whatever function this is a stub for just
> >> >> takes its arguments in the wrong order? If so, can we just fix it
> >> >> instead of using xchg here?
> >> >
> >> > 32-bit x86 and 64-bit x86 take the arguments to clone in a different
> >> > order, and stub32_clone fixes up the argument order then calls the
> >> > 64-bit sys_clone.
> >> >
> >> > I'd love to see *all* the 32-on-64 compat stubs for clone rewritten in C
> >> > under CONFIG_COMPAT. However, doing so would require encoding the
> >> > knowledge for each 64-bit architecture for how its corresponding 32-bit
> >> > architecture accepts arguments to clone, which is information that the
> >> > current CONFIG_CLONE_BACKWARDS{1,2,3} don't include; it would then
> >> > require cleaning up all the architecture-specific assembly stubs for
> >> > 32-bit clone entry points.
> >> >
> >> > In the meantime, doing that *just* for 32-bit x86 on 64-bit x86 doesn't
> >> > seem worth it, since it would require adding a new C entry point for
> >> > compat_sys_clone under arch/x86 somewhere.
> >> >
> >> > One cleanup at a time. :)
> >>
> >> Fine w/ me.
> >
> > Thanks.
> >
> >> >
> >> >> In general, I much prefer C code to new asm where it makes sense to
> >> >> make this tradeoff.
> >> >
> >> > Agreed completely. However, this is at least conservation-of-asm, or
> >> > reduction if you consider the pt_regs argument-grabbing hack to be
> >> > asm-esque code.
> >> >
> >> >> Other than that, this is a huge improvement. You'll have minor
> >> >> conflicts against -tip, though.
> >> >
> >> > Right, I've seen your current changes there. Should be a trivial merge
> >> > though.
> >> >
> >> > Would you mind providing an ack for the series, or at least for the
> >> > first two patches?
> >>
> >> I can give you an ok-in-principle on the first two. I'd need to stare
> >> at the awful code for a bit to understand the @!*&! clone variants to
> >> really ack them convincingly.
> >
> > I'd definitely appreciate the staring. :)
> >
> >> OTOH, it would be rather surprising if you messed it up in a way that
> >> still boots on all three variants (native 32-bit, native 64-bit, and
> >> compat).
> >>
> >> So, for the first two patches:
> >>
> >> Acked-by: Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> # assuming all bitnesses boot
> >
> > I did test all three, not just with booting but with a thread-local
> > storage test.
>
> And it's fairly clear that no one ever tested clone-based TLS in 32
> bits from a 64-bit ELF binary, because it was broken until very
> recently :-/
I'm not sure *anyone* other than exploit-seekers test 32-bit system
calls from a 64-bit binary. :)
> This stuff is too magical and too poorly documented for my tastes.
Agreed. That was my reaction when I figured out what was happening with
CLONE_SETTLS and pt_regs, and my goal with the first two patches in this
series was precisely to make it *less* magical. :)
- Josh Triplett
^ permalink raw reply
* Re: [PATCH v4 4/9] selftests: Add install target
From: Shuah Khan @ 2015-03-13 23:08 UTC (permalink / raw)
To: Michael Ellerman, davej-rdkfGonbjUTCLXcRTR1eJlpr/1R2p/CL
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <5503577C.9050002-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
On 03/13/2015 03:32 PM, Shuah Khan wrote:
> On 03/13/2015 11:20 AM, Shuah Khan wrote:
>> On 03/10/2015 10:06 PM, Michael Ellerman wrote:
>>> This adds make install support to selftests. The basic usage is:
>>>
>>> $ cd tools/testing/selftests
>>> $ make install
>>>
>>> That installs into tools/testing/selftests/install, which can then be
>>> copied where ever necessary.
>>>
>>> The install destination is also configurable using eg:
>>>
>>> $ INSTALL_PATH=/mnt/selftests make install
>>>
>>> The implementation uses two targets in the child makefiles. The first
>>> "install" is expected to install all files into $(INSTALL_PATH).
>>>
>>> The second, "emit_tests", is expected to emit the test instructions (ie.
>>> bash script) on stdout. Separating this from install means the child
>>> makefiles need no knowledge of the location of the test script.
>>>
>>> Signed-off-by: Michael Ellerman <mpe-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org>
>>
>> Thanks for doing the work. This patch will be applied to next and will
>> be queued for 4.1
>>
>
> ok. linux-kselftest next has both shared logic and install patches
> now. As I mentioned in my reply to shared logic patch, I had to drop
> the changes to timers Makefile to add shared logic, hence timers progs
> don't get installed at the moment.
>
> Could you please review to make sure, it looks right.
>
> As I said in the other email, if lib.mk could provide a way to run
> additional programs that require arguments in addition to RUN_TESTS.
> In the case of timers, there is one test that requires arguments. In
> some cases, e.g: memory hotplug, override works well since it is just
> one executable. In this case, there is a mix of executables that don't
> need any special handling and some that need it.
>
> You are welcome to add shared logic and install to timers if you would
> like.
>
I managed to get this taken care of before my weekend. You will see two
patches one with changes to lib.mk to add additional variables to avoid
overrides for cases like timers, and the other that updates timers to
use shared logic.
I did quick test running both make run_tests and make install to verify
timers programs are installed and run_tests works correctly.
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org | (970) 217-8978
^ permalink raw reply
* Re: [PATCH 0/6] CLONE_FD: Task exit notification via file descriptor
From: Thiago Macieira @ 2015-03-14 1:11 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Josh Triplett, David Drysdale, Al Viro, Andrew Morton,
Ingo Molnar, Kees Cook, Oleg Nesterov, Paul E. McKenney,
H. Peter Anvin, Rik van Riel, Thomas Gleixner, Michael Kerrisk,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API,
Linux FS Devel, X86 ML
In-Reply-To: <CALCETrWwkdWkNCsrcSAn+7f9SJCuYA-TV9=AygWMhXCC9Njp9Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Friday 13 March 2015 14:51:47 Andy Lutomirski wrote:
> In any event, we should find out what FreeBSD does in response to
> read(2) on the fd.
I've just successfully installed FreeBSD and compiled qtbase (main package of
Qt 5) on it.
I'll test pdfork during the weekend and report its behaviour.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
^ permalink raw reply
* [PATCH] selftests: Fix build failures when invoked from kselftest target
From: Shuah Khan @ 2015-03-14 1:45 UTC (permalink / raw)
To: linux-api, linux-kernel; +Cc: Shuah Khan
Several tests that rely on implicit build rules fail to build,
when invoked from the main Makefile kselftest target. These
failures are due to --no-builtin-rules and --no-builtin-variables
options set in the inherited MAKEFLAGS.
--no-builtin-rules eliminates the use of built-in implicit rules
and --no-builtin-variables is for not defining built-in variables.
These two options override the use of implicit rules resulting in
build failures. In addition, inherited LDFLAGS result in build
failures and there is no need to define LDFLAGS. Clear LDFLAGS
and MAKEFLAG when make is invoked from the main Makefile kselftest
target. Fixing this at selftests Makefile avoids changing the main
Makefile and keeps this change self contained at selftests level.
Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
tools/testing/selftests/Makefile | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 4e51122..8e09db7 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -22,6 +22,15 @@ TARGETS += vm
TARGETS_HOTPLUG = cpu-hotplug
TARGETS_HOTPLUG += memory-hotplug
+# Clear LDFLAGS and MAKEFLAGS if called from main
+# Makefile to avoid test build failures when test
+# Makefile doesn't have explicit build rules.
+ifeq (1,$(MAKELEVEL))
+undefine LDFLAGS
+override define MAKEFLAGS =
+endef
+endif
+
all:
for TARGET in $(TARGETS); do \
make -C $$TARGET; \
--
2.1.0
^ permalink raw reply related
* [PATCH] selftests: Fix kcmp build to not require headers install
From: Shuah Khan @ 2015-03-14 1:45 UTC (permalink / raw)
To: gorcunov, tranmanphong, mpe, akpm; +Cc: Shuah Khan, linux-api, linux-kernel
In-Reply-To: <1426297504-14432-1-git-send-email-shuahkh@osg.samsung.com>
Change CFLAGS to look in uapi to allow kcmp to be built without
requiring headers install. This will make it easier to run tests
without going through the headers install step.
Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
tools/testing/selftests/kcmp/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kcmp/Makefile b/tools/testing/selftests/kcmp/Makefile
index ff0eefd..d405ad4 100644
--- a/tools/testing/selftests/kcmp/Makefile
+++ b/tools/testing/selftests/kcmp/Makefile
@@ -1,5 +1,5 @@
CC := $(CROSS_COMPILE)$(CC)
-CFLAGS += -I../../../../usr/include/
+CFLAGS += -I../../../../include/uapi -I../../../../usr/include/
all: kcmp_test
--
2.1.0
^ permalink raw reply related
* Re: [PATCH v2 net-next 1/2] bpf: allow extended BPF programs access skb fields
From: Daniel Borkmann @ 2015-03-14 1:46 UTC (permalink / raw)
To: Alexei Starovoitov, David S. Miller
Cc: Thomas Graf, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1426273064-4837-2-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
On 03/13/2015 07:57 PM, Alexei Starovoitov wrote:
> introduce user accessible mirror of in-kernel 'struct sk_buff':
> struct __sk_buff {
> __u32 len;
> __u32 pkt_type;
> __u32 mark;
> __u32 queue_mapping;
> };
>
> bpf programs can do:
>
> int bpf_prog(struct __sk_buff *skb)
> {
> __u32 var = skb->pkt_type;
>
> which will be compiled to bpf assembler as:
>
> dst_reg = *(u32 *)(src_reg + 4) // 4 == offsetof(struct __sk_buff, pkt_type)
>
> bpf verifier will check validity of access and will convert it to:
>
> dst_reg = *(u8 *)(src_reg + offsetof(struct sk_buff, __pkt_type_offset))
> dst_reg &= 7
>
> since skb->pkt_type is a bitfield.
>
> Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
...
> +static u32 convert_skb_access(int skb_field, int dst_reg, int src_reg,
> + struct bpf_insn *insn_buf)
> +{
> + struct bpf_insn *insn = insn_buf;
> +
> + switch (skb_field) {
> + case SKF_AD_MARK:
> + BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, mark) != 4);
> +
> + *insn++ = BPF_LDX_MEM(BPF_W, dst_reg, src_reg,
> + offsetof(struct sk_buff, mark));
> + break;
> +
> + case SKF_AD_PKTTYPE:
> + *insn++ = BPF_LDX_MEM(BPF_B, dst_reg, src_reg, PKT_TYPE_OFFSET());
> + *insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, PKT_TYPE_MAX);
> +#ifdef __BIG_ENDIAN_BITFIELD
> + *insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, 5);
> +#endif
> + break;
> +
> + case SKF_AD_QUEUE:
> + BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, queue_mapping) != 2);
> +
> + *insn++ = BPF_LDX_MEM(BPF_H, dst_reg, src_reg,
> + offsetof(struct sk_buff, queue_mapping));
> + break;
> + }
> +
> + return insn - insn_buf;
> +}
> +
> static bool convert_bpf_extensions(struct sock_filter *fp,
> struct bpf_insn **insnp)
> {
> struct bpf_insn *insn = *insnp;
> + u32 cnt;
>
> switch (fp->k) {
> case SKF_AD_OFF + SKF_AD_PROTOCOL:
> @@ -167,13 +200,8 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
> break;
>
> case SKF_AD_OFF + SKF_AD_PKTTYPE:
> - *insn++ = BPF_LDX_MEM(BPF_B, BPF_REG_A, BPF_REG_CTX,
> - PKT_TYPE_OFFSET());
> - *insn = BPF_ALU32_IMM(BPF_AND, BPF_REG_A, PKT_TYPE_MAX);
> -#ifdef __BIG_ENDIAN_BITFIELD
> - insn++;
> - *insn = BPF_ALU32_IMM(BPF_RSH, BPF_REG_A, 5);
> -#endif
> + cnt = convert_skb_access(SKF_AD_PKTTYPE, BPF_REG_A, BPF_REG_CTX, insn);
> + insn += cnt - 1;
> break;
>
> case SKF_AD_OFF + SKF_AD_IFINDEX:
> @@ -197,10 +225,8 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
> break;
>
> case SKF_AD_OFF + SKF_AD_MARK:
> - BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, mark) != 4);
> -
> - *insn = BPF_LDX_MEM(BPF_W, BPF_REG_A, BPF_REG_CTX,
> - offsetof(struct sk_buff, mark));
> + cnt = convert_skb_access(SKF_AD_MARK, BPF_REG_A, BPF_REG_CTX, insn);
> + insn += cnt - 1;
> break;
>
> case SKF_AD_OFF + SKF_AD_RXHASH:
> @@ -211,10 +237,8 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
> break;
>
> case SKF_AD_OFF + SKF_AD_QUEUE:
> - BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, queue_mapping) != 2);
> -
> - *insn = BPF_LDX_MEM(BPF_H, BPF_REG_A, BPF_REG_CTX,
> - offsetof(struct sk_buff, queue_mapping));
> + cnt = convert_skb_access(SKF_AD_QUEUE, BPF_REG_A, BPF_REG_CTX, insn);
> + insn += cnt - 1;
> break;
>
> case SKF_AD_OFF + SKF_AD_VLAN_TAG:
> @@ -1147,13 +1171,55 @@ sk_filter_func_proto(enum bpf_func_id func_id)
...
> +static u32 sk_filter_convert_ctx_access(int dst_reg, int src_reg, int ctx_off,
> + struct bpf_insn *insn_buf)
> +{
> + struct bpf_insn *insn = insn_buf;
> +
> + switch (ctx_off) {
> + case offsetof(struct __sk_buff, len):
> + BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, len) != 4);
> +
> + *insn++ = BPF_LDX_MEM(BPF_W, dst_reg, src_reg,
> + offsetof(struct sk_buff, len));
> + break;
> +
> + case offsetof(struct __sk_buff, mark):
> + return convert_skb_access(SKF_AD_MARK, dst_reg, src_reg, insn);
> +
> + case offsetof(struct __sk_buff, pkt_type):
> + return convert_skb_access(SKF_AD_PKTTYPE, dst_reg, src_reg, insn);
> +
> + case offsetof(struct __sk_buff, queue_mapping):
> + return convert_skb_access(SKF_AD_QUEUE, dst_reg, src_reg, insn);
> + }
> +
> + return insn - insn_buf;
> }
Hmm, I actually liked the previous version much better. :(
Now, some members use convert_skb_access() and some skb members are
converted directly in-place in both, convert_bpf_extensions() _and_
in sk_filter_convert_ctx_access().
Previously, it was much more consistent, which I like better. And only
because of the simple BUILD_BUG_ON()? :/
^ permalink raw reply
* Re: [PATCH v2 net-next 1/2] bpf: allow extended BPF programs access skb fields
From: Daniel Borkmann @ 2015-03-14 2:06 UTC (permalink / raw)
To: Alexei Starovoitov, David S. Miller
Cc: Thomas Graf, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <550392F7.9040308-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
On 03/14/2015 02:46 AM, Daniel Borkmann wrote:
...
> Previously, it was much more consistent, which I like better. And only
> because of the simple BUILD_BUG_ON()? :/
Alternative is to move all of them into a central place, something like
in twsk_build_assert() or __mld2_query_bugs[].
^ permalink raw reply
* Re: [PATCH v2 net-next 1/2] bpf: allow extended BPF programs access skb fields
From: Alexei Starovoitov @ 2015-03-14 2:07 UTC (permalink / raw)
To: Daniel Borkmann, David S. Miller
Cc: Thomas Graf, linux-api, netdev, linux-kernel
In-Reply-To: <550392F7.9040308@iogearbox.net>
On 3/13/15 6:46 PM, Daniel Borkmann wrote:
> On 03/13/2015 07:57 PM, Alexei Starovoitov wrote:
>> introduce user accessible mirror of in-kernel 'struct sk_buff':
>> struct __sk_buff {
>> __u32 len;
>> __u32 pkt_type;
>> __u32 mark;
>> __u32 queue_mapping;
>> };
>>
>> bpf programs can do:
>>
>> int bpf_prog(struct __sk_buff *skb)
>> {
>> __u32 var = skb->pkt_type;
>>
>> which will be compiled to bpf assembler as:
>>
>> dst_reg = *(u32 *)(src_reg + 4) // 4 == offsetof(struct __sk_buff,
>> pkt_type)
>>
>> bpf verifier will check validity of access and will convert it to:
>>
>> dst_reg = *(u8 *)(src_reg + offsetof(struct sk_buff, __pkt_type_offset))
>> dst_reg &= 7
>>
>> since skb->pkt_type is a bitfield.
>>
>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ...
>> +static u32 convert_skb_access(int skb_field, int dst_reg, int src_reg,
>> + struct bpf_insn *insn_buf)
>> +{
>> + struct bpf_insn *insn = insn_buf;
>> +
>> + switch (skb_field) {
>> + case SKF_AD_MARK:
>> + BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, mark) != 4);
>> +
>> + *insn++ = BPF_LDX_MEM(BPF_W, dst_reg, src_reg,
>> + offsetof(struct sk_buff, mark));
>> + break;
>> +
>> + case SKF_AD_PKTTYPE:
>> + *insn++ = BPF_LDX_MEM(BPF_B, dst_reg, src_reg,
>> PKT_TYPE_OFFSET());
>> + *insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, PKT_TYPE_MAX);
>> +#ifdef __BIG_ENDIAN_BITFIELD
>> + *insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, 5);
>> +#endif
>> + break;
>> +
>> + case SKF_AD_QUEUE:
>> + BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, queue_mapping) != 2);
>> +
>> + *insn++ = BPF_LDX_MEM(BPF_H, dst_reg, src_reg,
>> + offsetof(struct sk_buff, queue_mapping));
>> + break;
>> + }
>> +
>> + return insn - insn_buf;
>> +}
>> +
>> static bool convert_bpf_extensions(struct sock_filter *fp,
>> struct bpf_insn **insnp)
>> {
>> struct bpf_insn *insn = *insnp;
>> + u32 cnt;
>>
>> switch (fp->k) {
>> case SKF_AD_OFF + SKF_AD_PROTOCOL:
>> @@ -167,13 +200,8 @@ static bool convert_bpf_extensions(struct
>> sock_filter *fp,
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_PKTTYPE:
>> - *insn++ = BPF_LDX_MEM(BPF_B, BPF_REG_A, BPF_REG_CTX,
>> - PKT_TYPE_OFFSET());
>> - *insn = BPF_ALU32_IMM(BPF_AND, BPF_REG_A, PKT_TYPE_MAX);
>> -#ifdef __BIG_ENDIAN_BITFIELD
>> - insn++;
>> - *insn = BPF_ALU32_IMM(BPF_RSH, BPF_REG_A, 5);
>> -#endif
>> + cnt = convert_skb_access(SKF_AD_PKTTYPE, BPF_REG_A,
>> BPF_REG_CTX, insn);
>> + insn += cnt - 1;
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_IFINDEX:
>> @@ -197,10 +225,8 @@ static bool convert_bpf_extensions(struct
>> sock_filter *fp,
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_MARK:
>> - BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, mark) != 4);
>> -
>> - *insn = BPF_LDX_MEM(BPF_W, BPF_REG_A, BPF_REG_CTX,
>> - offsetof(struct sk_buff, mark));
>> + cnt = convert_skb_access(SKF_AD_MARK, BPF_REG_A, BPF_REG_CTX,
>> insn);
>> + insn += cnt - 1;
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_RXHASH:
>> @@ -211,10 +237,8 @@ static bool convert_bpf_extensions(struct
>> sock_filter *fp,
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_QUEUE:
>> - BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, queue_mapping) != 2);
>> -
>> - *insn = BPF_LDX_MEM(BPF_H, BPF_REG_A, BPF_REG_CTX,
>> - offsetof(struct sk_buff, queue_mapping));
>> + cnt = convert_skb_access(SKF_AD_QUEUE, BPF_REG_A,
>> BPF_REG_CTX, insn);
>> + insn += cnt - 1;
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_VLAN_TAG:
>> @@ -1147,13 +1171,55 @@ sk_filter_func_proto(enum bpf_func_id func_id)
> ...
>> +static u32 sk_filter_convert_ctx_access(int dst_reg, int src_reg, int
>> ctx_off,
>> + struct bpf_insn *insn_buf)
>> +{
>> + struct bpf_insn *insn = insn_buf;
>> +
>> + switch (ctx_off) {
>> + case offsetof(struct __sk_buff, len):
>> + BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, len) != 4);
>> +
>> + *insn++ = BPF_LDX_MEM(BPF_W, dst_reg, src_reg,
>> + offsetof(struct sk_buff, len));
>> + break;
>> +
>> + case offsetof(struct __sk_buff, mark):
>> + return convert_skb_access(SKF_AD_MARK, dst_reg, src_reg, insn);
>> +
>> + case offsetof(struct __sk_buff, pkt_type):
>> + return convert_skb_access(SKF_AD_PKTTYPE, dst_reg, src_reg,
>> insn);
>> +
>> + case offsetof(struct __sk_buff, queue_mapping):
>> + return convert_skb_access(SKF_AD_QUEUE, dst_reg, src_reg, insn);
>> + }
>> +
>> + return insn - insn_buf;
>> }
>
> Hmm, I actually liked the previous version much better. :(
>
> Now, some members use convert_skb_access() and some skb members are
> converted directly in-place in both, convert_bpf_extensions() _and_
> in sk_filter_convert_ctx_access().
>
> Previously, it was much more consistent, which I like better. And only
> because of the simple BUILD_BUG_ON()? :/
not because of single build_bug_on, but because of having
a single place to adjust offsets and sizes when location of
sk_buff fields changes. that's the main advantage and it's a big one.
imo it's much cleaner than previous approach.
^ permalink raw reply
* Re: [PATCH v2 net-next 1/2] bpf: allow extended BPF programs access skb fields
From: Alexei Starovoitov @ 2015-03-14 2:08 UTC (permalink / raw)
To: Daniel Borkmann, David S. Miller
Cc: Thomas Graf, linux-api, netdev, linux-kernel
In-Reply-To: <550397B0.5070409@iogearbox.net>
On 3/13/15 7:06 PM, Daniel Borkmann wrote:
> On 03/14/2015 02:46 AM, Daniel Borkmann wrote:
> ...
>> Previously, it was much more consistent, which I like better. And only
>> because of the simple BUILD_BUG_ON()? :/
>
> Alternative is to move all of them into a central place, something like
> in twsk_build_assert() or __mld2_query_bugs[].
nope. that defeats the purpose of bug_on.
It needs to be next the code it's actually trying to protect.
^ permalink raw reply
* Re: [PATCH v2 net-next 1/2] bpf: allow extended BPF programs access skb fields
From: Daniel Borkmann @ 2015-03-14 2:16 UTC (permalink / raw)
To: Alexei Starovoitov, David S. Miller
Cc: Thomas Graf, linux-api, netdev, linux-kernel
In-Reply-To: <5503981C.6000801@plumgrid.com>
On 03/14/2015 03:08 AM, Alexei Starovoitov wrote:
> On 3/13/15 7:06 PM, Daniel Borkmann wrote:
>> On 03/14/2015 02:46 AM, Daniel Borkmann wrote:
>> ...
>>> Previously, it was much more consistent, which I like better. And only
>>> because of the simple BUILD_BUG_ON()? :/
>>
>> Alternative is to move all of them into a central place, something like
>> in twsk_build_assert() or __mld2_query_bugs[].
>
> nope. that defeats the purpose of bug_on.
Well, it doesn't. ;) It throws a build error thus the user is forced to
investigate that further.
^ permalink raw reply
* Re: [PATCH v2 net-next 1/2] bpf: allow extended BPF programs access skb fields
From: Alexei Starovoitov @ 2015-03-14 2:27 UTC (permalink / raw)
To: Daniel Borkmann, David S. Miller
Cc: Thomas Graf, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <55039A0D.20000-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
On 3/13/15 7:16 PM, Daniel Borkmann wrote:
> On 03/14/2015 03:08 AM, Alexei Starovoitov wrote:
>> On 3/13/15 7:06 PM, Daniel Borkmann wrote:
>>> On 03/14/2015 02:46 AM, Daniel Borkmann wrote:
>>> ...
>>>> Previously, it was much more consistent, which I like better. And only
>>>> because of the simple BUILD_BUG_ON()? :/
>>>
>>> Alternative is to move all of them into a central place, something like
>>> in twsk_build_assert() or __mld2_query_bugs[].
>>
>> nope. that defeats the purpose of bug_on.
>
> Well, it doesn't. ;) It throws a build error thus the user is forced to
> investigate that further.
according to this distorted logic all build_bug_on can be in one file
across the whole tree, since 'user is forced to investigate' ?!
^ permalink raw reply
* Re: [PATCH 2/2] selftests/timers: change to use shared logic to run and install tests
From: John Stultz @ 2015-03-14 3:14 UTC (permalink / raw)
To: Shuah Khan; +Cc: Thomas Gleixner, mpe-Gsx/Oe8HsFggBc27wqDAHg, Linux API, lkml
In-Reply-To: <e34c7abc6dff5922a45376b7a3085545db66eb6d.1426286799.git.shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
On Fri, Mar 13, 2015 at 3:57 PM, Shuah Khan <shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org> wrote:
> Change the timers Makefile to make use of shared run and install
> logic in lib.mk. Destructive tests are installed. Regular tests
> are emited to run_kselftest script to match the run_tests behavior.
>
> Signed-off-by: Shuah Khan <shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
> ---
> tools/testing/selftests/timers/Makefile | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/timers/Makefile b/tools/testing/selftests/timers/Makefile
> index 9da3498..61e7284 100644
> --- a/tools/testing/selftests/timers/Makefile
> +++ b/tools/testing/selftests/timers/Makefile
> @@ -7,19 +7,21 @@ bins = posix_timers nanosleep inconsistency-check nsleep-lat raw_skew \
> alarmtimer-suspend change_skew skew_consistency clocksource-switch \
> leap-a-day leapcrash set-tai set-2038
>
> +TEST_PROGS = posix_timers nanosleep nsleep-lat set-timer-lat mqueue-lat \
> + inconsistency-check raw_skew
> +TEST_FILES = threadtest alarmtimer-suspend valid-adjtimex change_skew \
> + skew_consistency clocksource-switch leap-a-day leapcrash \
> + set-tai set-2038
> +
> +RUN_TESTS_WITH_ARGS := ./threadtest -t 30 -n 8 || echo "selftests: threadtest [FAIL]"
> +
> +EMIT_TESTS_WITH_ARGS := echo "$(RUN_TESTS_WITH_ARGS)"
> +
So my make-foo isn't very strong, but no objections from me.
My only thoughts:
1) Would it be better if threadtest can be made to have better
defaults for kselftest so you don't need that extra logic?
2) While I get that TEST_FILES is likely going to be used to copy the
destructive tests over, It feels a little like its being bundled in
with something like data files that tests might need, which seems sort
of hackish. Would TEST_PROGS_EXTENDED or something be more clear and
make more sense?
thanks
-john
^ permalink raw reply
* Re: [PATCH v2 net-next 1/2] bpf: allow extended BPF programs access skb fields
From: Alexei Starovoitov @ 2015-03-14 4:59 UTC (permalink / raw)
To: Daniel Borkmann, David S. Miller
Cc: Thomas Graf, linux-api, netdev, linux-kernel
In-Reply-To: <55039C9D.6010602@plumgrid.com>
On 3/13/15 7:27 PM, Alexei Starovoitov wrote:
> On 3/13/15 7:16 PM, Daniel Borkmann wrote:
>> On 03/14/2015 03:08 AM, Alexei Starovoitov wrote:
>>> On 3/13/15 7:06 PM, Daniel Borkmann wrote:
>>>> On 03/14/2015 02:46 AM, Daniel Borkmann wrote:
>>>> ...
>>>>> Previously, it was much more consistent, which I like better. And only
>>>>> because of the simple BUILD_BUG_ON()? :/
>>>>
>>>> Alternative is to move all of them into a central place, something like
>>>> in twsk_build_assert() or __mld2_query_bugs[].
>>>
>>> nope. that defeats the purpose of bug_on.
>>
>> Well, it doesn't. ;) It throws a build error thus the user is forced to
>> investigate that further.
>
> according to this distorted logic all build_bug_on can be in one file
> across the whole tree, since 'user is forced to investigate' ?!
also note that this case and twsk_build_assert are different.
twsk_build_assert has no other choice then to have one function
that covers logic in the whole file, whereas in this patch:
+ BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, mark) != 4);
+ *insn++ = BPF_LDX_MEM(BPF_W, dst_reg, src_reg,
+ offsetof(struct sk_buff, mark));
the build_bug_on protect the line directly below.
Separating them just doesn't make sense at all.
^ 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