[lkml]   [2006]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: + audit-accounting-tty-locking.patch added to -mm tree
On 09/05, Andrew Morton wrote:
> Subject: audit/accounting: tty locking
> From: Alan Cox <>
> Add tty locking around the audit and accounting code.
> The whole current->signal-> locking is all deeply strange but it's for
> someone else to sort out. Add rather than replace the lock for acct.c

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().

But I don't understand ->signal->tty locking. I know nothing about job
control, looking into the tty_io.c for the first time.

->tty is set under task_lock()

->tty is cleared under lock_kernel() + tasklist_lock

except TIOCNOTTY, cleared under task_lock()

Note that include/linux/sched.h doesn't document that ->alloc_lock
protects ->tty, it is only used in tty_io.c for that purpose, why?

->tty is cleared under tty_mutex

->tty is cleared under tty_mutex + tasklist_lock

Btw, I think tiocsctty()/tty_open() is racy wrt to sys_setsid().
tiocsctty() can see the result of '->signal->leader = 1' before
sys_setsid() changed ->session/->pgrp and passed '->tty = NULL'.

I think tiocsctty() and tty_open() need tty_mutex, no?

> --- a/kernel/acct.c~audit-accounting-tty-locking
> +++ a/kernel/acct.c
> @@ -483,10 +483,14 @@ static void do_acct_process(struct file
> ac.ac_ppid = current->parent->tgid;
> #endif
> - read_lock(&tasklist_lock); /* pin current->signal */
> + mutex_lock(&tty_mutex);
> + /* FIXME: Whoever is responsible for current->signal locking needs
> + to use the same locking all over the kernel and document it */
> + read_lock(&tasklist_lock);
> ac.ac_tty = current->signal->tty ?
> old_encode_dev(tty_devnum(current->signal->tty)) : 0;
> read_unlock(&tasklist_lock);
> + mutex_unlock(&tty_mutex);

I think the purpose of read_lock(&tasklist_lock) was to protect ->tty,
not ->signal, so I believe this comment is not correct.

Am I understand correctly? tasklist_lock is not enough, ->tty could be
cleared by ioctl(TIOCNOTTY). tty_mutex can't protect from changing ->tty
pointer, but it protects from releasing tty_struct, yes?

What about ->tty usage in do_task_stat() then ?

> --- a/kernel/auditsc.c~audit-accounting-tty-locking
> +++ a/kernel/auditsc.c
> @@ -766,6 +766,8 @@ static void audit_log_exit(struct audit_
> audit_log_format(ab, " success=%s exit=%ld",
> (context->return_valid==AUDITSC_SUCCESS)?"yes":"no",
> context->return_code);
> +
> + mutex_lock(&tty_mutex);
> if (tsk->signal && tsk->signal->tty && tsk->signal->tty->name)

Ugh. I think this is ok, but I know nothing about audit.c. grep, grep ...
This 'tsk' should be == current. Probably we need a comment.

copy_process() calls audit_free() after cleanup_signal() (frees ->signal),
but in that case context->in_syscall can't be true, so audit_log_exit()
is not called, yes ?


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at
Please read the FAQ at

 \ /
  Last update: 2006-09-06 01:17    [W:0.024 / U:1.288 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site