linux-kernel - Re: clock_nanosleep() task

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]

Message-ID: <20130206112327.GA1824@redhat.com>
Date:	Wed, 6 Feb 2013 12:23:27 +0100
From:	Stanislaw Gruszka <sgruszka@...hat.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Oleg Nesterov <oleg@...hat.com>,
	Tommi Rantala <tt.rantala@...il.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Dave Jones <davej@...hat.com>,
	John Stultz <john.stultz@...aro.org>
Subject: Re: clock_nanosleep() task_struct leak

On Tue, Feb 05, 2013 at 11:55:19AM +0100, Thomas Gleixner wrote:
> On Tue, 5 Feb 2013, Stanislaw Gruszka wrote:
> > On Mon, Feb 04, 2013 at 08:32:23PM +0100, Oleg Nesterov wrote:
> > > On 02/01, Thomas Gleixner wrote:
> > > >
> > > > B1;2601;0cOn Fri, 1 Feb 2013, Tommi Rantala wrote:
> > > >
> > > > > Hello,
> > > > >
> > > > > Trinity discovered a task_struct leak with clock_nanosleep(), reproducible with:
> > > > >
> > > > > -----8<-----8<-----8<-----
> > > > > #include <time.h>
> > > > >
> > > > > static const struct timespec req;
> > > > >
> > > > > int main(void) {
> > > > >         return clock_nanosleep(CLOCK_PROCESS_CPUTIME_ID,
> > > > >                         TIMER_ABSTIME, &req, NULL);
> > > > > }
> > > > > -----8<-----8<-----8<-----
> > > 
> > > posix_cpu_timer_create()->get_task_struct() I guess...
> > > 
> > > Cough. I am not sure I ever understood this code, but now it certainly
> > > looks as if I never saw it before.
> > 
> > Looks on do_cpu_nanosleep() we call posix_cpu_timer_create(), but we do
> > not call posix_cpu_timer_del() at the end. Fix will not be super simple,
> > since we need to care about error cases. I can cook a patch if nobody
> > else want to do this.
> 
> Would be much appreciated!

Below is proposed fix. Error cases wasn't that bad since there are
various limitations when timer could be fired (i.e. timer which
already fired can not be fired again).

Tommi, please check if patch really fixes the problem. I tested it
with signal interrupt and timeout scenarios, but I don't know how
to confirm if it fix the leak or not.

diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index 125cb67..07a38b6 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -1424,6 +1424,7 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
 				/*
 				 * Our timer fired and was reset.
 				 */
+				posix_cpu_timer_del(&timer);
 				spin_unlock_irq(&timer.it_lock);
 				return 0;
 			}
@@ -1441,9 +1442,17 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
 		 * We were interrupted by a signal.
 		 */
 		sample_to_timespec(which_clock, timer.it.cpu.expires, rqtp);
-		posix_cpu_timer_set(&timer, 0, &zero_it, it);
+		error = posix_cpu_timer_set(&timer, 0, &zero_it, it);
+		if (!error)
+			posix_cpu_timer_del(&timer);
 		spin_unlock_irq(&timer.it_lock);
 
+		while (error == TIMER_RETRY) {
+			spin_lock_irq(&timer.it_lock);
+			error = posix_cpu_timer_del(&timer);
+			spin_unlock_irq(&timer.it_lock);
+		}
+
 		if ((it->it_value.tv_sec | it->it_value.tv_nsec) == 0) {
 			/*
 			 * It actually did fire already.
--
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/