Re: [PATCH] fix send_sigqueue() vs thread exit race
On Tue, 2005-08-23 at 20:17 +0400, Oleg Nesterov wrote:
> Thomas Gleixner wrote:
> > I still think the last patch I sent is still necessary.
>
> Thomas, you know that I like this change in __exit_{signal,sighand},
> but i think this change is dangerous, should go in a separate patch,
> and needs a lot of testing. But the decision is up to Ingo and Roland.
Maybe it makes more sense in context of Pauls RCU stuff.
> What do you think about this:
>
> int try_to_lock_this_beep_tasklist_lock(struct task_struct *group_leader)
> {
> while (unlikely(!read_trylock(_lock))) {
> if (group_leader->flags & PF_EXITING) {
> smp_rmb();
> if (thread_group_empty(group_leader))
> return 0;
> }
> cpu_relax();
> }
>
> return 1;
> }
>
> No need to re-check after we got tasklist, the signal will be flushed.
Makes sense, though I'm not sure if its safe to call
thread_group_empty() without tasklist lock held. I might be in case of
PF_EXITING, but it needs a comment at least.
> I think it's better to move the locking into the posix_timer_event, btw.
Hmm, not sure. I've seen similar stuff in the AIO patches. I guess this
should be solved safe at one place rather than all around the kernel.
tglx
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Thomas Gleixner wrote:
>
> On Mon, 2005-08-22 at 20:45 +0400, Oleg Nesterov wrote:
> >
> > kernel/posix-timers.c:common_timer_del() calls del_timer_sync(), after
> > that nobody can access this timer, so we don't need to lock timer->it_lock
> > at all in this case. No lock - no deadlock.
>
> It still deadlocks:
>
> CPU 0 CPU 1
> write_lock(_lock);
> __exit_signal()
> timer expires
> base->running_timer = timer
> send_group_sigqueue()
>read_lock(_lock();
> exit_itimers()
> del_timer_sync(timer)
> waits for ever because waits for ever on tasklist_lock
> base->running_timer == timer
Silly me.
> I still think the last patch I sent is still necessary.
Thomas, you know that I like this change in __exit_{signal,sighand},
but i think this change is dangerous, should go in a separate patch,
and needs a lot of testing. But the decision is up to Ingo and Roland.
I am looking at your previous patch:
> - read_lock(_lock);
> +retry:
> + if (unlikely(p->flags & PF_EXITING))
> + return -1;
> +
> + if (unlikely(!read_trylock(_lock))) {
> + cpu_relax();
> + goto retry;
> + }
> + if (unlikely(p->flags & PF_EXITING)) {
> + ret = -1;
> + goto out_err;
What do you think about this:
int try_to_lock_this_beep_tasklist_lock(struct task_struct *group_leader)
{
while (unlikely(!read_trylock(_lock))) {
if (group_leader->flags & PF_EXITING) {
smp_rmb();
if (thread_group_empty(group_leader))
return 0;
}
cpu_relax();
}
return 1;
}
No need to re-check after we got tasklist, the signal will be flushed.
I think it's better to move the locking into the posix_timer_event, btw.
In that case we can drop my patch.
What is your opinion, can it work?
P.S.
Thomas, thanks for explanation about posix-cpu-timers.
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Mon, 2005-08-22 at 20:45 +0400, Oleg Nesterov wrote: > But I know nothing about kernel/posix-cpu-timers.c, I doubt it will work > for posix_cpu_timer_del(). I don't have time to study posix-cpu-timers now. > However, I see that __exit_signal() calls posix_cpu_timers_exit_xxx(), so > may be it can work? timer->it.cpu.task is set to NULL by posix_cpu_timers_exit(), so the code in posix_cpu_timer_del returns before accessing tasklist_lock. The exit functions do not take any locks, but it is not necessary there. posix_run_cpu_timers(p) is called with p=current() and we have interrupts disabled, so the timer interrupt can not run on this CPU. The current exiting process can not run at the same time on a different CPU, so no race and lockup possible here. tglx - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Mon, 2005-08-22 at 20:45 +0400, Oleg Nesterov wrote:
> Thomas Gleixner wrote:
> Ok, exit_itimers()->itimer_delete() called when the last thread exits
> or does exec.
>
> kernel/posix-timers.c:common_timer_del() calls del_timer_sync(), after
> that nobody can access this timer, so we don't need to lock timer->it_lock
> at all in this case. No lock - no deadlock.
It still deadlocks:
CPU 0 CPU 1
write_lock(_lock);
__exit_signal()
timer expires
base->running_timer = timer
send_group_sigqueue()
read_lock(_lock();
exit_itimers()
del_timer_sync(timer)
waits for ever because waits for ever on tasklist_lock
base->running_timer == timer
I still think the last patch I sent is still necessary.
> But I know nothing about kernel/posix-cpu-timers.c, I doubt it will work
> for posix_cpu_timer_del(). I don't have time to study posix-cpu-timers now.
> However, I see that __exit_signal() calls posix_cpu_timers_exit_xxx(), so
> may be it can work?
>
>380 int posix_cpu_timer_del(struct k_itimer *timer)
>381 {
>382 struct task_struct *p = timer->it.cpu.task;
>383
>384 if (timer->it.cpu.firing)
>385 return TIMER_RETRY;
>386
>387 if (unlikely(p == NULL))
>388 return 0;
>389
>390 if (!list_empty(>it.cpu.entry)) {
>391 read_lock(_lock);
>
> Surely, it should be impossible to happen when process exists, otherwise
> it would deadlock immediately, we did write_lock(tasklist).
>
> Thomas, do you know something about posix-cpu-timers.c?
Not much. I look into this
tglx
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Mon, 2005-08-22 at 20:45 +0400, Oleg Nesterov wrote:
Thomas Gleixner wrote:
Ok, exit_itimers()-itimer_delete() called when the last thread exits
or does exec.
kernel/posix-timers.c:common_timer_del() calls del_timer_sync(), after
that nobody can access this timer, so we don't need to lock timer-it_lock
at all in this case. No lock - no deadlock.
It still deadlocks:
CPU 0 CPU 1
write_lock(tasklist_lock);
__exit_signal()
timer expires
base-running_timer = timer
send_group_sigqueue()
read_lock(tasklist_lock();
exit_itimers()
del_timer_sync(timer)
waits for ever because waits for ever on tasklist_lock
base-running_timer == timer
I still think the last patch I sent is still necessary.
But I know nothing about kernel/posix-cpu-timers.c, I doubt it will work
for posix_cpu_timer_del(). I don't have time to study posix-cpu-timers now.
However, I see that __exit_signal() calls posix_cpu_timers_exit_xxx(), so
may be it can work?
380 int posix_cpu_timer_del(struct k_itimer *timer)
381 {
382 struct task_struct *p = timer-it.cpu.task;
383
384 if (timer-it.cpu.firing)
385 return TIMER_RETRY;
386
387 if (unlikely(p == NULL))
388 return 0;
389
390 if (!list_empty(timer-it.cpu.entry)) {
391 read_lock(tasklist_lock);
Surely, it should be impossible to happen when process exists, otherwise
it would deadlock immediately, we did write_lock(tasklist).
Thomas, do you know something about posix-cpu-timers.c?
Not much. I look into this
tglx
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Mon, 2005-08-22 at 20:45 +0400, Oleg Nesterov wrote: But I know nothing about kernel/posix-cpu-timers.c, I doubt it will work for posix_cpu_timer_del(). I don't have time to study posix-cpu-timers now. However, I see that __exit_signal() calls posix_cpu_timers_exit_xxx(), so may be it can work? timer-it.cpu.task is set to NULL by posix_cpu_timers_exit(), so the code in posix_cpu_timer_del returns before accessing tasklist_lock. The exit functions do not take any locks, but it is not necessary there. posix_run_cpu_timers(p) is called with p=current() and we have interrupts disabled, so the timer interrupt can not run on this CPU. The current exiting process can not run at the same time on a different CPU, so no race and lockup possible here. tglx - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Thomas Gleixner wrote:
On Mon, 2005-08-22 at 20:45 +0400, Oleg Nesterov wrote:
kernel/posix-timers.c:common_timer_del() calls del_timer_sync(), after
that nobody can access this timer, so we don't need to lock timer-it_lock
at all in this case. No lock - no deadlock.
It still deadlocks:
CPU 0 CPU 1
write_lock(tasklist_lock);
__exit_signal()
timer expires
base-running_timer = timer
send_group_sigqueue()
read_lock(tasklist_lock();
exit_itimers()
del_timer_sync(timer)
waits for ever because waits for ever on tasklist_lock
base-running_timer == timer
Silly me.
I still think the last patch I sent is still necessary.
Thomas, you know that I like this change in __exit_{signal,sighand},
but i think this change is dangerous, should go in a separate patch,
and needs a lot of testing. But the decision is up to Ingo and Roland.
I am looking at your previous patch:
- read_lock(tasklist_lock);
+retry:
+ if (unlikely(p-flags PF_EXITING))
+ return -1;
+
+ if (unlikely(!read_trylock(tasklist_lock))) {
+ cpu_relax();
+ goto retry;
+ }
+ if (unlikely(p-flags PF_EXITING)) {
+ ret = -1;
+ goto out_err;
What do you think about this:
int try_to_lock_this_beep_tasklist_lock(struct task_struct *group_leader)
{
while (unlikely(!read_trylock(tasklist_lock))) {
if (group_leader-flags PF_EXITING) {
smp_rmb();
if (thread_group_empty(group_leader))
return 0;
}
cpu_relax();
}
return 1;
}
No need to re-check after we got tasklist, the signal will be flushed.
I think it's better to move the locking into the posix_timer_event, btw.
In that case we can drop my patch.
What is your opinion, can it work?
P.S.
Thomas, thanks for explanation about posix-cpu-timers.
Oleg.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Tue, 2005-08-23 at 20:17 +0400, Oleg Nesterov wrote:
Thomas Gleixner wrote:
I still think the last patch I sent is still necessary.
Thomas, you know that I like this change in __exit_{signal,sighand},
but i think this change is dangerous, should go in a separate patch,
and needs a lot of testing. But the decision is up to Ingo and Roland.
Maybe it makes more sense in context of Pauls RCU stuff.
What do you think about this:
int try_to_lock_this_beep_tasklist_lock(struct task_struct *group_leader)
{
while (unlikely(!read_trylock(tasklist_lock))) {
if (group_leader-flags PF_EXITING) {
smp_rmb();
if (thread_group_empty(group_leader))
return 0;
}
cpu_relax();
}
return 1;
}
No need to re-check after we got tasklist, the signal will be flushed.
Makes sense, though I'm not sure if its safe to call
thread_group_empty() without tasklist lock held. I might be in case of
PF_EXITING, but it needs a comment at least.
I think it's better to move the locking into the posix_timer_event, btw.
Hmm, not sure. I've seen similar stuff in the AIO patches. I guess this
should be solved safe at one place rather than all around the kernel.
tglx
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Thomas Gleixner wrote: > > But we can not check for p->sighand == NULL, as sighand is released > after exit_itimers() so we are still deadlock prone. So I think > __exit_sighand() should be called before exit_itimers(). Then we can do > > retry: > if (unlikely(!p->sighand)) > return -1; > > instead of checking for PF_EXITING. I think yes, and this is closely related to your and Paul "Use RCU to protect tasklist for unicast signals" patches. We don't need RCU here, but we do need this hypothetical lock_task_sighand() (and __exit_sighand from __exit_signal change). We still need tasklist_lock in send_group_queue for __group_complete_signal though. I hope Paul will solve this, it is needed for group_send_info() anyway. But for the near future I don't see simple and non-intrusive fix for this deadlock. I am waiting for George to confirm that the bug exists and we are not crazy :) Oleg. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Thomas Gleixner wrote: > > @@ -1427,7 +1434,18 @@ send_group_sigqueue(int sig, struct sigq > int ret = 0; > > BUG_ON(!(q->flags & SIGQUEUE_PREALLOC)); > - read_lock(_lock); > +retry: > + if (unlikely(p->flags & PF_EXITING)) > + return -1; > + I don't think this is correct. p == ->group_leader, it may have been exited and in EXIT_ZOMBIE state. But the thread group (process) is live, we should not stop posix timers. Oleg. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Mon, 2005-08-22 at 10:39 +0400, Oleg Nesterov wrote: > Thomas Gleixner wrote: > > > > @@ -1427,7 +1434,18 @@ send_group_sigqueue(int sig, struct sigq > > int ret = 0; > > > > BUG_ON(!(q->flags & SIGQUEUE_PREALLOC)); > > - read_lock(_lock); > > +retry: > > + if (unlikely(p->flags & PF_EXITING)) > > + return -1; > > + > > I don't think this is correct. p == ->group_leader, it may > have been exited and in EXIT_ZOMBIE state. But the thread > group (process) is live, we should not stop posix timers. Hmm, true. release_task() is not called in this case, so p->sighand is still there. But we can not check for p->sighand == NULL, as sighand is released after exit_itimers() so we are still deadlock prone. So I think __exit_sighand() should be called before exit_itimers(). Then we can do retry: if (unlikely(!p->sighand)) return -1; instead of checking for PF_EXITING. tglx - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Oleg,
On Mon, 2005-08-22 at 12:52 +0400, Oleg Nesterov wrote:
> Thomas Gleixner wrote:
> I think yes, and this is closely related to your and Paul
> "Use RCU to protect tasklist for unicast signals" patches.
>
> We don't need RCU here, but we do need this hypothetical
> lock_task_sighand() (and __exit_sighand from __exit_signal
> change).
>
> We still need tasklist_lock in send_group_queue for
> __group_complete_signal though. I hope Paul will solve
> this, it is needed for group_send_info() anyway.
>
> But for the near future I don't see simple and non-intrusive
> fix for this deadlock. I am waiting for George to confirm
> that the bug exists and we are not crazy :)
It exists. It triggers on preempt-RT and I can trigger it on vanilla SMP
by waiting for the timer expiry in release_task() before the
__exit_signal() call. That's reasonable, as it can happen that way by
chance too. It requires that the timer expires on a different CPU, but I
don't see a reason why this should not happen.
The patch below solves it for me.
tglx
diff -uprN --exclude-from=/usr/local/bin/diffit.exclude
linux-2.6.13-rc6/kernel/exit.c linux-2.6.13-rc6.signal/kernel/exit.c
--- linux-2.6.13-rc6/kernel/exit.c 2005-08-13 12:25:58.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/exit.c 2005-08-22 11:19:02.0
+0200
@@ -71,7 +71,6 @@ repeat:
__ptrace_unlink(p);
BUG_ON(!list_empty(>ptrace_list) ||
!list_empty(>ptrace_children));
__exit_signal(p);
- __exit_sighand(p);
/*
* Note that the fastpath in sys_times depends on __exit_signal having
* updated the counters before a task is removed from the tasklist of
diff -uprN --exclude-from=/usr/local/bin/diffit.exclude
linux-2.6.13-rc6/kernel/fork.c linux-2.6.13-rc6.signal/kernel/fork.c
--- linux-2.6.13-rc6/kernel/fork.c 2005-08-13 12:25:58.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/fork.c 2005-08-22 11:35:05.0
+0200
@@ -1132,7 +1132,8 @@ bad_fork_cleanup_mm:
bad_fork_cleanup_signal:
exit_signal(p);
bad_fork_cleanup_sighand:
- exit_sighand(p);
+ if (p->sighand)
+ exit_sighand(p);
bad_fork_cleanup_fs:
exit_fs(p); /* blocking */
bad_fork_cleanup_files:
diff -uprN --exclude-from=/usr/local/bin/diffit.exclude
linux-2.6.13-rc6/kernel/posix-timers.c
linux-2.6.13-rc6.signal/kernel/posix-timers.c
--- linux-2.6.13-rc6/kernel/posix-timers.c 2005-08-13 12:25:58.0
+0200
+++ linux-2.6.13-rc6.signal/kernel/posix-timers.c 2005-08-21
23:19:42.0 +0200
@@ -427,21 +427,23 @@ int posix_timer_event(struct k_itimer *t
timr->sigq->info.si_code = SI_TIMER;
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) {
- if (unlikely(timr->it_process->flags & PF_EXITING)) {
- timr->it_sigev_notify = SIGEV_SIGNAL;
- put_task_struct(timr->it_process);
- timr->it_process = timr->it_process->group_leader;
- goto group;
- }
- return send_sigqueue(timr->it_sigev_signo, timr->sigq,
- timr->it_process);
- }
- else {
- group:
- return send_group_sigqueue(timr->it_sigev_signo, timr->sigq,
- timr->it_process);
+ struct task_struct *leader;
+ int ret = send_sigqueue(timr->it_sigev_signo, timr->sigq,
+ timr->it_process);
+
+ 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_group_sigqueue(timr->it_sigev_signo, timr->sigq,
+ timr->it_process);
}
EXPORT_SYMBOL_GPL(posix_timer_event);
@@ -499,7 +501,8 @@ static void posix_timer_fn(unsigned long
remove_from_abslist(timr);
}
- if (posix_timer_event(timr, si_private))
+ /* Do not rearm the timer, when we are exiting */
+ if (posix_timer_event(timr, si_private) > 0)
/*
* signal was not sent because of sig_ignor
* we will not get a call back to restart it AND
diff -uprN --exclude-from=/usr/local/bin/diffit.exclude
linux-2.6.13-rc6/kernel/signal.c linux-2.6.13-rc6.signal/kernel/signal.c
--- linux-2.6.13-rc6/kernel/signal.c2005-08-13 12:25:58.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/signal.c 2005-08-22 11:51:08.0
+0200
@@ -395,6 +395,7 @@ void __exit_signal(struct task_struct *t
}
clear_tsk_thread_flag(tsk,TIF_SIGPENDING);
Re: [PATCH] fix send_sigqueue() vs thread exit race
Thomas Gleixner wrote:
>
> It exists. It triggers on preempt-RT and I can trigger it on vanilla SMP
> by waiting for the timer expiry in release_task() before the
> __exit_signal() call. That's reasonable, as it can happen that way by
> chance too. It requires that the timer expires on a different CPU, but I
> don't see a reason why this should not happen.
Ok, exit_itimers()->itimer_delete() called when the last thread exits
or does exec.
kernel/posix-timers.c:common_timer_del() calls del_timer_sync(), after
that nobody can access this timer, so we don't need to lock timer->it_lock
at all in this case. No lock - no deadlock.
But I know nothing about kernel/posix-cpu-timers.c, I doubt it will work
for posix_cpu_timer_del(). I don't have time to study posix-cpu-timers now.
However, I see that __exit_signal() calls posix_cpu_timers_exit_xxx(), so
may be it can work?
380 int posix_cpu_timer_del(struct k_itimer *timer)
381 {
382 struct task_struct *p = timer->it.cpu.task;
383
384 if (timer->it.cpu.firing)
385 return TIMER_RETRY;
386
387 if (unlikely(p == NULL))
388 return 0;
389
390 if (!list_empty(>it.cpu.entry)) {
391 read_lock(_lock);
Surely, it should be impossible to happen when process exists, otherwise
it would deadlock immediately, we did write_lock(tasklist).
Thomas, do you know something about posix-cpu-timers.c?
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Thomas Gleixner wrote:
It exists. It triggers on preempt-RT and I can trigger it on vanilla SMP
by waiting for the timer expiry in release_task() before the
__exit_signal() call. That's reasonable, as it can happen that way by
chance too. It requires that the timer expires on a different CPU, but I
don't see a reason why this should not happen.
Ok, exit_itimers()-itimer_delete() called when the last thread exits
or does exec.
kernel/posix-timers.c:common_timer_del() calls del_timer_sync(), after
that nobody can access this timer, so we don't need to lock timer-it_lock
at all in this case. No lock - no deadlock.
But I know nothing about kernel/posix-cpu-timers.c, I doubt it will work
for posix_cpu_timer_del(). I don't have time to study posix-cpu-timers now.
However, I see that __exit_signal() calls posix_cpu_timers_exit_xxx(), so
may be it can work?
380 int posix_cpu_timer_del(struct k_itimer *timer)
381 {
382 struct task_struct *p = timer-it.cpu.task;
383
384 if (timer-it.cpu.firing)
385 return TIMER_RETRY;
386
387 if (unlikely(p == NULL))
388 return 0;
389
390 if (!list_empty(timer-it.cpu.entry)) {
391 read_lock(tasklist_lock);
Surely, it should be impossible to happen when process exists, otherwise
it would deadlock immediately, we did write_lock(tasklist).
Thomas, do you know something about posix-cpu-timers.c?
Oleg.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Oleg,
On Mon, 2005-08-22 at 12:52 +0400, Oleg Nesterov wrote:
Thomas Gleixner wrote:
I think yes, and this is closely related to your and Paul
Use RCU to protect tasklist for unicast signals patches.
We don't need RCU here, but we do need this hypothetical
lock_task_sighand() (and __exit_sighand from __exit_signal
change).
We still need tasklist_lock in send_group_queue for
__group_complete_signal though. I hope Paul will solve
this, it is needed for group_send_info() anyway.
But for the near future I don't see simple and non-intrusive
fix for this deadlock. I am waiting for George to confirm
that the bug exists and we are not crazy :)
It exists. It triggers on preempt-RT and I can trigger it on vanilla SMP
by waiting for the timer expiry in release_task() before the
__exit_signal() call. That's reasonable, as it can happen that way by
chance too. It requires that the timer expires on a different CPU, but I
don't see a reason why this should not happen.
The patch below solves it for me.
tglx
diff -uprN --exclude-from=/usr/local/bin/diffit.exclude
linux-2.6.13-rc6/kernel/exit.c linux-2.6.13-rc6.signal/kernel/exit.c
--- linux-2.6.13-rc6/kernel/exit.c 2005-08-13 12:25:58.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/exit.c 2005-08-22 11:19:02.0
+0200
@@ -71,7 +71,6 @@ repeat:
__ptrace_unlink(p);
BUG_ON(!list_empty(p-ptrace_list) ||
!list_empty(p-ptrace_children));
__exit_signal(p);
- __exit_sighand(p);
/*
* Note that the fastpath in sys_times depends on __exit_signal having
* updated the counters before a task is removed from the tasklist of
diff -uprN --exclude-from=/usr/local/bin/diffit.exclude
linux-2.6.13-rc6/kernel/fork.c linux-2.6.13-rc6.signal/kernel/fork.c
--- linux-2.6.13-rc6/kernel/fork.c 2005-08-13 12:25:58.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/fork.c 2005-08-22 11:35:05.0
+0200
@@ -1132,7 +1132,8 @@ bad_fork_cleanup_mm:
bad_fork_cleanup_signal:
exit_signal(p);
bad_fork_cleanup_sighand:
- exit_sighand(p);
+ if (p-sighand)
+ exit_sighand(p);
bad_fork_cleanup_fs:
exit_fs(p); /* blocking */
bad_fork_cleanup_files:
diff -uprN --exclude-from=/usr/local/bin/diffit.exclude
linux-2.6.13-rc6/kernel/posix-timers.c
linux-2.6.13-rc6.signal/kernel/posix-timers.c
--- linux-2.6.13-rc6/kernel/posix-timers.c 2005-08-13 12:25:58.0
+0200
+++ linux-2.6.13-rc6.signal/kernel/posix-timers.c 2005-08-21
23:19:42.0 +0200
@@ -427,21 +427,23 @@ int posix_timer_event(struct k_itimer *t
timr-sigq-info.si_code = SI_TIMER;
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) {
- if (unlikely(timr-it_process-flags PF_EXITING)) {
- timr-it_sigev_notify = SIGEV_SIGNAL;
- put_task_struct(timr-it_process);
- timr-it_process = timr-it_process-group_leader;
- goto group;
- }
- return send_sigqueue(timr-it_sigev_signo, timr-sigq,
- timr-it_process);
- }
- else {
- group:
- return send_group_sigqueue(timr-it_sigev_signo, timr-sigq,
- timr-it_process);
+ struct task_struct *leader;
+ int ret = send_sigqueue(timr-it_sigev_signo, timr-sigq,
+ timr-it_process);
+
+ 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_group_sigqueue(timr-it_sigev_signo, timr-sigq,
+ timr-it_process);
}
EXPORT_SYMBOL_GPL(posix_timer_event);
@@ -499,7 +501,8 @@ static void posix_timer_fn(unsigned long
remove_from_abslist(timr);
}
- if (posix_timer_event(timr, si_private))
+ /* Do not rearm the timer, when we are exiting */
+ if (posix_timer_event(timr, si_private) 0)
/*
* signal was not sent because of sig_ignor
* we will not get a call back to restart it AND
diff -uprN --exclude-from=/usr/local/bin/diffit.exclude
linux-2.6.13-rc6/kernel/signal.c linux-2.6.13-rc6.signal/kernel/signal.c
--- linux-2.6.13-rc6/kernel/signal.c2005-08-13 12:25:58.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/signal.c 2005-08-22 11:51:08.0
+0200
@@ -395,6 +395,7 @@ void __exit_signal(struct task_struct *t
}
clear_tsk_thread_flag(tsk,TIF_SIGPENDING);
flush_sigqueue(tsk-pending);
+
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Mon, 2005-08-22 at 10:39 +0400, Oleg Nesterov wrote: Thomas Gleixner wrote: @@ -1427,7 +1434,18 @@ send_group_sigqueue(int sig, struct sigq int ret = 0; BUG_ON(!(q-flags SIGQUEUE_PREALLOC)); - read_lock(tasklist_lock); +retry: + if (unlikely(p-flags PF_EXITING)) + return -1; + I don't think this is correct. p == -group_leader, it may have been exited and in EXIT_ZOMBIE state. But the thread group (process) is live, we should not stop posix timers. Hmm, true. release_task() is not called in this case, so p-sighand is still there. But we can not check for p-sighand == NULL, as sighand is released after exit_itimers() so we are still deadlock prone. So I think __exit_sighand() should be called before exit_itimers(). Then we can do retry: if (unlikely(!p-sighand)) return -1; instead of checking for PF_EXITING. tglx - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Thomas Gleixner wrote: @@ -1427,7 +1434,18 @@ send_group_sigqueue(int sig, struct sigq int ret = 0; BUG_ON(!(q-flags SIGQUEUE_PREALLOC)); - read_lock(tasklist_lock); +retry: + if (unlikely(p-flags PF_EXITING)) + return -1; + I don't think this is correct. p == -group_leader, it may have been exited and in EXIT_ZOMBIE state. But the thread group (process) is live, we should not stop posix timers. Oleg. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Thomas Gleixner wrote: But we can not check for p-sighand == NULL, as sighand is released after exit_itimers() so we are still deadlock prone. So I think __exit_sighand() should be called before exit_itimers(). Then we can do retry: if (unlikely(!p-sighand)) return -1; instead of checking for PF_EXITING. I think yes, and this is closely related to your and Paul Use RCU to protect tasklist for unicast signals patches. We don't need RCU here, but we do need this hypothetical lock_task_sighand() (and __exit_sighand from __exit_signal change). We still need tasklist_lock in send_group_queue for __group_complete_signal though. I hope Paul will solve this, it is needed for group_send_info() anyway. But for the near future I don't see simple and non-intrusive fix for this deadlock. I am waiting for George to confirm that the bug exists and we are not crazy :) Oleg. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Sun, 2005-08-21 at 23:24 +0200, Thomas Gleixner wrote:
> Oleg,
Sorry for making noise. I introduced the race again.
tglx
--- linux-2.6.13-rc6.signal/kernel/signal.oleg.c2005-08-21
23:07:03.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/signal.c 2005-08-21 23:48:52.0
+0200
@@ -1381,8 +1381,15 @@ send_sigqueue(int sig, struct sigqueue *
int ret = 0;
BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
- read_lock(_lock);
+retry:
+ if (unlikely(p->flags & PF_EXITING))
+ return -1;
+
+ if (unlikely(!read_trylock(_lock))) {
+ cpu_relax();
+ goto retry;
+ }
if (unlikely(p->flags & PF_EXITING)) {
ret = -1;
goto out_err;
@@ -1427,7 +1434,18 @@ send_group_sigqueue(int sig, struct sigq
int ret = 0;
BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
- read_lock(_lock);
+retry:
+ if (unlikely(p->flags & PF_EXITING))
+ return -1;
+
+ if (unlikely(!read_trylock(_lock))) {
+ cpu_relax();
+ goto retry;
+ }
+ if (unlikely(p->flags & PF_EXITING)) {
+ ret = -1;
+ goto out_err;
+ }
spin_lock_irqsave(>sighand->siglock, flags);
handle_stop_signal(sig, p);
@@ -1461,8 +1479,9 @@ send_group_sigqueue(int sig, struct sigq
__group_complete_signal(sig, p);
out:
spin_unlock_irqrestore(>sighand->siglock, flags);
+out_err:
read_unlock(_lock);
- return(ret);
+ return ret;
}
/*
--- linux-2.6.13-rc6.signal/kernel/posix-timers.oleg.c 2005-08-21
23:09:58.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/posix-timers.c 2005-08-21
23:19:42.0 +0200
@@ -501,7 +501,8 @@ static void posix_timer_fn(unsigned long
remove_from_abslist(timr);
}
- if (posix_timer_event(timr, si_private))
+ /* Do not rearm the timer, when we are exiting */
+ if (posix_timer_event(timr, si_private) > 0)
/*
* signal was not sent because of sig_ignor
* we will not get a call back to restart it AND
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Oleg,
On Sun, 2005-08-21 at 14:59 +0400, Oleg Nesterov wrote:
> CPU_0 CPU_1
>
> release_task: posix_timer_fn:
> write_lock(tasklist); lock(timer->it_lock);
>
> exit_timers:send_sigxxx();
> lock(timer->it_lock)read_lock(tasklist);
>
> Deadlock.
>
> Dear cc-list, what do you think?
The patch below on top of your patch should solve this. We don't need
tasklist_lock to check p->flags. As you pointed out p cannot be invalid
in send_sigqueue as it's protected by get_task_struct() in
create_timer()
For send_group_sigqueue it's protected by exit_itimers() waiting for
k_itimer.it_lock.
It still does not solve the ugly dependency on tasklist_lock but at
least the race and the deadlock are fixed.
tglx
Signed-off-by: Thomas Gleixner <[EMAIL PROTECTED]>
--- linux-2.6.13-rc6.signal/kernel/signal.oleg.c2005-08-21
23:07:03.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/signal.c 2005-08-21 23:09:07.0
+0200
@@ -1381,11 +1381,14 @@ send_sigqueue(int sig, struct sigqueue *
int ret = 0;
BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
- read_lock(_lock);
- if (unlikely(p->flags & PF_EXITING)) {
- ret = -1;
- goto out_err;
+retry:
+ if (unlikely(p->flags & PF_EXITING))
+ return -1;
+
+ if ((unlikely(!read_trylock(_lock))) {
+ cpu_relax();
+ goto retry;
}
spin_lock_irqsave(>sighand->siglock, flags);
@@ -1414,7 +1417,6 @@ send_sigqueue(int sig, struct sigqueue *
out:
spin_unlock_irqrestore(>sighand->siglock, flags);
-out_err:
read_unlock(_lock);
return ret;
@@ -1427,7 +1429,14 @@ send_group_sigqueue(int sig, struct sigq
int ret = 0;
BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
- read_lock(_lock);
+retry:
+ if (unlikely(p->flags & PF_EXITING))
+ return -1;
+
+ if (unlikely(!read_trylock(_lock))) {
+ cpu_relax();
+ goto retry;
+ }
spin_lock_irqsave(>sighand->siglock, flags);
handle_stop_signal(sig, p);
--- linux-2.6.13-rc6.signal/kernel/posix-timers.oleg.c 2005-08-21
23:09:58.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/posix-timers.c 2005-08-21
23:19:42.0 +0200
@@ -501,7 +501,8 @@ static void posix_timer_fn(unsigned long
remove_from_abslist(timr);
}
- if (posix_timer_event(timr, si_private))
+ /* Do not rearm the timer, when we are exiting */
+ if (posix_timer_event(timr, si_private) > 0)
/*
* signal was not sent because of sig_ignor
* we will not get a call back to restart it AND
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Sun, 2005-08-21 at 14:41 +0400, Oleg Nesterov wrote: > If not: > > > timer event > > timr->it_process references a freed structure > > > > No, create_timer() does get_task_struct(timr->it_process), this > task may be EXIT_DEAD now, but the task_struct itself is valid, > and it's ->flags has PF_EXITING flag. Right, did not think about that. tglx - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Thomas Gleixner wrote:
>
> On Sat, 2005-08-20 at 20:58 +0400, Oleg Nesterov wrote:
> > [PATCH] fix send_sigqueue() vs thread exit race
> >
> > .
> > - read_lock(_lock);
> > + read_lock(_lock);
> > +
> > + if (unlikely(p->flags & PF_EXITING)) {
> > + ret = -1;
> > + goto out_err;
> > + }
> > +
>
> It's still racy. tasklist_lock does not protect anything here.
I hope no, but please clarify if I am wrong.
tasklist_lock protects against release_task()->__exit_sigxxx()
here.
> arm timer
> exit
If this is the last thread in thread group, exit_itimers()
will stop and clear ->posix_timers.
If not:
> timer event
> timr->it_process references a freed structure
>
No, create_timer() does get_task_struct(timr->it_process), this
task may be EXIT_DEAD now, but the task_struct itself is valid,
and it's ->flags has PF_EXITING flag.
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Sat, 2005-08-20 at 20:58 +0400, Oleg Nesterov wrote:
> [PATCH] fix send_sigqueue() vs thread exit race
>
> posix_timer_event() first checks that the thread (SIGEV_THREAD_ID
> case) does not have PF_EXITING flag, then it calls send_sigqueue()
> which locks task list. But if the thread exits in between the kernel
> will oops (->sighand == NULL after __exit_sighand).
>
> This patch moves the PF_EXITING check into the send_sigqueue(), it
> must be done atomically under tasklist_lock. When send_sigqueue()
> detects exiting thread it returns -1. In that case posix_timer_event
> will send the signal to thread group.
>
> Also, this patch fixes task_struct use-after-free in posix_timer_event.
>
> Signed-off-by: Oleg Nesterov <[EMAIL PROTECTED]>
>
> --- 2.6.13-rc6/kernel/signal.c~ 2005-08-18 23:10:28.0 +0400
> +++ 2.6.13-rc6/kernel/signal.c2005-08-20 23:05:21.0 +0400
> @@ -1366,16 +1366,16 @@ send_sigqueue(int sig, struct sigqueue *
> unsigned long flags;
> int ret = 0;
>
> - /*
> - * We need the tasklist lock even for the specific
> - * thread case (when we don't need to follow the group
> - * lists) in order to avoid races with "p->sighand"
> - * going away or changing from under us.
> - */
> BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
> - read_lock(_lock);
> + read_lock(_lock);
> +
> + if (unlikely(p->flags & PF_EXITING)) {
> + ret = -1;
> + goto out_err;
> + }
> +
It's still racy. tasklist_lock does not protect anything here.
arm timer
exit
timer event
timr->it_process references a freed structure
tglx
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Sat, 2005-08-20 at 20:58 +0400, Oleg Nesterov wrote:
[PATCH] fix send_sigqueue() vs thread exit race
posix_timer_event() first checks that the thread (SIGEV_THREAD_ID
case) does not have PF_EXITING flag, then it calls send_sigqueue()
which locks task list. But if the thread exits in between the kernel
will oops (-sighand == NULL after __exit_sighand).
This patch moves the PF_EXITING check into the send_sigqueue(), it
must be done atomically under tasklist_lock. When send_sigqueue()
detects exiting thread it returns -1. In that case posix_timer_event
will send the signal to thread group.
Also, this patch fixes task_struct use-after-free in posix_timer_event.
Signed-off-by: Oleg Nesterov [EMAIL PROTECTED]
--- 2.6.13-rc6/kernel/signal.c~ 2005-08-18 23:10:28.0 +0400
+++ 2.6.13-rc6/kernel/signal.c2005-08-20 23:05:21.0 +0400
@@ -1366,16 +1366,16 @@ send_sigqueue(int sig, struct sigqueue *
unsigned long flags;
int ret = 0;
- /*
- * We need the tasklist lock even for the specific
- * thread case (when we don't need to follow the group
- * lists) in order to avoid races with p-sighand
- * going away or changing from under us.
- */
BUG_ON(!(q-flags SIGQUEUE_PREALLOC));
- read_lock(tasklist_lock);
+ read_lock(tasklist_lock);
+
+ if (unlikely(p-flags PF_EXITING)) {
+ ret = -1;
+ goto out_err;
+ }
+
It's still racy. tasklist_lock does not protect anything here.
arm timer
exit
timer event
timr-it_process references a freed structure
tglx
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Thomas Gleixner wrote:
On Sat, 2005-08-20 at 20:58 +0400, Oleg Nesterov wrote:
[PATCH] fix send_sigqueue() vs thread exit race
.
- read_lock(tasklist_lock);
+ read_lock(tasklist_lock);
+
+ if (unlikely(p-flags PF_EXITING)) {
+ ret = -1;
+ goto out_err;
+ }
+
It's still racy. tasklist_lock does not protect anything here.
I hope no, but please clarify if I am wrong.
tasklist_lock protects against release_task()-__exit_sigxxx()
here.
arm timer
exit
If this is the last thread in thread group, exit_itimers()
will stop and clear -posix_timers.
If not:
timer event
timr-it_process references a freed structure
No, create_timer() does get_task_struct(timr-it_process), this
task may be EXIT_DEAD now, but the task_struct itself is valid,
and it's -flags has PF_EXITING flag.
Oleg.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
Oleg,
On Sun, 2005-08-21 at 14:59 +0400, Oleg Nesterov wrote:
CPU_0 CPU_1
release_task: posix_timer_fn:
write_lock(tasklist); lock(timer-it_lock);
exit_timers:send_sigxxx();
lock(timer-it_lock)read_lock(tasklist);
Deadlock.
Dear cc-list, what do you think?
The patch below on top of your patch should solve this. We don't need
tasklist_lock to check p-flags. As you pointed out p cannot be invalid
in send_sigqueue as it's protected by get_task_struct() in
create_timer()
For send_group_sigqueue it's protected by exit_itimers() waiting for
k_itimer.it_lock.
It still does not solve the ugly dependency on tasklist_lock but at
least the race and the deadlock are fixed.
tglx
Signed-off-by: Thomas Gleixner [EMAIL PROTECTED]
--- linux-2.6.13-rc6.signal/kernel/signal.oleg.c2005-08-21
23:07:03.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/signal.c 2005-08-21 23:09:07.0
+0200
@@ -1381,11 +1381,14 @@ send_sigqueue(int sig, struct sigqueue *
int ret = 0;
BUG_ON(!(q-flags SIGQUEUE_PREALLOC));
- read_lock(tasklist_lock);
- if (unlikely(p-flags PF_EXITING)) {
- ret = -1;
- goto out_err;
+retry:
+ if (unlikely(p-flags PF_EXITING))
+ return -1;
+
+ if ((unlikely(!read_trylock(tasklist_lock))) {
+ cpu_relax();
+ goto retry;
}
spin_lock_irqsave(p-sighand-siglock, flags);
@@ -1414,7 +1417,6 @@ send_sigqueue(int sig, struct sigqueue *
out:
spin_unlock_irqrestore(p-sighand-siglock, flags);
-out_err:
read_unlock(tasklist_lock);
return ret;
@@ -1427,7 +1429,14 @@ send_group_sigqueue(int sig, struct sigq
int ret = 0;
BUG_ON(!(q-flags SIGQUEUE_PREALLOC));
- read_lock(tasklist_lock);
+retry:
+ if (unlikely(p-flags PF_EXITING))
+ return -1;
+
+ if (unlikely(!read_trylock(tasklist_lock))) {
+ cpu_relax();
+ goto retry;
+ }
spin_lock_irqsave(p-sighand-siglock, flags);
handle_stop_signal(sig, p);
--- linux-2.6.13-rc6.signal/kernel/posix-timers.oleg.c 2005-08-21
23:09:58.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/posix-timers.c 2005-08-21
23:19:42.0 +0200
@@ -501,7 +501,8 @@ static void posix_timer_fn(unsigned long
remove_from_abslist(timr);
}
- if (posix_timer_event(timr, si_private))
+ /* Do not rearm the timer, when we are exiting */
+ if (posix_timer_event(timr, si_private) 0)
/*
* signal was not sent because of sig_ignor
* we will not get a call back to restart it AND
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Sun, 2005-08-21 at 14:41 +0400, Oleg Nesterov wrote: If not: timer event timr-it_process references a freed structure No, create_timer() does get_task_struct(timr-it_process), this task may be EXIT_DEAD now, but the task_struct itself is valid, and it's -flags has PF_EXITING flag. Right, did not think about that. tglx - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix send_sigqueue() vs thread exit race
On Sun, 2005-08-21 at 23:24 +0200, Thomas Gleixner wrote:
Oleg,
Sorry for making noise. I introduced the race again.
tglx
--- linux-2.6.13-rc6.signal/kernel/signal.oleg.c2005-08-21
23:07:03.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/signal.c 2005-08-21 23:48:52.0
+0200
@@ -1381,8 +1381,15 @@ send_sigqueue(int sig, struct sigqueue *
int ret = 0;
BUG_ON(!(q-flags SIGQUEUE_PREALLOC));
- read_lock(tasklist_lock);
+retry:
+ if (unlikely(p-flags PF_EXITING))
+ return -1;
+
+ if (unlikely(!read_trylock(tasklist_lock))) {
+ cpu_relax();
+ goto retry;
+ }
if (unlikely(p-flags PF_EXITING)) {
ret = -1;
goto out_err;
@@ -1427,7 +1434,18 @@ send_group_sigqueue(int sig, struct sigq
int ret = 0;
BUG_ON(!(q-flags SIGQUEUE_PREALLOC));
- read_lock(tasklist_lock);
+retry:
+ if (unlikely(p-flags PF_EXITING))
+ return -1;
+
+ if (unlikely(!read_trylock(tasklist_lock))) {
+ cpu_relax();
+ goto retry;
+ }
+ if (unlikely(p-flags PF_EXITING)) {
+ ret = -1;
+ goto out_err;
+ }
spin_lock_irqsave(p-sighand-siglock, flags);
handle_stop_signal(sig, p);
@@ -1461,8 +1479,9 @@ send_group_sigqueue(int sig, struct sigq
__group_complete_signal(sig, p);
out:
spin_unlock_irqrestore(p-sighand-siglock, flags);
+out_err:
read_unlock(tasklist_lock);
- return(ret);
+ return ret;
}
/*
--- linux-2.6.13-rc6.signal/kernel/posix-timers.oleg.c 2005-08-21
23:09:58.0 +0200
+++ linux-2.6.13-rc6.signal/kernel/posix-timers.c 2005-08-21
23:19:42.0 +0200
@@ -501,7 +501,8 @@ static void posix_timer_fn(unsigned long
remove_from_abslist(timr);
}
- if (posix_timer_event(timr, si_private))
+ /* Do not rearm the timer, when we are exiting */
+ if (posix_timer_event(timr, si_private) 0)
/*
* signal was not sent because of sig_ignor
* we will not get a call back to restart it AND
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] fix send_sigqueue() vs thread exit race
[PATCH] fix send_sigqueue() vs thread exit race
posix_timer_event() first checks that the thread (SIGEV_THREAD_ID
case) does not have PF_EXITING flag, then it calls send_sigqueue()
which locks task list. But if the thread exits in between the kernel
will oops (->sighand == NULL after __exit_sighand).
This patch moves the PF_EXITING check into the send_sigqueue(), it
must be done atomically under tasklist_lock. When send_sigqueue()
detects exiting thread it returns -1. In that case posix_timer_event
will send the signal to thread group.
Also, this patch fixes task_struct use-after-free in posix_timer_event.
Signed-off-by: Oleg Nesterov <[EMAIL PROTECTED]>
--- 2.6.13-rc6/kernel/signal.c~ 2005-08-18 23:10:28.0 +0400
+++ 2.6.13-rc6/kernel/signal.c 2005-08-20 23:05:21.0 +0400
@@ -1366,16 +1366,16 @@ send_sigqueue(int sig, struct sigqueue *
unsigned long flags;
int ret = 0;
- /*
-* We need the tasklist lock even for the specific
-* thread case (when we don't need to follow the group
-* lists) in order to avoid races with "p->sighand"
-* going away or changing from under us.
-*/
BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
- read_lock(_lock);
+ read_lock(_lock);
+
+ if (unlikely(p->flags & PF_EXITING)) {
+ ret = -1;
+ goto out_err;
+ }
+
spin_lock_irqsave(>sighand->siglock, flags);
-
+
if (unlikely(!list_empty(>list))) {
/*
* If an SI_TIMER entry is already queue just increment
@@ -1385,7 +1385,7 @@ send_sigqueue(int sig, struct sigqueue *
BUG();
q->info.si_overrun++;
goto out;
- }
+ }
/* Short-circuit ignored signals. */
if (sig_ignored(p, sig)) {
ret = 1;
@@ -1400,8 +1400,10 @@ send_sigqueue(int sig, struct sigqueue *
out:
spin_unlock_irqrestore(>sighand->siglock, flags);
+out_err:
read_unlock(_lock);
- return(ret);
+
+ return ret;
}
int
--- 2.6.13-rc6/kernel/posix-timers.c~ 2005-08-18 21:37:08.0 +0400
+++ 2.6.13-rc6/kernel/posix-timers.c2005-08-20 23:21:23.0 +0400
@@ -427,21 +427,23 @@ int posix_timer_event(struct k_itimer *t
timr->sigq->info.si_code = SI_TIMER;
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) {
- if (unlikely(timr->it_process->flags & PF_EXITING)) {
- timr->it_sigev_notify = SIGEV_SIGNAL;
- put_task_struct(timr->it_process);
- timr->it_process = timr->it_process->group_leader;
- goto group;
- }
- return send_sigqueue(timr->it_sigev_signo, timr->sigq,
- timr->it_process);
- }
- else {
- group:
- return send_group_sigqueue(timr->it_sigev_signo, timr->sigq,
- timr->it_process);
+ struct task_struct *leader;
+ int ret = send_sigqueue(timr->it_sigev_signo, timr->sigq,
+ timr->it_process);
+
+ 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_group_sigqueue(timr->it_sigev_signo, timr->sigq,
+ timr->it_process);
}
EXPORT_SYMBOL_GPL(posix_timer_event);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] fix send_sigqueue() vs thread exit race
[PATCH] fix send_sigqueue() vs thread exit race
posix_timer_event() first checks that the thread (SIGEV_THREAD_ID
case) does not have PF_EXITING flag, then it calls send_sigqueue()
which locks task list. But if the thread exits in between the kernel
will oops (-sighand == NULL after __exit_sighand).
This patch moves the PF_EXITING check into the send_sigqueue(), it
must be done atomically under tasklist_lock. When send_sigqueue()
detects exiting thread it returns -1. In that case posix_timer_event
will send the signal to thread group.
Also, this patch fixes task_struct use-after-free in posix_timer_event.
Signed-off-by: Oleg Nesterov [EMAIL PROTECTED]
--- 2.6.13-rc6/kernel/signal.c~ 2005-08-18 23:10:28.0 +0400
+++ 2.6.13-rc6/kernel/signal.c 2005-08-20 23:05:21.0 +0400
@@ -1366,16 +1366,16 @@ send_sigqueue(int sig, struct sigqueue *
unsigned long flags;
int ret = 0;
- /*
-* We need the tasklist lock even for the specific
-* thread case (when we don't need to follow the group
-* lists) in order to avoid races with p-sighand
-* going away or changing from under us.
-*/
BUG_ON(!(q-flags SIGQUEUE_PREALLOC));
- read_lock(tasklist_lock);
+ read_lock(tasklist_lock);
+
+ if (unlikely(p-flags PF_EXITING)) {
+ ret = -1;
+ goto out_err;
+ }
+
spin_lock_irqsave(p-sighand-siglock, flags);
-
+
if (unlikely(!list_empty(q-list))) {
/*
* If an SI_TIMER entry is already queue just increment
@@ -1385,7 +1385,7 @@ send_sigqueue(int sig, struct sigqueue *
BUG();
q-info.si_overrun++;
goto out;
- }
+ }
/* Short-circuit ignored signals. */
if (sig_ignored(p, sig)) {
ret = 1;
@@ -1400,8 +1400,10 @@ send_sigqueue(int sig, struct sigqueue *
out:
spin_unlock_irqrestore(p-sighand-siglock, flags);
+out_err:
read_unlock(tasklist_lock);
- return(ret);
+
+ return ret;
}
int
--- 2.6.13-rc6/kernel/posix-timers.c~ 2005-08-18 21:37:08.0 +0400
+++ 2.6.13-rc6/kernel/posix-timers.c2005-08-20 23:21:23.0 +0400
@@ -427,21 +427,23 @@ int posix_timer_event(struct k_itimer *t
timr-sigq-info.si_code = SI_TIMER;
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) {
- if (unlikely(timr-it_process-flags PF_EXITING)) {
- timr-it_sigev_notify = SIGEV_SIGNAL;
- put_task_struct(timr-it_process);
- timr-it_process = timr-it_process-group_leader;
- goto group;
- }
- return send_sigqueue(timr-it_sigev_signo, timr-sigq,
- timr-it_process);
- }
- else {
- group:
- return send_group_sigqueue(timr-it_sigev_signo, timr-sigq,
- timr-it_process);
+ struct task_struct *leader;
+ int ret = send_sigqueue(timr-it_sigev_signo, timr-sigq,
+ timr-it_process);
+
+ 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_group_sigqueue(timr-it_sigev_signo, timr-sigq,
+ timr-it_process);
}
EXPORT_SYMBOL_GPL(posix_timer_event);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
