* [patch 2/5] s390: crypto: Protect poll timeout update
[not found] <20150413210009.682000343@linutronix.de>
@ 2015-04-13 21:02 ` Thomas Gleixner
2015-04-17 13:27 ` Ingo Tuchscherer
0 siblings, 1 reply; 2+ messages in thread
From: Thomas Gleixner @ 2015-04-13 21:02 UTC (permalink / raw)
To: LKML
Cc: Peter Zijlstra, Ingo Molnar, Ingo Tuchscherer, Martin Schwidefsky,
Heiko Carstens, linux-s390
[-- Attachment #1: s390-crypto-protect-hrtimer-update.patch --]
[-- Type: text/plain, Size: 2717 bytes --]
The poll timeout update via sysfs runs completely unprotected against
other usage sites of the timer. Take the proper lock before fiddling
with the timer.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Tuchscherer <ingo.tuchscherer@de.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: linux-s390@vger.kernel.org
---
P.S: I have serious doubts about the following snippets in that code:
@@ -1174,11 +1174,13 @@ static ssize_t poll_timeout_store(struct
poll_timeout = time;
hr_time = ktime_set(0, poll_timeout);
spin_lock_bh(&ap_poll_timer_lock);
if (!hrtimer_is_queued(&ap_poll_timer) ||
--> !hrtimer_forward(&ap_poll_timer, hrtimer_get_expires(&ap_poll_timer), hr_time)) {
Why does it check, whether the forwarding resulted in an overrun?
If the timer is not queued, then it is either expired or has never
been started. So if the poll timeout gets written, then the timer is
started when
timer->expires + hr_time < now
This results in random behaviour at best.
hrtimer_set_expires(&ap_poll_timer, hr_time);
hrtimer_start_expires(&ap_poll_timer, HRTIMER_MODE_ABS);
}
@@ -1552,11 +1553,16 @@ static inline void __ap_schedule_poll_timeout
spin_lock_bh(&ap_poll_timer_lock);
if (hrtimer_is_queued(&ap_poll_timer) || ap_suspend_flag)
goto out;
---> if (ktime_to_ns(hrtimer_expires_remaining(&ap_poll_timer)) <= 0) {
The check above does not make any sense either. Again, if the timer is
not queued then it is either expired or was never started. As the
timer is never canceled except on module unload the condition is
always true.
hr_time = ktime_set(0, poll_timeout);
hrtimer_forward_now(&ap_poll_timer, hr_time);
hrtimer_restart(&ap_poll_timer);
}
But that's not scope of that patch and I leave this to the s390
wizards to digest.
---
drivers/s390/crypto/ap_bus.c | 2 ++
1 file changed, 2 insertions(+)
Index: linux/drivers/s390/crypto/ap_bus.c
===================================================================
--- linux.orig/drivers/s390/crypto/ap_bus.c
+++ linux/drivers/s390/crypto/ap_bus.c
@@ -1174,11 +1174,13 @@ static ssize_t poll_timeout_store(struct
poll_timeout = time;
hr_time = ktime_set(0, poll_timeout);
+ spin_lock_bh(&ap_poll_timer_lock);
if (!hrtimer_is_queued(&ap_poll_timer) ||
!hrtimer_forward(&ap_poll_timer, hrtimer_get_expires(&ap_poll_timer), hr_time)) {
hrtimer_set_expires(&ap_poll_timer, hr_time);
hrtimer_start_expires(&ap_poll_timer, HRTIMER_MODE_ABS);
}
+ spin_unlock_bh(&ap_poll_timer_lock);
return count;
}
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [patch 2/5] s390: crypto: Protect poll timeout update
2015-04-13 21:02 ` [patch 2/5] s390: crypto: Protect poll timeout update Thomas Gleixner
@ 2015-04-17 13:27 ` Ingo Tuchscherer
0 siblings, 0 replies; 2+ messages in thread
From: Ingo Tuchscherer @ 2015-04-17 13:27 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, Peter Zijlstra, Ingo Molnar, mschwid2, heicars2, linux-s390
[-- Attachment #1: Type: text/plain, Size: 6381 bytes --]
You're right. Thanks for pointing this out.
I'll fix these issues and bring them upstream.
Mit freundlichen Grüßen / Kind regards
Ingo Tuchscherer
Software Development - Linux on z Systems
IBM Systems &Technology Group
Phone: +49-7031-16-1986 IBM Deutschland (Embedded
image moved
to file:
pic14137.gif)
E-Mail: ingo.tuchscherer@de.ibm.com Schoenaicher Str. 220
71032 Boeblingen
Germany
IBM Deutschland
Research &
Development
GmbH /
Vorsitzender des
Aufsichtsrats:
Martina Koederitz
Geschäftsführung:
Dirk Wittkopp
Sitz der
Gesellschaft:
Böblingen /
Registergericht:
Amtsgericht
Stuttgart, HRB
243294
From: Thomas Gleixner <tglx@linutronix.de>
To: LKML <linux-kernel@vger.kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar
<mingo@kernel.org>, Ingo Tuchscherer/Germany/IBM@IBMDE,
mschwid2@linux.vnet.ibm.com, heicars2@linux.vnet.ibm.com,
linux-s390@vger.kernel.org
Date: 04/13/2015 11:02 PM
Subject: [patch 2/5] s390: crypto: Protect poll timeout update
The poll timeout update via sysfs runs completely unprotected against
other usage sites of the timer. Take the proper lock before fiddling
with the timer.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Tuchscherer <ingo.tuchscherer@de.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: linux-s390@vger.kernel.org
---
P.S: I have serious doubts about the following snippets in that code:
@@ -1174,11 +1174,13 @@ static ssize_t poll_timeout_store(struct
poll_timeout = time;
hr_time = ktime_set(0, poll_timeout);
spin_lock_bh(&ap_poll_timer_lock);
if (!hrtimer_is_queued(&ap_poll_timer) ||
--> !hrtimer_forward(&ap_poll_timer, hrtimer_get_expires
(&ap_poll_timer), hr_time)) {
Why does it check, whether the forwarding resulted in an overrun?
If the timer is not queued, then it is either expired or has never
been started. So if the poll timeout gets written, then the timer is
started when
timer->expires + hr_time < now
This results in random behaviour at best.
hrtimer_set_expires(&ap_poll_timer, hr_time);
hrtimer_start_expires(&ap_poll_timer, HRTIMER_MODE_ABS);
}
@@ -1552,11 +1553,16 @@ static inline void __ap_schedule_poll_timeout
spin_lock_bh(&ap_poll_timer_lock);
if (hrtimer_is_queued(&ap_poll_timer) || ap_suspend_flag)
goto out;
---> if (ktime_to_ns(hrtimer_expires_remaining(&ap_poll_timer)) <= 0) {
The check above does not make any sense either. Again, if the timer is
not queued then it is either expired or was never started. As the
timer is never canceled except on module unload the condition is
always true.
hr_time = ktime_set(0, poll_timeout);
hrtimer_forward_now(&ap_poll_timer, hr_time);
hrtimer_restart(&ap_poll_timer);
}
But that's not scope of that patch and I leave this to the s390
wizards to digest.
---
drivers/s390/crypto/ap_bus.c | 2 ++
1 file changed, 2 insertions(+)
Index: linux/drivers/s390/crypto/ap_bus.c
===================================================================
--- linux.orig/drivers/s390/crypto/ap_bus.c
+++ linux/drivers/s390/crypto/ap_bus.c
@@ -1174,11 +1174,13 @@ static ssize_t poll_timeout_store(struct
poll_timeout = time;
hr_time = ktime_set(0, poll_timeout);
+ spin_lock_bh(&ap_poll_timer_lock);
if (!hrtimer_is_queued(&ap_poll_timer) ||
!hrtimer_forward(&ap_poll_timer, hrtimer_get_expires
(&ap_poll_timer), hr_time)) {
hrtimer_set_expires(&ap_poll_timer, hr_time);
hrtimer_start_expires(&ap_poll_timer,
HRTIMER_MODE_ABS);
}
+ spin_unlock_bh(&ap_poll_timer_lock);
return count;
}
[-- Attachment #2: pic14137.gif --]
[-- Type: image/gif, Size: 1851 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2015-04-17 13:27 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20150413210009.682000343@linutronix.de>
2015-04-13 21:02 ` [patch 2/5] s390: crypto: Protect poll timeout update Thomas Gleixner
2015-04-17 13:27 ` Ingo Tuchscherer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox