Re: more signal locking bugs?

[Manfred Spraul]

On Sun, 16 Feb 2003, Linus Torvalds wrote:

> 
> On Sun, 16 Feb 2003, Manfred Spraul wrote:
> >
> > ABBA is not a deadlock, because linux read_locks permit recursive calls.
> > 
> >     read_lock(tasklist_lock);
> >     task_lock(tsk);
> >     read_lock(tasklist_lock);
> > 
> > Does not deadlock, nor any other ordering.
> > 
> > The tasklist_lock is never taken for write from bh or irq context.
> 
> Doesn't matter.
> 
> 	CPU1 - do_exit()	)
> 	write_lock_irq(&tasklist_lock);	
> 	task_lock(tsk);	
> 
> BOOM, you're dead.
> 
> See? ABBA _does_ happen with the task lock.
>
But these lines are not in 2.4 or 2.5.61.
The current rule to nesting tasklist_lock and task_lock is
- read_lock(&tasklist_lock) and task_lock can be mixed in any order.
- write_lock_irq(&tasklist_lock) and task_lock are incompatible.

What about this minimal patch? The performance critical operation is 
signal delivery - we should fix the synchronization between signal 
delivery and exec first.

--
	Manfred
<<
--- 2.5/fs/proc/array.c	2003-02-15 10:29:22.000000000 +0100
+++ build-2.5/fs/proc/array.c	2003-02-16 20:58:37.000000000 +0100
@@ -208,23 +208,27 @@
 {
 	sigset_t ign, catch;
 
-	buffer += sprintf(buffer, "SigPnd:\t");
-	buffer = render_sigset_t(&p->pending.signal, buffer);
-	*buffer++ = '\n';
-	buffer += sprintf(buffer, "ShdPnd:\t");
-	buffer = render_sigset_t(&p->signal->shared_pending.signal, buffer);
-	*buffer++ = '\n';
-	buffer += sprintf(buffer, "SigBlk:\t");
-	buffer = render_sigset_t(&p->blocked, buffer);
-	*buffer++ = '\n';
-
-	collect_sigign_sigcatch(p, &ign, &catch);
-	buffer += sprintf(buffer, "SigIgn:\t");
-	buffer = render_sigset_t(&ign, buffer);
-	*buffer++ = '\n';
-	buffer += sprintf(buffer, "SigCgt:\t"); /* Linux 2.0 uses "SigCgt" */
-	buffer = render_sigset_t(&catch, buffer);
-	*buffer++ = '\n';
+	read_lock(&tasklist_lock);
+	if (p->signal && p->sighand) {
+		buffer += sprintf(buffer, "SigPnd:\t");
+		buffer = render_sigset_t(&p->pending.signal, buffer);
+		*buffer++ = '\n';
+		buffer += sprintf(buffer, "ShdPnd:\t");
+		buffer = render_sigset_t(&p->signal->shared_pending.signal, buffer);
+		*buffer++ = '\n';
+		buffer += sprintf(buffer, "SigBlk:\t");
+		buffer = render_sigset_t(&p->blocked, buffer);
+		*buffer++ = '\n';
+
+		collect_sigign_sigcatch(p, &ign, &catch);
+		buffer += sprintf(buffer, "SigIgn:\t");
+		buffer = render_sigset_t(&ign, buffer);
+		*buffer++ = '\n';
+		buffer += sprintf(buffer, "SigCgt:\t"); /* Linux 2.0 uses "SigCgt" */
+		buffer = render_sigset_t(&catch, buffer);
+		*buffer++ = '\n';
+	}
+	read_unlock(&tasklist_lock);
 
 	return buffer;
 }
<<