Messages in this thread Patches in this message | | | Subject | Re: [patch 2/5] s390: crypto: Protect poll timeout update | From | Ingo Tuchscherer <> | Date | Fri, 17 Apr 2015 15:27:26 +0200 |
| |
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; }
[unhandled content-type:image/gif]
| |