| 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
| ||
|
Date: Fri, 1 Aug 2008 20:06:11 +0400
From: Oleg Nesterov <oleg@...sign.ru>
To: Ingo Molnar <mingo@...e.hu>, Mark McLoughlin <markmc@...hat.com>,
Michael Kerrisk <mtk.manpages@...glemail.com>,
Roland McGrath <roland@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org
Subject: Q: posix_timer_event: can't we kill the "switch to ->group_leader on failure" ?
posix_timer_event() drops SIGEV_THREAD_ID and switches to ->group_leader
if send_sigqueue() fails.
Is this really useful? I don't understand the point.
But more importantly, I think this code gives the false promises, it
doesn't work reliably.
send_sigqueue() can only fail if the thread dies. But it can die before
it dequeues the SI_TIMER signal, in that case the timer stops anyway.
Isn't it better to just remove this code?
--- 26-rc2/kernel/posix-timers.c~1_DONT_RESEND 2008-07-23 20:24:05.000000000 +0400
+++ 26-rc2/kernel/posix-timers.c 2008-08-01 19:37:47.000000000 +0400
@@ -298,6 +298,7 @@ void do_schedule_next_timer(struct sigin
int posix_timer_event(struct k_itimer *timr, int si_private)
{
+ int ret;
/*
* FIXME: if ->sigq is queued we can race with
* dequeue_signal()->do_schedule_next_timer().
@@ -316,20 +317,10 @@ int posix_timer_event(struct k_itimer *t
timr->sigq->info.si_tid = timr->it_id;
timr->sigq->info.si_value = timr->it_sigev_value;
- if (timr->it_sigev_notify & SIGEV_THREAD_ID) {
- struct task_struct *leader;
- int ret = send_sigqueue(timr->sigq, timr->it_process, 0);
-
- if (likely(ret >= 0))
- return ret;
-
- timr->it_sigev_notify = SIGEV_SIGNAL;
- leader = timr->it_process->group_leader;
- put_task_struct(timr->it_process);
- timr->it_process = leader;
- }
-
- return send_sigqueue(timr->sigq, timr->it_process, 1);
+ ret = send_sigqueue(timr->sigq, timr->it_process,
+ !(timr->it_sigev_notify & SIGEV_THREAD_ID));
+ /* if we failed to send the signal, the timer stops */
+ return ret > 0;
}
EXPORT_SYMBOL_GPL(posix_timer_event);
-------------------------------------------------------------------------------
If we can do the above, we can simplify the code further, see the patch
below (should be 2 patches).
Thoughts?
Oleg.
--- 26-rc2/kernel/posix-timers.c~2_GET_LEADER 2008-08-01 19:37:47.000000000 +0400
+++ 26-rc2/kernel/posix-timers.c 2008-08-01 19:43:09.000000000 +0400
@@ -541,10 +541,9 @@ sys_timer_create(const clockid_t which_c
spin_lock_irqsave(&process->sighand->siglock, flags);
if (!(process->flags & PF_EXITING)) {
new_timer->it_process = process;
+ get_task_struct(process);
list_add(&new_timer->list,
&process->signal->posix_timers);
- if (new_timer->it_sigev_notify == (SIGEV_SIGNAL|SIGEV_THREAD_ID))
- get_task_struct(process);
spin_unlock_irqrestore(&process->sighand->siglock, flags);
} else {
spin_unlock_irqrestore(&process->sighand->siglock, flags);
@@ -561,6 +560,7 @@ sys_timer_create(const clockid_t which_c
new_timer->it_sigev_signo = SIGALRM;
new_timer->it_sigev_value.sival_int = new_timer->it_id;
process = current->group_leader;
+ get_task_struct(process);
spin_lock_irqsave(&process->sighand->siglock, flags);
new_timer->it_process = process;
list_add(&new_timer->list, &process->signal->posix_timers);
@@ -853,8 +853,7 @@ retry_delete:
* This keeps any tasks waiting on the spin lock from thinking
* they got something (see the lock code above).
*/
- if (timer->it_sigev_notify == (SIGEV_SIGNAL|SIGEV_THREAD_ID))
- put_task_struct(timer->it_process);
+ put_task_struct(timer->it_process);
timer->it_process = NULL;
unlock_timer(timer, flags);
@@ -881,8 +880,7 @@ retry_delete:
* This keeps any tasks waiting on the spin lock from thinking
* they got something (see the lock code above).
*/
- if (timer->it_sigev_notify == (SIGEV_SIGNAL|SIGEV_THREAD_ID))
- put_task_struct(timer->it_process);
+ put_task_struct(timer->it_process);
timer->it_process = NULL;
unlock_timer(timer, flags);
--- 26-rc2/fs/exec.c~2_GET_LEADER 2008-07-22 15:46:04.000000000 +0400
+++ 26-rc2/fs/exec.c 2008-08-01 19:44:31.000000000 +0400
@@ -757,7 +757,6 @@ static int de_thread(struct task_struct
struct signal_struct *sig = tsk->signal;
struct sighand_struct *oldsighand = tsk->sighand;
spinlock_t *lock = &oldsighand->siglock;
- struct task_struct *leader = NULL;
int count;
if (thread_group_empty(tsk))
@@ -795,7 +794,7 @@ static int de_thread(struct task_struct
* and to assume its PID:
*/
if (!thread_group_leader(tsk)) {
- leader = tsk->group_leader;
+ struct task_struct *leader = tsk->group_leader;
sig->notify_count = -1; /* for exit_notify() */
for (;;) {
@@ -849,8 +848,9 @@ static int de_thread(struct task_struct
BUG_ON(leader->exit_state != EXIT_ZOMBIE);
leader->exit_state = EXIT_DEAD;
-
write_unlock_irq(&tasklist_lock);
+
+ release_task(leader);
}
sig->group_exit_task = NULL;
@@ -859,8 +859,6 @@ static int de_thread(struct task_struct
no_thread_group:
exit_itimers(sig);
flush_itimer_signals();
- if (leader)
- release_task(leader);
if (atomic_read(&oldsighand->count) != 1) {
struct sighand_struct *newsighand;
--
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/
Powered by blists - more mailing lists