[PATCH] x86_64 ia32 syscall restart fix

[PATCH] x86_64 ia32 syscall restart fix

[Roland McGrath]

The code to restart syscalls after signals depends on checking for a
negative orig_ax, and for particular negative -ERESTART* values in ax.
These fields are 64 bits and for a 32-bit task they get zero-extended.
The syscall restart behavior is lost, a regression from a native 32-bit
kernel and from 64-bit tasks' behavior.  This patch fixes the problem by
doing sign-extension where it matters.  For orig_ax, the only time the
value should be -1 but winds up as 0x0ffffffff is via a 32-bit ptrace
call.  So the patch changes ptrace to sign-extend the 32-bit orig_eax
value when it's stored; it doesn't change the checks on orig_ax, though
it uses the new current_syscall() inline to better document the subtle
importance of the used of signedness there.  The ax value is stored a
lot of ways and it seems hard to get them all sign-extended at their
origins.  So for that, we use the current_syscall_ret() to sign-extend
it only for 32-bit tasks at the time of the -ERESTART* comparisons.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 arch/x86/kernel/ptrace.c    |    9 ++++++++-
 arch/x86/kernel/signal_64.c |   38 +++++++++++++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index d862e39..3975351 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -1035,10 +1035,17 @@ static int putreg32(struct task_struct *child, unsigned regno, u32 value)
 	R32(esi, si);
 	R32(ebp, bp);
 	R32(eax, ax);
-	R32(orig_eax, orig_ax);
 	R32(eip, ip);
 	R32(esp, sp);
 
+	case offsetof(struct user32, regs.orig_eax):
+		/*
+		 * Sign-extend the value so that orig_eax = -1
+		 * causes (long)orig_ax < 0 tests to fire correctly.
+		 */
+		regs->orig_ax = (long) (s32) value;
+		break;
+
 	case offsetof(struct user32, regs.eflags):
 		return set_flags(child, value);
 
diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
index 7347bb1..ce317ee 100644
--- a/arch/x86/kernel/signal_64.c
+++ b/arch/x86/kernel/signal_64.c
@@ -311,6 +311,35 @@ give_sigsegv:
 }
 
 /*
+ * Return -1L or the syscall number that @regs is executing.
+ */
+static long current_syscall(struct pt_regs *regs)
+{
+	/*
+	 * We always sign-extend a -1 value being set here,
+	 * so this is always either -1L or a syscall number.
+	 */
+	return regs->orig_ax;
+}
+
+/*
+ * Return a value that is -EFOO if the system call in @regs->orig_ax
+ * returned an error.  This only works for @regs from @current.
+ */
+static long current_syscall_ret(struct pt_regs *regs)
+{
+#ifdef CONFIG_IA32_EMULATION
+	if (test_thread_flag(TIF_IA32))
+		/*
+		 * Sign-extend the value so (int)-EFOO becomes (long)-EFOO
+		 * and will match correctly in comparisons.
+		 */
+		return (int) regs->ax;
+#endif
+	return regs->ax;
+}
+
+/*
  * OK, we're invoking a handler
  */	
 
@@ -327,9 +356,9 @@ handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
 #endif
 
 	/* Are we from a system call? */
