copy_signal cleanup: clean tty_audit_fork()
in [Kernel]
|
Prev: locking, x86: Slightly shorten __ticket_spin_trylock()
Next: UBI: flush wl before clearing update marker
From: Oleg Nesterov on 2 Dec 2009 09:30 On 12/01, Veaceslav Falico wrote: > > Remove unneeded initialization in tty_audit_fork(). > > Signed-off-by: Veaceslav Falico <vfalico(a)redhat.com> > --- > > diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c > index ac16fbe..283a15b 100644 > --- a/drivers/char/tty_audit.c > +++ b/drivers/char/tty_audit.c > @@ -148,7 +148,6 @@ void tty_audit_fork(struct signal_struct *sig) > spin_lock_irq(¤t->sighand->siglock); > sig->audit_tty = current->signal->audit_tty; > spin_unlock_irq(¤t->sighand->siglock); > - sig->tty_audit_buf = NULL; > } OK, but given that this function is "far" from copy_signal() path and it is not inline, perhaps it makes sense to add the comment which explains *sig must be zeroed, and the only caller is copy_signal(). Hmm. Off-topic, but why do we take ->siglock? ->audit_tty is boolean, afaics ->siglock buys nothing. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 5 Dec 2009 12:10 On 12/04, Veaceslav Falico wrote: > > Remove unneeded initialization in tty_audit_fork(). > It is called only via copy_signal() and is useless after > the kmem_cache_zalloc() was used. > > Signed-off-by: Veaceslav Falico <vfalico(a)redhat.com> > --- > > diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c > index ac16fbe..283a15b 100644 > --- a/drivers/char/tty_audit.c > +++ b/drivers/char/tty_audit.c > @@ -148,7 +148,6 @@ void tty_audit_fork(struct signal_struct *sig) > spin_lock_irq(¤t->sighand->siglock); > sig->audit_tty = current->signal->audit_tty; > spin_unlock_irq(¤t->sighand->siglock); > - sig->tty_audit_buf = NULL; > } Can't comment the changes in audit code, but the patch looks obviously correct. Off-topic question to this who understands this code. But afaics we can also remove ->siglock from this helper and make it really trivial for being inline. ->siglock buys nothing, we just read a boolean. In fact, after the quick grep I do not understand how ->siglock is connected to ->audit_tty. OK, it protects tty_audit_buf, but why we always take ->siglock to access ->audit_tty ? Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Miloslav Trmac on 5 Dec 2009 15:10 ----- "Oleg Nesterov" <oleg(a)redhat.com> wrote: > On 12/04, Veaceslav Falico wrote: > > diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c > > index ac16fbe..283a15b 100644 > > --- a/drivers/char/tty_audit.c > > +++ b/drivers/char/tty_audit.c > > @@ -148,7 +148,6 @@ void tty_audit_fork(struct signal_struct *sig) > > spin_lock_irq(¤t->sighand->siglock); > > sig->audit_tty = current->signal->audit_tty; > > spin_unlock_irq(¤t->sighand->siglock); > > - sig->tty_audit_buf = NULL; > > } > > Off-topic question to this who understands this code. > > But afaics we can also remove ->siglock from this helper and make > it really trivial for being inline. ->siglock buys nothing, we just > read a boolean. In fact, after the quick grep I do not understand > how ->siglock is connected to ->audit_tty. OK, it protects > tty_audit_buf, > but why we always take ->siglock to access ->audit_tty ? AFAIK there is no explicit documentation of the atomicity semantics expected by the Linux kernel (both from the hardware and from the compiler), so every access to the boolean is protected by a lock, to be on the safe side. Mirek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 6 Dec 2009 10:00 On 12/05, Miloslav Trmac wrote: > > ----- "Oleg Nesterov" <oleg(a)redhat.com> wrote: > > On 12/04, Veaceslav Falico wrote: > > > diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c > > > index ac16fbe..283a15b 100644 > > > --- a/drivers/char/tty_audit.c > > > +++ b/drivers/char/tty_audit.c > > > @@ -148,7 +148,6 @@ void tty_audit_fork(struct signal_struct *sig) > > > spin_lock_irq(¤t->sighand->siglock); > > > sig->audit_tty = current->signal->audit_tty; > > > spin_unlock_irq(¤t->sighand->siglock); > > > - sig->tty_audit_buf = NULL; > > > } > > > > Off-topic question to this who understands this code. > > > > But afaics we can also remove ->siglock from this helper and make > > it really trivial for being inline. ->siglock buys nothing, we just > > read a boolean. In fact, after the quick grep I do not understand > > how ->siglock is connected to ->audit_tty. OK, it protects > > tty_audit_buf, > > but why we always take ->siglock to access ->audit_tty ? > AFAIK there is no explicit documentation of the atomicity semantics > expected by the Linux kernel (both from the hardware and from the compiler), > so every access to the boolean is protected by a lock, to be on the safe side. Not sure I understand, but the kernel relies on fact it is always safe to load/store a word. What atomicity semantics do you mean and how ->siglock can help? Sure, we can race with AUDIT_TTY_SET, but this can happen with or without this lock. This "race" is unavoidable and harmless. I believe every spin_lock(siglock) around ->audit_tty is bogus. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Miloslav Trmac on 6 Dec 2009 10:20 ----- "Oleg Nesterov" <oleg(a)redhat.com> wrote: > On 12/05, Miloslav Trmac wrote: > > > Off-topic question to this who understands this code. > > > > > > But afaics we can also remove ->siglock from this helper and make > > > it really trivial for being inline. ->siglock buys nothing, we just > > > read a boolean. In fact, after the quick grep I do not understand > > > how ->siglock is connected to ->audit_tty. OK, it protects > > > tty_audit_buf, > > > but why we always take ->siglock to access ->audit_tty ? > > AFAIK there is no explicit documentation of the atomicity semantics > > expected by the Linux kernel (both from the hardware and from the compiler), > > so every access to the boolean is protected by a lock, to be on the safe side. > > Not sure I understand, but the kernel relies on fact it is always safe > to load/store a word. And is "word" an "unsigned", "unsigned long" or "intptr_t"? Must it be suitably aligned, and if so, what is "suitably"? Where is this documented? > What atomicity semantics do you mean and how ->siglock can help? At the very least, "any access will read the last value stored and not result in undefined behavior, even if other threads attempt to access the value". In user-space, per POSIX, the only way to guarantee this is using explicit synchronization primitives. > I believe every spin_lock(siglock) around ->audit_tty is bogus. It might very well be. Again, I just wanted to be sure, given my limited understanding of the Linux memory model assumptions. Mirek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Next
|
Last
Pages: 1 2 Prev: locking, x86: Slightly shorten __ticket_spin_trylock() Next: UBI: flush wl before clearing update marker |