Re: [HACK] utrace: fix utrace_resume()-finish_resume_report() logic

2009-11-16 Thread Roland McGrath
In short, it is just wrong to call finish_resume_report() in utrace_resume() without reporting loop, because utrace never clears TIF_NOTIFY_RESUME. It's not supposed to. The arch code clears TIF_NOTIFY_RESUME before calling tracehook_notify_resume(). This implies that utrace must keep its

Re: [PATCH] utrace: utrace_attach_task() forgets to return when -utrace == NULL

2009-11-16 Thread Roland McGrath
Just in case fyi, I cooked the almost identical patch yesterday, but it didn't help: make xcheck crashes. Not that I really expected it can help on x86. Right. Not sure I understand exactly... Yes, sometimes we know we don't need read_barrier_depends(). For example, @@ -302,7

Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-16 Thread Roland McGrath
Whatever temporary hacks are fine by me one way or the other. They will just be coming out later along with assorted other cleanup. We certainly do want to get this right in the utrace layer. The change we talked about before seems simple enough and should cover this without new kludges in the

Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes

2009-11-16 Thread Roland McGrath
Yes it does. I am almost sure the bug should be trivial, but somehow can't find find it. Just fyi, to ensure this is connected to utrace-indirect I applied the hack below and the bug goes away. Does s/kmem_cache_zalloc/kzalloc/ really have anything to do with it? Isn't it just the allocation

Re: [PATCH] utrace: utrace_attach_task() forgets to return when -utrace == NULL

2009-11-16 Thread Roland McGrath
Yes, sure. I meant, we shouldn't worry that this barrier adds too much overhead to task_utrace_struct(). Ah! Sorry, I misread you. Yes, good point. We don't have the explicit barrier, but I think it is not needed. In this case utrace_attach_task() does unlock+lock at least once before

Re: [PATCH 127] move ptrace_resume()-send_sigtrap() logic into ptrace_report_signal()

2009-11-13 Thread Roland McGrath
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

Re: [PATCH 0/1] Was: utrace-cleanup branch

2009-11-12 Thread Roland McGrath
I did some tweaks that I think address the several things you've raised. But I didn't try to reply point by point. I've merged everything up now, so the utrace-cleanup branch is gone. Please review the current code and post about anything we still need to fix. I merged into utrace-ptrace and

Re: utrace_report_syscall_entry() finish_report()

2009-11-12 Thread Roland McGrath
Simple example. The tracee stopped in syscall-entry. the tracer does PTRACE_SINGLESTEP. With the recent changes in utrace-cleanup utrace_control() doesn't set TIF_SINGLESTEP, and the tracee passes syscall_trace_leave() without TIF_SINGLESTEP. Ah, yes. Well, the point of the arch/tracehook

Re: [PATCH 127] move ptrace_resume()-send_sigtrap() logic into ptrace_report_signal()

2009-11-12 Thread Roland McGrath
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,

Re: [PATCH 3/5] ptrace: change tracehook_report_syscall_exit() to handle stepping

2009-11-11 Thread Roland McGrath
Well, ptrace_report_syscall() checks current-exit_code after ptrace_notify() and does send_sig(). But yes, PTRACE_SETSIGINFO is doesn't work. Oh, right. Aside from PTRACE_SETSIGINFO, it's more subtly wrong in that it generates a new signal (to be seen again by ptrace) rather than delivering

Re: utrace_report_syscall_entry() finish_report()

2009-11-11 Thread Roland McGrath
I saw you have utrace-syscall-resumed branch but never looked at it. For those not concerned with its purpose, there is one thing you must know. Every report_syscall_entry callback must do: if (utrace_syscall_action(action) == UTRACE_SYSCALL_RESUMED) return

Re: [PATCH 0/5] for upstream, user_single_step_siginfo() changes

2009-11-10 Thread Roland McGrath
Seriously, I don't think it makes sense to factor out memset + si_signo = SIGTRAP. Imho we need a single helper which can/should be overriden by arch's. Sure. I am fine with whatever hashes out upstream for this. Thanks, Roland

Re: [PATCH 0/2] Was: introduce suppress_sigtrap() to prevent unwanted send_sigtrap()

2009-11-10 Thread Roland McGrath
But syscall_trace_leave() uses TRAP_BRKPT on x86. Should I change 4/5 ? Oh, I was sloppy in checking the values this time. Then probably the default TRAP_SINGLE_STEP should be 0 instead just to make clear that every arch needs to define it. But your other way is fine instead too. The new