-	if ((long)regs->orig_ax >= 0) {
+	if (current_syscall(regs) >= 0) {
 		/* If so, check system call restarting.. */
-		switch (regs->ax) {
+		switch (current_syscall_ret(regs)) {
 		        case -ERESTART_RESTARTBLOCK:
 			case -ERESTARTNOHAND:
 				regs->ax = -EINTR;
@@ -426,10 +455,9 @@ static void do_signal(struct pt_regs *regs)
 	}
 
 	/* Did we come from a system call? */
-	if ((long)regs->orig_ax >= 0) {
+	if (current_syscall(regs) >= 0) {
 		/* Restart the system call - no handlers present */
-		long res = regs->ax;
-		switch (res) {
+		switch (current_syscall_ret(regs)) {
 		case -ERESTARTNOHAND:
 		case -ERESTARTSYS:
 		case -ERESTARTNOINTR:

Re: [PATCH] x86_64 ia32 syscall restart fix

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> wrote:

> The code to restart syscalls after signals depends on checking for a 
> negative orig_ax, and for particular negative -ERESTART* values in ax. 
> These fields are 64 bits and for a 32-bit task they get zero-extended. 
> The syscall restart behavior is lost, a regression from a native 
> 32-bit kernel and from 64-bit tasks' behavior.  This patch fixes the 
> problem by doing sign-extension where it matters.  For orig_ax, the 
> only time the value should be -1 but winds up as 0x0ffffffff is via a 
> 32-bit ptrace call.  So the patch changes ptrace to sign-extend the 
> 32-bit orig_eax value when it's stored; it doesn't change the checks 
> on orig_ax, though it uses the new current_syscall() inline to better 
> document the subtle importance of the used of signedness there.  The 
> ax value is stored a lot of ways and it seems hard to get them all 
> sign-extended at their origins.  So for that, we use the 
> current_syscall_ret() to sign-extend it only for 32-bit tasks at the 
> time of the -ERESTART* comparisons.

thanks, applied.

	Ingo

Re: [PATCH] x86_64 ia32 syscall restart fix

[Linus Torvalds]

On Fri, 29 Feb 2008, Ingo Molnar wrote:
> 
> * Roland McGrath <roland@redhat.com> wrote:
> 
> > The code to restart syscalls after signals depends on checking for a 
> > negative orig_ax, and for particular negative -ERESTART* values in ax. 
> > These fields are 64 bits and for a 32-bit task they get zero-extended. 
> > The syscall restart behavior is lost, a regression from a native 
> > 32-bit kernel and from 64-bit tasks' behavior.  This patch fixes the 
> > problem by doing sign-extension where it matters.  For orig_ax, the 
> > only time the value should be -1 but winds up as 0x0ffffffff is via a 
> > 32-bit ptrace call.  So the patch changes ptrace to sign-extend the 
> > 32-bit orig_eax value when it's stored; it doesn't change the checks 
> > on orig_ax, though it uses the new current_syscall() inline to better 
> > document the subtle importance of the used of signedness there.  The 
> > ax value is stored a lot of ways and it seems hard to get them all 
> > sign-extended at their origins.  So for that, we use the 
> > current_syscall_ret() to sign-extend it only for 32-bit tasks at the 
> > time of the -ERESTART* comparisons.
> 
> thanks, applied.

Btw, can we please try to keep commit log messages readable?

The above "blob of text" could/should have more structure than being just 
one big block, and could have been structured as a few shorter paragraphs 
to make it easier to read: (1) problem description (2) patch description 
and (3) explanation of why patch was done it was done.

I don't know about you guys, but I read a *lot* of emails (and commit 
messages), and I hate seeing big blobs of text without structure. Give it 
a few breaks to make it easier to read, like just making new paragraphs, 
ie something like:

> The code to restart syscalls after signals depends on checking for a 
> negative orig_ax, and for particular negative -ERESTART* values in ax. 
> These fields are 64 bits and for a 32-bit task they get zero-extended. 
> The syscall restart behavior is lost, a regression from a native 32-bit 
> kernel and from 64-bit tasks' behavior.
>
> This patch fixes the problem by doing sign-extension where it matters.  
> For orig_ax, the only time the value should be -1 but winds up as 
> 0x0ffffffff is via a 32-bit ptrace call.  So the patch changes ptrace to 
> sign-extend the 32-bit orig_eax value when it's stored; it doesn't 
> change the checks on orig_ax, though it uses the new current_syscall() 
> inline to better document the subtle importance of the used of 
> signedness there.
>
> The ax value is stored a lot of ways and it seems hard to get them all 
> sign-extended at their origins.  So for that, we use the 
> current_syscall_ret() to sign-extend it only for 32-bit tasks at the 
> time of the -ERESTART* comparisons.

and now you have a bit of a breather space and some visual cues for whare 
you are in the text.

Yeah, maybe it's just me, but I like my whitespace. Ihaveareallyhardtime
readingtextthatdoesn'thavethepropermarkersforwhereconceptsstartandbegin, 
andthatreallydoesincludetheverticalwhitespacetoo.

Now, the only reason I mention this is that normally I would probably just 
have fixed this up myself without even a comment (because it's such a tiny 
detail that it's not not worth one), but when Ingo merges it I'll now get 
it through git and it will be fixed.

			Linus "yeah, I can be anal" Torvalds

Re: [PATCH] x86_64 ia32 syscall restart fix

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > > The code to restart syscalls after signals depends on checking for a 
> > > negative orig_ax, and for particular negative -ERESTART* values in ax. 
> > > These fields are 64 bits and for a 32-bit task they get zero-extended. 
> > > The syscall restart behavior is lost, a regression from a native 
> > > 32-bit kernel and from 64-bit tasks' behavior.  This patch fixes the 
> > > problem by doing sign-extension where it matters.  For orig_ax, the 
> > > only time the value should be -1 but winds up as 0x0ffffffff is via a 
> > > 32-bit ptrace call.  So the patch changes ptrace to sign-extend the 
> > > 32-bit orig_eax value when it's stored; it doesn't change the checks 
> > > on orig_ax, though it uses the new current_syscall() inline to better 
> > > document the subtle importance of the used of signedness there.  The 
> > > ax value is stored a lot of ways and it seems hard to get them all 
> > > sign-extended at their origins.  So for that, we use the 
> > > current_syscall_ret() to sign-extend it only for 32-bit tasks at the 
> > > time of the -ERESTART* comparisons.
> > 
> > thanks, applied.
> 
> Btw, can we please try to keep commit log messages readable?

yeah - the minute i added the patch i pinged Roland about that offline.

> Yeah, maybe it's just me, but I like my whitespace. 
> Ihaveareallyhardtime 
> readingtextthatdoesn'thavethepropermarkersforwhereconceptsstartandbegin, 
> andthatreallydoesincludetheverticalwhitespacetoo.

heh :)

> Now, the only reason I mention this is that normally I would probably 
> just have fixed this up myself without even a comment (because it's 
> such a tiny detail that it's not not worth one), but when Ingo merges 
> it I'll now get it through git and it will be fixed.

currently the reality is that i have to fix over 90% of the commit 
messages that go towards you :-/

While i'd like that proportion to be a lot lower, it's really hard for 
people to write good commit messages for fixes: people tend to send 
their fixes the minute they find the problem (being happy about having 
found and fixed a problem!), so the commit message gets little 
attention.

another effect is that kernel generalist people like Roland have a very 
large list of todo items so when they write up the commit message they 
might be thinking about the next unsolved problem already - and the 
commit message becomes a quick, unstructured and semi-automatic 
brain-dump of all details in essence :-/

Also, who am i to complain about the commit message - i'm often the one 
who has put the bug in to begin with! [ So i'm perfectly happy with you 
volunteering to take over that role ;-) ]

But yes, it's easier for me too to sort and prioritize patches if their 
description has good structure, so i regularly try to remind high-volume 
patch submitters about that. ( With little success i have to say - 
commit messages seem to be suffering from the same curse of inattention 
as other types of documentation do :-/ )

	Ingo

Re: [PATCH] x86_64 ia32 syscall restart fix

[Linus Torvalds]

On Fri, 29 Feb 2008, Ingo Molnar wrote:
> 
> currently the reality is that i have to fix over 90% of the commit 
> messages that go towards you :-/

Heh, yeah. My percentage ends up being much lower, mostly because patches 
that come through Andrew are generally already cleaned-up (I edit those 
too, but it tends to be one or two per batch, not more than that).

I'm happy you do edit them, because not everybody does, and I do think 
it's part of being a subsystem maintainer, but I also end up occasionally 
sending emails to the parties involved to try to keep editing to a mimumum 
in the future - I personally suspect that it's to a large degree because 
people don't think about the effect in the logs..

(Some other projects also tend to have very different models for what a 
commit message should look like, so much of it is probably "cultural" too. 

I've seen projects that consistently had totally unreadable one-liner 
commit messages because (a) nobody ever read them anyway (because the log 
just isn't useful when it's per-file) and (b) people were encouraged to 
just use things like 'cvs ci -m"Fix bug"' to check in their stuff.

So the kernel is probably fairly odd in generally not asking for any 
fixed-format stuff at all (like the GNU changelogs do) but instead writing 
a small human-readable novella ;)

			Linus

Re: [PATCH] x86_64 ia32 syscall restart fix

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> So the kernel is probably fairly odd in generally not asking for any 
> fixed-format stuff at all (like the GNU changelogs do) but instead 
> writing a small human-readable novella ;)

i think it's also a good learning tool. Subscribing to git-commits-head 
might be more useful to a newbie these days than subscribing to lkml :-/

and one area where commit messages are totally important IMO is bug 
forensics. For every regression we find we try to put in the commit ID 
that broke it. Information like that is vital to have a good (and 
objective) picture about how bugs get into and get out of the kernel and 
it also alerts us to change/improve infrastructure if certain categories 
of bugs happen too often.

	Ingo

Re: [PATCH] x86_64 ia32 syscall restart fix

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> and one area where commit messages are totally important IMO is bug 
> forensics. For every regression we find we try to put in the commit ID 
> that broke it. Information like that is vital to have a good (and 
> objective) picture about how bugs get into and get out of the kernel 
> and it also alerts us to change/improve infrastructure if certain 
> categories of bugs happen too often.

another "commit space" feature Thomas and me was thinking about was to 
put in "backport suggestions" for -stable the following way:

   Backport-suggested-by: Ingo Molnar <mingo@elte.hu>

and the -stable tree could then notice it, and once it has been 
backported, they could put in their "done" notifiers via:

   Backported-from: 67ca7bde2e9d3516b5

or:

   Backport-rejected: 67ca7bde2e9d3516b5

This way the act of suggesting backports to the -stable tree (and their 
rejection) could be fully automated, and the answer to the rather 
difficult question:

   "has -stable picked up all backport requests, and if not, why?"

could be scripted up.

A further (small) variation of this scheme: if a fix is noticed to be a 
backport candidate later on, or a user notices that a fix that has gone 
upstream fixes a -stable bug too, this information could be signalled in 
a separate, special, empty commit:

   Backport-suggested-by: 67ca7bde2e9d35, Ingo Molnar <mingo@elte.hu>


this way subsystem maintainers could have a reliable protocol of getting 
fixes integrated into -stable - purely via the commit messages in your 
tree.

... but then we decided that handling x86 architecture maintainance is 
work enough already, without us complicating our own life any further 
;-)

But the idea is solid nevertheless, and if everyone did it the -stable 
guys would have a much easier life as well :-) [ We could start doing it 
in x86.git if there's general agreement and if the -stable guys 
specifically asked for this. ]

	Ingo

Re: [PATCH] x86_64 ia32 syscall restart fix

[Andrew Morton]

On Fri, 29 Feb 2008 18:37:05 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > and one area where commit messages are totally important IMO is bug 
> > forensics. For every regression we find we try to put in the commit ID 
> > that broke it. Information like that is vital to have a good (and 
> > objective) picture about how bugs get into and get out of the kernel 
> > and it also alerts us to change/improve infrastructure if certain 
> > categories of bugs happen too often.
> 
> another "commit space" feature Thomas and me was thinking about was to 
> put in "backport suggestions" for -stable the following way:
> 
>    Backport-suggested-by: Ingo Molnar <mingo@elte.hu>
> 
> and the -stable tree could then notice it, and once it has been 
> backported, they could put in their "done" notifiers via:
> 
>    Backported-from: 67ca7bde2e9d3516b5
> 
> or:
> 
>    Backport-rejected: 67ca7bde2e9d3516b5
> 
> This way the act of suggesting backports to the -stable tree (and their 
> rejection) could be fully automated, and the answer to the rather 
> difficult question:
> 
>    "has -stable picked up all backport requests, and if not, why?"
> 
> could be scripted up.
> 
> A further (small) variation of this scheme: if a fix is noticed to be a 
> backport candidate later on, or a user notices that a fix that has gone 
> upstream fixes a -stable bug too, this information could be signalled in 
> a separate, special, empty commit:
> 
>    Backport-suggested-by: 67ca7bde2e9d35, Ingo Molnar <mingo@elte.hu>
> 
> 
> this way subsystem maintainers could have a reliable protocol of getting 
> fixes integrated into -stable - purely via the commit messages in your 
> tree.
> 
> ... but then we decided that handling x86 architecture maintainance is 
> work enough already, without us complicating our own life any further 
> ;-)
> 
> But the idea is solid nevertheless, and if everyone did it the -stable 
> guys would have a much easier life as well :-) [ We could start doing it 
> in x86.git if there's general agreement and if the -stable guys 
> specifically asked for this. ]
> 

I believe the -stable guys have a bot which trolls the mainline commits
mailing list for "cc:.*stable@kernel.org".  So anybody anywhere in the
patch delivery chain can append "Cc: <stable@kernel.org>" and things
should get appropriate consideration.

The place where I suspect there is a lot of lossage is people simply not
thinking about whether a fix should be backported.  I'm forever fussing
about that for the patches I handle (and I still miss some) but I have a
suspicion that not all tree-owners do this fully.

Re: [PATCH] x86_64 ia32 syscall restart fix

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> I believe the -stable guys have a bot which trolls the mainline 
> commits mailing list for "cc:.*stable@kernel.org".  So anybody 
> anywhere in the patch delivery chain can append "Cc: 
> <stable@kernel.org>" and things should get appropriate consideration.

ok, didnt know about that.

> The place where I suspect there is a lot of lossage is people simply 
> not thinking about whether a fix should be backported.  I'm forever 
> fussing about that for the patches I handle (and I still miss some) 
> but I have a suspicion that not all tree-owners do this fully.

we watch out for this, but still, about 50% of the cases, the 
realization "this should be backported" comes later on. Often because 
fixes get applied with low latency, and testers lag in realizing that 
some particular -stable problem is fixed by a -git fix. Sometimes people 
do bisection in search of backportable fixes - that too has a lag.

so the more formal:

    Backport-suggested-by: commit-id, person

entry would solve both cases. Also, a commit entry in -stable:

    Backported-from: commit-id

would finish the transaction. [ But this is clearly something that the 
-stable folks have to request - it wont help much if we start doing it 
but the -stable folks ignore the entries :-) ]

	Ingo

