[PATCH v2 0/2] rtmutex wait_lock is irq safe

[PATCH v2 0/2] rtmutex wait_lock is irq safe

[Anna-Maria Gleixner]

Since commit b4abf91047cf ("rtmutex: Make wait_lock irq safe") the rtmutex
wait_lock is irq safe. Therefore the irqsave/restore in kernel/signal is no
longer required (see Patch 2/2). During discussions about v1 of this patch,
Eric Biederman noticed, that there is a no longer valid rcu_read_unlock()
documentation.

Therefore sending a short queue: fixing first the documentation of
rcu_read_unlock() and afterwards removing irqsave/restore in kernel/signal.

v1..v2:

 - Add new patch updating rcu documentation as suggested by Eric Biederman
 - Udpate commit message of kernel/signal patch

Thanks,

	Anna-Maria


Anna-Maria Gleixner (2):
  rcu: Update documentation of rcu_read_unlock()
  signal: Remove no longer required irqsave/restore

 include/linux/rcupdate.h |  4 +---
 kernel/signal.c          | 24 +++++++-----------------
 2 files changed, 8 insertions(+), 20 deletions(-)

-- 
2.15.1

[PATCH v2 1/2] rcu: Update documentation of rcu_read_unlock()

[Anna-Maria Gleixner]

Since commit b4abf91047cf ("rtmutex: Make wait_lock irq safe") the
explanation in rcu_read_unlock() documentation about irq unsafe rtmutex
wait_lock is no longer valid.

Remove it to prevent kernel developers reading the documentation to rely on
it.

Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
---
 include/linux/rcupdate.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 36360d07f25b..64644fda3b22 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -653,9 +653,7 @@ static inline void rcu_read_lock(void)
  * Unfortunately, this function acquires the scheduler's runqueue and
  * priority-inheritance spinlocks.  This means that deadlock could result
  * if the caller of rcu_read_unlock() already holds one of these locks or
- * any lock that is ever acquired while holding them; or any lock which
- * can be taken from interrupt context because rcu_boost()->rt_mutex_lock()
- * does not disable irqs while taking ->wait_lock.
+ * any lock that is ever acquired while holding them.
  *
  * That said, RCU readers are never priority boosted unless they were
  * preempted.  Therefore, one way to avoid deadlock is to make sure
-- 
2.15.1

Re: [PATCH v2 1/2] rcu: Update documentation of rcu_read_unlock()

[Paul E. McKenney]

On Fri, May 25, 2018 at 11:05:06AM +0200, Anna-Maria Gleixner wrote:
> Since commit b4abf91047cf ("rtmutex: Make wait_lock irq safe") the
> explanation in rcu_read_unlock() documentation about irq unsafe rtmutex
> wait_lock is no longer valid.
> 
> Remove it to prevent kernel developers reading the documentation to rely on
> it.
> 
> Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
> Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Or let me know if you would like me to carry this patch.  Either way,
just let me know!

							Thanx, Paul

> ---
>  include/linux/rcupdate.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 36360d07f25b..64644fda3b22 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -653,9 +653,7 @@ static inline void rcu_read_lock(void)
>   * Unfortunately, this function acquires the scheduler's runqueue and
>   * priority-inheritance spinlocks.  This means that deadlock could result
>   * if the caller of rcu_read_unlock() already holds one of these locks or
> - * any lock that is ever acquired while holding them; or any lock which
> - * can be taken from interrupt context because rcu_boost()->rt_mutex_lock()
> - * does not disable irqs while taking ->wait_lock.
> + * any lock that is ever acquired while holding them.
>   *
>   * That said, RCU readers are never priority boosted unless they were
>   * preempted.  Therefore, one way to avoid deadlock is to make sure
> -- 
> 2.15.1
> 

Re: [PATCH v2 1/2] rcu: Update documentation of rcu_read_unlock()

[Anna-Maria Gleixner]

On Fri, 25 May 2018, Paul E. McKenney wrote:

> On Fri, May 25, 2018 at 11:05:06AM +0200, Anna-Maria Gleixner wrote:
> > Since commit b4abf91047cf ("rtmutex: Make wait_lock irq safe") the
> > explanation in rcu_read_unlock() documentation about irq unsafe rtmutex
> > wait_lock is no longer valid.
> > 
> > Remove it to prevent kernel developers reading the documentation to rely on
> > it.
> > 
> > Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
> > Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
> 
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> Or let me know if you would like me to carry this patch.  Either way,
> just let me know!
> 

Thanks! Thomas told be he will take both.

Anna-Maria


