From patchwork Thu Oct 19 20:28:38 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kees Cook X-Patchwork-Id: 828333 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=netdev-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="hRUQowJO"; dkim-atps=neutral Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3yJ0rT4K7yz9t5Q for ; Fri, 20 Oct 2017 07:28:57 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753737AbdJSU2o (ORCPT ); Thu, 19 Oct 2017 16:28:44 -0400 Received: from mail-pf0-f193.google.com ([209.85.192.193]:52384 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752581AbdJSU2m (ORCPT ); Thu, 19 Oct 2017 16:28:42 -0400 Received: by mail-pf0-f193.google.com with SMTP id e64so7626958pfk.9 for ; Thu, 19 Oct 2017 13:28:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:mime-version:content-disposition; bh=sTvvNY43L+MrFx5poj65oOr3n8NJRDUTEdG1fc9miwY=; b=hRUQowJOS8zlQF3SnmcfVw11jvLlBuNcxT10ssu2hhPdndADn6R9YoPYYkKx38ynoo 2DOdM6qG46wXr+z1C65W8W4L0FRGBvt+iCFVpODNRadExshR2LHo1dAhPptpoRaCL/6s ULM6TtMDISS/LFEjBgMsXS7OccN/p+l1ftKyA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:mime-version :content-disposition; bh=sTvvNY43L+MrFx5poj65oOr3n8NJRDUTEdG1fc9miwY=; b=g1qt59xb7xB2RuLYAP68oVg7MCS9fJ+OKzUmEbUsuLtsu34hAxlHD0CQqYArCvaCia NmCthBTKXu1T69pI/Fs5MKd1Auvu/6hV3dNeuNfDM5V6SZtcOVdjFAFZTSb/RpeSsNa+ of0Hod0ATmnFl/+G1dqE3Ci11bgmmDqredFf+bHlbLjm9TnoF4u2izQQtNQwmlWQIihX CP4HmnWFXQiqJZ5baFcVGrDAf6AwZJzMH79oFTrIj33rmzcVyC+ZQFGNJtfdo6o5ZIcF nCdhkpd3gzxI0mZXI35rvhRcvJDW883B3I/W3xYhcDIFVyVsZDKkhsLFhvaK5fPAVag+ bPNw== X-Gm-Message-State: AMCzsaWvrP9tRZJ83x+a3q+tW1phUwQ3bsFJTXCwA5IY68709V9boypz ZE4AZhlIp5hyEecO/rZ1sAFd8A== X-Google-Smtp-Source: ABhQp+QFkCGf5ykCaYAFZTU4Qj+e0ajLuCFx9naMD5DlcaKDgJ+6SKmiqQEpGId77xWJu2zJkUaCZA== X-Received: by 10.99.97.151 with SMTP id v145mr2374757pgb.184.1508444921983; Thu, 19 Oct 2017 13:28:41 -0700 (PDT) Received: from www.outflux.net (173-164-112-133-Oregon.hfc.comcastbusiness.net. [173.164.112.133]) by smtp.gmail.com with ESMTPSA id x27sm30195314pfe.172.2017.10.19.13.28.39 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 Oct 2017 13:28:39 -0700 (PDT) Date: Thu, 19 Oct 2017 13:28:38 -0700 From: Kees Cook To: Thomas Gleixner Cc: Craig Gallek , Eric Dumazet , "David S. Miller" , netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] timer: Provide wrappers safe for use with LOCKDEP Message-ID: <20171019202838.GA43223@beast> MIME-Version: 1.0 Content-Disposition: inline Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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: [] tcp_write_timer+0x24/0xd0 [ 580.848834] but task is already holding lock: [ 580.850107] ((timer)#2){+.-.}, at: [] 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. Reported-by: Craig Gallek Suggested-by: Eric Dumazet Signed-off-by: Kees Cook --- This means that all the trees taking conversions are at risk for harmless LOCKDEP warnings until this is landed. Putting this in v4.14 doesn't make sense since no one is going to rebase their trees. Also, it depends on the timer_setup_on_stack() function that was added in -next in timers/core. Any thoughts on the best way to deal with this? --- include/linux/timer.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/include/linux/timer.h b/include/linux/timer.h index 10685c33e679..09950482309b 100644 --- a/include/linux/timer.h +++ b/include/linux/timer.h @@ -150,6 +150,7 @@ static inline void init_timer_on_stack_key(struct timer_list *timer, #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) @@ -165,6 +166,19 @@ static inline void timer_setup_on_stack(struct timer_list *timer, __setup_timer_on_stack(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)