Re: [PATCH] x86_64 ia32 syscall restart fix

[Roland McGrath]

And here I thought I was so good for using coherent English sentences
with proper punctuation and capitalization, not to mention actually
explaining the issues of the change.  I must admit I've been a little
bewildered in the past when Ingo has changed my log entries to be
ungrammatical, eschew standard punctuation, and with an apparent
vendetta on the shift key, not to mention sometimes removed half the
relevant technical detail to boot.

I suppose I shouldn't be surprised at being harangued for making my log
entries too informative.  People love to remove clauses out of the
middle of my code comments too--so they can match the norm of run-on
sentences, I guess.

Most log entries I see are short.  But they really don't explain the
problem being fixed so I know enough about it without digging out some
long mailing list threads.  If that's your preference, then I guess we'll
just have to keep having Ingo remove the text I write.  At least it will
be in the archives from my posting.  (Reading it is the only way I'll
remember myself what details mattered, after a week has gone by.)

If what you want is formulaic log text that always puts blank lines in
between bug description, change description, and change justification, I
can do that.  If there is any place that documents the conventions you
want in log entries, I've overlooked it.

As Ingo alluded to, most of the time my number one focus is to record all
the important facts and decisions before I go back to something else or
knock off for the night.  I hate nothing more than looking at an old
change of mine and not being able to figure out what some of the logic
behind it was.  If there's one thing I can rely on, it's that I'll forget
what I knew the first time until I spend hours the second time
recapitulating the same debugging to find out that I may have had some
sense after all six months back.

Anyway, if the worst push-back on my submissions is a handful
of foreigners telling me how to write English, then I guess
things are ... as they should be. ;-)


As to the question of tagging for backports, I am not really clear on
what the intended criteria are.  Take this bug for example.  It is a
consistent behavior that has existed since the dawn of time (I've only
actually checked back to 2.6.9).  It's wrong, but can't be a surprise
on any system based on a stable release.  It's a regression of the
64-bit kernel vs the native 32-bit kernel, but not a version-to-version
regression.  The fix is straightforward to backport and has very low
risk.  How does all that translate into whether a given stable version
wants a backport or not?


Thanks,
Roland

Re: [PATCH] x86_64 ia32 syscall restart fix

[Linus Torvalds]

On Fri, 29 Feb 2008, Roland McGrath wrote:
> 
> I suppose I shouldn't be surprised at being harangued for making my log
> entries too informative.

No, no, no. We want them long and informative. 

The only thing I asked for was that text should have the visual separation 
also between different paragraphs.

I at no point asked you to make the thing shorter, I asked you to split it 
up and MAKE IT VISUALLY LONGER by adding the proper vertical whitespace 
(which is how we do paragraphs).

> If what you want is formulaic log text that always puts blank lines in
> between bug description, change description, and change justification, I
> can do that.  If there is any place that documents the conventions you
> want in log entries, I've overlooked it.