> 
> > ---
> >  include/linux/rcupdate.h | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 36360d07f25b..64644fda3b22 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -653,9 +653,7 @@ static inline void rcu_read_lock(void)
> >   * Unfortunately, this function acquires the scheduler's runqueue and
> >   * priority-inheritance spinlocks.  This means that deadlock could result
> >   * if the caller of rcu_read_unlock() already holds one of these locks or
> > - * any lock that is ever acquired while holding them; or any lock which
> > - * can be taken from interrupt context because rcu_boost()->rt_mutex_lock()
> > - * does not disable irqs while taking ->wait_lock.
> > + * any lock that is ever acquired while holding them.
> >   *
> >   * That said, RCU readers are never priority boosted unless they were
> >   * preempted.  Therefore, one way to avoid deadlock is to make sure
> > -- 
> > 2.15.1
> > 
> 
> 

[tip:core/urgent] rcu: Update documentation of rcu_read_unlock()

[tip-bot for Anna-Maria Gleixner]

Commit-ID:  ec84b27f9b3b569f9235413d1945a2006b97b0aa
Gitweb:     https://git.kernel.org/tip/ec84b27f9b3b569f9235413d1945a2006b97b0aa
Author:     Anna-Maria Gleixner <anna-maria@linutronix.de>
AuthorDate: Fri, 25 May 2018 11:05:06 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 10 Jun 2018 06:14:01 +0200

rcu: Update documentation of rcu_read_unlock()

Since commit b4abf91047cf ("rtmutex: Make wait_lock irq safe") the
explanation in rcu_read_unlock() documentation about irq unsafe rtmutex
wait_lock is no longer valid.

Remove it to prevent kernel developers reading the documentation to rely on
it.

Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: bigeasy@linutronix.de
Link: https://lkml.kernel.org/r/20180525090507.22248-2-anna-maria@linutronix.de

---
 include/linux/rcupdate.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index e679b175b411..65163aa0bb04 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -652,9 +652,7 @@ static inline void rcu_read_lock(void)
  * Unfortunately, this function acquires the scheduler's runqueue and
  * priority-inheritance spinlocks.  This means that deadlock could result
  * if the caller of rcu_read_unlock() already holds one of these locks or
- * any lock that is ever acquired while holding them; or any lock which
- * can be taken from interrupt context because rcu_boost()->rt_mutex_lock()
- * does not disable irqs while taking ->wait_lock.
+ * any lock that is ever acquired while holding them.
  *
  * That said, RCU readers are never priority boosted unless they were
  * preempted.  Therefore, one way to avoid deadlock is to make sure

[PATCH v2 2/2] signal: Remove no longer required irqsave/restore

[Anna-Maria Gleixner]

