On 10/07, Roland McGrath wrote:
>
> > +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 (!lock_task_sighand(tracee, &flags))
> > + return err;
> > + /*
> > + * Make sure the compiler reads ->siginfo only once, if we race
> > + * with SIGKILL ->siginfo can be cleared under us. But the memory
> > + * it points to can't go away, and since we hold ->siglock we can't
> > + * race with get_signal_to_deliver() pathes clobber this memory.
>
> s/pathes clobber/paths that clobber/
Thanks,
> > + * See also the comment in ptrace_report_signal().
> > + */
> > + context_info = ACCESS_ONCE(context->siginfo);
> > + if (context_info) {
> > + if (write)
> > + *context_info = *info;
> > + else
> > + *info = *context_info;
> > + err = 0;
> > + }
> > + unlock_task_sighand(tracee, &flags);
>
> Couldn't we instead do:
>
> context_info = ACCESS_ONCE(context->siginfo);
> if (context_info) {
> *info = *context_info;
> smp_rmb();
> if (ACCESS_ONCE(context->siginfo))
> err = 0;
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.
I thought about
*info = *context_info;
rmb();
if (fatal_ignal_pending(tracee))
return -ERR;
But I think it is better to do theses cleanups after V1. See also
my reply to next reviews.
Oleg.
