On 10/27, Roland McGrath wrote: > > > Not sure I understand... This is like utrace->vfork_stop:1, it is > > only visible to utrace code. > > Show me the change the the utrace_control kerneldoc wording that makes it > match what difference you propose this implementation detail would make. > When you consider other engines using UTRACE_BLOCKSTEP and all other such > interactions, I think you'll find that there isn't a clean API description > that could be implemented using a flag like you suggested.
Again, I don't understand. Let me repeat, I tried to propose something that do not make any visible difference except fixes the problem with get_user_pages() under spin_lock(). 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. > > So. I am still not sure this is the best solution (in fact this doesn't > > even look like a good solution to me), but afaics we can preserve the > > current API and fix the problem if we add utrace->set_singlestep and > > utrace->set_blockstep. > > This approach loses the existing possibility e.g. for an engine that tried > UTRACE_BLOCKSTEP to find out that an earlier engine is already using > UTRACE_SINGLESTEP and so the stop it gets next won't necessarily be at a > block boundary. Afaics, the current code can't work properly in this case anyway. But yes, the pseudo-code I showed is not complete, it was only to roughly explain what I mean. > As I alluded to before, the version of this approach that seems like it > could be clean is to store an enum utrace_resume_action rather than a flag > (or two flags). Thinking about it more, my intuition is that this could be > the way to go. This field could also replace the report and interrupt bits > we have now, and that starts to feel clean. At firts glance, this looks right to me. > In fact, with a broader view it might not even be desireable to do > user_enable_single_step in the tracer rather than the tracee. 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. > Those two use access_process_vm to look at the PC instruction, > and we could (later) optimize those to use copy_from_user when called on > current, which has much lower overhead. Yes, I thought about this too. > So I now have reversed in complementary directions. As to the API, I do > think it would be nice to avoid always getting a callback when there really > is nothing else going on--though I still insist engines are broken if they > don't have one so they can react to other engines' actions. But as to the > mechanics, I now think we should favor calling user_enable_single_step et > al in current rather than not. OK. Unless I misunderstand "avoid always getting a callback" above, this means the tracee should call enable_step() after wakeup in utrace_stop(), yes? In this case ptrace doesn't need any changes for now. > > As for ptrace. If utrace_control(SINGLESTEP) doesn't set TIF_SINGLESTEP, > > then we need more (probably nasty) changes. report_quiesce/interrupt should > > somehow figure out whether we need send_sigtrap() if ->resume == XXXSTEP. > > I may have lost the plot again, sorry. You're talking about the case of > PTRACE_SINGLESTEP at a syscall-exit stop, which is not even consistent > across machines in the vanilla kernel. Right? Not necessary in syscall-exit stop, but yes. 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. Oleg.