Commit a841796f11c9 ("signal: align __lock_task_sighand() irq disabling and
RCU") introduced a rcu read side critical section with interrupts
disabled. The changelog suggested that a better long-term fix would be "to
make rt_mutex_unlock() disable irqs when acquiring the rt_mutex structure's
->wait_lock".

This long-term fix has been made in commit b4abf91047cf ("rtmutex: Make
wait_lock irq safe") for a different reason.

Therefore revert commit a841796f11c9 ("signal: align >
__lock_task_sighand() irq disabling and RCU") as the interrupt disable
dance is not longer required.

The change was tested on the base of b4abf91047cf ("rtmutex: Make wait_lock
irq safe") with a four hour run of rcutorture scenario TREE03 with lockdep
enabled as suggested by Paul McKenney.

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/signal.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 9c33163a6165..19679ad77aa6 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1244,19 +1244,12 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
 {
 	struct sighand_struct *sighand;
 
+	rcu_read_lock();
 	for (;;) {
-		/*
-		 * Disable interrupts early to avoid deadlocks.
-		 * See rcu_read_unlock() comment header for details.
-		 */
-		local_irq_save(*flags);
-		rcu_read_lock();
 		sighand = rcu_dereference(tsk->sighand);
-		if (unlikely(sighand == NULL)) {
-			rcu_read_unlock();
-			local_irq_restore(*flags);
+		if (unlikely(sighand == NULL))
 			break;
-		}
+
 		/*
 		 * This sighand can be already freed and even reused, but
 		 * we rely on SLAB_TYPESAFE_BY_RCU and sighand_ctor() which
@@ -1268,15 +1261,12 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
 		 * __exit_signal(). In the latter case the next iteration
 		 * must see ->sighand == NULL.
 		 */
-		spin_lock(&sighand->siglock);
-		if (likely(sighand == tsk->sighand)) {
-			rcu_read_unlock();
+		spin_lock_irqsave(&sighand->siglock, *flags);
+		if (likely(sighand == tsk->sighand))
 			break;
-		}
-		spin_unlock(&sighand->siglock);
-		rcu_read_unlock();
-		local_irq_restore(*flags);
+		spin_unlock_irqrestore(&sighand->siglock, *flags);
 	}
+	rcu_read_unlock();
 
 	return sighand;
 }
-- 
2.15.1

[tip:core/urgent] signal: Remove no longer required irqsave/restore

[tip-bot for Anna-Maria Gleixner]

Commit-ID:  59dc6f3c6d81c0c4379025c4eb56919391d62b67
Gitweb:     https://git.kernel.org/tip/59dc6f3c6d81c0c4379025c4eb56919391d62b67
Author:     Anna-Maria Gleixner <anna-maria@linutronix.de>
AuthorDate: Fri, 25 May 2018 11:05:07 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 10 Jun 2018 06:14:01 +0200

signal: Remove no longer required irqsave/restore

Commit a841796f11c9 ("signal: align __lock_task_sighand() irq disabling and
RCU") introduced a rcu read side critical section with interrupts
disabled. The changelog suggested that a better long-term fix would be "to
make rt_mutex_unlock() disable irqs when acquiring the rt_mutex structure's
->wait_lock".

This long-term fix has been made in commit b4abf91047cf ("rtmutex: Make
wait_lock irq safe") for a different reason.

Therefore revert commit a841796f11c9 ("signal: align >
__lock_task_sighand() irq disabling and RCU") as the interrupt disable
dance is not longer required.

The change was tested on the base of b4abf91047cf ("rtmutex: Make wait_lock
irq safe") with a four hour run of rcutorture scenario TREE03 with lockdep
enabled as suggested by Paul McKenney.

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: bigeasy@linutronix.de
Link: https://lkml.kernel.org/r/20180525090507.22248-3-anna-maria@linutronix.de

---
 kernel/signal.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 0f865d67415d..8d8a940422a8 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1244,19 +1244,12 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
 {
 	struct sighand_struct *sighand;
 
+	rcu_read_lock();
 	for (;;) {
-		/*
-		 * Disable interrupts early to avoid deadlocks.
-		 * See rcu_read_unlock() comment header for details.
-		 */
-		local_irq_save(*flags);
-		rcu_read_lock();
 		sighand = rcu_dereference(tsk->sighand);
-		if (unlikely(sighand == NULL)) {
-			rcu_read_unlock();
-			local_irq_restore(*flags);
+		if (unlikely(sighand == NULL))
 			break;
-		}
+
 		/*
 		 * This sighand can be already freed and even reused, but
 		 * we rely on SLAB_TYPESAFE_BY_RCU and sighand_ctor() which
@@ -1268,15 +1261,12 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
 		 * __exit_signal(). In the latter case the next iteration
 		 * must see ->sighand == NULL.
 		 */
-		spin_lock(&sighand->siglock);
-		if (likely(sighand == tsk->sighand)) {
-			rcu_read_unlock();
+		spin_lock_irqsave(&sighand->siglock, *flags);
+		if (likely(sighand == tsk->sighand))
 			break;
-		}
-		spin_unlock(&sighand->siglock);
-		rcu_read_unlock();
-		local_irq_restore(*flags);
+		spin_unlock_irqrestore(&sighand->siglock, *flags);
 	}
+	rcu_read_unlock();
 
 	return sighand;
 }

Re: [PATCH v2 0/2] rtmutex wait_lock is irq safe

[Eric W. Biederman]

Anna-Maria Gleixner <anna-maria@linutronix.de> writes:

> Since commit b4abf91047cf ("rtmutex: Make wait_lock irq safe") the rtmutex
> wait_lock is irq safe. Therefore the irqsave/restore in kernel/signal is no
> longer required (see Patch 2/2). During discussions about v1 of this patch,
> Eric Biederman noticed, that there is a no longer valid rcu_read_unlock()
> documentation.
>
> Therefore sending a short queue: fixing first the documentation of
> rcu_read_unlock() and afterwards removing irqsave/restore in
> kernel/signal.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

>
> v1..v2:
>
>  - Add new patch updating rcu documentation as suggested by Eric Biederman
>  - Udpate commit message of kernel/signal patch
>
> Thanks,
>
> 	Anna-Maria
>
>
> Anna-Maria Gleixner (2):
>   rcu: Update documentation of rcu_read_unlock()
>   signal: Remove no longer required irqsave/restore
>
>  include/linux/rcupdate.h |  4 +---
>  kernel/signal.c          | 24 +++++++-----------------
>  2 files changed, 8 insertions(+), 20 deletions(-)

[PATCH] kernel/signal: Remove no longer required irqsave/restore

[Sebastian Andrzej Siewior]

From: Anna-Maria Gleixner <anna-maria@linutronix.de>

Commit a841796f11c9 ("signal: align __lock_task_sighand() irq disabling and
RCU") introduced a rcu read side critical section with interrupts
disabled. The changelog suggested that a better long-term fix would be "to
make rt_mutex_unlock() disable irqs when acquiring the rt_mutex structure's
->wait_lock".

This long-term fix has been made in commit 4abf91047cf ("rtmutex: Make >
wait_lock irq safe") for different reason.

Therefore revert commit a841796f11c9 ("signal: align >
__lock_task_sighand() irq disabling and RCU") as the interrupt disable
dance is not longer required.

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/signal.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index c6e4c83dc090..16b87c54d027 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1244,19 +1244,12 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
 {
 	struct sighand_struct *sighand;
 
+	rcu_read_lock();
 	for (;;) {
-		/*
-		 * Disable interrupts early to avoid deadlocks.
-		 * See rcu_read_unlock() comment header for details.
-		 */
-		local_irq_save(*flags);
-		rcu_read_lock();
 		sighand = rcu_dereference(tsk->sighand);
-		if (unlikely(sighand == NULL)) {
-			rcu_read_unlock();
-			local_irq_restore(*flags);
+		if (unlikely(sighand == NULL))
 			break;
-		}
+
 		/*
 		 * This sighand can be already freed and even reused, but
 		 * we rely on SLAB_TYPESAFE_BY_RCU and sighand_ctor() which
@@ -1268,15 +1261,12 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
 		 * __exit_signal(). In the latter case the next iteration
 		 * must see ->sighand == NULL.
 		 */
-		spin_lock(&sighand->siglock);
-		if (likely(sighand == tsk->sighand)) {
-			rcu_read_unlock();
+		spin_lock_irqsave(&sighand->siglock, *flags);
+		if (likely(sighand == tsk->sighand))
 			break;
-		}
-		spin_unlock(&sighand->siglock);
-		rcu_read_unlock();
-		local_irq_restore(*flags);
+		spin_unlock_irqrestore(&sighand->siglock, *flags);
 	}
+	rcu_read_unlock();
 
 	return sighand;
 }
-- 
2.17.0

[tip:core/urgent] signal: Remove no longer required irqsave/restore

[tip-bot for Anna-Maria Gleixner]

Commit-ID:  e79e0f38083e607da5d7b493e7a0f78ba38d788e
Gitweb:     https://git.kernel.org/tip/e79e0f38083e607da5d7b493e7a0f78ba38d788e
Author:     Anna-Maria Gleixner <anna-maria@linutronix.de>
AuthorDate: Fri, 4 May 2018 16:40:14 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 7 Jun 2018 22:18:55 +0200

signal: Remove no longer required irqsave/restore

Commit a841796f11c9 ("signal: align __lock_task_sighand() irq disabling and
RCU") introduced a rcu read side critical section with interrupts
disabled. The changelog suggested that a better long-term fix would be "to
make rt_mutex_unlock() disable irqs when acquiring the rt_mutex structure's
->wait_lock".

This long-term fix has been made in commit 4abf91047cf ("rtmutex: Make >
wait_lock irq safe") for different reason.

Therefore revert commit a841796f11c9 ("signal: align >
__lock_task_sighand() irq disabling and RCU") as the interrupt disable
dance is not longer required.

Testing was done over an extensive period with RCU torture, especially with
TREE03 as requested by Paul.

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: "Paul E . McKenney" <paulmck@linux.vnet.ibm.com>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Link: https://lkml.kernel.org/r/20180504144014.5378-1-bigeasy@linutronix.de
---
 kernel/signal.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 0f865d67415d..8d8a940422a8 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1244,19 +1244,12 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
 {
 	struct sighand_struct *sighand;
 
+	rcu_read_lock();
 	for (;;) {
-		/*
-		 * Disable interrupts early to avoid deadlocks.
-		 * See rcu_read_unlock() comment header for details.
-		 */
-		local_irq_save(*flags);
-		rcu_read_lock();
 		sighand = rcu_dereference(tsk->sighand);
-		if (unlikely(sighand == NULL)) {
-			rcu_read_unlock();
-			local_irq_restore(*flags);
+		if (unlikely(sighand == NULL))
 			break;
-		}
+
 		/*
 		 * This sighand can be already freed and even reused, but
 		 * we rely on SLAB_TYPESAFE_BY_RCU and sighand_ctor() which
@@ -1268,15 +1261,12 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
 		 * __exit_signal(). In the latter case the next iteration
 		 * must see ->sighand == NULL.
 		 */
-		spin_lock(&sighand->siglock);
-		if (likely(sighand == tsk->sighand)) {
-			rcu_read_unlock();
+		spin_lock_irqsave(&sighand->siglock, *flags);
+		if (likely(sighand == tsk->sighand))
 			break;
-		}
-		spin_unlock(&sighand->siglock);
-		rcu_read_unlock();
-		local_irq_restore(*flags);
+		spin_unlock_irqrestore(&sighand->siglock, *flags);
 	}
+	rcu_read_unlock();
 
 	return sighand;
 }