Re: utrace_report_syscall_entry() finish_report()

2009-11-10 Thread Roland McGrath
Ok. I realize it's largely separate, but I think we want to hash through this along with the other set of after-report-behavior problems. Also, it doesn't seem sensible to fiddle with utrace_report_syscall_entry separate from resolving UTRACE_SYSCALL_RESUMED change to the API. There was not

Re: [PATCH 0/2] Was: introduce suppress_sigtrap() to prevent unwanted send_sigtrap()

2009-11-09 Thread Roland McGrath
OK, how about these 2 simple patches for upstream? Then we can change powerpc, etc. Perhaps, instead of arch_has_fill_sigtrap_info we can start with the patch below? Since tracehook_report_syscall_exit() is inline we can can add the if (step) code without ifdef's. I don't understand the

Re: [PATCH 1/2] teach tracehook_report_syscall_exit() to handle stepping

2009-11-09 Thread Roland McGrath
I wouldn't make the behavior conditional on the arch hook's definition. I think a consistent change to a real SIGTRAP signal is better for any arch. Until each arch defines the hook, it can just get the default siginfo_t contents of 0-fill. This arch hook is purely for the single-step case. For

Re: utrace tests

2009-11-09 Thread Roland McGrath
Wondering if there are some existing utrace tests that can be used for regression testing. There is a ntrace tests under, http://people.redhat.com/roland/utrace/old/ Are those tests still relevant? I'm not really sure what you mean by relevant. That old testing code is certainly not

Re: [PATCH 128] introduce suppress_sigtrap() to prevent unwanted send_sigtrap()

