Messages in this thread | | | Date | Wed, 18 Oct 2006 20:55:58 +0400 | From | Oleg Nesterov <> | Subject | Re: [RFC][PATCH] ->signal->tty locking |
| |
On 10/17, Peter Zijlstra wrote: > > How about something like this; I'm still shaky on the lifetime rules of > tty objects, I'm about to add a refcount and spinlock/mutex to > tty_struct, this is madness....
Sorry for delay, a couple of minor nits...
> static void do_tty_hangup(void *data) > { > @@ -1355,14 +1355,18 @@ static void do_tty_hangup(void *data) > read_lock(&tasklist_lock); > if (tty->session > 0) { > do_each_task_pid(tty->session, PIDTYPE_SID, p) { > + spin_lock_irq(&p->sighand->siglock); > if (p->signal->tty == tty) > p->signal->tty = NULL; > - if (!p->signal->leader) > + if (!p->signal->leader) { > + spin_unlock_irq(&p->sighand->siglock); > continue; > - group_send_sig_info(SIGHUP, SEND_SIG_PRIV, p); > - group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p); > + } > + __group_send_sig_info(SIGHUP, SEND_SIG_PRIV, p); > + __group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p);
So we are skipping security_task_kill() and audit_signal_info(). I don't claim this is bad, I just don't know.
> @@ -2899,6 +2919,7 @@ static int tiocsctty(struct tty_struct * > */ > if (!current->signal->leader || current->signal->tty) > return -EPERM; > + mutex_lock(&tty_mutex);
This is still racy (consider 2 threads doing tiocsctty() at the same time), probably it is better to take tty_mutex before the check?
> --- linux-2.6.18.noarch.orig/include/linux/tty.h > +++ linux-2.6.18.noarch/include/linux/tty.h > @@ -338,5 +338,33 @@ static inline dev_t tty_devnum(struct tt > return MKDEV(tty->driver->major, tty->driver->minor_start) + tty->index; > } > > +static inline void proc_set_tty(struct task_struct *p, struct tty_struct *tty) > +{ > + spin_lock_irq(&p->sighand->siglock); > + p->signal->tty = tty; > + spin_unlock_irq(&p->sighand->siglock); > +}
Note that it is always called with tty == NULL parameter. That is why I proposed proc_clear_tty(struct task_struct *p). We can't use this helper for tiocsctty/tty_open anyway.
> +static inline void session_clear_tty(pid_t session) > +{ > + struct task_struct *p; > + do_each_task_pid(session, PIDTYPE_SID, p) { > + proc_set_tty(p, NULL); > + } while_each_task_pid(session, PIDTYPE_SID, p); > +} > +
I'd suggest to move it to tty_io.c and make it static (not inline).
> =================================================================== > --- linux-2.6.18.noarch.orig/security/selinux/hooks.c > +++ linux-2.6.18.noarch/security/selinux/hooks.c > @@ -1708,9 +1708,10 @@ static inline void flush_unauthorized_fi > struct tty_struct *tty; > struct fdtable *fdt; > long j = -1; > + int drop_tty = 0; > > mutex_lock(&tty_mutex); > - tty = current->signal->tty; > + tty = current_get_tty(); > if (tty) { > file_list_lock(); > file = list_entry(tty->tty_files.next, typeof(*file), f_u.fu_list); > @@ -1723,12 +1724,18 @@ static inline void flush_unauthorized_fi > struct inode *inode = file->f_dentry->d_inode; > if (inode_has_perm(current, inode, > FILE__READ | FILE__WRITE, NULL)) { > - /* Reset controlling tty. */ > - current->signal->tty = NULL; > - current->signal->tty_old_pgrp = 0; > + drop_tty = 1; > } > } > file_list_unlock(); > + > + if (drop_tty) { > + /* Reset controlling tty. */ > + spin_lock_irq(¤t->sighand->siglock); > + current->signal->tty = NULL; > + current->signal->tty_old_pgrp = 0;
Probably the last line should go to proc_clear_tty() ?
On the other hand, when signal->tty != NULL, ->tty_old_pgrp should be == 0, may be it is unneeded.
In any case, I think we should use proc_set_tty() here.
Oleg.
- 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/
| |