login
Search
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 --- ...
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, 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 --
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 --
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 --
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 --
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 --
On Fri, 29 Feb 2008 18:37:05 +0100 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. --
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
--
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 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. 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 --
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 ...
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 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 --
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)) --
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 --
> Hmm. Why make this conditional on CONFIG_IA32_EMULATION and TIF_IA32? That is fine by me. Thanks, Roland --
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 --
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
--
yes, it's lined up for the next batch of fixes. Ingo --
