Re: Q: utrace_reset() && UTRACE_EVENT(REAP)

[Roland McGrath]

> > Wait, what?  You just said that set_notify_resume() already implies an mb().
> 
> Yes, but the other side lacks a barrier. UTRACE_REPORT does
> 
>       utrace->report = 1;
>       wmb(); // actually mb, but wmb is enough
>       set _TIF_NOTIFY_RESUME;
> 
> do_notify_resume()->utrace_resume()->start_report() path does
> 
>       if (_TIF_NOTIFY_RESUME)
>               // !!! we need rmb in between !!!
>               if (utrace->report)
>                       ...
> 
> and it can miss ->report.

I see.  We have a similar problem for (the first) attach, too, right?
utrace_add_engine does:

        utrace_flags |= UTRACE_EVENT(REAP);
        utrace->report = 1;
        wmb(); // actually mb, but wmb is enough
        set _TIF_NOTIFY_RESUME;

do_notify_resume()->tracehook_notify_resume() path does:

        if (_TIF_NOTIFY_RESUME)
                // !!! we need rmb in between !!!
                if (utrace_flags != 0)
                        utrace_resume()

This is what I put in (4d8a6fd6):

--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -616,6 +616,12 @@ static inline void set_notify_resume(struct task_struct 
*task)
 static inline void tracehook_notify_resume(struct pt_regs *regs)
 {
        struct task_struct *task = current;
+       /*
+        * This pairs with the barrier implicit in set_notify_resume().
+        * It ensures that we read the nonzero utrace_flags set before
+        * set_notify_resume() was called by utrace setup.
+        */
+       smp_rmb();
        if (task_utrace_flags(task))
                utrace_resume(task, regs);
 }


Thanks,
Roland