lkml.org 
[lkml]   [2006]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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(&current->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/

\
 
 \ /
  Last update: 2006-10-18 18:59    [W:0.046 / U:1.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site