Btw, is it allowed to use utrace_control(UTRACE_DETACH) from ->report_any() ?
If yes, then we need a bit more fixes...
On 08/31, Oleg Nesterov wrote:
>
> On 08/30, Roland McGrath wrote:
> >
> > > This case is not unlikely, for example ENGINE_STOP is cleared after the
> > > previous wakeup. We are running, if ENGINE_STOP is set, UTRACE_STOP was
> > > used and utrace_reset() was called after that. Otherwise, _perhaps_ it
> > > was cleared by UTRACE_DETACH.
> >
> > What we want is to change things so that this case *is* unlikely. That is,
> > we really want to avoid any extra O(n) work such as utrace_reset calls. It
> > should never be common to do one of those we don't really want to do. And,
> > all else being equal, when some extra O(n) work is unavoidably required,
> > IMHO it's better that it be in the tracer than in the tracee.
>
> Yes, agreed.
>
> > I took a different tack and committed these:
> >
> > 96fe3cc Revert "utrace_stop: fix UTRACE_DETACH race"
> > d1a14ce utrace: change UTRACE_STOP bookkeeping
> > 64a8ca3 utrace_reset cleanup
>
> I think this should work, but I'd like to re-read these changes carefully,
> this all is subtle. And, at first glance, we need something like the patch
> below.
>
> Because now utrace_control(UTRACE_DETACH) does
>
> case UTRACE_DETACH:
> ...
> if (engine_wants_stop(engine))
> resume = true;
>
> OK, we need to call utrace_reset() to recalc ->utrace_flags. But
> utrace_reset()
> assumes it is safe to play with utrace->attached list, now this is not true.
> IOW, with this patch utrace_control(UTRACE_DETACH)->utrace_reset() can race
> with REPORT_CALLBACKS/etc which does list_for_each_entry(->attached).
> (minor, but perhaps resume should be renamed to "bool reset")
>
> "task != current" before utrace_wakeup() doesn't look right too, I think we
> should check ->stopped instead. Harmless, but otherwise we can call wakeup()
> while it is not needed.
>
> Oleg.
>
> --- a/kernel/utrace.c
> +++ b/kernel/utrace.c
> @@ -722,11 +722,12 @@ static bool utrace_reset(struct task_str
> __releases(utrace->lock)
> {
> struct utrace_engine *engine, *next;
> + bool xxx = (utrace->stopped || task == current);
> unsigned long flags = 0;
> LIST_HEAD(detached);
>
> - splice_attaching(utrace);
> -
> + if (xxx)
> + splice_attaching(utrace);
> /*
> * Update the set of events of interest from the union
> * of the interests of the remaining tracing engines.
> @@ -734,7 +735,7 @@ static bool utrace_reset(struct task_str
> * We'll collect them on the detached list.
> */
> list_for_each_entry_safe(engine, next, &utrace->attached, entry) {
> - if (engine->ops == &utrace_detached_ops) {
> + if (xxx && engine->ops == &utrace_detached_ops) {
> engine->ops = NULL;
> list_move(&engine->entry, &detached);
> } else {
> @@ -765,7 +766,7 @@ static bool utrace_reset(struct task_str
> */
> utrace->interrupt = utrace->report = utrace->signal_handler = 0;
>
> - if (!(flags & ENGINE_STOP) && task != current)
> + if (!(flags & ENGINE_STOP) && utrace->stopped)
> /*
> * No more engines want it stopped. Wake it up.
> */