2009-11-05 Thread Roland McGrath
1. (upstream) add arch hook(s) for single-step SIGTRAP siginfo_t + arch fields (i.e. x86's thread.error_code + thread.trap_no). OK. say, arch_fill_sigtrap_info(siginfo_t *info, ...). Something like that, right. Since this is only for single-step in particular, that should be in the

Re: [PATCH 0/1] Was: utrace-cleanup branch

2009-11-04 Thread Roland McGrath
3. Now that we have utrace-resume, can't we kill report-resume_action ? I thought this initially when making the change and then decided against it. I don't recall exactly what was in my mind at the time. It would take some more thought now to be sure whether there is a semantic problem. But

Re: [PATCH 128] introduce suppress_sigtrap() to prevent unwanted send_sigtrap()

2009-11-04 Thread Roland McGrath
In this case the tracee resumes and stops after syscall_trace_leave() to report PTRACE_EVENT_FORK, but since it passes syscall_trace_leave() with TIF_SINGLESTEP set the tracee gets the unwanted send_sigtrap(). Note: of course, this is hack. I think this should be move to utrace layer.

Re: [PATCH 0/1] Was: utrace-cleanup branch

2009-11-03 Thread Roland McGrath
8. Completely off-topic, but utrace_control() has a very strange comment under case UTRACE_INTERRUPT, * When it's stopped, we know it's always going * through utrace_get_signal() and will recalculate. can't recall if it were ever true, but surely it is not now? I

Re: [PATCH 0/1] Was: utrace-cleanup branch

2009-11-03 Thread Roland McGrath
4. One of the changes in utrace_get_signal() doesn't look exactly right. if (utrace-resume UTRACE_RESUME || utrace-signal_handler) { ... if (resume UTRACE_REPORT) { report.action = resume;

Re: [PATCH 0/1] Was: utrace-cleanup branch

2009-11-03 Thread Roland McGrath
7. utrace_attach_task() has an implicit wmb() between -utrace = new_utrace and -utrace_flags = REAP, this is good. But, for example, tracehook_force_sigpending() does not have rmb(), this means utrace_interrupt_pending() can OOPS in theory. Ok. Please send a patch. Off hand it

Re: [PATCH 0/1] Was: utrace-cleanup branch

2009-11-03 Thread Roland McGrath
2. Cosmetic, but the usage of utrace_task_alloc() looks a bit strange. Why it returns bool, not struct utrace * ? The pointer it would return is always target-utrace or it's NULL. So the bool just says which of those it would be instead. Either way I imagine it should be inlined so the

Re: upstream/utrace: copy_process() TIF_SINGLESTEP

2009-11-02 Thread Roland McGrath
You are right. I added step-fork to ptrace-tests for this. The place that should do this is arch/*:copy_thread. TIF_* bits are arch implementation details. x86 and powerpc both have a TIF_SINGLESTEP that should be cleared, and others might too. Each arch maintainer should check if their

Re: [PATCH 127] move ptrace_resume()-send_sigtrap() logic into ptrace_report_signal()

2009-11-02 Thread Roland McGrath
- 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?

Re: [PATCH 122] ptrace_request(PTRACE_KILL) should not(?) return -ESRCH

2009-10-30 Thread Roland McGrath
Damn, this PTRACE_KILL is so weird. Yes. It's useless. Not regressing on the expectations of stupid old programs that don't really know what it does is all that's required. Apart from that, what it should actually do? It should work like PTRACE_CONT,SIGKILL but the only error case is -ESRCH

Re: utrace-cleanup branch

2009-10-29 Thread Roland McGrath
To optimize out other checks + mb() in the likely stopped case? Yes. Thanks, Roland

Re: utrace-cleanup branch

2009-10-29 Thread Roland McGrath
Can't comment right now, need to read the code. Such gentlemanly restraint. Afaics, we can't just remove utrace_finish_jctl() and the similar code in utrace_stop(). We need void utrace_finish_jctl(void) { struct utrace *utrace = task_utrace_struct(current);

Re: utrace-indirect branch

2009-10-29 Thread Roland McGrath
Yes, this is the question ;) At this point I have an irrational distaste for utrace_struct.h and a pathological displeasure at having a contentious upstream discussion on this again. We need to get the right code in upstream, and in the right state before another lifetime passes. I'm leaving

Re: PTRACE_EVENT_SIGTRAP

2009-10-29 Thread Roland McGrath
This seems like something we could change upstream first, to clarify and separate the behavior change. If the arch bits about choosing si_code et al are resolved, then it is simple to make the old kernel's tracehook calls post a signal instead of using ptrace_notify. Ah, yes. I forgot

Re: utrace_control(,,UTRACE_SINGLESTEP)

2009-10-29 Thread Roland McGrath
Yes, we can't control the callback order, and this means we have the same problem even without utrace_control(). Right, but that is the known item on the list to consider for API refinements. If at least utrace_control doesn't add any new wrinkles to that issue, then we know that if we later

PTRACE_EVENT_SIGTRAP

2009-10-28 Thread Roland McGrath
This is used only in the UTRACE_SIGNAL_HANDLER case. That means after tracehook_signal_handler(), which is where a signal handler has just been set up. For reference, in old ptrace, tracehook_signal_handler() is: if (stepping) ptrace_notify(SIGTRAP); This means that

utrace-indirect branch

2009-10-28 Thread Roland McGrath
Please take a look at the patch below and tell me what you think. This is the new(ish) utrace-indirect branch (not to be confused with what's now old/utrace-indirect). I first made it a while ago, but I don't recall if I ever mentioned it. This compiles and looks right, but I have not done any

utrace-cleanup branch

2009-10-28 Thread Roland McGrath
I've made a new branch, utrace-cleanup. This forks from utrace-indirect and has: 26fefca utrace: sticky resume action 28b2774 utrace: remove -stopped field Those are the two changes we talked about during tangential ptrace discussions. Again, I have compiled these but not tested a lick. I'd

Re: [PATCH 118] (upstream) introduce kernel/ptrace.h

2009-10-27 Thread Roland McGrath
I'm skeptical this is the desireable way to move the code around. Of course, for all such things, I am fine with whatever upstream likes. But here are my concerns: That is not friendly to git history at all. If you move big chunks of code to different files, it's ideal to do it in a sequence of

utrace_control(,,UTRACE_SINGLESTEP)

2009-10-27 Thread Roland McGrath
[I have no idea why you appended this to that message introducing patches that are not related to this at all. Please use separate threads for separate topics, and choose clearly apropos Subject: lines.] I am thinking how can we fix utrace_control(SINGLESTEP). I don't have good ideas so far.

Re: [PATCH 0/7] utrace-ptrace V1

2009-10-27 Thread Roland McGrath
5/7 belongs first and I've already merged it as prerequisite to utrace. We can send that upstream without delay. I hope it can get queued quickly regardless of the review delays for the utrace and ptrace work. All the other preparatory patches are just to introduce PT_PTRACED as the distinction

Re: [PATCH 118] (upstream) introduce kernel/ptrace.h

2009-10-27 Thread Roland McGrath
Not sure I understand. Do you mean it is possible to move the code from the old file to the new one in a git-friendly manner? Afaics, there is no way to do this, git can only hanlde renames. (but my git skills is close to zero). What I meant is a sequence of patches like: 1. move non-common

Re: utrace_control(,,UTRACE_SINGLESTEP)

2009-10-27 Thread Roland McGrath
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

Re: [PATCH 7/7] implement utrace-ptrace

2009-10-27 Thread Roland McGrath
--- /dev/null 2009-10-25 19:46:00.608018007 +0100 +++ V1/kernel/ptrace-utrace.c 2009-10-26 03:56:46.0 +0100 Generally, needs more comments. That's not news, I'm sure. But just giving reactions as I would if doing upstream review without other context. +struct ptrace_context { +

Re: [PATCH 115] (upstream) ptrace_init_task: cleanup the usage of ptrace_link()

2009-10-26 Thread Roland McGrath
I think you can send this one upstream right away with no controversy. It does pure cleanup. (Is it even needed for utrace in particular?) Acked-by: Roland McGrath rol...@redhat.com Thanks, Roland

Re: utrace-ptrace v1 todo list

2009-10-20 Thread Roland McGrath
The definition of v1 is very simple and absolutely precise. The code is v1 when Roland thinks we can make the RFC patch and send it for review. Ok. My definition of success remains whatever works to get it merged. As I see it, we need the following changes. utrace: -

Re: [PATCH 96] ptrace_report_clone: uglify CLONE_PTRACE/CLONE_UNTRACED behaviour to match upstream

2009-10-20 Thread Roland McGrath
HOWEVER!!! man 2 clone: CLONE_PTRACE If CLONE_PTRACE is specified, and the calling process is being traced, then trace the child also (see ptrace(2)). That is accurate. OK, CLONE_UNTRACED (since Linux 2.5.46) If CLONE_UNTRACED

Re: [PATCH 100-103] ptrace_resume cleanup/simplification

2009-10-20 Thread Roland McGrath
What about PTRACE_SYSEMU_SINGLESTEP ? I will read the code tomorrow, but it is easy to miss some detail and we don't have any test-cases. These exist purely for UML. So the real test cases are to use UML. To start with, make sure that check_sysemu() gets the same results as on the vanilla

Re: Stopped detach/attach status

2009-10-16 Thread Roland McGrath
My point was, the discussed problems with ptrace stop probably are not ptrace-only, we need other changes. Hopefully we should address them after v1. Sure. But I'd prefer to delay this discussion unless you think we should fix this right now. I'm glad to stop thinking about it for a

Re: [PATCH 93] ptrace_report_exit: fix WARN_ON() condition

2009-10-16 Thread Roland McGrath
IMHO the truly desireable behavior is to distinguish real SIGKILL (userland kill, oom_kill) from normal group-exit. Yes. We already discussed this and I agree. I _think_ it is not hard to implement, but (again) I'd suggest to do this later. Agreed. (I think I just said that this is not

Re: [PATCH 2/4] utrace_reap: do not play with -reporting

2009-10-15 Thread Roland McGrath
I still didn't find the time to read the code around set/clear -reporting, it is subtle and needs a fresh head. Ok. I had remembered that your earlier reviews included review all use of memory barriers in utrace, so I didn't realize this stuff needed review. But at least, I think

Re: [PATCH 83] ptrace_attach_task: suppress WARN_ON(err)

2009-10-15 Thread Roland McGrath
ptrace_attach_task: engine = utrace_attach_task(CREATE | EXCLUSIVE); err = utrace_set_events(); WARN_ON(err !tracee-exit_state); Looks correct but it is not. utrace_attach_task() can return EINPROGRESS. utrace_set_events can, yes. Note that start_callback/etc sets

Re: utrace-ptrace ptrace_check_attach()

2009-10-15 Thread Roland McGrath
And then you check whether it's really in a proper ptrace stop, see that it isn't, and use UTRACE_RESUME. So far I don't really understand to do this correctly, but OK. I guess I'm missing something because it seems trivial to me. ptrace_check_attach() does verify we are ptracer. If the

Re: [PATCH 74-78] implement the partly detached engines

2009-10-15 Thread Roland McGrath
Not sure. Suppose we call utrace_control(old, UTRACE_DETACH) right before the tracee utrace_get_signal() calls ops-report_signal(). Then utrace_control() returns -EINPROGRESS. If it returned 0, then ops-report_signal will not be called. If that's not so, utrace_control is broken.

Re: [PATCH 85] ptrace_attach_task: rely on utrace_barrier(), don't check -ops

2009-10-15 Thread Roland McGrath
Tracee, finish_callback() path: if (action == UTRACE_DETACH) engine-ops = utrace_detached_ops; utrace-reporting = NULL; no barries, no utrace-lock() in between. Tracer, utrace_barrier() under utrace-lock: if (engine-ops == utrace_detached_ops)

Re: [PATCH 90] ptrace_wake_up: add bool force_wakeup argument for implicit detach

2009-10-15 Thread Roland McGrath
ptrace_detach(sig) checks valid_signal(sig) to detect the explicit detach and passes bool voluntary to ptrace_wake_up(). ptrace_detach_task() does. ptrace_detach() has already bailed out if !valid_signal(sig) was really pass in from userland. valid_signal(0) = true, so this is

Re: [PATCH 91-94] misc attach/detach fixes

2009-10-15 Thread Roland McGrath
Roland, do you see other problems with attach/detach which should be fixed before v1? It depends what v1 means, on which I am not entirely clear. If you just mean being work-alike compatible, then the proof is in the pudding. Like I said before, if it doesn't regress any tests, including on

Re: [PATCH 83] ptrace(DETACH, SIGKILL) should really kill the tracee

2009-10-12 Thread Roland McGrath
The one thing that anyone using PTRACE_DETACH,SIGKILL does perhaps expect is that the detaching and killing are atomic. That is, it's not possible for another thread in the tracer's process to get the WIFEXITED wait result for the tracee. Conversely, that race is possible if the tracer

Re: [PATCH 83] ptrace(DETACH, SIGKILL) should really kill the tracee

2009-10-11 Thread Roland McGrath
Roland, Jan, what user-space expects ptrace(DETACH, SIGKILL) should do? My guess: this should really kill the tracee asap, hence this patch. As far as killing, it is no more reliable than PTRACE_CONT,SIGKILL. i.e., will fail if it's not stopped, will be dropped on the floor if stopped at

Re: utrace-ptrace ptrace_check_attach()

2009-10-11 Thread Roland McGrath
Issues with ptrace_check_attach(), - it does utrace_control(UTRACE_STOP). This is wrong, ptrace_check_attach() must be passive, while utrace_control(UTRACE_STOP) can actualy stop the tracee. This is not inherently problematic on its own. I'd say it's OK if

Re: utrace-ptrace detach with signal semantics

2009-10-11 Thread Roland McGrath
I'm replying out of order. So I'm sorry I started rehashing some of this same stuff in the other thread before reading where you'd already referred to it. What if the tracee reports a signal and stops, the tracer detaches but does not wake it up because of SIGNAL_STOP_STOPPED ? In this case

Re: utrace-ptrace detach with signal semantics

2009-10-11 Thread Roland McGrath
I think always-reply-to-all is the best policy ;) For some people's mail-handling habits it makes a difference, so it is always safest not to trim individuals. For me personally, I always see the list copies just the same as the ones CC'd to me personally, so I don't care to be in the CC list

Re: [PATCH 59] make sure SINGLESTEP doesn't miss SYSCALL_EXIT

2009-10-07 Thread Roland McGrath
Sorry I'm so long in following up on this. I know you've hacked a lot more since. The vanilla kernel relies on syscall_trace_leave() which does send_sigtrap(TRAP_BRKPT). But with utrace-ptrace the tracee sleeps in do_notify_resume() path, syscall_trace_leave() won't be called. It's even

Re: [PATCH 53-56] kill -ev_array + fix stepping

2009-10-07 Thread Roland McGrath
On top of 4492770dc8d2312da9518e8b85fb0e49dc3da510 in your utrace-ptrace branch. I had done an upstream merge since then and also merged some more of yours. So to get utrace-ptrace back to a state where 53-70 apply cleanly, I reverted these first: 6752625 introduce context_siginfo() helper

Re: [PATCH 59] don't use task_struct-ptrace_message

2009-10-07 Thread Roland McGrath
task_struct-ptrace_message is no longer needed. Woo! I wonder why compat_ptrace_request() does (compat_ulong_t)ptrace_message, put_user(x, ptr) uses __typeof__(*ptr). I think it's just being explicit about the truncation to keep its intent clear to someone reading the code. Thanks, Roland

Re: [PATCH 62] introduce ptrace_rw_siginfo() helper

2009-10-07 Thread Roland McGrath
+static int ptrace_rw_siginfo(struct task_struct *tracee, + struct ptrace_context *context, + siginfo_t *info, bool write) +{ + unsigned long flags; + siginfo_t *context_info; + int err = -ESRCH; + + if

Re: [PATCH 63] convert ptrace_getsiginfo() to use ptrace_rw_siginfo()

2009-10-07 Thread Roland McGrath
+ if (context-siginfo) + return ptrace_rw_siginfo(tracee, context, info, false); - return error; + memset(info, 0, sizeof(*info)); + info-si_signo = SIGTRAP; + info-si_code = context-ev_code; // XXX: ev_code was already cleared!!! + info-si_pid =

Re: [PATCH 64] convert ptrace_setsiginfo() to use ptrace_rw_siginfo()

2009-10-07 Thread Roland McGrath
+ if (context-siginfo) + return ptrace_rw_siginfo(tracee, context, info, true); Superfluous test, just call it unconditionally as with the get case. Then both helpers have everything in common, and they are static anyway, so you might as well just call ptrace_rw_siginfo from

Re: [PATCH 70] move event 8 into syscall_code()

2009-10-07 Thread Roland McGrath
Make it something like: static inline void set_stop_code(struct ptrace_context *ctx, int event) { ctx-stop_code = (event 8) | SIGTRAP | (event = PTRACE_EVENT_SYSCALL_ENTRY (ctx-options PTRACE_O_TRACESYSGOOD) ?

Re: [PATCH 70] move event 8 into syscall_code()

2009-10-07 Thread Roland McGrath
void set_stop_code(struct ptrace_context *ctx, int event) { ctx-stop_code = (event 8) | SIGTRAP; } void set_syscall_code(struct ptrace_context *ctx, int event) { set_stop_code(ctx, event); if (PTRACE_O_TRACESYSGOOD)

Re: [PATCH 62] introduce ptrace_rw_siginfo() helper

2009-10-07 Thread Roland McGrath
I don't think this can work. context-siginfo can be cleared and then set again in between. If we race with SIGKILL, utrace_get_signal() can dequeue another signal != SIGKILL and start the reporting loop. That's not supposed to be possible. See sigset_t sigkill_only; et al. I guess it is

Re: utrace-ptrace detach with signal semantics

2009-10-07 Thread Roland McGrath
Currently, if a tracer does ptrace(DETACH, tracee, SIGXXX) and then another/same tracer does ptrace(ATTACH, tracee) then SIGXXX will not be reported to the new tracer. Correct. Why? Should utrace-ptrace be 100% compatible here? I think it should, yes. There is a rationale for it that makes

Re: utrace-ptrace detach with signal semantics

2009-10-07 Thread Roland McGrath
Naive programs expect the first signal after PTRACE_ATTACH will be SIGSTOP. They should not, this is just wrong. And I think the proposed change doesn't change the behaviour in this sense. It does not in the general sense. But it does change the behavior of certain 100% predictable

Re: utrace-ptrace detach with signal semantics

2009-10-07 Thread Roland McGrath
OK. Let's see what Roland thinks. I think the whole stoppedness vs detach et al discussion is a separate issue from the basic PTRACE_DETACH,signr behavior, and I think I want to discuss that in the other thread we have whose subject says it's about that. I think I've covered the (separate)

Re: utrace-ptrace detach with signal semantics

2009-10-07 Thread Roland McGrath
In which specific cases SIGNR can get ignored? There are two fundamental kinds of ptrace stops: real signal stops, and fake (or ptrace_notify()) stops. In the latter, you are not in the right place in the kernel for direct signal delivery, so it never works fully normally. All the optional

Re: utrace-ptrace detach with signal semantics

2009-10-07 Thread Roland McGrath
after which the full range of [oops, left that thought unfinished!] ... signal control options works (immediate delivery, SETSIGIFNO obeyed)

Re: Stopped detach/attach status

2009-10-07 Thread Roland McGrath
I do not know. I'd leave this to Roland. I mean, if he thinks this should be fixed - I'll try to fix. But. This all looks unfixeable to me. In my opinion, the kernel is obviously wrong, and test-case are wrong too. And any fix in this area is user-visible and can break the current

Re: [PATCH 56-58] context-siginfo fixes

2009-09-25 Thread Roland McGrath
This was copy-and-pasted from the old code. Shouldn't we rely use UTRACE_SIGNAL_HOLD instead ? Perhaps so, but I'd rather leave it alone for now. The send_sig_info path has various other checks that could be relevant to the kludgey old ptrace semantics for this arcane case. Fresh

Re: [PATCH 56] ptrace_resume_signal() should use context-siginfo under -siglock

2009-09-25 Thread Roland McGrath
- Change ptrace_resume_signal() to use context-siginfo under -siglock, like ptrace_{get,set}siginfo() do. I don't think the log/comments give a clear picture of why this is the thing to do. To wit, I'm not even sure at the moment it is necessary. It probably is, but I have to convince myself

Re: [PATCH 53-55] (Was: Q: what user_enable_single_step() actually means?)

2009-09-25 Thread Roland McGrath
It's a further oddity that you can single-step (or not) into the system call and then get a ptrace stop inside it, that being for PTRACE_EVENT_FORK et al. And utrace-ptrace should be compatible here, yes? As far as the sequence of stops that a ptracer observes, the general answer is

Re: utrace_control(XXXSTEP)-is_setting_trap_flag() is not safe

2009-09-25 Thread Roland McGrath
And. If we have multiple tracers, then utrace_control(SINGLESTEP) just fools the caller, another engine can do utrace_control(RESUME) at any moment. Sure, but more likely nobody will. I think you are right, utrace_control() should simply consider UTRACE_XXXSTEP as UTRACE_REPORT. All else

Re: Q: what user_enable_single_step() actually means?

2009-09-23 Thread Roland McGrath
On Wed, 23 Sep 2009 02:36:54 +0200, Roland McGrath wrote: It would be worthwhile to cons a version of this test case that uses PTRACE_SINGLESTEP instead of PTRACE_SYSCALL. I think your situation is tickling the same issue, but we should have an empirical test. [...] I have a fix in hand

Re: utrace_control(XXXSTEP)-is_setting_trap_flag() is not safe

2009-09-23 Thread Roland McGrath
Btw, I believe we have another problem. utrace_control(SINGLESTEP) calls user_enable_single_step() under utrace-lock. I don't really understand the magic in enable_single_step() but is_setting_trap_flag() calls access_process_vm(), this doesn't look good under spinlock. Yes, this has been on

Re: [PATCH 53-55] (Was: Q: what user_enable_single_step() actually means?)

2009-09-23 Thread Roland McGrath
Yes, but this has nothing to do with utrace-ptrace. If we last used PTRACE_CONT, the tracee stops in utrace_resume() path before return to the user-mode, syscall_trace_leave() can't be called. If I follow what you mean, that is just the x86 bug (now fixed upstream). Both tests fail. The 1st

Re: [PATCH 54] PTRACE_SINGLESTEP shouldn't bypass SYSCALL_EXIT

2009-09-23 Thread Roland McGrath
However, the current behaviour is that PTRACE_SINGLESTEP acts like PTRACE_SYSCALL (but see below), if the tracee is not going to return to user-mode yet. Say, PTRACE_EVENT_VFORK. Not really. Given what I just wrote, I'm not sure if you want me to merge these. I'm sure you'll be changing it

Re: [PATCH 53-55] (Was: Q: what user_enable_single_step() actually means?)

2009-09-23 Thread Roland McGrath
The tracee stops and reports PTRACE_EVENT_FORK from do_notify_resume(), after that syscall_trace_leave() can't (and must not) be called. Oh, I see what you mean. That is all under your control from the utrace API level. You can use a syscall-exit report first if you want to for keeping track

Re: Q: what user_enable_single_step() actually means?

2009-09-22 Thread Roland McGrath
Thanks Roland. But now I am even more confused... I get that combination a lot! ;-) Just to keep everyone confused, note that this thread now has approximately nothing to do with single-step. It really does have something to do with single-step, but only in the special sense of

Re: Q: what user_enable_single_step() actually means?

2009-09-20 Thread Roland McGrath
- PTRACE_SINGLESTEP does user_enable_single_step() - when the tracee returns to user mode, the next instruction causes exception, do_debug()-send_sigtrap() sends SIGTRAP - the tracee notices the signal and reports this SIGTRAP Correct. But if the tracee is inside

Re: problem: utrace-ptrace jctl stacked stop

2009-09-18 Thread Roland McGrath
I'll point out that the only symptom that matters to ptrace is -exit_code and that issue disappears if you stop overloading it for ptrace purposes, which is the clean thing to do in the long run anyway. But there are subtler issues that haven't yet directly affected ptrace. In this scenario, the

Re: [PATCH 38] make sure PTRACE_CONT disables SYSCALL_EXIT report

2009-09-18 Thread Roland McGrath
With current utrace it is no longer a fulltime assignment so it is OK this way. Thanks for tending the suite, it's been very helpful. Roland

Re: [PATCH 39] make sure PTRACE_SYSCALL reports SYSCALL_EXIT

2009-09-18 Thread Roland McGrath
Confused... Do you think something is wrong with the current code? No, I was just being explicit about the nonobvious quirks of the semantics. (They merit some comments in the eventual code.) IOW, I assume this test-case [...] is right, correct? Yes. Thanks, Roland

Re: [PATCH 35] convert ptrace_report_syscall_entry()

2009-09-17 Thread Roland McGrath
I am worried about PTRACE_SYSEMU, I continue to ignore this magic which I don't understand yet... Hopefully I will be able to add the necessary changes later. It should not be a big complexity. The semantics is that the entry report always does like UTRACE_SYSCALL_ABORT to skip the actual

Re: [PATCH 42] do_ptrace_notify_stop: kill -ev_message != 0 check

2009-09-17 Thread Roland McGrath
do_ptrace_notify_stop() doesn't change -ptrace_message if the new value is zero. Not sure what I was thinking about when I wrote this. Perhaps you were thinking of the upstream code before tracehook.h was introduced. Ancient code left whatever was in ptrace_message before there for the 0

Re: [PATCH 38] make sure PTRACE_CONT disables SYSCALL_EXIT report

2009-09-17 Thread Roland McGrath
I am a bit surprised there is nothing in ptrace-tests to check CONT/SYSCALL behaviour. I had to write this one: That suite has grown only through regression tests for bugs that we've noticed. So it doesn't test a case if my past implementations never broke that case, nor if a regression never

Re: [PATCH 40] PTRACE_EVENT_VFORK_DONE: set ev_options = PTRACE_O_TRACEVFORKDONE

2009-09-17 Thread Roland McGrath
ptrace_report_clone() still needs more changes. I think it is simple to fix it now, but can't we simplify the utrace's behaviour first? utrace_report_clone() does not set utrace-vfork_stop without CLONE_VFORK, this adds some complications. Perhaps we can kill CLONE_VFORK check? Please

Re: [PATCH 30] remove the current PTRACE_EVENT_VFORK_DONE logic

2009-09-14 Thread Roland McGrath
1. I assume that, on every arch, we alway check TIF_NOTIFY_RESUME and TIF_SIGPENDING before TIF_SYSCALL_TRACE, right? No, these are only checked when you would otherwise return to user mode. IOW, can I assume that syscall_trace_leave() should not be called

Re: [PATCH 24-27] initial stop/resume changes

2009-09-09 Thread Roland McGrath
I merged it all. But I tend to think the function pointer is overkill. It's probably simpler and cleaner in the end to just store PTRACE_EVENT_* and have a switch, or something like that. Thanks, Roland

Re: [PATCH 4/4] utrace_release_task: check REAP before utrace_reap()

2009-09-09 Thread Roland McGrath
I think we can do this optimization, but see below. Nice work! I've merged it all. Thanks, Roland

Re: [PATCH 2/4] utrace_reap: do not play with -reporting

2009-09-09 Thread Roland McGrath
1. Can't we set -reporting _after_ checking engine-flags ? (unlikely) I'm not sure...it is early the morning again. ;-) 2. Is utrace_barrier() correct Note the example above, it assumes that utrace_barrier() itself is the barrier wrt reporting.

Re: Bug: report_reap is never called

2009-09-07 Thread Roland McGrath
Oops! I broke that in d4bcb571, a change I no longer remember the reason for. I'm sure it was for a subtler bug Oleg noticed, should be a thread in this archive about it. Should be fixed now in commit 2e12892. Thanks, Roland

Re: [PATCH 4/4] utrace_release_task: check REAP before utrace_reap()

2009-09-07 Thread Roland McGrath
Again, I am not sure this make sense as a cleanup, up to you. But utrace_release_task() can check UTRACE_EVENT(REAP) and optimize out utrace_reap() when there are no attached engines. In the original code, utrace_release_task() would not be called at all when there are no engines attached.

Re: [PATCH 3/4] utrace_reap: do not play with lock/unlock/restart

2009-09-07 Thread Roland McGrath
(Note I'm replying to 3/4 before 2/4.) IOW, nobody change change engine-ops or REAP bit in flags. Nobody can add the new engine or remove it from -attached. We can do all work lockless and without barriers. Makes sense. In commit ac6e19c the body looks like this: { struct

Re: [PATCH 2/4] utrace_reap: do not play with -reporting

2009-09-07 Thread Roland McGrath
But more importantly, I think utrace_reap() never needs to synchronize with utrace_barrier(). Once we drop utrace-lock, any other caller which takes this lock must see both -exit_state and utrace-reap. This means utrace_control() or utrace_set_events() which sets/clears REAP must not succeed.

<    1   2   3   4   >