[PATCH] Re: [Bug 7210] New: Clone flag CLONE_PARENT_TIDPTR leaves invalid results in memory.

[Daniel Jacobowitz]

From: Daniel Jacobowitz <dan@xxxxxxxxxxxxxxxx>

Do not implement CLONE_PARENT_SETTID until we know that clone will succeed.
If we do it too early NPTL's data structures temporarily reference a
non-existant TID.

Signed-off-by: Daniel Jacobowitz <dan@xxxxxxxxxxxxxxxx>

---
On Tue, Sep 26, 2006 at 08:59:15PM -0700, Linus Torvalds wrote:
> 
> 
> On Tue, 26 Sep 2006, Roland McGrath wrote:
> >
> > It can go last, right before return, after unlock.
> > Userland only cares that parent_tidptr set before parent syscall returns,
> > and child_tidptr set before child returns.
> 
> Ok, as long as people are sure, I don't care. Then we have to just ignore 
> the error, though, since we can't recover (we've already "exposed" the 
> child on the task lists).
> 
> I don't think it's a big deal. Ignoring the error just means that if you 
> pass in an invalid ptr, it's as if the bit to set that value wasn't set. 
> Not a problem.
> 
> Especially if there is a test-program, can we just have a patch to try 
> that has been verified? It _sounded_ like somebody actually had a program 
> that could trigger this with some horrid code that sent signals and cloned 
> all the time?

I never got back to you about this...

Refresher, if there isn't enough above: CLONE_PARENT_SETTID is
currently implemented right after a TID is assigned.  There's a lot of
clone left to go at that point including a check for pending signals
which can lead to clone failing.  This leaves a TID in NPTL's thread
list which doesn't correspond to a thread.

I found Sunday another place where this is a problem, besides the
process-global UID stuff in glibc.  GDB tries to attach to the
nonexistant thread and gets upset.  I've made it cope, but at the same
time it provides a convenient test case.

Without the attached patch, tls.exp in the GDB testsuite would
intermittently report that it could not attach to a thread - always
within half an hour.  With the patch it ran for four hours without
a problem.

 kernel/fork.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Index: linux-source-2.6.18/kernel/fork.c
===================================================================
--- linux-source-2.6.18.orig/kernel/fork.c	2007-01-02 13:45:28.000000000 -0500
+++ linux-source-2.6.18/kernel/fork.c	2007-01-02 13:52:09.000000000 -0500
@@ -1012,10 +1012,6 @@ static struct task_struct *copy_process(
 	delayacct_tsk_init(p);	/* Must remain after dup_task_struct() */
 	copy_flags(clone_flags, p);
 	p->pid = pid;
-	retval = -EFAULT;
-	if (clone_flags & CLONE_PARENT_SETTID)
-		if (put_user(p->pid, parent_tidptr))
-			goto bad_fork_cleanup_delays_binfmt;
 
 	INIT_LIST_HEAD(&p->children);
 	INIT_LIST_HEAD(&p->sibling);
@@ -1251,6 +1247,14 @@ static struct task_struct *copy_process(
 	total_forks++;
 	spin_unlock(&current->sighand->siglock);
 	write_unlock_irq(&tasklist_lock);
+
+	/*
+	 * Now that we know the fork has succeeded, record the new
+	 * TID.  It's too late to back out if this fails.
+	 */
+	if (clone_flags & CLONE_PARENT_SETTID)
+		put_user(p->pid, parent_tidptr);
+
 	proc_fork_connector(p);
 	return p;
 
@@ -1281,7 +1285,6 @@ bad_fork_cleanup_policy:
 bad_fork_cleanup_cpuset:
 #endif
 	cpuset_exit(p);
-bad_fork_cleanup_delays_binfmt:
 	delayacct_tsk_free(p);
 	if (p->binfmt)
 		module_put(p->binfmt->module);

-- 
Daniel Jacobowitz
CodeSourcery
-
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/