From 52f737c2da40259ac9962170ce608b6fb1b55ee4 Mon Sep 17 00:00:00 2001
From: Kees Cook <keescook@chromium.org>
Date: Thu, 19 Oct 2017 13:28:38 -0700
Subject: [PATCH] timer: Provide wrappers safe for use with LOCKDEP
Git-commit: 52f737c2da40259ac9962170ce608b6fb1b55ee4
Patch-mainline: v4.15-rc1
References: FATE#326887
Under LOCKDEP, the timer lock_class_key (set up in __setup_timer) needs
to be tied to the caller's context, so an inline for timer_setup()
won't work. We do, however, want to keep the inline version around for
argument type checking, though, so this provides macro wrappers in the
LOCKDEP case.
This fixes the case of different timers sharing the same LOCKDEP instance,
and producing a false positive warning:
[ 580.840858] ======================================================
[ 580.842299] WARNING: possible circular locking dependency detected
[ 580.843684] 4.14.0-rc4+ #17 Not tainted
[ 580.844554] ------------------------------------------------------
[ 580.845945] swapper/9/0 is trying to acquire lock:
[ 580.847024] (slock-AF_INET){+.-.}, at: [<ffffffff84ea4c34>] tcp_write_timer+0x24/0xd0
[ 580.848834]
but task is already holding lock:
[ 580.850107] ((timer)#2){+.-.}, at: [<ffffffff846df7c0>] call_timer_fn+0x0/0x300
[ 580.851663]
which lock already depends on the new lock.
[ 580.853439]
the existing dependency chain (in reverse order) is:
[ 580.855311]
-> #1 ((timer)#2){+.-.}:
[ 580.856538] __lock_acquire+0x114d/0x11a0
[ 580.857506] lock_acquire+0xb0/0x1d0
[ 580.858373] del_timer_sync+0x3c/0xb0
[ 580.859260] inet_csk_reqsk_queue_drop+0x7f/0x1b0
...
-> #0 (slock-AF_INET){+.-.}:
[ 580.884980] check_prev_add+0x666/0x700
[ 580.885790] __lock_acquire+0x114d/0x11a0
[ 580.886575] lock_acquire+0xb0/0x1d0
[ 580.887289] _raw_spin_lock+0x2c/0x40
[ 580.888021] tcp_write_timer+0x24/0xd0
...
[ 580.900055] Possible unsafe locking scenario:
[ 580.901043] CPU0 CPU1
[ 580.901797] ---- ----
[ 580.902540] lock((timer)#2);
[ 580.903046] lock(slock-AF_INET);
[ 580.904006] lock((timer)#2);
[ 580.904915] lock(slock-AF_INET);
[ 580.905502]
In this report, del_timer_sync() is from:
inet_csk_reqsk_queue_drop()
reqsk_queue_unlink()
del_timer_sync(&req->rsk_timer)
but tcp_write_timer()'s timer is attached to icsk_retransmit_timer. Both
had the same lock_class_key, since they were using timer_setup(). Switching
to a macro allows for a separate context, avoiding the false positive.
Fixes: 686fef928bba ("timer: Prepare to change timer callback argument type")
Reported-by: Craig Gallek <cgallek@google.com>
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: netdev@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>
Link: https://lkml.kernel.org/r/20171019202838.GA43223@beast
Acked-by: Takashi Iwai <tiwai@suse.de>
---
include/linux/timer.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -171,6 +171,7 @@ static inline void init_timer_on_stack_k
#define TIMER_DATA_TYPE unsigned long
#define TIMER_FUNC_TYPE void (*)(TIMER_DATA_TYPE)
+#ifndef CONFIG_LOCKDEP
static inline void timer_setup(struct timer_list *timer,
void (*callback)(struct timer_list *),
unsigned int flags)
@@ -178,6 +179,19 @@ static inline void timer_setup(struct ti
__setup_timer(timer, (TIMER_FUNC_TYPE)callback,
(TIMER_DATA_TYPE)timer, flags);
}
+#else
+/*
+ * Under LOCKDEP, the timer lock_class_key (set up in __init_timer) needs
+ * to be tied to the caller's context, so an inline (above) won't work. We
+ * do want to keep the inline for argument type checking, though.
+ */
+# define timer_setup(timer, callback, flags) \
+ __setup_timer(timer, (TIMER_FUNC_TYPE)callback, \
+ (TIMER_DATA_TYPE)timer, flags)
+# define timer_setup_on_stack(timer, callback, flags) \
+ __setup_timer_on_stack(timer, (TIMER_FUNC_TYPE)callback,\
+ (TIMER_DATA_TYPE)timer, flags)
+#endif
#define from_timer(var, callback_timer, timer_fieldname) \
container_of(callback_timer, typeof(*var), timer_fieldname)