On 09/05, Andrew Morton wrote:
>
> Subject: audit/accounting: tty locking
> From: Alan Cox <[email protected]>
>
> 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_io.c:
->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?
daemonize:
->tty is cleared under tty_mutex
sys_setsid:
->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 ?
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]
