Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932936AbbDMVCH (ORCPT ); Mon, 13 Apr 2015 17:02:07 -0400 Received: from www.linutronix.de ([62.245.132.108]:33717 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932832AbbDMVCC (ORCPT ); Mon, 13 Apr 2015 17:02:02 -0400 Message-Id: <20150413210035.099736663@linutronix.de> User-Agent: quilt/0.63-1 Date: Mon, 13 Apr 2015 21:02:20 -0000 From: Thomas Gleixner To: LKML Cc: Peter Zijlstra , Ingo Molnar , Ingo Tuchscherer , Martin Schwidefsky , Heiko Carstens , linux-s390@vger.kernel.org Subject: [patch 2/5] s390: crypto: Protect poll timeout update References: <20150413210009.682000343@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15 Content-Disposition: inline; filename=s390-crypto-protect-hrtimer-update.patch X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001,URIBL_BLOCKED=0.001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2964 Lines: 83 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 Cc: Ingo Tuchscherer Cc: Martin Schwidefsky Cc: Heiko Carstens 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; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/