Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1736798 > unrolled thread

[PATCH RT] locking/rtmutex: don't drop the wait_lock twice

Back to article view | Back to linux.kernel

Contents

  [PATCH RT] locking/rtmutex: don't drop the wait_lock twice Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-09-21 17:50 +0200
    Re: [PATCH RT] locking/rtmutex: don't drop the wait_lock twice Steven Rostedt <rostedt@goodmis.org> - 2017-09-21 18:40 +0200
      Re: [PATCH RT] locking/rtmutex: don't drop the wait_lock twice Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-09-21 18:50 +0200
        Re: [PATCH RT] locking/rtmutex: don't drop the wait_lock twice Steven Rostedt <rostedt@goodmis.org> - 2017-09-21 19:00 +0200
          Re: [PATCH RT] locking/rtmutex: don't drop the wait_lock twice Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-09-21 19:40 +0200
            Re: [PATCH RT] locking/rtmutex: don't drop the wait_lock twice Steven Rostedt <rostedt@goodmis.org> - 2017-09-21 20:00 +0200
              Re: [PATCH RT] locking/rtmutex: don't drop the wait_lock twice Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-09-21 20:20 +0200
                [PATCH RT v2] locking/rtmutex: don't drop the wait_lock twice Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-09-21 20:50 +0200

#1736798 — [PATCH RT] locking/rtmutex: don't drop the wait_lock twice

Since the futex rework, __rt_mutex_start_proxy_lock() does no longer
acquire the wait_lock so it must not drop it. Otherwise the lock is not
only unlocked twice but also the preemption counter is underflown.

