Messages in this thread | | | Date | Wed, 8 Sep 2010 14:44:06 +0200 | From | Oleg Nesterov <> | Subject | Re: [patch 3/3] audit: Use rcu for task lookup protection |
| |
On 09/07, Thomas Gleixner wrote: > > On Tue, 7 Sep 2010, Oleg Nesterov wrote: > > > But, sorry, can't resists ;) off-topic nit. > > > > > @@ -873,17 +873,16 @@ static int audit_receive_msg(struct sk_b > > > case AUDIT_TTY_GET: { > > > struct audit_tty_status s; > > > struct task_struct *tsk; > > > + unsigned long flags; > > > > > > - read_lock(&tasklist_lock); > > > + rcu_read_lock(); > > > tsk = find_task_by_vpid(pid); > > > - if (!tsk) > > > - err = -ESRCH; > > > - else { > > > - spin_lock_irq(&tsk->sighand->siglock); > > > + if (tsk && lock_task_sighand(tsk, &flags)) { > > > s.enabled = tsk->signal->audit_tty != 0; > > > > Yes, this is what original code does, it takes ->siglock every time > > around read/write of ->audit_tty. And this looks absolutely bogus. > > Say, tty_audit_fork(). Why does it take ->siglock ?
Yes, I still think ->audit_tty doesn't need the locking.
> > As for ->tty_audit_buf, I am not sure ->siglock is the best choice, > > perhaps task_lock() would be better.
OOPS, I misread the code. ->tty_audit_buf is per-process (of course!). Well, unless I missed something again, tty_audit_push() and tty_audit_tiocsti() can access ->tty_audit_buf lockless.
> > Once again, I think the patch is fine. Just it seems to me this code > > needs more cleanups. > > Yeah, thought about that, but that's not in the scope of what I was > working on. I leave that to the audit folks. :)
Yes, yes, sure.
Oleg.
| |