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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
