[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <1161080221.3036.38.camel@taijtu>
Date: Tue, 17 Oct 2006 12:17:01 +0200
From: Peter Zijlstra <a.p.zijlstra@...llo.nl>
To: Oleg Nesterov <oleg@...sign.ru>
Cc: linux-kernel <linux-kernel@...r.kernel.org>,
Prarit Bhargava <prarit@...hat.com>,
Alan Cox <alan@...rguk.ukuu.org.uk>
Subject: Re: [RFC][PATCH] ->signal->tty locking
On Tue, 2006-10-17 at 12:10 +0400, Oleg Nesterov wrote:
> On 10/16, Peter Zijlstra wrote:
> >
> > Oleg wrote:
> > "Historically ->signal/->sighand (both ptrs and their contents) were globally
> > protected by tasklist_lock. 'current' can use these pointers lockless, they
> > can't be changed under him.
> >
> > Nowadays ->signal/->sighand are _also_ protected by ->sighand->siglock.
> > Unless you are current, you can't lock ->siglock directly (without holding
> > tasklist_lock), you should use lock_task_sighand()."
> >
> > Then, to be consistent with the rest of the kernel, ->signal->tty
> > locking should look like so:
> >
> > mutex_lock(&tty_mutex)
> > read_lock(&tasklist_lock)
> > lock_task_sighand(p, &flags)
>
> I've also started similar patches, but have no time to finish it.
>
> I don't think we need tasklist_lock. I think ->sighand->siglock is enough.
Right, sys_unshare() makes tasklist_lock meaningless wrt ->siglock.
> So do_task_stat() doesn't need to take tty_mutex at all.
>
> However, tty_mutex protects ->tty from release_dev(tty), so it is also
> possible to do:
>
> mutex_lock(&tty_mutex);
> tty = task->signal->tty;
> barrier();
> if (tty) {
> // ->tty could be changed/cleared from under us,
> // but it can't be released while we are holding
> // tty_mutex
> do_something(tty);
> }
> ...
Nice, I think we have to convert all those callers like sys_vhangup() to
this form.
> > @@ -1350,20 +1357,26 @@ static void do_tty_hangup(void *data)
> > This should get done automatically when the port closes and
> > tty_release is called */
> >
> > + mutex_lock(&tty_mutex);
>
> I am not sure it is needed.
Right, this would only be needed when using the tty, not when changing
signal->tty.
> > read_lock(&tasklist_lock);
> > if (tty->session > 0) {
> > do_each_task_pid(tty->session, PIDTYPE_SID, p) {
> > + lock_task_sighand(p, &flags);
> > if (p->signal->tty == tty)
> > p->signal->tty = NULL;
> > + unlock_task_sighand(p, &flags);
>
> We don't need lock_task_sighand() here, we can use spin_lock_irq(->siglock).
>
> We are holding tasklist_lock. This means that all tasks found by
> do_each_task_pid() have a valid ->signal/->sighand != NULL.
> tasklist_lock protects against release_task()->__exit_signal() and
> from changing ->sighand by de_thread().
I think sys_unshare() spoils the game here; it changes ->sighand in
midair without holding tasklist_lock. So any ->sighand but current's is
fair game.
Hmm, either sys_unshare() is broken in that it doesn't take the
tasklist_lock or a lot of other code is broken.
let us take send_sig_info() vs. sys_unshare()
1 2
read_lock(&tasklist_lock)
spin_lock_irqsave(&p->sighand->siglock, flags);
rcu_assign_pointer(current->sighand, new_sigh)
spin_unlock_irqsave(&p->sighand->siglock, flags);
read_unlock(&tasklist_lock);
what happens when 2's current is 1's p....
> > @@ -2910,20 +2953,23 @@ static int tiocsctty(struct tty_struct *
> >
> > read_lock(&tasklist_lock);
> > do_each_task_pid(tty->session, PIDTYPE_SID, p) {
> > + lock_task_sighand(p, &flags);
> > p->signal->tty = NULL;
> > + unlock_task_sighand(p, &flags);
> > } while_each_task_pid(tty->session, PIDTYPE_SID, p);
> > read_unlock(&tasklist_lock);
> > - } else
> > + } else {
> > + mutex_unlock(&tty_mutex);
> > return -EPERM;
> > + }
> > }
> > - mutex_lock(&tty_mutex);
> > - task_lock(current);
> > - current->signal->tty = tty;
> > - task_unlock(current);
> > - mutex_unlock(&tty_mutex);
> > - current->signal->tty_old_pgrp = 0;
> > tty->session = current->signal->session;
> > tty->pgrp = process_group(current);
> > + lock_task_sighand(current, &flags);
> > + current->signal->tty = tty;
> > + current->signal->tty_old_pgrp = 0;
> > + unlock_task_sighand(current, &flags);
> > + mutex_unlock(&tty_mutex);
> > return 0;
> > }
>
> There is a very similar code in tty_open(), probably we need another
> helper, proc_set_tty().
>
> But I am not sure about locking. I think we should check
> ->signal->leader/->signal->tty and set ->tty in proc_set_tty()
> under ->siglock, this way we can remove tty_mutex from sys_setsid().
Right, use tty_mutex when using the tty, use ->sighand when changing
signal->tty.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists