linux-kernel - Re: [PATCH 4/4] posix-timers: turn it_signal into it

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 6 Sep 2011 21:45:58 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Oleg Nesterov <oleg@...hat.com>
cc:	Eric Dumazet <eric.dumazet@...il.com>,
	Andi Kleen <ak@...ux.intel.com>,
	Andi Kleen <andi@...stfloor.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 4/4] posix-timers: turn it_signal into it_valid flag

On Tue, 6 Sep 2011, Oleg Nesterov wrote:

> On 09/06, Thomas Gleixner wrote:
> >
> > On Tue, 6 Sep 2011, Oleg Nesterov wrote:
> > >
> > > The problem is, it can be already dequeued.
> >
> > Right, but we can solve this by moving the whole detach code into rcu.
> 
> Hmm, I don't understand...
> 
> > --- linux-2.6.orig/kernel/posix-timers.c
> > +++ linux-2.6/kernel/posix-timers.c
> > @@ -495,22 +495,30 @@ static void k_itimer_rcu_free(struct rcu
> >  {
> >  	struct k_itimer *tmr = container_of(head, struct k_itimer, it.rcu);
> >  
> > +	put_pid(tmr->it_pid);
> > +	sigqueue_free(tmr->sigq);
> >  	kmem_cache_free(posix_timers_cache, tmr);
> 
> Why do we need to move put_pid/sigqueue_free ?
> 
> The caller of release_posix_timer() should cancel the timer, we can can
> do this even before idr_remove() with or without this patch.

All callers cancel the timer. Right, put_pid() and sigqueue_free() can
stay where they are.
 
> >  static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
> >  {
> > -	if (it_id_set) {
> > -		unsigned long flags;
> > -		spin_lock_irqsave(&idr_lock, flags);
> > -		idr_remove(&posix_timers_id, tmr->it_id);
> > -		spin_unlock_irqrestore(&idr_lock, flags);
> > -	}
> > -	put_pid(tmr->it_pid);
> > -	sigqueue_free(tmr->sigq);
> > -	call_rcu(&tmr->it.rcu, k_itimer_rcu_free);
> > +	if (it_id_set)
> > +		call_rcu(&tmr->it.rcu, k_itimer_rcu_free_idr);
> 
> But how this can help? Suppose that the task is preempted right
> after dequeue_signal() drops ->siglock. We need rcu_read_lock()
> before unlock then, no?

Crap, you are right, but that's fortunately an easy to solve one :)

> And. This breaks the accounting logic. I mean the patch from Andi
> which adds the limits.

That's a different problem and really, it does not break it by any
means. When the timer is released, then the count is decreased and we
can safely assume that the memory is going to be freed in the next
grace period. If that's not the case, then we have a totally different
problem which is not fixable by any limits to the number of timers per
process.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/