From patchwork Thu Oct 27 15:05:26 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Steven Rostedt X-Patchwork-Id: 11824 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp286506wru; Thu, 27 Oct 2022 08:10:55 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4vfsj0O26MlUtwzVNySGmkjqC+HoTBS97cYTmbG3tzl1nhAcPTxdeL21c5G3HWuEhiQPAj X-Received: by 2002:a17:907:75e9:b0:7a4:31c:ae0d with SMTP id jz9-20020a17090775e900b007a4031cae0dmr22901080ejc.79.1666883454861; Thu, 27 Oct 2022 08:10:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666883454; cv=none; d=google.com; s=arc-20160816; b=e5VZPjcVsZKPTEmESbSQ/NIhip2w3jdn4XBwRh//AKMSOmx3pziBMVFyygyle7A11W q309C0fpOH7ZdYt5g93xtwVpz0qrdfLonszKgL7Q+d+I5lW8m6RYTZRgP6OlOWWh3sbg hlAbi1hT34Z4S6noBU7CgObpIjH/9VpbxrPxws8YHcM8GcEb7+zc8YJYSo0xHAbaXG6A kZW2KWSSmdKhon5X6uETcxPRoXqoPqtNeJa4h+rfzRI5IcRU1RsNB95cJZd2H40NkrAE wW4Tj2g0XxSi5oSifkZL0ZCOO+A74uT4dBeadNkGFLQNzmfY+Xsoazw8NTG/WjZ7Ta9j PsrQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:references:subject:cc:to:from:date :user-agent:message-id; bh=T0HU7luPS6b76RCNSuiumirBJ19Zy5OZROh5GVDCjQQ=; b=e+75SRoLaY0SnTtRi9N2O2o63pNqOwiXpKlnnY4NI17oga0bTAWWNnaKS8esv7/A39 GI0n4lZiKU+FRNmVHuOE++W0GlG8xmQXZJU7xCEZn6QEbctmREE9s2FVm4XUzvy5MtaC VlNQwoiKjCmYD/3o/AZXX9K2cawiQDVhSuElVRp+isHYoo6PIdCUD89DiaeDr+XURYvm r/vUf/3RrHLRQG6xeROrzyyl+WoBw0qclBjnXwz/pIwWz7XLv8GnE2XEiXtBydUZktsz St5rgGLEqXv4HRilEv47CsAaHDwfhsZMDi7HqRhE9CHAibozt4efpoz3gtbAjt0qF/a5 Z/JA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id d1-20020a50cd41000000b004589ea2d983si1587373edj.287.2022.10.27.08.10.28; Thu, 27 Oct 2022 08:10:54 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236338AbiJ0PJt (ORCPT + 99 others); Thu, 27 Oct 2022 11:09:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56762 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235994AbiJ0PJP (ORCPT ); Thu, 27 Oct 2022 11:09:15 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 458A618F251 for ; Thu, 27 Oct 2022 08:09:13 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id C0717B826AA for ; Thu, 27 Oct 2022 15:09:11 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 61C9BC433D7; Thu, 27 Oct 2022 15:09:10 +0000 (UTC) Received: from rostedt by gandalf.local.home with local (Exim 4.96) (envelope-from ) id 1oo4VZ-00BvS7-1L; Thu, 27 Oct 2022 11:09:25 -0400 Message-ID: <20221027150925.248421571@goodmis.org> User-Agent: quilt/0.66 Date: Thu, 27 Oct 2022 11:05:26 -0400 From: Steven Rostedt To: linux-kernel@vger.kernel.org Cc: Linus Torvalds , Thomas Gleixner , Stephen Boyd , Guenter Roeck Subject: [RFC][PATCH v2 01/31] timers: Add del_timer_shutdown() to be called before freeing timers References: <20221027150525.753064657@goodmis.org> MIME-Version: 1.0 X-Spam-Status: No, score=-6.7 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747853985502613853?= X-GMAIL-MSGID: =?utf-8?q?1747853985502613853?= From: "Steven Rostedt (Google)" We are hitting a common bug were a timer is being triggered after it is freed. This causes a corruption in the timer link list and crashes the kernel. Unfortunately it is not easy to know what timer it was that was freed. Looking at the code, it appears that there are several cases that del_timer() is used when del_timer_sync() should have been. Add a del_timer_shutdown() that not only does a del_timer_sync() but will mark the timer as terminated in case it gets rearmed, it will trigger a WARN_ON. The del_timer_shutdown() is more likely to be used by developers that are about to free a timer, then using del_timer_sync() as the latter is not as obvious to being needed for freeing. Having the word "shutdown" in the name of the function will hopefully help developers know that that function needs to be called before freeing. The added bonus is the marking of the timer as being freed such that it will trigger a warning if it gets rearmed. At least that way if the system crashes on a freed timer, at least we may see which timer it was that was freed. This code is taken from Thomas Gleixner's "untested" version from my original patch and modified after testing and with some other comments from Linus addressed. As well as some extra comments added. Link: https://lore.kernel.org/all/87pmlrkgi3.ffs@tglx/ Signed-off-by: Steven Rostedt (Google) --- .../RCU/Design/Requirements/Requirements.rst | 2 +- Documentation/core-api/local_ops.rst | 2 +- Documentation/kernel-hacking/locking.rst | 4 ++ include/linux/timer.h | 27 +++++++++--- kernel/time/timer.c | 43 +++++++++++-------- 5 files changed, 54 insertions(+), 24 deletions(-) diff --git a/Documentation/RCU/Design/Requirements/Requirements.rst b/Documentation/RCU/Design/Requirements/Requirements.rst index a0f8164c8513..5c27f94a4fc2 100644 --- a/Documentation/RCU/Design/Requirements/Requirements.rst +++ b/Documentation/RCU/Design/Requirements/Requirements.rst @@ -1858,7 +1858,7 @@ unloaded. After a given module has been unloaded, any attempt to call one of its functions results in a segmentation fault. The module-unload functions must therefore cancel any delayed calls to loadable-module functions, for example, any outstanding mod_timer() must be dealt -with via del_timer_sync() or similar. +with via del_timer_shutdown(). Unfortunately, there is no way to cancel an RCU callback; once you invoke call_rcu(), the callback function is eventually going to be diff --git a/Documentation/core-api/local_ops.rst b/Documentation/core-api/local_ops.rst index 2ac3f9f29845..a3a63cd57006 100644 --- a/Documentation/core-api/local_ops.rst +++ b/Documentation/core-api/local_ops.rst @@ -191,7 +191,7 @@ Here is a sample module which implements a basic per cpu counter using static void __exit test_exit(void) { - del_timer_sync(&test_timer); + del_timer_shutdown(&test_timer); } module_init(test_init); diff --git a/Documentation/kernel-hacking/locking.rst b/Documentation/kernel-hacking/locking.rst index 6805ae6e86e6..c8b852ab8214 100644 --- a/Documentation/kernel-hacking/locking.rst +++ b/Documentation/kernel-hacking/locking.rst @@ -1009,6 +1009,10 @@ use del_timer_sync() (``include/linux/timer.h``) to handle this case. It returns the number of times the timer had to be deleted before we finally stopped it from adding itself back in. +Before freeing a timer, del_timer_shutdown() shoud be called which will keep +it from being rearmed, although if it is rearmed, it will produce a warning. + + Locking Speed ============= diff --git a/include/linux/timer.h b/include/linux/timer.h index 648f00105f58..daccfe33da42 100644 --- a/include/linux/timer.h +++ b/include/linux/timer.h @@ -183,12 +183,29 @@ extern int timer_reduce(struct timer_list *timer, unsigned long expires); extern void add_timer(struct timer_list *timer); extern int try_to_del_timer_sync(struct timer_list *timer); +extern int __del_timer_sync(struct timer_list *timer, bool free); -#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT) - extern int del_timer_sync(struct timer_list *timer); -#else -# define del_timer_sync(t) del_timer(t) -#endif +static inline int del_timer_sync(struct timer_list *timer) +{ + return __del_timer_sync(timer, false); +} + +/** + * del_timer_shutdown - called before freeing the timer + * @timer: The timer to be freed + * + * Shutdown the timer before freeing. This will return when all pending timers + * have finished and it is safe to free the timer. + * + * Note, after calling this, if the timer is added back to the queue + * it will fail to be added and a WARNING will be triggered. + * + * Returns if it deactivated a pending timer or not. + */ +static inline int del_timer_shutdown(struct timer_list *timer) +{ + return __del_timer_sync(timer, true); +} #define del_singleshot_timer_sync(t) del_timer_sync(t) diff --git a/kernel/time/timer.c b/kernel/time/timer.c index 717fcb9fb14a..111a3550b3f2 100644 --- a/kernel/time/timer.c +++ b/kernel/time/timer.c @@ -1017,7 +1017,8 @@ __mod_timer(struct timer_list *timer, unsigned long expires, unsigned int option unsigned int idx = UINT_MAX; int ret = 0; - BUG_ON(!timer->function); + if (WARN_ON_ONCE(!timer->function)) + return -EINVAL; /* * This is a common optimization triggered by the networking code - if @@ -1193,7 +1194,8 @@ EXPORT_SYMBOL(timer_reduce); */ void add_timer(struct timer_list *timer) { - BUG_ON(timer_pending(timer)); + if (WARN_ON_ONCE(timer_pending(timer))) + return; __mod_timer(timer, timer->expires, MOD_TIMER_NOTPENDING); } EXPORT_SYMBOL(add_timer); @@ -1210,7 +1212,8 @@ void add_timer_on(struct timer_list *timer, int cpu) struct timer_base *new_base, *base; unsigned long flags; - BUG_ON(timer_pending(timer) || !timer->function); + if (WARN_ON_ONCE(timer_pending(timer) || !timer->function)) + return; new_base = get_timer_cpu_base(timer->flags, cpu); @@ -1266,14 +1269,7 @@ int del_timer(struct timer_list *timer) } EXPORT_SYMBOL(del_timer); -/** - * try_to_del_timer_sync - Try to deactivate a timer - * @timer: timer to delete - * - * This function tries to deactivate a timer. Upon successful (ret >= 0) - * exit the timer is not queued and the handler is not running on any CPU. - */ -int try_to_del_timer_sync(struct timer_list *timer) +static int __try_to_del_timer_sync(struct timer_list *timer, bool free) { struct timer_base *base; unsigned long flags; @@ -1285,11 +1281,25 @@ int try_to_del_timer_sync(struct timer_list *timer) if (base->running_timer != timer) ret = detach_if_pending(timer, base, true); + if (free) + timer->function = NULL; raw_spin_unlock_irqrestore(&base->lock, flags); return ret; } + +/** + * try_to_del_timer_sync - Try to deactivate a timer + * @timer: timer to delete + * + * This function tries to deactivate a timer. Upon successful (ret >= 0) + * exit the timer is not queued and the handler is not running on any CPU. + */ +int try_to_del_timer_sync(struct timer_list *timer) +{ + return __try_to_del_timer_sync(timer, false); +} EXPORT_SYMBOL(try_to_del_timer_sync); #ifdef CONFIG_PREEMPT_RT @@ -1365,10 +1375,10 @@ static inline void timer_sync_wait_running(struct timer_base *base) { } static inline void del_timer_wait_running(struct timer_list *timer) { } #endif -#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT) /** - * del_timer_sync - deactivate a timer and wait for the handler to finish. + * __del_timer_sync - deactivate a timer and wait for the handler to finish. * @timer: the timer to be deactivated + * @free: Set to true if the timer is about to be freed * * This function only differs from del_timer() on SMP: besides deactivating * the timer it also makes sure the handler has finished executing on other @@ -1402,7 +1412,7 @@ static inline void del_timer_wait_running(struct timer_list *timer) { } * * The function returns whether it has deactivated a pending timer or not. */ -int del_timer_sync(struct timer_list *timer) +int __del_timer_sync(struct timer_list *timer, bool free) { int ret; @@ -1432,7 +1442,7 @@ int del_timer_sync(struct timer_list *timer) lockdep_assert_preemption_enabled(); do { - ret = try_to_del_timer_sync(timer); + ret = __try_to_del_timer_sync(timer, free); if (unlikely(ret < 0)) { del_timer_wait_running(timer); @@ -1442,8 +1452,7 @@ int del_timer_sync(struct timer_list *timer) return ret; } -EXPORT_SYMBOL(del_timer_sync); -#endif +EXPORT_SYMBOL(__del_timer_sync); static void call_timer_fn(struct timer_list *timer, void (*fn)(struct timer_list *),