[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: pthread_kill is racy: probably needs kernel change



> this is buggy - it leads to a deadlock/lockup, because the tasklist_lock
> is already held at this point and get_user() might sleep/block.

Here is a new version of the patch that fixes this problem and creates
a new syscall, sys_tkilldet, with number 258.

The NPTL patch uses the new syscall, optimizes the sig == 0 case and
corrects the return values of pthread_kill and pthread_cancel.

Please tell me whether this is acceptable, so that I can submit this
to Linus/LKML.


NPTL patch:

diff -urNdp nptl/pthread_cancel.c nptl_ldb/pthread_cancel.c
--- nptl/pthread_cancel.c	2002-10-09 00:50:11.000000000 +0200
+++ nptl_ldb/pthread_cancel.c	2002-11-04 17:19:15.000000000 +0100
@@ -17,6 +17,7 @@
    Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
    02111-1307 USA.  */
 
+#include <errno.h>
 #include <signal.h>
 #include "pthreadP.h"
 #include "atomic.h"
@@ -28,6 +29,9 @@ pthread_cancel (th)
 {
   volatile struct pthread *pd = (volatile struct pthread *) th;
 
+  if(!pd->tid)
+	  return ESRCH;
+  
   while (1)
     {
       int oldval = pd->cancelhandling;
@@ -46,9 +50,7 @@ pthread_cancel (th)
 	{
 	  /* The cancellation handler will take care of marking the
 	     thread as canceled.  */
-	  pthread_kill (th, SIGCANCEL);
-
-	  break;
+	  return pthread_kill (th, SIGCANCEL);
 	}
 
       /* Mark the thread as canceled.  This has to be done
diff -urNdp nptl/sysdeps/unix/sysv/linux/pthread_kill.c nptl_ldb/sysdeps/unix/sysv/linux/pthread_kill.c
--- nptl/sysdeps/unix/sysv/linux/pthread_kill.c	2002-09-19 11:57:31.000000000 +0200
+++ nptl_ldb/sysdeps/unix/sysv/linux/pthread_kill.c	2002-11-04 17:17:45.000000000 +0100
@@ -31,6 +31,10 @@ pthread_kill (threadid, signo)
 {
   struct pthread *pd = (struct pthread *) threadid;
 
+  /* Don't call the kernel just to check if it's running */
+  if(!signo)
+	  return pd->tid ? 0 : ESRCH;
+  
   /* We have a special syscall to do the work.  */
-  return INLINE_SYSCALL (tkill, 2, pd->tid, signo);
+  return -INLINE_SYSCALL (tkilldet, 2, &pd->tid, signo);
 }



Linux patch:

--- linux-2.5.44/kernel/signal.c	2002-11-01 13:32:32.000000000 +0100
+++ linux-2.5.44_ldb/kernel/signal.c	2002-11-04 18:04:17.000000000 +0100
@@ -21,6 +21,7 @@
 #include <asm/param.h>
 #include <asm/uaccess.h>
 #include <asm/siginfo.h>
+#include <linux/mm.h>
 
 /*
  * SLAB caches for signal bits.
@@ -1576,6 +1577,137 @@ sys_tkill(int pid, int sig)
 	return error;
 }
 
+static inline struct page *__pin_page(unsigned long addr)
+{
+	struct mm_struct *mm = current->mm;
+	struct page *page, *tmp;
+	int err;
+
+	/*
+	 * Do a quick atomic lookup first - this is the fastpath.
+	 */
+	spin_lock(&mm->page_table_lock);		
+	page = follow_page(mm, addr, 0);
+	
+	if (likely(page != NULL)) {	
+		if (!PageReserved(page))
+			get_page(page);
+		spin_unlock(&mm->page_table_lock);
+		return page;
+	}
+
+	/*
+	 * No luck - need to fault in the page:
+	 */
+repeat_lookup:
+	spin_unlock(&mm->page_table_lock);	
+
+	/* with highmem it's already down */
+#ifndef CONFIG_HIGHMEM
+	down_read(&mm->mmap_sem);
+#endif
+	
+	err = get_user_pages(current, mm, addr, 1, 0, 0, &page, NULL);
+	
+#ifndef CONFIG_HIGHMEM	
+	up_read(&mm->mmap_sem);
+#endif
+	
+	if (err < 0)
+		return NULL;
+
+	spin_lock(&mm->page_table_lock);	
+	/*
+	 * Since the faulting happened with locks released, we have to
+	 * check for races:
+	 */
+	tmp = follow_page(mm, addr, 0);
+	if (tmp != page) {
+		put_page(page);
+		goto repeat_lookup;
+	}
+	spin_unlock(&mm->page_table_lock);
+
+	return page;
+}
+
+/*
+ *  Send a signal to only one task, even if it's a CLONE_THREAD task.
+ *  Do so without races for CLONE_DETACHED | CLONE_CLEARTID taks
+ */
+asmlinkage long
+sys_tkilldet(unsigned long pid_addr, int sig)
+{
+	struct siginfo info;
+	int error;
+	struct task_struct *p;
+	int pid;
+	struct page* page;
+	unsigned off;
+
+	/* Must be "naturally" aligned */	
+	if (pid_addr % sizeof(int))
+		return -EINVAL;
+	if (pid_addr >= TASK_SIZE)
+		return -EFAULT;
+
+	info.si_signo = sig;
+	info.si_errno = 0;
+	info.si_code = SI_TKILL;
+	info.si_pid = current->pid;
+	info.si_uid = current->uid;
+	
+	/* need to do this since we must read
+	   from userspace with the tasklist lock held to prevent the
+	   CLONE_DETACHED task from vanishing under us (if it is
+	   already vanished CLONE_CLEARTID will have cleared the *ppid
+	   value).
+
+	   If !highmem we read from the kernel physical memory mapping,
+	   otherwise we down the mm sem and read from userspace.
+	*/
+
+#ifdef CONFIG_HIGHMEM
+	down_read(&current->mm->mmap_sem);
+#endif	
+	off = pid_addr % PAGE_SIZE;
+	page = __pin_page(pid_addr - off);
+	
+	if(!page)
+		goto out_efault;
+
+	read_lock(&tasklist_lock);
+#ifdef CONFIG_HIGHMEM	
+	__get_user(pid, (int*)pid_attr);
+	up_read(&current->mm->mmap_sem);
+#else
+	pid = *(int*)((char*)page_address(page) + off);
+#endif
+	put_page(page);
+	
+	/* This is only valid for single tasks */	
+	error = -EINVAL;
+	if (pid < 0)
+		goto out_unlock;
+
+	error = -ESRCH;
+	if(pid && (p = find_task_by_pid(pid))) {	
+		spin_lock_irq(&p->sig->siglock);
+		error = specific_send_sig_info(sig, &info, p, 0);
+		spin_unlock_irq(&p->sig->siglock);
+	}
+
+  out_unlock:
+	read_unlock(&tasklist_lock);
+	return error;
+
+  out_efault:
+#ifdef CONFIG_HIGHMEM	
+	up_read(&current->mm->mmap_sem);
+#endif	
+	return -EFAULT;
+}
+
 asmlinkage long
 sys_rt_sigqueueinfo(int pid, int sig, siginfo_t *uinfo)
 {
--- linux-2.5.45-bk20021104/include/asm-i386/unistd.h	2002-11-04 18:20:07.000000000 +0100
+++ linux-2.5.44_ldb/include/asm-i386/unistd.h	2002-11-04 18:18:31.000000000 +0100
@@ -262,6 +262,7 @@
 #define __NR_sys_epoll_ctl	255
 #define __NR_sys_epoll_wait	256  
 #define __NR_remap_file_pages	257
+#define __NR_tkilldet		258
 
 /* user-visible error numbers are in the range -1 - -124: see <asm-i386/errno.h> */
 
--- linux-2.5.45-bk20021104/arch/i386/kernel/entry.S	2002-11-04 18:28:16.000000000 +0100
+++ linux-2.5.44_ldb/arch/i386/kernel/entry.S	2002-11-04 18:28:03.000000000 +0100
@@ -741,6 +741,7 @@ ENTRY(sys_call_table)
 	.long sys_epoll_ctl	/* 255 */
 	.long sys_epoll_wait
 	.long sys_remap_file_pages
+	.long sys_tkilldet
 
 	.rept NR_syscalls-(.-sys_call_table)/4
 		.long sys_ni_syscall

Attachment: pgp00042.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]