diff mbox

what's parisc execve_wrapper doing in the end?

Message ID 1349444664.3638.46.camel@dabdike.int.hansenpartnership.com (mailing list archive)
State Superseded
Headers show

Commit Message

James Bottomley Oct. 5, 2012, 1:44 p.m. UTC
On Fri, 2012-10-05 at 12:07 +0100, James Bottomley wrote:
> I tried out the code at
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/signal.git
> experimental-kernel_thread
> 
> and it gives me this panic on boot.

OK, found the fix:  the idle thread is a kernel thread, but it doesn't
come through kernel_thread().  The fix is to check for it (fortunately
it has the signal usp == 0).

I'm now getting as freeing the init memory, which then hangs, so I
suspect some type of execve failure trying to start the initrd... I'm
debugging.

James

---



--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Al Viro Oct. 5, 2012, 2:48 p.m. UTC | #1
On Fri, Oct 05, 2012 at 02:44:24PM +0100, James Bottomley wrote:
> On Fri, 2012-10-05 at 12:07 +0100, James Bottomley wrote:
> > I tried out the code at
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/viro/signal.git
> > experimental-kernel_thread
> > 
> > and it gives me this panic on boot.
> 
> OK, found the fix:  the idle thread is a kernel thread, but it doesn't
> come through kernel_thread().  The fix is to check for it (fortunately
> it has the signal usp == 0).

Um...  I see, but I really wonder if that's the right fix.  FWIW, sparc
will have the same problem...  Hell knows.  OTOH, it's a nice way to
get of implicit interplay between copy_thread() and idle_regs() - note
that SMP architectures doing default idle_regs() need to be damn careful
about what they do in their "is that kernel thread" logics; all-zeros
pt_regs might give varying results on user_mode(regs) tests, etc.
Might be better to go for
	if (p->flags & PF_KTHREAD) {
		if (!usp) {
			we are starting an idle thread
		} else {
			we are setting things up for kernel_thread()
		}
	} else {
		we are forking
	}
kind of logics, looking at regs only in the last case.  And to hell with
(separate and overridable) idle_regs() once everything goes that way...
 
> I'm now getting as freeing the init memory, which then hangs, so I
> suspect some type of execve failure trying to start the initrd... I'm
> debugging.

What are you using for toolchain, BTW?  With gcc 4.3 / binutils 2.20
(cross-builds) I'm getting panics from mainline kernel on parisc32 all
way back to 2.6.28...
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley Oct. 5, 2012, 2:55 p.m. UTC | #2
On Fri, 2012-10-05 at 15:48 +0100, Al Viro wrote:
> On Fri, Oct 05, 2012 at 02:44:24PM +0100, James Bottomley wrote:
> > On Fri, 2012-10-05 at 12:07 +0100, James Bottomley wrote:
> > > I tried out the code at
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/viro/signal.git
> > > experimental-kernel_thread
> > > 
> > > and it gives me this panic on boot.
> > 
> > OK, found the fix:  the idle thread is a kernel thread, but it doesn't
> > come through kernel_thread().  The fix is to check for it (fortunately
> > it has the signal usp == 0).
> 
> Um...  I see, but I really wonder if that's the right fix.  FWIW, sparc
> will have the same problem...  Hell knows.  OTOH, it's a nice way to
> get of implicit interplay between copy_thread() and idle_regs() - note
> that SMP architectures doing default idle_regs() need to be damn careful
> about what they do in their "is that kernel thread" logics; all-zeros
> pt_regs might give varying results on user_mode(regs) tests, etc.
> Might be better to go for
> 	if (p->flags & PF_KTHREAD) {
> 		if (!usp) {
> 			we are starting an idle thread
> 		} else {
> 			we are setting things up for kernel_thread()
> 		}
> 	} else {
> 		we are forking
> 	}
> kind of logics, looking at regs only in the last case.  And to hell with
> (separate and overridable) idle_regs() once everything goes that way...

But there's not a lot of point.  forking an idle thread actually doesn't
care about any of the register execution setup because it never really
uses it to execute.  That's why it was safe for us to use the user
thread setup ... I suppose the interior of the kernel thread case could
be conditioned on if (usp).


> > I'm now getting as freeing the init memory, which then hangs, so I
> > suspect some type of execve failure trying to start the initrd... I'm
> > debugging.
> 
> What are you using for toolchain, BTW?  With gcc 4.3 / binutils 2.20
> (cross-builds) I'm getting panics from mainline kernel on parisc32 all
> way back to 2.6.28...

My toolchain is

