From: josh@joshtriplett.org
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>,
Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
Peter Zijlstra <peterz@infradead.org>,
Thomas Gleixner <tglx@linutronix.de>,
Linus Torvalds <torvalds@linux-foundation.org>,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
x86@kernel.org
Subject: Re: [PATCHv2 1/2] clone: Support passing tls argument via C rather than pt_regs magic
Date: Wed, 13 May 2015 16:25:07 -0700 [thread overview]
Message-ID: <20150513232507.GA22262@cloud> (raw)
In-Reply-To: <20150513155628.65dc253bea9485cb7910678b@linux-foundation.org>
On Wed, May 13, 2015 at 03:56:28PM -0700, Andrew Morton wrote:
> On Mon, 11 May 2015 12:29:19 -0700 Josh Triplett <josh@joshtriplett.org> wrote:
>
> > clone with CLONE_SETTLS accepts an argument to set the thread-local
> > storage area for the new thread. sys_clone declares an int argument
> > tls_val in the appropriate point in the argument list (based on the
> > various CLONE_BACKWARDS variants), but doesn't actually use or pass
> > along that argument. Instead, sys_clone calls do_fork, which calls
> > copy_process, which calls the arch-specific copy_thread, and copy_thread
> > pulls the corresponding syscall argument out of the pt_regs captured at
> > kernel entry (knowing what argument of clone that architecture passes
> > tls in).
> >
> > Apart from being awful and inscrutable, that also only works because
> > only one code path into copy_thread can pass the CLONE_SETTLS flag, and
> > that code path comes from sys_clone with its architecture-specific
> > argument-passing order. This prevents introducing a new version of the
> > clone system call without propagating the same architecture-specific
> > position of the tls argument.
> >
> > However, there's no reason to pull the argument out of pt_regs when
> > sys_clone could just pass it down via C function call arguments.
> >
> > Introduce a new CONFIG_HAVE_COPY_THREAD_TLS for architectures to opt
> > into, and a new copy_thread_tls that accepts the tls parameter as an
> > additional unsigned long (syscall-argument-sized) argument.
> > Change sys_clone's tls argument to an unsigned long (which does
> > not change the ABI), and pass that down to copy_thread_tls.
> >
> > Architectures that don't opt into copy_thread_tls will continue to
> > ignore the C argument to sys_clone in favor of the pt_regs captured at
> > kernel entry, and thus will be unable to introduce new versions of the
> > clone syscall.
> >
> > Patch co-authored by Josh Triplett and Thiago Macieira.
> >
> > ...
> >
> > @@ -1698,20 +1701,34 @@ long do_fork(unsigned long clone_flags,
> > return nr;
> > }
> >
> > +#ifndef CONFIG_HAVE_COPY_THREAD_TLS
> > +/* For compatibility with architectures that call do_fork directly rather than
> > + * using the syscall entry points below. */
> > +long do_fork(unsigned long clone_flags,
> > + unsigned long stack_start,
> > + unsigned long stack_size,
> > + int __user *parent_tidptr,
> > + int __user *child_tidptr)
> > +{
> > + return _do_fork(clone_flags, stack_start, stack_size,
> > + parent_tidptr, child_tidptr, 0);
> > +}
> > +#endif
>
> drivers/misc/kgdbts.c:lookup_addr() has a reference to do_fork().
> Doesn't link, with a basic `make allmodconfig'.
Odd; not sure how it built with allyesconfig at the time (which I did
test).
However, dropping the #ifndef is the wrong fix. do_fork will go away
*completely* once all architectures opt into
CONFIG_HAVE_COPY_THREAD_TLS, and architectures that opt in won't pass
through do_fork. kgdb wants to capture forks, so it wants _do_fork.
The right fix is to make _do_fork non-static (which makes me sad, but oh
well), and make kgdb reference _do_fork instead of do_fork (though the
string should remain "do_fork" for compatibility):
Here's an incremental patch for that, to be squashed into the first of
the two patches per your standard procedure for -mm; does this fix the
issue you observed?
--- 8< ---
>From fd599319630b33b829dc50b4f3c88016e715cd76 Mon Sep 17 00:00:00 2001
From: Josh Triplett <josh@joshtriplett.org>
Date: Wed, 13 May 2015 08:18:47 -0700
Subject: [PATCH] Fix "clone: Support passing tls argument via C rather than pt_regs magic" for kgdb
Should be squashed into "clone: Support passing tls argument via C
rather than pt_regs magic". kgdb wants to reference the real fork
function, which is now _do_fork.
Reported-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
drivers/misc/kgdbts.c | 2 +-
include/linux/sched.h | 1 +
kernel/fork.c | 13 ++++++-------
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index 36f5d52..9a60bd4 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c
@@ -220,7 +220,7 @@ static unsigned long lookup_addr(char *arg)
else if (!strcmp(arg, "sys_open"))
addr = (unsigned long)do_sys_open;
else if (!strcmp(arg, "do_fork"))
- addr = (unsigned long)do_fork;
+ addr = (unsigned long)_do_fork;
else if (!strcmp(arg, "hw_break_val"))
addr = (unsigned long)&hw_break_val;
addr = (unsigned long) dereference_function_descriptor((void *)addr);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2cc88c6..9686abe 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2514,6 +2514,7 @@ extern int do_execveat(int, struct filename *,
const char __user * const __user *,
const char __user * const __user *,
int);
+extern long _do_fork(unsigned long, unsigned long, unsigned long, int __user *, int __user *, unsigned long);
extern long do_fork(unsigned long, unsigned long, unsigned long, int __user *, int __user *);
struct task_struct *fork_idle(int);
extern pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long flags);
diff --git a/kernel/fork.c b/kernel/fork.c
index b3dadf4..b493aba 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1629,13 +1629,12 @@ struct task_struct *fork_idle(int cpu)
* It copies the process, and if successful kick-starts
* it and waits for it to finish using the VM if required.
*/
-static long _do_fork(
- unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
- int __user *parent_tidptr,
- int __user *child_tidptr,
- unsigned long tls)
+long _do_fork(unsigned long clone_flags,
+ unsigned long stack_start,
+ unsigned long stack_size,
+ int __user *parent_tidptr,
+ int __user *child_tidptr,
+ unsigned long tls)
{
struct task_struct *p;
int trace = 0;
--
2.1.4
prev parent reply other threads:[~2015-05-13 23:25 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-11 19:29 [PATCHv2 1/2] clone: Support passing tls argument via C rather than pt_regs magic Josh Triplett
2015-05-12 21:22 ` Andrew Morton
2015-05-12 21:38 ` Peter Zijlstra
2015-05-12 21:49 ` Andrew Morton
2015-05-13 8:34 ` [RFC PATCH] Documentation/arch: Add Documentation/arch-TODO Ingo Molnar
2015-05-13 8:56 ` [RFC PATCH v2] " Ingo Molnar
2015-05-13 9:24 ` [RFC PATCH v3] " Ingo Molnar
2015-05-13 9:46 ` Ingo Molnar
2015-05-13 9:47 ` [RFC PATCH v5] " Ingo Molnar
2015-05-13 13:18 ` Borislav Petkov
2015-05-13 13:48 ` [RFC PATCH v6] Documentation/arch: Add Documentation/arch-features.txt Ingo Molnar
2015-05-13 16:27 ` Josh Triplett
2015-05-13 16:53 ` Josh Triplett
2015-05-14 10:16 ` Ingo Molnar
2015-05-14 10:31 ` Josh Triplett
2015-05-13 22:05 ` Andrew Morton
2015-05-14 10:02 ` Ingo Molnar
2015-05-14 10:15 ` [PATCH] Documentation/arch: Add kernel feature descriptions and arch support status under Documentation/features/ Ingo Molnar
2015-05-14 10:35 ` [PATCH v2] " Ingo Molnar
2015-05-14 19:38 ` Andrew Morton
2015-05-14 19:59 ` Ingo Molnar
2015-05-14 22:33 ` Stephen Rothwell
2015-05-15 7:38 ` Ingo Molnar
2015-05-15 7:51 ` Ingo Molnar
2015-05-15 9:37 ` Ingo Molnar
2015-05-14 20:34 ` Richard Weinberger
2015-05-14 22:57 ` Michael Ellerman
2015-05-15 7:49 ` Ingo Molnar
2015-05-18 1:37 ` Michael Ellerman
2015-05-18 8:54 ` Ingo Molnar
2015-05-19 2:02 ` Michael Ellerman
2015-05-22 15:49 ` Jonathan Corbet
2015-05-23 8:07 ` Ingo Molnar
2015-05-24 18:44 ` Jonathan Corbet
2015-06-03 11:03 ` [GIT PULL] Documentation/features: Add kernel feature descriptions and arch support status files " Ingo Molnar
2015-06-08 23:08 ` Jonathan Corbet
2015-05-14 7:08 ` [RFC PATCH v6] Documentation/arch: Add Documentation/arch-features.txt Ingo Molnar
2015-05-14 3:55 ` Paul Mackerras
2015-05-14 7:02 ` Ingo Molnar
2015-05-13 14:09 ` [RFC PATCH v5] Documentation/arch: Add Documentation/arch-TODO Michele Curti
2015-05-13 14:42 ` Geert Uytterhoeven
2015-05-14 7:21 ` H. Peter Anvin
2015-05-14 7:21 ` H. Peter Anvin
2015-05-13 0:53 ` [PATCHv2 1/2] clone: Support passing tls argument via C rather than pt_regs magic josh
2015-05-13 22:56 ` Andrew Morton
2015-05-13 23:25 ` josh [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150513232507.GA22262@cloud \
--to=josh@joshtriplett.org \
--cc=akpm@linux-foundation.org \
--cc=hpa@zytor.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).