linux-kernel - Re: [PATCH 3/3] ptrace_untrace: fix the SIGNAL_STOP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sun,  8 Feb 2009 19:59:27 -0800 (PST)
From:	Roland McGrath <roland@...hat.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Jerome Marchand <jmarchan@...hat.com>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] ptrace_untrace: fix the SIGNAL_STOP_STOPPED check

> Because we have another case. The group stop is in progress, and some
> thread T does do_signal_stop()->finish_stop(). It is TASK_STOPPED.
> Now we do PTRACE_ATTACH + PTRACE_DETACH. And the second sys_ptrace()
> changes T->state to TASK_TRACED.

There is no problem here.  The original TASK_STOPPED and the new
TASK_TRACED count the same as far as group-stop accounting.

> And. It it also possible that we ptrace the single sub-thread, then
> the group stop starts. The first thread which enters do_signal_stop()
> will not count the TASK_TRACED child, so it should stay stopped.

As it will, by checking for group_stop_count || SIGNAL_STOP_STOPPED.

> Perhaps this is just my misunderstanding, but
> 
> 	/*
> 	 * Turn a tracing stop into a normal stop now, since with no tracer there
> 	 * would be no way to wake it up with SIGCONT or SIGKILL.
> 
> This looks as if we always do /TRACED/STOPPED/ unconditionally.

... until you read the next sentence that describes the other case.

> 	                                                           If there was a
> 	 * signal sent that would resume the child, but didn't because it was in
> 	 * TASK_TRACED, resume it now.
> 
> No, we resume it not because it may have signals, and we don't even check
> it has pending signals.

The comment is accurate: it doesn't have anything to do with pending signals.
An ignored SIGCONT "was a signal sent that would resume the child", but is
not pending.  (Likewise a caught SIGCONT already dequeued by another thread.)
SIGNAL_STOP_STOPPED being clear is what indicates that "there was a signal
sent that would resume the child".  (It is prepare_signal() that would have
resumed the task, not any present or past state of pendingness of any signal.)

> 	 * Requires that irqs be disabled.
> 	 */
> 
> this is correct ;)

... too. ;-)

Seriously, feel free to rewrite comments so they are more unambiguous.  
But this one is not incorrect when interpreted as I do (as it was
intended), just apparently ambiguous to some eyes such as yours.
The purpose of comments is to be clear to everyone, so change may be
warranted.  But clarity of unambiguous expression is not found in
interpreting ambiguity as being unambiguously clear and wrong.

Thanks,
Roland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/