Skip to contentSkip to conversation

posix timers: release_posix_timer: kill the bogus put_task_struct(->it_process)

RFC In review Oliver Pinter 4 participants 4 messages Aug 2, 2008 – Aug 3, 2008

lore
Share & tools
lore

Oliver Pinter

author
#0 lore raw

RFC 1/1 posix timers: release_posix_timer: kill the bogus put_task_struct(->it_process)

It is an RFC for sending this patch for stable, when this patch needed, then send ACK and CC stable, if not then send NAK.

---

Quoted 1 line

From 96347e7759e2e433c427defa0fa1adfc8cce6226 Mon Sep 17 00:00:00 2001

From: Oleg Nesterov <[email protected]>
Date: Fri, 25 Jul 2008 01:47:27 -0700
Subject: [PATCH] posix timers: release_posix_timer: kill the bogus put_task_struct(->it_process);

[ Upstream commit 96347e7759e2e433c427defa0fa1adfc8cce6226 ]

release_posix_timer() can't be called with ->it_process != NULL. Once sys_timer_create() sets ->it_process it must not call release_posix_timer(), otherwise we can race with another thread doing sys_timer_delete(), this timer is visible to idr_find() and unlocked.

The same is true for two other callers (actually, for any possible caller), sys_timer_delete() and itimer_delete(). They must clear ->it_process before unlock_timer() + release_posix_timer().

Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Roland McGrath <[email protected]>
Cc john stultz <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Roland McGrath <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>
CC Oliver Pinter <[email protected]>
kernel/posix-timers.c +0 -3
index 17f5326..9a21681 100644
@@ -449,9 +449,6 @@ static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
 		spin_unlock_irqrestore(&idr_lock, flags);
 	}
 	sigqueue_free(tmr->sigq);
-	if (unlikely(tmr->it_process) &&
-	    tmr->it_sigev_notify == (SIGEV_SIGNAL|SIGEV_THREAD_ID))
-		put_task_struct(tmr->it_process);
 	kmem_cache_free(posix_timers_cache, tmr);
 }
#1 lore raw
Quoted 2 lines · On Sat, 2 Aug 2008 23:45:20 +0200 Oliver Pinter…

It is an RFC for sending this patch for stable, when this patch needed, then send ACK and CC stable, if not then send NAK.

what is the bugzilla number or oops / bug description associated with this one?

Signature · 3 lines
If you want to reach me at my work email, use [email protected]
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org
#2 lore raw
In reply to Oliver Pinter
Quoted 2 lines · Oliver Pinter

It is an RFC for sending this patch for stable, when this patch needed, then send ACK and CC stable, if not then send NAK.

I'd say big NAK. Have you ever looked at the full commit message and patch at all? It says "release_posix_timer() can't be called with ->it_process != NULL.". Point. The rest is the explanation why this can't happen. And looking at the patch, we see that it just removes code that actually never gets executed under the mentioned preconditions. It's a pure cleanup patch and doesn't qualify for -stable. Same goes for the other posix timer patch you mailed out.

Quoted 40 lines

---
From 96347e7759e2e433c427defa0fa1adfc8cce6226 Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <[email protected]>
Date: Fri, 25 Jul 2008 01:47:27 -0700
Subject: [PATCH] posix timers: release_posix_timer: kill the bogus put_task_struct(->it_process);

[ Upstream commit 96347e7759e2e433c427defa0fa1adfc8cce6226 ]

release_posix_timer() can't be called with ->it_process != NULL. Once sys_timer_create() sets ->it_process it must not call release_posix_timer(), otherwise we can race with another thread doing sys_timer_delete(), this timer is visible to idr_find() and unlocked.

The same is true for two other callers (actually, for any possible caller), sys_timer_delete() and itimer_delete(). They must clear ->it_process before unlock_timer() + release_posix_timer().

Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Roland McGrath <[email protected]>
Cc john stultz <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Roland McGrath <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>
CC Oliver Pinter <[email protected]>
kernel/posix-timers.c +0 -3
index 17f5326..9a21681 100644
@@ -449,9 +449,6 @@ static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
 		spin_unlock_irqrestore(&idr_lock, flags);
 	}
 	sigqueue_free(tmr->sigq);
-	if (unlikely(tmr->it_process) &&
-	    tmr->it_sigev_notify == (SIGEV_SIGNAL|SIGEV_THREAD_ID))
-		put_task_struct(tmr->it_process);
 	kmem_cache_free(posix_timers_cache, tmr);
 }
#3 lore raw
Quoted 6 lines · Sven Wegener
Quoted 2 lines · Oliver Pinter

It is an RFC for sending this patch for stable, when this patch needed, then send ACK and CC stable, if not then send NAK.

Quoted 7 lines

I'd say big NAK. Have you ever looked at the full commit message and patch at all? It says "release_posix_timer() can't be called with ->it_process != NULL.". Point. The rest is the explanation why this can't happen. And looking at the patch, we see that it just removes code that actually never gets executed under the mentioned preconditions. It's a pure cleanup patch and doesn't qualify for -stable. Same goes for the other posix timer patch you mailed out.

I agree. Perhaps the changelog was badly written...

These 2 patches are just cleanups which remove the dead code, this is not the -stable material.

Oleg.