Hello!
Additional RCU signal fixes to cover a race with exec() and to add
some rcu_dereference()s from earlier patches.
Signed-off-by: <[email protected]>
---
signal.c | 28 +++++++++++++++-------------
1 files changed, 15 insertions(+), 13 deletions(-)
diff -urpNa -X dontdiff linux-2.6.14-mm0/kernel/signal.c linux-2.6.14-mm0-fix/kernel/signal.c
--- linux-2.6.14-mm0/kernel/signal.c 2005-11-04 14:37:18.000000000 -0800
+++ linux-2.6.14-mm0-fix/kernel/signal.c 2005-11-04 17:23:40.000000000 -0800
@@ -337,7 +337,7 @@ void exit_sighand(struct task_struct *ts
write_lock_irq(&tasklist_lock);
rcu_read_lock();
if (tsk->sighand != NULL) {
- struct sighand_struct *sighand = tsk->sighand;
+ struct sighand_struct *sighand = rcu_dereference(tsk->sighand);
spin_lock(&sighand->siglock);
__exit_sighand(tsk);
spin_unlock(&sighand->siglock);
@@ -352,13 +352,14 @@ void exit_sighand(struct task_struct *ts
void __exit_signal(struct task_struct *tsk)
{
struct signal_struct * sig = tsk->signal;
- struct sighand_struct * sighand = tsk->sighand;
+ struct sighand_struct * sighand;
if (!sig)
BUG();
if (!atomic_read(&sig->count))
BUG();
rcu_read_lock();
+ sighand = rcu_dereference(tsk->sighand);
spin_lock(&sighand->siglock);
posix_cpu_timers_exit(tsk);
if (atomic_dec_and_test(&sig->count)) {
@@ -1100,7 +1101,7 @@ void zap_other_threads(struct task_struc
}
/*
- * Must be called with the tasklist_lock held for reading!
+ * Must be called under rcu_read_lock() or with tasklist_lock read-held.
*/
int group_send_sig_info(int sig, struct siginfo *info, struct task_struct *p)
{
@@ -1386,7 +1387,7 @@ send_sigqueue(int sig, struct sigqueue *
{
unsigned long flags;
int ret = 0;
- struct sighand_struct *sh = p->sighand;
+ struct sighand_struct *sh;
BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
@@ -1405,7 +1406,15 @@ send_sigqueue(int sig, struct sigqueue *
goto out_err;
}
+retry:
+ sh = rcu_dereference(p->sighand);
+
spin_lock_irqsave(&sh->siglock, flags);
+ if (p->sighand != sh) {
+ /* We raced with exec() in a multithreaded process... */
+ spin_unlock_irqrestore(&sh->siglock, flags);
+ goto retry;
+ }
/*
* We do the check here again to handle the following scenario:
@@ -1464,15 +1473,8 @@ send_group_sigqueue(int sig, struct sigq
BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
- while (!read_trylock(&tasklist_lock)) {
- if (!p->sighand)
- return -1;
- cpu_relax();
- }
- if (unlikely(!p->sighand)) {
- ret = -1;
- goto out_err;
- }
+ read_lock(&tasklist_lock);
+ /* Since it_lock is held, p->sighand cannot be NULL. */
spin_lock_irqsave(&p->sighand->siglock, flags);
handle_stop_signal(sig, p);
"Paul E. McKenney" wrote:
>
> @@ -1386,7 +1387,7 @@ send_sigqueue(int sig, struct sigqueue *
> {
> unsigned long flags;
> int ret = 0;
> - struct sighand_struct *sh = p->sighand;
> + struct sighand_struct *sh;
>
> BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
>
> @@ -1405,7 +1406,15 @@ send_sigqueue(int sig, struct sigqueue *
> goto out_err;
> }
>
> +retry:
> + sh = rcu_dereference(p->sighand);
> +
> spin_lock_irqsave(&sh->siglock, flags);
> + if (p->sighand != sh) {
> + /* We raced with exec() in a multithreaded process... */
> + spin_unlock_irqrestore(&sh->siglock, flags);
> + goto retry;
p->sighand can't be changed, de_thread calls exit_itimers() before
changing ->sighand. But I still think it can be NULL, and send_sigqueue()
should return -1 in that case.
> @@ -1464,15 +1473,8 @@ send_group_sigqueue(int sig, struct sigq
>
> BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
>
> - while (!read_trylock(&tasklist_lock)) {
> - if (!p->sighand)
> - return -1;
> - cpu_relax();
> - }
> - if (unlikely(!p->sighand)) {
> - ret = -1;
> - goto out_err;
> - }
> + read_lock(&tasklist_lock);
> + /* Since it_lock is held, p->sighand cannot be NULL. */
> spin_lock_irqsave(&p->sighand->siglock, flags);
Again, I think the comment is wrong.
However, now I think we really have a race with exec, and this race was not
introduced by your patches!
If !thread_group_leader() does exec de_thread() calls release_task(->group_leader)
before calling exit_itimers(). This means that send_group_sigqueue() which
always has p == ->group_leader parameter can oops here.
Oleg.
On Sat, Nov 05, 2005 at 07:32:47PM +0300, Oleg Nesterov wrote:
> "Paul E. McKenney" wrote:
> >
> > @@ -1386,7 +1387,7 @@ send_sigqueue(int sig, struct sigqueue *
> > {
> > unsigned long flags;
> > int ret = 0;
> > - struct sighand_struct *sh = p->sighand;
> > + struct sighand_struct *sh;
> >
> > BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
> >
> > @@ -1405,7 +1406,15 @@ send_sigqueue(int sig, struct sigqueue *
> > goto out_err;
> > }
> >
> > +retry:
> > + sh = rcu_dereference(p->sighand);
> > +
> > spin_lock_irqsave(&sh->siglock, flags);
> > + if (p->sighand != sh) {
> > + /* We raced with exec() in a multithreaded process... */
> > + spin_unlock_irqrestore(&sh->siglock, flags);
> > + goto retry;
>
> p->sighand can't be changed, de_thread calls exit_itimers() before
> changing ->sighand. But I still think it can be NULL, and send_sigqueue()
> should return -1 in that case.
OK, I put the NULL check in with my previous patch.
And you are absolutely right in the de_thread() case. I need to add
more cases to steamroller...
> > @@ -1464,15 +1473,8 @@ send_group_sigqueue(int sig, struct sigq
> >
> > BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
> >
> > - while (!read_trylock(&tasklist_lock)) {
> > - if (!p->sighand)
> > - return -1;
> > - cpu_relax();
> > - }
> > - if (unlikely(!p->sighand)) {
> > - ret = -1;
> > - goto out_err;
> > - }
> > + read_lock(&tasklist_lock);
> > + /* Since it_lock is held, p->sighand cannot be NULL. */
> > spin_lock_irqsave(&p->sighand->siglock, flags);
>
> Again, I think the comment is wrong.
>
> However, now I think we really have a race with exec, and this race was not
> introduced by your patches!
This patch was not mine, though I guess that it is by now. ;-)
> If !thread_group_leader() does exec de_thread() calls release_task(->group_leader)
> before calling exit_itimers(). This means that send_group_sigqueue() which
> always has p == ->group_leader parameter can oops here.
But in that case, __exit_sighand(->group_leader) would have been called,
so ->sighand would be NULL. And none of this can change while we are holding
tasklist_lock.
If we don't want to be hitting the exec()ed task with a signal, the
thing to do would be to drop the signal, as in the attached patch.
I believe that this is an acceptable approach, since had the timer
fired slightly later, it would have been disabled, right?
Thoughts?
Thanx, Paul
Signed-off-by: <[email protected]>
diff -urpNa -X dontdiff linux-2.6.14-mm0-fix-2/kernel/signal.c linux-2.6.14-mm0-fix-3/kernel/signal.c
--- linux-2.6.14-mm0-fix-2/kernel/signal.c 2005-11-05 15:05:38.000000000 -0800
+++ linux-2.6.14-mm0-fix-3/kernel/signal.c 2005-11-05 16:27:52.000000000 -0800
@@ -1481,6 +1481,10 @@ send_group_sigqueue(int sig, struct sigq
read_lock(&tasklist_lock);
while (p->group_leader != p)
p = p->group_leader;
+ if (p->sighand == NULL) {
+ ret = 1;
+ goto out_err;
+ }
spin_lock_irqsave(&p->sighand->siglock, flags);
handle_stop_signal(sig, p);
"Paul E. McKenney" wrote:
>
> > If !thread_group_leader() does exec de_thread() calls release_task(->group_leader)
> > before calling exit_itimers(). This means that send_group_sigqueue() which
> > always has p == ->group_leader parameter can oops here.
>
> But in that case, __exit_sighand(->group_leader) would have been called,
> so ->sighand would be NULL.
Yes, that is why (I think) oops can happen.
> And none of this can change while we are holding
> tasklist_lock.
Yes, but de_thread()->release_task(->group_leader) can take tasklist_lock
before us.
> If we don't want to be hitting the exec()ed task with a signal, the
> thing to do would be to drop the signal, as in the attached patch.
> I believe that this is an acceptable approach, since had the timer
> fired slightly later, it would have been disabled, right?
>
> Thoughts?
>
> Thanx, Paul
>
> Signed-off-by: <[email protected]>
>
> diff -urpNa -X dontdiff linux-2.6.14-mm0-fix-2/kernel/signal.c linux-2.6.14-mm0-fix-3/kernel/signal.c
> --- linux-2.6.14-mm0-fix-2/kernel/signal.c 2005-11-05 15:05:38.000000000 -0800
> +++ linux-2.6.14-mm0-fix-3/kernel/signal.c 2005-11-05 16:27:52.000000000 -0800
> @@ -1481,6 +1481,10 @@ send_group_sigqueue(int sig, struct sigq
> read_lock(&tasklist_lock);
> while (p->group_leader != p)
> p = p->group_leader;
> + if (p->sighand == NULL) {
> + ret = 1;
Oh, I think there is another problem here. I'll post a separate
message.
Oleg.
Oleg Nesterov wrote:
>
> "Paul E. McKenney" wrote:
> >
> > + if (p->sighand == NULL) {
> > + ret = 1;
>
> Oh, I think there is another problem here. I'll post a separate
> message.
Sorry, I was not clear. This problem is unrelated. Yes, I think we
should drop the signal. But please note that ret = 1 (sig_ignored)
means (surprise!) "reschedule and re-arm this timer right now" in
cpu-timer case. It is not critical, but may be ret = 0 is better.
Oleg.
On Sun, Nov 06, 2005 at 05:46:09PM +0300, Oleg Nesterov wrote:
> Oleg Nesterov wrote:
> >
> > "Paul E. McKenney" wrote:
> > >
> > > + if (p->sighand == NULL) {
> > > + ret = 1;
> >
> > Oh, I think there is another problem here. I'll post a separate
> > message.
>
> Sorry, I was not clear. This problem is unrelated. Yes, I think we
> should drop the signal. But please note that ret = 1 (sig_ignored)
> means (surprise!) "reschedule and re-arm this timer right now" in
> cpu-timer case. It is not critical, but may be ret = 0 is better.
OK. Seems like the next firing of the timer would then see the
changed situation, so the current code should at least be safe.
Thoughts?
Thanx, Paul