jejb@ion> gcc -v
Using built-in specs.
Target: hppa-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 4.4.5-8'
--with-bugurl=file:///usr/share/doc/gcc-4.4/README.Bugs
--enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr
--program-suffix=-4.4 --enable-shared --enable-multiarch
--enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib
--without-included-gettext --enable-threads=posix
--with-gxx-include-dir=/usr/include/c++/4.4 --libdir=/usr/lib
--enable-nls --enable-clocale=gnu --enable-libstdcxx-debug
--disable-libssp --enable-objc-gc --disable-libstdcxx-pch
--enable-checking=release --build=hppa-linux-gnu --host=hppa-linux-gnu
--target=hppa-linux-gnu
Thread model: posix
gcc version 4.4.5 (Debian 4.4.5-8) 


It's the one that was in debian testing before parisc got dumped

James


--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro Oct. 5, 2012, 7:21 p.m. UTC | #3
On Fri, Oct 05, 2012 at 03:55:36PM +0100, James Bottomley wrote:

> But there's not a lot of point.  forking an idle thread actually doesn't
> care about any of the register execution setup because it never really
> uses it to execute.  That's why it was safe for us to use the user
> thread setup ... I suppose the interior of the kernel thread case could
> be conditioned on if (usp).

FWIW, I'm digging through the fork_idle() interplay with copy_thread()
right now and the more I'm looking the less I like having called
copy_thread() at all on that path.  Note that copy_process() can trivially
tell it from the normal (do_fork()) caller, just by checking if pid
is non-NULL.  And it has to check that anyway when it decides to generate
the pid for child...

Hell knows; I wonder how much of copy_thread() work is needed in that case...
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John David Anglin Oct. 5, 2012, 10:54 p.m. UTC | #4
On 5-Oct-12, at 10:48 AM, Al Viro wrote:

> What are you using for toolchain, BTW?  With gcc 4.3 / binutils 2.20
> (cross-builds) I'm getting panics from mainline kernel on parisc32 all
> way back to 2.6.28...


Current debian unstable tools are available in shirka.esiee.fr:/home/ 
dave/archives
and shirka.esiee.fr:/home/dave/archives-new.  It's pretty straight  
forward to update
binutils, gcc, libc, etc.

gcc 4.4, 4.5, 4.6 and 4.7 are available.  In particular, 4.6 and 4.7  
have significant
bug fixes.

James seems to have an account on shirka.

I don't have any place to distribute the debs that are on shirka, or  
the time to remove
obsolete builds.  Any volunteers?

Dave
--
John David Anglin	dave.anglin@bell.net



--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro Oct. 5, 2012, 11:04 p.m. UTC | #5
On Fri, Oct 05, 2012 at 03:55:36PM +0100, James Bottomley wrote:
> On Fri, 2012-10-05 at 15:48 +0100, Al Viro wrote:
> > On Fri, Oct 05, 2012 at 02:44:24PM +0100, James Bottomley wrote:
> > > On Fri, 2012-10-05 at 12:07 +0100, James Bottomley wrote:
> > > > I tried out the code at
> > > > 
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/viro/signal.git
> > > > experimental-kernel_thread
> > > > 
> > > > and it gives me this panic on boot.
> > > 
> > > OK, found the fix:  the idle thread is a kernel thread, but it doesn't
> > > come through kernel_thread().  The fix is to check for it (fortunately
> > > it has the signal usp == 0).
> > 
> > Um...  I see, but I really wonder if that's the right fix.  FWIW, sparc
> > will have the same problem...  Hell knows.  OTOH, it's a nice way to
> > get of implicit interplay between copy_thread() and idle_regs() - note
> > that SMP architectures doing default idle_regs() need to be damn careful
> > about what they do in their "is that kernel thread" logics; all-zeros
> > pt_regs might give varying results on user_mode(regs) tests, etc.
> > Might be better to go for
> > 	if (p->flags & PF_KTHREAD) {
> > 		if (!usp) {
> > 			we are starting an idle thread
> > 		} else {
> > 			we are setting things up for kernel_thread()
> > 		}
> > 	} else {
> > 		we are forking
> > 	}
> > kind of logics, looking at regs only in the last case.  And to hell with
> > (separate and overridable) idle_regs() once everything goes that way...
> 
> But there's not a lot of point.  forking an idle thread actually doesn't
> care about any of the register execution setup because it never really
> uses it to execute.  That's why it was safe for us to use the user
> thread setup ... I suppose the interior of the kernel thread case could
> be conditioned on if (usp).

BTW, speaking of parisc copy_thread()...  Why the hell do we bother
with *cregs = *pregs in userland case?  It's a part of task_struct,
after all, and we have copied that wholesale in arch_dup_task_struct().

Another thing: why do we bother with
        STREG   %r30,PT_GR21(%r1)
in fork wrapper?  We bloody well know what the offset will be, after all -
right in the beginning of that sucker we'd done
        LDREG   TI_TASK-THREAD_SZ_ALGN-FRAME_SIZE(%r30), %r1