It's _purely_ an issue of overly long paragraphs, not about the language 
or any "formulaic" crap.

The fact is, paragraphs help make things more readable. 

What I find funny is how this email you just wrote actually has *much* 
shorter paragraphs than the changelog entry I replied to. 

Go back and look. The explanation I objected to was a single paragraph, 15 
lines long. That's about two thirds of the screen with no break.

And then, in the email reply you just sent (the one I'm replying to here), 
the longest one was nine lines and most were four or six lines, with one 
being three lines. Why? Because it's simply MORE READABLE.

I didn't ask you to write denser or less readable log messages, I asked 
for exactly the reverse. So I don't understand why you then made your 
reply be a rant about how you could make less explanatory changelog 
messages.

		Linus

Re: [stable] [PATCH] x86_64 ia32 syscall restart fix

[Greg KH]

On Fri, Feb 29, 2008 at 10:20:14PM +0100, Ingo Molnar wrote:
> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > I believe the -stable guys have a bot which trolls the mainline 
> > commits mailing list for "cc:.*stable@kernel.org".  So anybody 
> > anywhere in the patch delivery chain can append "Cc: 
> > <stable@kernel.org>" and things should get appropriate consideration.
> 
> ok, didnt know about that.

Yes, it's quite handy, so just add that to your patch and we
automatically grab it.

I've added that info to the stable rules file in Documentation, should I
"announce" it anywhere else?

> > The place where I suspect there is a lot of lossage is people simply 
> > not thinking about whether a fix should be backported.  I'm forever 
> > fussing about that for the patches I handle (and I still miss some) 
> > but I have a suspicion that not all tree-owners do this fully.
> 
> we watch out for this, but still, about 50% of the cases, the 
> realization "this should be backported" comes later on. Often because 
> fixes get applied with low latency, and testers lag in realizing that 
> some particular -stable problem is fixed by a -git fix. Sometimes people 
> do bisection in search of backportable fixes - that too has a lag.
> 
> so the more formal:
> 
>     Backport-suggested-by: commit-id, person

I normally just add the person who suggested the patch to be added to
the Cc: if they are not already on the signed-off-by: list.

Does it really matter who suggested that -stable pick it up?

> 
> entry would solve both cases. Also, a commit entry in -stable:
> 
>     Backported-from: commit-id

We already add that info to the commit, but not necessarily in a
"standard" format.  If you want it in something like this, it's trivial
to provide it.

thanks,

greg k-h

[PATCH] x86_64 ptrace orig_ax on ia32 task

[Roland McGrath]

This makes 64-bit ptrace calls setting the 64-bit orig_ax field
for a 32-bit task sign-extend the low 32 bits up to 64.  This
matches what a 64-bit debugger expects when tracing a 32-bit task.

This follows on my "x86_64 ia32 syscall restart fix".
This didn't matter until that was fixed.

The debugger ignores or zeros the high half of every register slot it
sets (including the orig_rax pseudo-register) uniformly.  It expects
that the setting of the low 32 bits always has the same meaning as a
32-bit debugger setting those same 32 bits with native 32-bit
facilities.

This never arose before because the syscall restart check never
matched any -ERESTART* values due to lack of sign extension.  Before
that fix, even 32-bit ptrace setting orig_eax to -1 failed to trigger
the restart check anyway.  So this was never noticed as a regression
of 64-bit debuggers vs 32-bit debuggers on the same 64-bit kernel.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 arch/x86/kernel/ptrace.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index f8eed1b..92b44e1 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -322,6 +322,20 @@ static int putreg(struct task_struct *child,
 	case offsetof(struct user_regs_struct, flags):
 		return set_flags(child, value);
 
+#ifdef CONFIG_IA32_EMULATION
+	case offsetof(struct user_regs_struct, orig_ax):
+		/*
+		 * For a 32-bit task, setting only the low 32 bits and
+		 * leaving the high bits untouched (all 0) has the same
+		 * effect as setting those bits via 32-bit ptrace would.
+		 * This means sign-extending an orig_eax of -1, which
+		 * here is an orig_rax of (u32)-1.
+		 */
+		if (test_tsk_thread_flag(child, TIF_IA32))
+			value = (long) (s32) value;
+		break;
+#endif
+
 #ifdef CONFIG_X86_64
 	case offsetof(struct user_regs_struct,fs_base):
 		if (value >= TASK_SIZE_OF(child))

Re: [PATCH] x86_64 ptrace orig_ax on ia32 task

[Linus Torvalds]

On Fri, 7 Mar 2008, Roland McGrath wrote:
> 
> This makes 64-bit ptrace calls setting the 64-bit orig_ax field
> for a 32-bit task sign-extend the low 32 bits up to 64.  This
> matches what a 64-bit debugger expects when tracing a 32-bit task.

Hmm. Why make this conditional on CONFIG_IA32_EMULATION and TIF_IA32?

That field is never really 64-bit anyway. The only allowable values are
 - system call number (== small positive value)
 - a small negative number for traps

So I'd suggest just making it entirely unconditionally just do

	case offsetof(struct user_regs_struct, orig_ax):
		value = (long) (s32) value;
		break;

and be done with it. Why have arbitrarily different code on x86 and x86-64 
when there is no real reason for it?

		Linus

Re: [PATCH] x86_64 ptrace orig_ax on ia32 task

[Roland McGrath]

> Hmm. Why make this conditional on CONFIG_IA32_EMULATION and TIF_IA32?

I just wasn't touching the case that wasn't broken.

> So I'd suggest just making it entirely unconditionally just do

That is fine by me.


Thanks,
Roland

Re: [PATCH] x86_64 ptrace orig_ax on ia32 task

[Chuck Ebbert]

On 03/07/2008 05:56 PM, Roland McGrath wrote:
> This makes 64-bit ptrace calls setting the 64-bit orig_ax field
> for a 32-bit task sign-extend the low 32 bits up to 64.  This
> matches what a 64-bit debugger expects when tracing a 32-bit task.
> 
> This follows on my "x86_64 ia32 syscall restart fix".
> This didn't matter until that was fixed.
> 

That original patch is still unmerged as of -rc5.

Re: [PATCH] x86_64 ptrace orig_ax on ia32 task

[Linus Torvalds]

On Mon, 10 Mar 2008, Chuck Ebbert wrote:
> 
> That original patch is still unmerged as of -rc5.

The original is, but the modified one that just does this unconditionally 
for x86-64 isn't. See commit 84c6f6046c5a2189160a8f0dca8b90427bf690ea.

			Linus

Re: [PATCH] x86_64 ptrace orig_ax on ia32 task

[Roland McGrath]

> On Mon, 10 Mar 2008, Chuck Ebbert wrote:
> > 
> > That original patch is still unmerged as of -rc5.
> 
> The original is, but the modified one that just does this unconditionally 
> for x86-64 isn't. See commit 84c6f6046c5a2189160a8f0dca8b90427bf690ea.

Chuck was not referring to my original version of that patch.
As that patch's log says:
    
    This follows on my "x86_64 ia32 syscall restart fix".  This didn't
    matter until that was fixed.
    
The "x86_64 ia32 syscall restart fix" patch was not merged before the follow-on.
It's in Ingo's x86/for-linux as commit 5813a19cba5735b629cdeb156863dab814759128.


Thanks,
Roland

Re: [PATCH] x86_64 ptrace orig_ax on ia32 task

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> wrote:

> > On Mon, 10 Mar 2008, Chuck Ebbert wrote:
> > > 
> > > That original patch is still unmerged as of -rc5.
> > 
> > The original is, but the modified one that just does this unconditionally 
> > for x86-64 isn't. See commit 84c6f6046c5a2189160a8f0dca8b90427bf690ea.
> 
> Chuck was not referring to my original version of that patch.
> As that patch's log says:
>     
>     This follows on my "x86_64 ia32 syscall restart fix".  This didn't
>     matter until that was fixed.
>     
> The "x86_64 ia32 syscall restart fix" patch was not merged before the follow-on.
> It's in Ingo's x86/for-linux as commit 5813a19cba5735b629cdeb156863dab814759128.

yes, it's lined up for the next batch of fixes.

	Ingo