Re: [PATCH 127] move ptrace_resume()-send_sigtrap() logic into ptrace_report_signal()
Nothing wrong (I think), but this is more complex and implies more unnecessary work. You mean the code path taken is longer. But the code's logic remains simple. If ptrace_report_signal() sends the trap, we actually send the signal twice. Firstly ptrace_resume() does utrace_control(UTRACE_INTERRUPT) (ok, this is not the signal sending but sort of), then -report_signal() notices we need the trap and send the real signal, then we return to get_signal_to_deliver() which calls utrace_get_signal() again, it dequeues SIGTRAP and calls ptrace_report_signal() again to finally report SIGTRAP to the tracer. I don't think this extra step is particuarly costly given the context. But: I am not sure yet, but I think the current code is not optimal for that, it was written to report *info without force_sig_info(). I don't really follow that bit. But I'll just see what new code you come up with. So, lets revert this change for now to be compatible. This change is simple and can be done again at any time, probably along with upstream change. Ok. Thanks, Roland
Re: [PATCH 127] move ptrace_resume()-send_sigtrap() logic into ptrace_report_signal()
On 11/02, Roland McGrath wrote: - unlike send_sigtrap()-force_sig_info() we don't unblock SIGTRAP or reset the handler This is nicer for debugger things actually, but we don't have such niceness for real traps and won't soon. IMHO it is best to start with doing exactly what the old x86 code does, which is force_sig_info--which is also making all machines uniformly do what a real single-step trap does and is thus consistent in this PTRACE_SINGLESTEP case matching all others, which we have been saying is a sensible principle. these changes looks good (but see the next patch). However, any user-visible change is dangerous. Let's not roll it in. As well as it just being bad form to roll that into implementation detail changes, it makes things less consistent rather than more (on x86, anyway, and arguably across the board). OK. So, we should revert this change and send the trap from ptrace_resume(), - instead of send_sigtrap() we should use user_single_step_siginfo() + force_sig_info() - PTRACE_EVENT_SYSCALL_EXIT shouldn't send the trap, this was x86 specific behaviour, and this is fixed in upstream by [PATCH v2 5/5] ptrace: x86: change syscall_trace_leave() to rely on tracehook when stepping we recently sent. (btw, I wrongly thought x86 is right and other machines should be fixed). Correct? If yes: - force_sig_info() needs tasklist_lock. we can change force_sig_info() to use lock_task_sighand(), but perhaps we should not worry about this now - what should we do with PTRACE_EVENT_SIGTRAP ? So, get rid of PTRACE_EVENT_SIGTRAP. Instead for the case of UTRACE_SIGNAL_HANDLER when stepping, initialize *info to look like a vanilla SIGTRAP and then fall into the default case for real signals. should UTRACE_SIGNAL_HANDLER use force_sig_info() too or it can just deliver this *info as you initially suggested ? Or we can forget about this change for now? I agree in advance with everything you prefer, you definetely know better. Anything else I missed? Oleg.
Re: [PATCH 127] move ptrace_resume()-send_sigtrap() logic into ptrace_report_signal()
So, we should revert this change and send the trap from ptrace_resume(), What's wrong with ptrace_report_signal doing it? - instead of send_sigtrap() we should use user_single_step_siginfo() + force_sig_info() Right. - PTRACE_EVENT_SYSCALL_EXIT shouldn't send the trap, this was x86 specific behaviour, and this is fixed in upstream by Right. (btw, I wrongly thought x86 is right and other machines should be fixed). In these corner cases it is always debatable what right is. I didn't really think and decide about it until recently when we decided about the upstream change. - force_sig_info() needs tasklist_lock. we can change force_sig_info() to use lock_task_sighand(), but perhaps we should not worry about this now It would be much better if we can leave the signals code alone. (It can always use more cleanups, but let those happen later on their own.) I think ideally we should try to invoke it the way it's invoked now, i.e. force_sig_info() is called on current. - what should we do with PTRACE_EVENT_SIGTRAP ? So, get rid of PTRACE_EVENT_SIGTRAP. Instead for the case of UTRACE_SIGNAL_HANDLER when stepping, initialize *info to look like a vanilla SIGTRAP and then fall into the default case for real signals. should UTRACE_SIGNAL_HANDLER use force_sig_info() too or it can just deliver this *info as you initially suggested ? Or we can forget about this change for now? We need to make it consistent with the upstream ptrace behavior, so we can't forget about anything that's needed for that. Today tracehook_signal_handler() uses ptrace_notify(). So that is more like an implicit fake signal report than a real queued SIGTRAP. (Still we won't ignore the resume signal any more, but that is a desireable change.) Conversely, we could make an upstream change along the lines of the tracehook_report_syscall_exit() change, so that this uses a real SIGTRAP instead of ptrace_notify(). That has some logic to it. But today this case is consistent across machines, so we could consider that behavior change separately after utrace merges. I agree in advance with everything you prefer, you definetely know better. :-) Perhaps I do when I have it all in mind at the same moment. But my brain capacity is feeling a bit low this week. Anything else I missed? I hope not! This area is not really fresh in my mind right now. Thanks, Roland
Re: [PATCH 127] move ptrace_resume()-send_sigtrap() logic into ptrace_report_signal()
- it sets task-thread.trap_no/error_code under CONFIG_X86, what should it do in the #else case? This can't be this way. It has to be a proper arch hook of some kind. - it sets info-si_addr = KSTK_EIP() which doesn't check user_mode_vm(). Hopefully this is OK? Ditto. The si_code/si_addr settings are altogether arch-specific. As long as you are using x86 details with #ifdef for hack drafts, just copy the exact details used in x86's send_sigtrap. - with or without this patch utrace-ptrace assumes that PTRACE_SINGLESTEP after PTRACE_EVENT_SYSCALL_EXIT needs fill_sigtrap_info()'ed signal, this may break or fix !x86 machines, and I don't know how to check. Perhaps ptrace_resume(PTRACE_EVENT_SYSCALL_EXIT) needs ifdef's. It should be easy enough to add a ptrace-tests case that distinguishes the two behaviors. I think we can then call the non-x86 behaviors broken and harmonize on the x86 behavior to make that test pass everywhere. - unlike send_sigtrap()-force_sig_info() we don't unblock SIGTRAP or reset the handler This is nicer for debugger things actually, but we don't have such niceness for real traps and won't soon. IMHO it is best to start with doing exactly what the old x86 code does, which is force_sig_info--which is also making all machines uniformly do what a real single-step trap does and is thus consistent in this PTRACE_SINGLESTEP case matching all others, which we have been saying is a sensible principle. these changes looks good (but see the next patch). However, any user-visible change is dangerous. Let's not roll it in. As well as it just being bad form to roll that into implementation detail changes, it makes things less consistent rather than more (on x86, anyway, and arguably across the board). Thanks, Roland