so we rely on %r30 having been (unsigned long)current_thread_info() + 
THREAD_SZ_ALGN + FRAME_SIZE.  Then we add FRAME_SIZE again.  IOW, the
offset is a known constant.  Hell, in child_return you rely on its
value...   While we are at it, I'm not sure you need to go through
wrapper_exit on the way out in parent - saving cr27 can be done via
e.g. r28 instead of r3, at which point you can simply branch to
sys_clone() with no work left for wrapper_exit.  *Child* obviously
needs to restore these registers, so let it do that in child_return,
but why bother in parent?  After all, we are talking about the callee-saved
registers, so sys_clone() is going to revert whatever changes it makes
to them...

BTW, TIF_SYSCALL_TRACE and singlestepping are turned off in child, so I don't
see any need for child_return to know where the parent had come from - it
won't have anything to do in tracesys_exit anyway.

I've folded your fixes and pushed the result; I've added (again, completely
untested) optimizations along the lines of the above on top of those, as
a separate commit.  Comments?
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro Oct. 5, 2012, 11:32 p.m. UTC | #6
On Fri, Oct 05, 2012 at 06:54:02PM -0400, John David Anglin wrote:
> On 5-Oct-12, at 10:48 AM, Al Viro wrote:
> 
> >What are you using for toolchain, BTW?  With gcc 4.3 / binutils 2.20
> >(cross-builds) I'm getting panics from mainline kernel on parisc32 all
> >way back to 2.6.28...
> 
> 
> Current debian unstable tools are available in
> shirka.esiee.fr:/home/dave/archives
> and shirka.esiee.fr:/home/dave/archives-new.  It's pretty straight
> forward to update
> binutils, gcc, libc, etc.
> 
> gcc 4.4, 4.5, 4.6 and 4.7 are available.  In particular, 4.6 and 4.7
> have significant
> bug fixes.
> 
> James seems to have an account on shirka.
> 
> I don't have any place to distribute the debs that are on shirka, or
> the time to remove
> obsolete builds.  Any volunteers?

Hmm...  debian-ports would be a natural place, but TBH I'd very much prefer
to deal with cross-builds - doing kernel builds on a 100MHz 7100LC with
96M of RAM is really not fun...
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John David Anglin Oct. 6, 2012, 12:15 a.m. UTC | #7
On 5-Oct-12, at 7:32 PM, Al Viro wrote:

> On Fri, Oct 05, 2012 at 06:54:02PM -0400, John David Anglin wrote:
>> On 5-Oct-12, at 10:48 AM, Al Viro wrote:
>>
>>> What are you using for toolchain, BTW?  With gcc 4.3 / binutils 2.20
>>> (cross-builds) I'm getting panics from mainline kernel on parisc32  
>>> all
>>> way back to 2.6.28...
>>
>>
>> Current debian unstable tools are available in
>> shirka.esiee.fr:/home/dave/archives
>> and shirka.esiee.fr:/home/dave/archives-new.  It's pretty straight
>> forward to update
>> binutils, gcc, libc, etc.
>>
>> gcc 4.4, 4.5, 4.6 and 4.7 are available.  In particular, 4.6 and 4.7
>> have significant
>> bug fixes.
>>
>> James seems to have an account on shirka.
>>
>> I don't have any place to distribute the debs that are on shirka, or
>> the time to remove
>> obsolete builds.  Any volunteers?
>
> Hmm...  debian-ports would be a natural place, but TBH I'd very much  
> prefer
> to deal with cross-builds - doing kernel builds on a 100MHz 7100LC  
> with
> 96M of RAM is really not fun...
>

Right...

I had setup a buildd system system on lafayette.esiee.fr but  
unfortunately
a processor module died.  I also never received a response from debian- 
ports
(several emails) about restarting buildd for parisc.

I've continued to build packages on the rp3440 that I have at home.   
It doesn't have
much trouble keeping up with unstable.  At the same time, I use it for  
gcc testing
and development.

I would say you should be able to build cross tools using the binutils  
2.22 branch
and gcc 4.6 or 4.7.  They should work better than gcc 4.3 with  
binutils 2.20.

The reason I say that is I have built many debian packages with the  
current tools
and I have been slowly chipping away at the toolchain bugs that I have  
found.

Dave
--
John David Anglin	dave.anglin@bell.net



--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
index a67f122..44e8534 100644
--- a/arch/parisc/kernel/process.c
+++ b/arch/parisc/kernel/process.c
@@ -254,7 +254,7 @@  copy_thread(unsigned long clone_flags, unsigned long usp,
 	extern void * const hpux_child_return;
 #endif
 
-	if (unlikely(p->flags & PF_KTHREAD)) {
+	if (unlikely((p->flags & PF_KTHREAD) && usp != 0)) {
 		memset(cregs, 0, sizeof(struct pt_regs));
 		/* kernel thread */
 		cregs->ksp = (unsigned long)stack + THREAD_SZ_ALGN;