Thanks for the review.
Andrew Morton napsal(a):
> On Wed, 06 Jun 2007 12:10:28 +0200 Miloslav Trmac <[email protected]> wrote:
>> +/**
>> + * tty_audit_opening - A TTY is being opened.
>> + *
>> + * As a special hack, tasks that close all their TTYs and open new ones
>> + * are assumed to be system daemons (e.g. getty) and auditing is
>> + * automatically disabled for them.
>> + */
>> +void
>> +tty_audit_opening(void)
>> +{
>> + int disable;
>> +
>> + disable = 1;
>> + spin_lock(¤t->sighand->siglock);
>> + if (current->signal->audit_tty == 0)
>> + disable = 0;
>> + spin_unlock(¤t->sighand->siglock);
>> + if (!disable)
>> + return;
>> +
>> + task_lock(current);
>> + if (current->files) {
>> + struct fdtable *fdt;
>> + unsigned i;
>> +
>> + /*
>> + * We don't take a ref to the file, so we must hold ->file_lock
>> + * instead.
>> + */
>> + spin_lock(¤t->files->file_lock);
>
> So we make file_lock nest inside task_lock(). Was that lock ranking
> already being used elsewhere in the kernel, or is it a new association?
It is used in __do_SAK ().
> Has this code had full coverage testing with all lockdep features enabled?
>
> (I suspect not - lockdep should have gone wild over the siglock thing)
It was not. The new version will be.
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index d13276d..a071a96 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -423,6 +424,32 @@ static int kauditd_thread(void *dummy)
>> return 0;
>> }
>>
>> +static int
>> +audit_prepare_user_tty(pid_t pid, uid_t loginuid)
>> +{
>> + struct task_struct *tsk;
>> + int err;
>> +
>> + read_lock(&tasklist_lock);
>> + tsk = find_task_by_pid(pid);
>> + err = -ESRCH;
>> + if (!tsk)
>> + goto out;
>> + err = 0;
>> +
>> + spin_lock(&tsk->sighand->siglock);
>> + if (!tsk->signal->audit_tty)
>> + err = -EPERM;
>> + spin_unlock(&tsk->sighand->siglock);
> So siglock nests inside tasklist_lock? Sounds reasonable. Is this a
> preexisting association, or did this patch just create it?
This is used in send_sig_info() and several other functions in
kernel/signal.c.
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index d58e74b..3ae4904 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -506,6 +506,8 @@ struct signal_struct {
>> #ifdef CONFIG_TASKSTATS
>> struct taskstats *stats;
>> #endif
>> + unsigned audit_tty:1;
>> + struct tty_audit_buf *tty_audit_buf;
>> };
>
> hm, bitfields are risky. If someone adds another one, it will land in
> the same word and external locking will be needed. You do seem to be using
> ->siglock to cover this - was that to address the bitfield non-atomicity
> problem?
I don't know what the memory access atomicity assumptions are in the
kernel, so I have used the basic rule that any write<->read conflict on
a variable with type other than atomic_t must be prevented by a lock.
This happens to work for the bit field as well.
> A suitable (but somewhat less pretty) way to resolve all this is to not use
> bitfields at all: add `unsigned long flags' and use set_bit/clear_bit/etc.
The new patch replaces the bit field by a simple "unsigned", a whole
word is allocated for the bit field anyway.
>>
>> break;
>> + case AUDIT_TTY_GET: {
>> + struct audit_tty_status s;
>> + struct task_struct *tsk;
>> +
>> + read_lock(&tasklist_lock);
>> + tsk = find_task_by_pid(pid);
>> + if (!tsk)
>> + err = -ESRCH;
>> + else {
>> + spin_lock(&tsk->sighand->siglock);
>> + s.enabled = tsk->signal->audit_tty != 0;
>> + spin_unlock(&tsk->sighand->siglock);
>
> The locking here looks dubious. tsk->signal->audit_tty can change state
> the instant ->siglock gets unlocked, in which case s.enabled is now wrong.
The user-space process must avoid concurrent AUDIT_TTY_SET to get
reasonable results. There's nothing better the kernel can do.
> If that is acceptable then we didn't need that locking at all.
So I can assume that int-sized reads are always atomic with respect to
concurrent writes?
Mirek
-
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]
