linux-kernel - Re: lock_task_sighand() && rcu

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 4 May 2014 21:17:57 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: lock_task_sighand() && rcu_boost()

On 05/04, Paul E. McKenney wrote:
>
> On Sat, May 03, 2014 at 06:11:33PM +0200, Oleg Nesterov wrote:
> >
> > OK, if we can't rcu_read_unlock() with irqs disabled, then we can at least
> > cleanup it (and document the problem).
>
> Just to clarify (probably unnecessarily), it is OK to invoke rcu_read_unlock()
> with irqs disabled, but only if preemption has been disabled throughout
> the entire RCU read-side critical section.

Yes, yes, I understand, thanks.

> > and add rcu_read_unlock() into unlock_task_sighand().
>
> That should also work.

OK.

> > But. I simply can't understand why lockdep should complain? Why it is bad
> > to lock/unlock ->wait_lock with irqs disabled?
>
> Well, lockdep doesn't -always- complain, and some cases are OK.
>
> The problem is that if the RCU read-side critical section has been
> preempted, and if this task gets RCU priority-boosted in the meantime,
> then the task will need to acquire scheduler rq and pi locks at
> rcu_read_unlock() time.

Yes,

> If the reason that interrupts are disabled at
> rcu_read_unlock() time is that either rq or pi locks are held (or some
> other locks are held that are normally acquired while holding rq or
> pi locks), then we can deadlock.  And lockdep will of course complain.

Yes. but not in this case?

> If I recall corectly, at one point, the ->siglock lock was acquired
> while holding the rq locks, which would have resulted in lockdep
> complaints.

No, this must not be possible. signal_wake_up_state() was always called
under ->siglock and it does wake_up_state() which takes rq/pi locks.

And if lock_task_sighand() is preempted after rcu_read_lock(), then the
caller doesn't hold any lock.

So perhaps we can revert a841796f11c90d53 ?

Otherwise please see below.

> Hmmm...  A better description of the bad case might be as follows:
>
> 	Deadlock can occur if you have an RCU read-side critical
> 	section that is anywhere preemptible, and where the outermost
> 	rcu_read_unlock() is invoked while holding and lock acquired
> 	by either wakeup_next_waiter() or rt_mutex_adjust_prio(),
> 	or while holding any lock that is ever acquired while holding
> 	one of those locks.
>
> Does that help?
>
> Avoiding this bad case could be a bit ugly, as it is a dynamic set
> of locks that is acquired while holding any lock acquired by either
> wakeup_next_waiter() or rt_mutex_adjust_prio().  So I simplified the
> rule by prohibiting invoking rcu_read_unlock() with irqs disabled
> if the RCU read-side critical section had ever been preemptible.

OK, if you prefer to enforce this rule even if (say) lock_task_sighand()
is fine, then it needs the comment. And a cleanup ;)

We can move rcu_read_unlock() into unlock_task_sighand() as I suggested
before, or we can simply add preempt_disable/enable into lock_(),

	struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
						   unsigned long *flags)
	{
		struct sighand_struct *sighand;
		/*
		 * COMMENT TO EXPLAIN WHY
		 */
		preempt_disable();
		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);
		}
		rcu_read_unlock();
		preempt_enable();

		return sighand;
	}

The only problem is the "COMMENT" above. Perhaps the "prohibit invoking
rcu_read_unlock() with irqs disabled if ..." rule should documented
near/above rcu_read_unlock() ? In this case that COMMENT could simply
say "see the comment above rcu_read_unlock()".

What do you think?

Oleg.

--
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/