Cc: rt-stable@vger.kernel.org
Reported-by: Gusenleitner Klaus <gus@keba.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/locking/rtmutex.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index f03876322d4a..79f49d73e4d0 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -2281,7 +2281,6 @@ int __rt_mutex_start_proxy_lock(struct rt_mutex *lock,
 	raw_spin_lock(&task->pi_lock);
 	if (task->pi_blocked_on) {
 		raw_spin_unlock(&task->pi_lock);
-		raw_spin_unlock_irq(&lock->wait_lock);
 		return -EAGAIN;
 	}
 	task->pi_blocked_on = PI_REQUEUE_INPROGRESS;
-- 
2.14.1

[toc] | [next] | [standalone]

#1736837

On Thu, 21 Sep 2017 17:48:43 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> Since the futex rework, __rt_mutex_start_proxy_lock() does no longer
> acquire the wait_lock so it must not drop it. Otherwise the lock is not
> only unlocked twice but also the preemption counter is underflown.
> 
> Cc: rt-stable@vger.kernel.org
> Reported-by: Gusenleitner Klaus <gus@keba.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/locking/rtmutex.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> index f03876322d4a..79f49d73e4d0 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -2281,7 +2281,6 @@ int __rt_mutex_start_proxy_lock(struct rt_mutex *lock,
>  	raw_spin_lock(&task->pi_lock);
>  	if (task->pi_blocked_on) {
>  		raw_spin_unlock(&task->pi_lock);
> -		raw_spin_unlock_irq(&lock->wait_lock);

Hmm, before this patch, irqs are enabled when returning with -EAGAIN.
But now they are not. Should that be:

		raw_spin_unlock_irq(&taks->pi_lock);

or is there something that changes this?

-- Steve


>  		return -EAGAIN;
>  	}
>  	task->pi_blocked_on = PI_REQUEUE_INPROGRESS;

[toc] | [prev] | [next] | [standalone]

#1736843

On 2017-09-21 12:31:05 [-0400], Steven Rostedt wrote:
> > diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> > index f03876322d4a..79f49d73e4d0 100644
> > --- a/kernel/locking/rtmutex.c
> > +++ b/kernel/locking/rtmutex.c
> > @@ -2281,7 +2281,6 @@ int __rt_mutex_start_proxy_lock(struct rt_mutex *lock,
> >  	raw_spin_lock(&task->pi_lock);
> >  	if (task->pi_blocked_on) {
> >  		raw_spin_unlock(&task->pi_lock);
> > -		raw_spin_unlock_irq(&lock->wait_lock);
> 
> Hmm, before this patch, irqs are enabled when returning with -EAGAIN.
> But now they are not. Should that be:
> 
> 		raw_spin_unlock_irq(&taks->pi_lock);
> 
> or is there something that changes this?

There is something else. Before that futex rework there was just
rt_mutex_start_proxy_lock() and it did lock & unlock of ->wait_lock.
This no longer the case after the rework. So now the caller does this.

> -- Steve

Sebastian

[toc] | [prev] | [next] | [standalone]

#1736854

On Thu, 21 Sep 2017 18:43:02 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 2017-09-21 12:31:05 [-0400], Steven Rostedt wrote:
> > > diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> > > index f03876322d4a..79f49d73e4d0 100644
> > > --- a/kernel/locking/rtmutex.c
> > > +++ b/kernel/locking/rtmutex.c
> > > @@ -2281,7 +2281,6 @@ int __rt_mutex_start_proxy_lock(struct rt_mutex *lock,
> > >  	raw_spin_lock(&task->pi_lock);
> > >  	if (task->pi_blocked_on) {
> > >  		raw_spin_unlock(&task->pi_lock);
> > > -		raw_spin_unlock_irq(&lock->wait_lock);  
> > 
> > Hmm, before this patch, irqs are enabled when returning with -EAGAIN.
> > But now they are not. Should that be:
> > 
> > 		raw_spin_unlock_irq(&taks->pi_lock);
> > 
> > or is there something that changes this?  
> 
> There is something else. Before that futex rework there was just
> rt_mutex_start_proxy_lock() and it did lock & unlock of ->wait_lock.
> This no longer the case after the rework. So now the caller does this.
>

So this actually fixes two bugs then? Anyway, probably want to add that
in the change log to explain why it is ok to change the irq semantics
here too.

Thanks!

-- Steve

[toc] | [prev] | [next] | [standalone]

#1736972

On 2017-09-21 12:50:27 [-0400], Steven Rostedt wrote:
> On Thu, 21 Sep 2017 18:43:02 +0200
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > On 2017-09-21 12:31:05 [-0400], Steven Rostedt wrote:
> > > > diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> > > > index f03876322d4a..79f49d73e4d0 100644
> > > > --- a/kernel/locking/rtmutex.c
> > > > +++ b/kernel/locking/rtmutex.c
> > > > @@ -2281,7 +2281,6 @@ int __rt_mutex_start_proxy_lock(struct rt_mutex *lock,
> > > >  	raw_spin_lock(&task->pi_lock);
> > > >  	if (task->pi_blocked_on) {
> > > >  		raw_spin_unlock(&task->pi_lock);
> > > > -		raw_spin_unlock_irq(&lock->wait_lock);  
> > > 
> > > Hmm, before this patch, irqs are enabled when returning with -EAGAIN.
> > > But now they are not. Should that be:
> > > 
> > > 		raw_spin_unlock_irq(&taks->pi_lock);
> > > 
> > > or is there something that changes this?  
> > 
> > There is something else. Before that futex rework there was just
> > rt_mutex_start_proxy_lock() and it did lock & unlock of ->wait_lock.
> > This no longer the case after the rework. So now the caller does this.
> >
> 
> So this actually fixes two bugs then? Anyway, probably want to add that
> in the change log to explain why it is ok to change the irq semantics
> here too.

No, it does not. It fixes only one and this one was introduced while I
rebased RT ontop of the futex work - the patch "futex: Fix bug on when a
requeued RT task times out" to be exact.
If you look at the code in the v4.9 or v4.11 RT then you see that there
is just rt_mutex_start_proxy_lock()() and this function acquires and
releases ->wait_lock lock. After the futex rework the locking changed
and I missed to adapt the RT-only patch I mentioned.
Again: without the patch, the ->wait_lock is dropped twice in the error
case here: once here and the second time by the caller and this has only
been like this since the futex-rework. So this does not apply to v4.1-RT
for instance because the futex rework got into v4.9.18-rt14 and I don't
recall that you backported it.

> Thanks!
> 
> -- Steve

Sebastian

[toc] | [prev] | [next] | [standalone]

#1736983

On Thu, 21 Sep 2017 19:35:12 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> No, it does not. It fixes only one and this one was introduced while I
> rebased RT ontop of the futex work - the patch "futex: Fix bug on when a
> requeued RT task times out" to be exact.
> If you look at the code in the v4.9 or v4.11 RT then you see that there
> is just rt_mutex_start_proxy_lock()() and this function acquires and
> releases ->wait_lock lock. After the futex rework the locking changed
> and I missed to adapt the RT-only patch I mentioned.
> Again: without the patch, the ->wait_lock is dropped twice in the error
> case here: once here and the second time by the caller and this has only
> been like this since the futex-rework. So this does not apply to v4.1-RT
> for instance because the futex rework got into v4.9.18-rt14 and I don't
> recall that you backported it.

Looking at the code, you are correct that it only fixes one bug. But I
would still include a statement about why there's a change in semantics
here (there still is, the old way enabled interrupts before returning,
this way does not). The reason this is OK, is because the
spin_unlock_irq() is called immediately after this function.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1736992

On 2017-09-21 13:51:35 [-0400], Steven Rostedt wrote:
> On Thu, 21 Sep 2017 19:35:12 +0200
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > No, it does not. It fixes only one and this one was introduced while I
> > rebased RT ontop of the futex work - the patch "futex: Fix bug on when a
> > requeued RT task times out" to be exact.
> > If you look at the code in the v4.9 or v4.11 RT then you see that there
> > is just rt_mutex_start_proxy_lock()() and this function acquires and
> > releases ->wait_lock lock. After the futex rework the locking changed
> > and I missed to adapt the RT-only patch I mentioned.
> > Again: without the patch, the ->wait_lock is dropped twice in the error
> > case here: once here and the second time by the caller and this has only
> > been like this since the futex-rework. So this does not apply to v4.1-RT
> > for instance because the futex rework got into v4.9.18-rt14 and I don't
> > recall that you backported it.
> 
> Looking at the code, you are correct that it only fixes one bug. But I
> would still include a statement about why there's a change in semantics
> here (there still is, the old way enabled interrupts before returning,
> this way does not). The reason this is OK, is because the
> spin_unlock_irq() is called immediately after this function.

The spin_lock_irq() was never called in this function. I will update it.

> -- Steve

Sebastian

[toc] | [prev] | [next] | [standalone]

#1737010 — [PATCH RT v2] locking/rtmutex: don't drop the wait_lock twice

Since the futex rework, __rt_mutex_start_proxy_lock() does no longer
acquire the wait_lock so it must not drop it. Otherwise the lock is not
only unlocked twice but also the preemption counter is underflown.

It is okay to remove that line because this function does not disable
interrupts nor does it acquire the ->wait_lock. The caller does this
so it is wrong do it here (after the futex rework).

Cc: rt-stable@vger.kernel.org
Reported-by: Gusenleitner Klaus <gus@keba.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: update patch description.

 kernel/locking/rtmutex.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index f03876322d4a..79f49d73e4d0 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -2281,7 +2281,6 @@ int __rt_mutex_start_proxy_lock(struct rt_mutex *lock,
 	raw_spin_lock(&task->pi_lock);
 	if (task->pi_blocked_on) {
 		raw_spin_unlock(&task->pi_lock);
-		raw_spin_unlock_irq(&lock->wait_lock);
 		return -EAGAIN;
 	}
 	task->pi_blocked_on = PI_REQUEUE_INPROGRESS;
-- 
2.14.1

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web