On 10/27, Roland McGrath wrote: > > > And yes, while I don't pretend I know what should be done, I agree the > > current API (I don't mean the documentation, I mean the actual code) is > > not right. > > The behavior is fine in practice, it's just that the lockdep rules are > violated (and that's indeed horribly wrong in the cases that matter, which > are vanishingly rare in real-world practice).
In this case I confused again. Let's forget about get_user_pages() under spin_lock(), pretend it works. Two engines, E1 and E2, the tracee sleeps in utrace_resume()->utrace_stop(). E1 does utrace_control(UTRACE_RESUME), E2 does utrace_control(UTRACE_SINGLESTEP). How this can work? If E2 calls utrace_control() first, the subsequent UTRACE_RESUME does user_disable_single_step(), and (in general) E2 has no chance to re-assert SINGLESTEP. > > I agree. But I can't explain why I agree ;) I mean, I feel this should be > > more clean, but given that I do not understand the low level details even > > remotely I can't judge. > > What level of details don't you understand? > I hope you understand everything in utrace.c! Yes, I hope I understand utrace.c ;) I meant user_enable_single_step(). I don't really understand it even in x86 case, to many low level magic. > Speaking of which, it really looks like it's already the case now that > ->stopped exactly matches task_is_traced(). Yes. I believe, currently ->stopped can go away. > I know we discussed this > before and put it off. By the time we discussed it, ->stopped was a bit more complicated iirc, it could be set in some subtle ->exit_state cases. Afaics, we can kill ->stopped, but utrace_stop() and utrace_finish_jctl() should do unconditial spin_unlock_wait(utrace->lock) after resume (or lock/unlock). Of course, unlike ->stopped, task_is_traced() == T is not stable under under untrace->lock, but I think we don't care. > > And yes, this is not even consistent across machines (as you explained > > before), that is why I am very nervous when I think about the changes > > in this area. > > Ok. I think this is a corner where we will have some play in cleaning up > and unifying the behavior upstream without much complaint. In the status > quo, x86 passes only 0 to tracehook_report_syscall_exit and will synthesize > a SIGTRAP only if TIF_SINGLESTEP is set on its return (i.e. the last call > user_enable_*_step was after the last call to user_disable_single_step). > Other machines either pass step=1 when user_enable_*_step had been used to > reach tracehook_report_syscall_exit and do nothing else, or pass step=0 and > have no other related checks I can see. > > For us, step=1 doesn't really give any new information. It means > PTRACE_{SINGLE,BLOCK}STEP was used to hit the syscall insn (or used after > PTRACE_SYSCALL stopped for syscall-entry). But now ptrace_context.resume > tells us that if we check it. > > So, if nothing else, we can do user_disable_single_step somewhere inside > tracehook_report_syscall_exit to disarm the x86 code and then behave > uniformly across machines inside there. But, the x86 maintainers are > generally friendly to our changes if we are clear about their purpose. Yes, but sometimes ptrace (current implementation) has no chance to notice this case and it relies on syscall_trace_leave()->send_sigtrap(), we are going to return to user-mode without utrace->report/interrupt set. > On all those other machines with unconditional step=0, we can assume that > either they don't have single-step at all, PTRACE_SINGLESTEP over a syscall > insn is already broken (actually stops after the following insn instead of > after the syscall insn), or their hardware works differently so it doesn't > have this issue, and regardless that they never much cared about the nit. Horror. Horror, horror, horror. I just realized that ptrace_resume()->send_sigtrap() can only work on x86. I always knew this should be changed, but I hoped this more or less correct for now. But send_sigtrap() doesn't even exist on !x86 machines! > [...snip...] > > Then arch-independent code can fill siginfo_t just right for a synthetic > single-step trap. Yes, I think we should do something arch-independent. I need to read the code and think, then I'll ask you stupid questions. Oleg.