From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Subject: Re: what's parisc execve_wrapper doing in the end? Date: Sat, 6 Oct 2012 00:04:41 +0100 Message-ID: <20121005230441.GB2616@ZenIV.linux.org.uk> References: <20121004045150.GH23473@ZenIV.linux.org.uk> <1349343019.2706.3.camel@dabdike.int.hansenpartnership.com> <1349435268.3638.42.camel@dabdike.int.hansenpartnership.com> <1349444664.3638.46.camel@dabdike.int.hansenpartnership.com> <20121005144819.GO23473@ZenIV.linux.org.uk> <1349448936.3638.64.camel@dabdike.int.hansenpartnership.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Parisc List To: James Bottomley Return-path: In-Reply-To: <1349448936.3638.64.camel@dabdike.int.hansenpartnership.com> List-ID: List-Id: linux-parisc.vger.kernel.org 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?