Re: [PATCH 1/2] signal: simplify deadlock-avoidance in lock_task_sighand()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Tue, Sep 23, 2014 at 09:03:48PM +0200, Oleg Nesterov wrote:
> On 09/22, Steven Rostedt wrote:
> >
> > On Mon, 22 Sep 2014 21:11:30 +0200
> > Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > > > @@ -1261,30 +1261,25 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
> > > > >  					   unsigned long *flags)
> > > > >  {
> > > > >  	struct sighand_struct *sighand;
> > > > > -
> > > > > +	/*
> > > > > +	 * We are going to do rcu_read_unlock() under spin_lock_irqsave().
> > > > > +	 * Make sure we can not be preempted after rcu_read_lock(), see
> > > > > +	 * rcu_read_unlock() comment header for details.
> > > > > +	 */
> > > > > +	preempt_disable();
> > > >
> > > > The sad part is, this is going to break -rt.
> > >
> > > Hmm, why??
> >
> > Because in -rt, siglock is a mutex.
> 
> Yes, thanks... I thougt that -rt should handle this somehow, we have
> more examples of preempt_disable() + spin_lock().
> 
> OK, let's forger this patch. It was supposed to be a cleanup, it should
> not disturb -rt.
> 
> > > In fact this deadlock is not really possible in any case, scheduler locks
> > > should be fine under ->siglock (for example, signal_wake_up() is called
> > > under this lock).
> > >
> > > But, the comment above rcu_read_unlock() says:
> > >
> > > 	Given that the set of locks acquired by rt_mutex_unlock() might change
> > > 	at any time, a somewhat more future-proofed approach is to make sure
> > > 	that that preemption never happens ...
> >
> > Hmm, I'm not sure we need to worry about this. As in -rt siglock is a
> > mutex, which is rt_mutex() itself, I highly doubt we will have
> > rt_mutex_unlock() grab siglock, otherwise that would cause havoc in -rt.
> 
> Yes. And, the changelog in a841796f "signal: align __lock_task_sighand() irq
> disabling and RCU" says:
> 
> 	It is therefore possible that this RCU read-side critical
> 	section will be preempted and later RCU priority boosted, which means
> 	that rcu_read_unlock() will call rt_mutex_unlock() in order to deboost
> 	itself, but with interrupts disabled. This results in lockdep splats
> 	...
> 	It is quite possible that a better long-term fix is to make rt_mutex_unlock()
> 	disable irqs when acquiring the rt_mutex structure's ->wait_lock.
> 
> but this doesn't look right, raw_spin_lock(&lock->wait_lock) should be
> fine with irqs disabled or I am totally confused. rt_mutex_adjust_prio()
> does _irqsave/irqrestore, so this can't enable interrupts.
> 
> Paul, will you agree if we turn it into

If you guys continue the guarantee of no deadlock, I am OK with this change.

							Thanx, Paul

> struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
> 					   unsigned long *flags)
> {
> 	struct sighand_struct *sighand;
> 
> 	rcu_read_lock();
> 	for (;;) {
> 		sighand = rcu_dereference(tsk->sighand);
> 		if (unlikely(sighand == NULL))
> 			break;
> 
> 		spin_lock_irqsave(&sighand->siglock, *flags);
> 		if (likely(sighand == tsk->sighand))
> 			break;
> 		spin_unlock_irqrestore(&sighand->siglock, *flags);
> 	}
> 	/*
> 	 * On the succesfull return we hold ->siglock. According to comment
> 	 * above rcu_read_unlock() this is against the rules, but scheduler
> 	 * locks are fine under this lock, signal_wake_up() takes them too.
> 	 */
> 	rcu_read_unlock();
> 
> 	return sighand;
> }
> 
> ?
> 
> Or I can leave this code alone, this is the minor cleanup. Just to me this
> sequence
> 
> 	local_irq_save();
> 	rcu_read_lock();
> 	spin_lock();
> 
> looks a bit confusing/annoying even with the comment.
> 
> Oleg.
>