Re: [PATCH 2.6.13-rc6-rt9] PI aware dynamic priority adjustment

[Oleg Nesterov <oleg@tv-sign.ru>]

Thomas Gleixner wrote:
> 
> On Sat, 2005-08-20 at 18:10 +0400, Oleg Nesterov wrote:
> 
> > 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.
> 
> > posix_timer_event() runs under k_itimer.it_lock, but this does not
> > help if that thread was not the only one in thread group, in this
> > case we don't call exit_itimers().
> 
> I added exit_notify_itimers() for that case. It is synchronized vs.
> posix_timer_event() via the timer lock and therefor protects against the
> race described above.
> 
> It solves the problem for the only user of send_sigqueue for now, but I
> think we should have a more general interface to such functionality to
> allow simple usage.
> 
> tglx
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> diff -uprN --exclude-from=/usr/local/bin/diffit.exclude linux-2.6.13-rc6/include/linux/sched.h linux-2.6.13-rc6.work/include/linux/sched.h
> --- linux-2.6.13-rc6/include/linux/sched.h      2005-08-13 12:25:56.000000000 +0200
> +++ linux-2.6.13-rc6.work/include/linux/sched.h 2005-08-20 17:43:36.000000000 +0200
> @@ -1051,6 +1051,7 @@ extern void __exit_signal(struct task_st
>  extern void exit_sighand(struct task_struct *);
>  extern void __exit_sighand(struct task_struct *);
>  extern void exit_itimers(struct signal_struct *);
> +extern void exit_notify_itimers(struct signal_struct *);
> 
>  extern NORET_TYPE void do_group_exit(int);
> 
> diff -uprN --exclude-from=/usr/local/bin/diffit.exclude linux-2.6.13-rc6/kernel/posix-timers.c linux-2.6.13-rc6.work/kernel/posix-timers.c
> --- linux-2.6.13-rc6/kernel/posix-timers.c      2005-08-13 12:25:58.000000000 +0200
> +++ linux-2.6.13-rc6.work/kernel/posix-timers.c 2005-08-20 17:43:36.000000000 +0200
> @@ -1169,6 +1169,33 @@ void exit_itimers(struct signal_struct *
>  }
> 
>  /*
> + * This is called by __exit_signal, when there are still references to
> + * the shared signal_struct
> + */
> +void exit_notify_itimers(struct signal_struct *sig)
> +{
> +       struct k_itimer *timer;
> +       struct list_head *tmp;
> +       unsigned long flags;
> +
> +       list_for_each(tmp, &sig->posix_timers) {
> +
> +               timer = list_entry(tmp, struct k_itimer, list);
> +
> +               /* Protect against posix_timer_fn */
> +               spin_lock_irqsave(&timer->it_lock, flags);

I think this is deadlockable. We already (release_task) hold
tasklist_lock here. What if this timer has no SIGEV_THREAD_ID ?

posix_timer_fn locks timer->it_lock first, then locks tasklist_lock
in send_group_sigqueue().

Also, sys_timer_delete() locks ->it_lock, then ->sighand.lock.

I think the better fix would be re-check ->sighand in send_sigqueue,
like Paul's patches do.

Oleg.