[PATCH 62] introduce ptrace_rw_siginfo() helper

[Oleg Nesterov]

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.