Re: more signal locking bugs?

Manfred Spraul
2003-02-16T19:57:53Z

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;
}
<<