Messages in this thread | | | Date | Sat, 14 Oct 2006 11:06:55 -0400 | From | Prarit Bhargava <> | Subject | Re: [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/
| |