linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Paul Mackerras <paulus@samba.org>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: linux-kernel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-arch@vger.kernel.org, David Miller <davem@davemloft.net>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>
Subject: Re: [RFC][CFT][CFReview] execve and kernel_thread unification work
Date: Thu, 11 Oct 2012 20:00:23 +1100	[thread overview]
Message-ID: <20121011090023.GA28427@bloggs.ozlabs.ibm.com> (raw)
In-Reply-To: <20121001213809.GA31155@ZenIV.linux.org.uk>

I just looked through the "powerpc: split ret_from_fork" commit in
your for-next branch, and I have a couple of comments.

First, on 64-bit powerpc, if kernel_thread() is called on a function
in a module, and that function returns, we'll then jump to do_exit
with r2 pointing to the module's TOC rather than the main kernel's
TOC, with disastrous results.  It would be easily solved by adding a

	ld	r2,PACATOC(r13)

before the branch to .do_exit in the 64-bit ret_from_kernel_thread().
Maybe no-one ever calls kernel_thread() on a function in a module
today, but I don't want to rely on that being true forever, especially
since it's only one more instruction to guard against the possibility.

Secondly, while the code as you have it is correct, to my mind it
would be cleaner to put the function descriptor address in r14 on
64-bit, rather than the function text address, and then dereference it
to get the TOC and text address at the point of doing the call.  That
would reduce the differences between 32-bit and 64-bit in
copy_thread() and make it more obvious that ret_from_kernel_thread()
is setting the right TOC value in r2.

In fact copy_thread() doesn't need to set r2 in the 32-bit case, since
_switch() sets r2 to the task_struct for the task it's switching to,
and ignores the r2 value in the stack frame.  So with my suggestion
copy_thread() wouldn't need to set childregs->gpr[2] at all (for the
kernel thread case).

Paul.

  parent reply	other threads:[~2012-10-11  9:01 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-10-01 21:38 [RFC][CFT][CFReview] execve and kernel_thread unification work Al Viro
2012-10-05 16:07 ` Catalin Marinas
2012-10-06 16:19   ` Al Viro
2012-10-09 19:48 ` Chris Metcalf
2012-10-10  4:51   ` Al Viro
2012-10-11  9:00 ` Paul Mackerras [this message]
2012-10-11 12:53   ` Al Viro
2012-10-12  0:16     ` Paul Mackerras
2012-10-12  1:09       ` [git pull] signal.git, pile 2 (was Re: [RFC][CFT][CFReview] execve and kernel_thread unification work) Al Viro
2012-10-12  5:32         ` Paul Mackerras
2012-10-12  7:55         ` Benjamin Herrenschmidt
2012-10-15  1:30 ` [RFC][CFT][CFReview] execve and kernel_thread unification work Al Viro
2012-10-17  6:16   ` Greg Ungerer
2012-10-17 14:02   ` Catalin Marinas
2012-10-17 16:34     ` Al Viro
2012-10-17 16:40       ` Catalin Marinas

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=20121011090023.GA28427@bloggs.ozlabs.ibm.com \
    --to=paulus@samba.org \
    --cc=benh@kernel.crashing.org \
    --cc=davem@davemloft.net \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@ZenIV.linux.org.uk \
    /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).