Re: PATCH? tracehook_report_clone: fix false positives

[Roland McGrath]

> Firtsly, I don't understand CLONE_PTRACE check. Suppose that untraced
> task does clone(CLONE_PTRACE). In that case we create the untraced
> child (this is correct) but still we send SIGSTOP.
> 
> I do not really know if this bug or not, but this doesn't look right.
> At least this should be commented, imho. And, looking at 2.6.26, I think
> the behaviour was different before tracehooks.
> 
> So, I assume this is bug for now.

You're right.  CLONE_PTRACE when not traced will misbehave (not that anyone
ever uses it).  The old code just checked child->ptrace, and that is fine
to do again now.  I probably changed that thinking it had a race--which it
does--with asynchronous PTRACE_ATTACH after an untraced fork.  But that is
a harmless race as you explained.

ACK on the 2.6.30 patch attached.

> So, I am going to send the patch below. But this leads to another question:
> should not we move these sigaddset() + set_tsk_thread_flag() into
> ptrace_init_task() ?

It might make sense to consolidate them.  But note that ptrace_attach()
uses send_sig_info().  With SEND_SIG_FORCED, this does almost nothing more
than sigaddset() (i.e. no queue entry).  But it does do prepare_signal(),
which will clear any pending SIGCONTs.  It's possible that something in
userland manages to rely on that behavior for the asynchronous attach case
(unrelated to startup-time races).  It wouldn't hurt for the creation-time
case to use send_sig_info() too, though it would go through a bunch more
code to do nothing effectual but sigaddset() in the end.


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