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

[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 think what's true now is what's always been true.  The comment is a bit
confusing, though.  The two kinds of stopped are inside do_signal_stop()
and inside utrace_stop().  In the former, it is inside the signal-checking
loop and will check again.  In the latter, utrace_stop() ends with a call
to recalc_sigpending().


Thanks,
Roland

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

[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;
   finish_resume_report(report);
   }
 
Yes, we need finish_resume_report() here, but since report-takers
is not set, finish_report_reset() will always call utrace_reset().
 
OTOH, we can't set -takers before finish_resume_report(), we can
miss UTRACE_DETACH request. utrace_control(DETACH)-utrace_do_stop()
does not change utrace-resume != UTRACE_RESUME.
 
And since utrace_do_stop() never upgrades utrace-resume, we have
another problem: UTRACE_STOP request can be lost here.

Hmm.  Maybe this?

diff --git a/kernel/utrace.c b/kernel/utrace.c
index f46854b..000 100644  
--- a/kernel/utrace.c
+++ b/kernel/utrace.c
@@ -1740,8 +1740,6 @@ static void finish_resume_report(struct 
 struct task_struct *task,
 struct utrace *utrace)
 {
-   finish_report_reset(task, utrace, report);
-
switch (report-action) {
case UTRACE_STOP:
utrace_stop(task, utrace, report-resume_action);
@@ -1829,6 +1827,8 @@ void utrace_resume(struct task_struct *t
report.action = UTRACE_RESUME;
list_for_each_entry(engine, utrace-attached, entry)
start_callback(utrace, report, engine, task, 0);
+
+   finish_report_reset(task, utrace, report);
}
 
/*
@@ -2147,6 +2147,7 @@ int utrace_get_signal(struct task_struct
 * as in utrace_resume(), above.  After we've dealt with that,
 * our caller will relock and come back through here.
 */
+   finish_report_reset(task, utrace, report);
finish_resume_report(report, task, utrace);
 
if (unlikely(fatal_signal_pending(task))) {


 5. utrace_resume() has the same problems. If report-action != REPORT
we do not call callbacks and finish_resume_report() is called with
-takers == F.

 6. But! I think utrace_resume() was always wrong here. Because it
calls start_callback(events = 0) and thus we nevet set -takers.

I think that change covers these too.  What do you think?


Thanks,
Roland

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

[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 seems to me it isn't worth using a
barrier because the winner of that race doesn't matter.  So:

struct utrace *utrace = task_utrace_struct(current);
return likely(utrace)  utrace-resume == UTRACE_INTERRUPT;

is fine, right?  (With comment changes to explain the need for the check.)


Thanks,
Roland

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

[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 check on the return value
from kmem_cache_zalloc is the only test/branch.  I have no special
opinion about the cosmetic choice.  Whatever you and/or upstream
prefer is fine.  Send a patch if you would like it to be different.


Thanks,
Roland