Re: [PATCHv2 1/2] clone: Support passing tls argument via C rather than pt_regs magic

[josh]

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@xxxxxxxxxxxxxxxx> 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< ---