> > I'm not entirely positive there aren't any cases where another callback > > would be made before the report_signal. > > Yes. But, afaics, if another callback can be called before report_signal > then we don't have a valid ->last_siginfo.
I agree it sounds like a nice thing to be entirely positive about. But then some of the failure modes of the buggy old patch have to do with last_siginfo being wrong in ways I didn't understand, IIRC. > IOW. ptrace_detach(signr) should have effect only if the tracee sleeps > in utrace_get_signal()->finish_resume_report() path, right ? (I am ignoring > report_syscall case for now). syscall is the only contrary case that comes to mind. But indeed I think we can ignore it because the syscall case has its own special and different semantics (i.e. queuing) that we want to handle differently, as we discussed earlier. > This means, before utrace_get_signal() is called with ->interrupt == T, > the only possible callback is utrace_report_jctl()->ptrace_report_quiesce(). All sounds right to me. I'm just not confident I haven't missed something. That's what we have you for! ;-) > I think this should be safe. The tracee should be stopped. We can only > race with SIGKILL, but given that old ->ops = ptrace_utrace_ops can't go > away I think nothing bad can happen. Or we can do > > spin_lock(utrace->lock); > if (utrace->stopped) > engine->ops = new_ops; > /* else: it is killed, we don't care */ > spin_unlock(utrace_lock); > > Anyway. I am not proud of this idea, and it needs more thinking in any case. I don't doubt that we can find a set of rules by which it can work. But IMHO it's far better to keep it a very simple API rule that ->data belongs entirely to the engine but no other field should be touched directly. Of course, ptrace can always be a behind-the-curtain exception. But whenever we can avoid that, we will. > Or. Perhaps. detach can just mark engine as detached, and ptrace_attach() > does utrace_attach_task() without UTRACE_ATTACH_EXCLUSIVE. This means > we should rely on PT_PTRACED, not good. And this means we should teach > ptrace_utrace_ops's methods to take this case into account. I don't think > this is good idea... Here's another idea. Don't ever leave the regular ptrace_ops engine attached after PTRACE_DETACH. Instead, add another engine with a separate ops vector and event mask, just for the parting signal case. That engine is very simple: it only has to set UTRACE_EVENT(QUIESCE) implement the callbacks to inject the parting signal (and handle the corner cases). This way, the UTRACE_ATTACH_MATCH_OPS|UTRACE_ATTACH_EXCLUSIVE &ptrace_ops can still act as the arbiter of who is ptracing. (Though IIRC we still had an issue there with the order of the permission checks, but leave that aside for the moment.) When a new PTRACE_ATTACH comes along, it can get the new &ptrace_ops engine in place first. Then, knowing it's the sole tracer, it can also look up the lingering parting-signal engine with MATCH_OPS and notice the situation. I haven't thought through all the details. But this seems like a promising direction to me. > Roland, if only I knew how to improve the old utrace-ptrace.patch! > If only I knew how to write it correctly from scratch... Well, this is why we spent time understanding all the data structures controlling ptrace and cleaning them up. You now know more than anyone else about exactly what ptrace keeps track of and what requirements the moving parts have on each other wrt synchronization and the like. > And giwen that it must be ready asap, my only hope is to make some fixes > against this old utrace-ptrace.patch. And I still have no idea how to > solve the problems with detach. Well, perhaps swithing engine->ops can > work... We want it to be merged upstream ASAP. What will make that possible will be when it is clean enough, readable enough, behaves correctly, is rock-solid in the known torture tests, and is otherwise non-regressing, to the satisfaction of the upstream reviewers. If it's not ready by that definition, then there is no "ready" that matters, no matter how soon. Of course, always the sooner the better, and for all the aforementioned axes of perfection, incremental stumbling progress toward them is better than no running code. But AFAICT there is no benefit in any kind of rush job that doesn't take the time to get the innards right enough to pass muster upstream. > Other problems with attach/detach which are more or less simple: It sounds like you have fixes in mind. Just do them. Or just note them for later and leave these corners wrong while you first make progress on something that works at all. > 5. ptrace_report_clone() is racy. parent->parent can exit and untrace > child before utrace_attach_task(child, UTRACE_ATTACH_CREATE ...). This seems like it would only matter if we lack proper synchronization with whatever governs the tracer's list of tracees. Any of the kinds of attach ought to take the appropriate lock for that, no? > Anything else I missed? I don't know. I know more about what was wrong with the old patch than what wasn't right. I think it is now well past time to just get something going and see what you hit. We have the ptrace-tests suite for lots of behavior semantics tests. Run its 'make check' and see what pops out, and that should give you plenty to think about. Its 'make xcheck' is the torture tests for things like race conditions that you have been concentrating on. The issues like security check orders and such won't even be caught by those. But all of that you can leave to fester while we start to work on a clean implementation of the core functionality. Just don't poke those cases, and they won't poke you back. We can give them a fresh look in the context of some code we are happy(er) with in the other respects. > But again. I still have no idea how to fix the problems with > ptrace_detach(signr) which doesn't do UTRACE_DETACH. I suspect the extra-engine approach will be promising. > I am stuck. Break on through! Thanks, Roland
