lkml.org 
[lkml]   [2006]   [Oct]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH]: disassociate tty locking fixups
Alan Cox wrote:

>Ugly but I don't think the patches are sufficient. Firstly you need to
>hold the task lock if you are poking around some other users ->signal,
>or that may itself change. (disassociate_ctty seems to have this wrong)

Ah -- okay.

So the locking order is (for example):

mutex_lock(&tty_mutex);
read_lock(&tasklist_lock);
task_lock(current);

Correct?

>Secondly you appear to have lock ordering issues (you lock tty_mutex in
>both orders relative to the task list lock) (you take tty_mutex first,
>then the task lock which is correct, but then you drop and retake the
>tty_mutex while holding the task lock, which may deadlock)

Fixed.

>Can you also explain why the ctty change proposed is neccessary ?

disassociate_ctty can call tty_vhangup which calls do tty_hangup directly.
do_tty_hangup can then set p->signal->tty = NULL, and after returning to
disassociate_ctty, the value of tty will now contain a bad pointer. (I can
reproduce this behaviour by running the gdb testsuite with slab debug on)

>NAK the actual code, provisionally agree with the basic diagnosis of
>insufficient locking.

Arjan wrote:

>in addition, are you sure you don't need to revalidate anything after
>retaking the lock?

The only place I need to revalidate data (AFAICT) is in
disassociate_ctty where
I re-check to see if current->signal->tty is still valid. Admittedly, I am
looking at a very specific failure path though.

I'll rework the patch and post later.

P.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-10-14 17:09    [W:0.160 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site