Re: [libvirt] [PATCH] LXC: make sure fuse thread start to run before we do clone

[Gao feng <gaofeng at cn dot fujitsu dot com>]

On 11/09/2013 03:42 AM, Rich Felker wrote:
> On Fri, Nov 08, 2013 at 01:30:09PM +0800, Daniel P. Berrange wrote:
>> On Thu, Nov 07, 2013 at 09:15:43PM +0800, Gao feng wrote:
>>> I met a problem that container blocked by seteuid/setegid
>>> which is call in lxcContainerSetID on UP system and libvirt
>>> compiled with --with-fuse=yes.
>>>
>>> I looked into the glibc's codes, and found setxid in glibc
>>> calls futex() to wait for other threads to change their
>>> setxid_futex to 0(see setxid_mark_thread in glibc).
>>>
>>> since the process created by clone system call will not
>>> share the memory with the other threads and the context
>>> of memory doesn't changed until we call execl.(COW)
>>>
>>> So if the process which created by clone is called before
>>> fuse thread being stated, the new setxid_futex of fuse
>>> thread will not be saw in this process, it will be blocked
>>> forever.
>>>
>>> Maybe this problem should be fixed in glibc, but I send
>>> this patch as a quick fix.
>>
>> Can you show a stack trace of the threads/processes deadlocking
> 
> I think this is a symptom of setxid not being async-signal-safe like
> it's required to be. I'm not sure if we have a bug tracker entry for
> that; if not, it should be added. But if clone() is being used except
> in a fork-like manner, this is probably invalid application usage too.
> 

I post a patch to the glibc community, but I can't find my patch on the
mail list archive. the patch is attached. do you think this glibc patch
is needed or we just should add some bug tracker on manpage?


>From d9c505032bc869e940bd33dd1e37d8568d33951a Mon Sep 17 00:00:00 2001
From: Gao feng <gaofeng@cn.fujitsu.com>
Date: Fri, 8 Nov 2013 09:22:43 +0800
Subject: [PATCH] nptl: fix block problem caused by setxid called from clone

The process created by clone will have a copy of memory of
parent process, if parent creates some thread before call
clone, the stacked_used, __stack_user list is invalid for
the child process created by clone. and if this child process
runs before the threads, the setxid_futex of these threads
will never be changed to 0 for this child process. since
it only has a copied memory. so the setxid will be blocked
by futex forever.

This patch skips operation of the threads in different thread
group.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 nptl/allocatestack.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 1e0fe1f..8fc2591 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -1075,12 +1075,19 @@ __nptl_setxid (struct xid_command *cmdp)
 
   struct pthread *self = THREAD_SELF;
 
+  INTERNAL_SYSCALL_DECL (err);
+  pid_t pid = INTERNAL_SYSCALL(getpid, err, 0);
+
   /* Iterate over the list with system-allocated threads first.  */
   list_t *runp;
   list_for_each (runp, &stack_used)
     {
       struct pthread *t = list_entry (runp, struct pthread, list);
-      if (t == self)
+      /* setxid may be called by the process created by clone,
+       * this process has a copy of parent's memory but it is
+       * running in different thread group, skip the threads
+       * in different thread group. */
+      if (t == self || t->pid != pid)
 	continue;
 
       setxid_mark_thread (cmdp, t);
@@ -1090,7 +1097,7 @@ __nptl_setxid (struct xid_command *cmdp)
   list_for_each (runp, &__stack_user)
     {
       struct pthread *t = list_entry (runp, struct pthread, list);
-      if (t == self)
+      if (t == self || t->pid != pid)
 	continue;
 
       setxid_mark_thread (cmdp, t);
@@ -1106,7 +1113,7 @@ __nptl_setxid (struct xid_command *cmdp)
       list_for_each (runp, &stack_used)
 	{
 	  struct pthread *t = list_entry (runp, struct pthread, list);
-	  if (t == self)
+	  if (t == self || t->pid != pid)
 	    continue;
 
 	  signalled += setxid_signal_thread (cmdp, t);
@@ -1115,7 +1122,7 @@ __nptl_setxid (struct xid_command *cmdp)
       list_for_each (runp, &__stack_user)
 	{
 	  struct pthread *t = list_entry (runp, struct pthread, list);
-	  if (t == self)
+	  if (t == self || t->pid != pid)
 	    continue;
 
 	  signalled += setxid_signal_thread (cmdp, t);
@@ -1135,7 +1142,7 @@ __nptl_setxid (struct xid_command *cmdp)
   list_for_each (runp, &stack_used)
     {
       struct pthread *t = list_entry (runp, struct pthread, list);
-      if (t == self)
+      if (t == self || t->pid != pid)
 	continue;
 
       setxid_unmark_thread (cmdp, t);
@@ -1144,7 +1151,7 @@ __nptl_setxid (struct xid_command *cmdp)
   list_for_each (runp, &__stack_user)
     {
       struct pthread *t = list_entry (runp, struct pthread, list);
-      if (t == self)
+      if (t == self || t->pid != pid)
 	continue;
 
       setxid_unmark_thread (cmdp, t);
@@ -1152,7 +1159,7 @@ __nptl_setxid (struct xid_command *cmdp)
 
   /* This must be last, otherwise the current thread might not have
      permissions to send SIGSETXID syscall to the other threads.  */
-  INTERNAL_SYSCALL_DECL (err);
+
   result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, err, 3,
 				 cmdp->id[0], cmdp->id[1], cmdp->id[2]);
   if (INTERNAL_SYSCALL_ERROR_P (result, err))
-- 
1.8.3.1