From patchwork Sun Nov 6 23:30:41 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Steven Rostedt X-Patchwork-Id: 16205 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp1730221wru; Sun, 6 Nov 2022 15:48:44 -0800 (PST) X-Google-Smtp-Source: AMsMyM5HxhMyY2IQ3m0LJmEDasZwV1JiXMMmq3C2GYTcQePeLweKWH6ht+9YkFcGQVQvKe49/yZg X-Received: by 2002:a17:906:5f8a:b0:7ad:ca14:3ae with SMTP id a10-20020a1709065f8a00b007adca1403aemr39368643eju.509.1667778523996; Sun, 06 Nov 2022 15:48:43 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1667778523; cv=none; d=google.com; s=arc-20160816; b=tJ90TGhU4DOSy4zfUCz5J1F75Jbko9UhooWcERrNofnQC28Z6TJYFhKKKVc/RfFea2 OssGlX0nNwjRDudC4YYS4EViCSs9FVva8IOAjZTMmDsr90WTalbh86eXrYo8Q25SviNh 1+x2t5su3ztoFU62vy4UVmmIf0G8uRTUakiSRcEiqHfmeTQXvNkDEW04QSoYggzJanRZ FKwzPF8oZuoNKxQzwvf4TmeDvlyfQkGFU7CpFSTnkyhuijFYfwq8ml+wGGUldujBJclp BHjOTiHTxh74fGH1TDTLPfkJVgv9ChqahfiaTftjQiLpkPZkdk5xo1W97gOhic0jK56g Wlrg== 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=dUPH4dZCLu8gyVpgBjwXBvmd+u75mtelYyFpM10NnKg=; b=r4/+G0kcD6rAx2SQYF94sbGeHxtej4Nvw8RRfb3a3v8+amn8BngX8sdbQtSgd4jIxB tbyjFjxIADbXrXwxc6HeU94hLeYLkj4TRyah3Hi3nuumHI3nPO7lHcIc3a00GIz8fDXE HGyaNCb9tJOqYM8d1RBXGPJc7LPKO+obR3BUiWN93SorxBSMu87RkGgEjS1N0rA/0nd9 AkC3+bC0/L2rYaHSoQNws8pulJGmyHu4qmcvjG6zqYk5lm7ktJMkYNg5MOHUC9qcTDGq qA/ynFTVTqyvMZexUUZMSdAPWqSDNDscl5VZWujDDaWbrLLCkx/Nze/1sGB6lzZtFgVb ndOw== 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 c24-20020a170906171800b0078dc3cb8b48si5237083eje.625.2022.11.06.15.48.20; Sun, 06 Nov 2022 15:48:43 -0800 (PST) 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 S230222AbiKFXeQ (ORCPT + 99 others); Sun, 6 Nov 2022 18:34:16 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43510 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230185AbiKFXeG (ORCPT ); Sun, 6 Nov 2022 18:34:06 -0500 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E1A42D124 for ; Sun, 6 Nov 2022 15:34:04 -0800 (PST) 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 dfw.source.kernel.org (Postfix) with ESMTPS id 7B8A160DFF for ; Sun, 6 Nov 2022 23:34:04 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D3FF6C43145; Sun, 6 Nov 2022 23:34:03 +0000 (UTC) Received: from rostedt by gandalf.local.home with local (Exim 4.96) (envelope-from ) id 1orp9u-008UQe-1u; Sun, 06 Nov 2022 18:34:34 -0500 Message-ID: <20221106233434.425162916@goodmis.org> User-Agent: quilt/0.66 Date: Sun, 06 Nov 2022 18:30:41 -0500 From: Steven Rostedt To: linux-kernel@vger.kernel.org Cc: Linus Torvalds , Thomas Gleixner , Stephen Boyd , Guenter Roeck , Anna-Maria Gleixner , Andrew Morton , Julia Lawall Subject: [for-linus][PATCH 4/5] timers: Add timer_shutdown_sync() and timer_shutdown() to be called before freeing timers References: <20221106233037.815236769@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?1748792533658675316?= X-GMAIL-MSGID: =?utf-8?q?1748792533658675316?= 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 timer_shutdown_sync() 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 timer_shutdown_sync() 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. There's some situations that already know that the timer is shutdown and does not need to perform the synchronization (or can not due to its context). For these locations there's timer_shutdown() that only shuts down the timer (prevents it from being rearmed) but does not add checks if the timer is currently running. 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/ Link: https://lkml.kernel.org/r/20221106212702.363575800@goodmis.org Link: https://lore.kernel.org/all/20221105060024.598488967@goodmis.org/ Cc: Linus Torvalds Cc: Thomas Gleixner Cc: Stephen Boyd Cc: Anna-Maria Gleixner Cc: Andrew Morton Cc: Julia Lawall Tested-by: Guenter Roeck Signed-off-by: Steven Rostedt (Google) --- .../RCU/Design/Requirements/Requirements.rst | 2 +- Documentation/core-api/local_ops.rst | 2 +- Documentation/kernel-hacking/locking.rst | 5 ++ include/linux/timer.h | 62 ++++++++++++++++-- kernel/time/timer.c | 64 ++++++++++--------- 5 files changed, 97 insertions(+), 38 deletions(-) diff --git a/Documentation/RCU/Design/Requirements/Requirements.rst b/Documentation/RCU/Design/Requirements/Requirements.rst index a0f8164c8513..ec6de88846b9 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 timer_shutdown_sync(). 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..0b42ceaaf3c4 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); + timer_shutdown_sync(&test_timer); } module_init(test_init); diff --git a/Documentation/kernel-hacking/locking.rst b/Documentation/kernel-hacking/locking.rst index 6805ae6e86e6..eb341b69fd15 100644 --- a/Documentation/kernel-hacking/locking.rst +++ b/Documentation/kernel-hacking/locking.rst @@ -1009,6 +1009,11 @@ 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, timer_shutdown() or timer_shutdown_sync() should 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..0b959b52d0db 100644 --- a/include/linux/timer.h +++ b/include/linux/timer.h @@ -168,12 +168,45 @@ static inline int timer_pending(const struct timer_list * timer) return !hlist_unhashed_lockless(&timer->entry); } +extern int __del_timer(struct timer_list * timer, bool free); + extern void add_timer_on(struct timer_list *timer, int cpu); -extern int del_timer(struct timer_list * timer); extern int mod_timer(struct timer_list *timer, unsigned long expires); extern int mod_timer_pending(struct timer_list *timer, unsigned long expires); extern int timer_reduce(struct timer_list *timer, unsigned long expires); +/** + * del_timer - deactivate a timer. + * @timer: the timer to be deactivated + * + * del_timer() deactivates a timer - this works on both active and inactive + * timers. + * + * The function returns whether it has deactivated a pending timer or not. + * (ie. del_timer() of an inactive timer returns 0, del_timer() of an + * active timer returns 1.) + */ +static inline int del_timer(struct timer_list *timer) +{ + return __del_timer(timer, false); +} + +/** + * timer_shutdown - deactivate a timer and shut it down + * @timer: the timer to be deactivated + * + * timer_shutdown() deactivates a timer - this works on both active + * and inactive timers, and will prevent it from being rearmed. + * + * The function returns whether it has deactivated a pending timer or not. + * (ie. timer_shutdown() of an inactive timer returns 0, + * timer_shutdown() of an active timer returns 1.) + */ +static inline int timer_shutdown(struct timer_list *timer) +{ + return __del_timer(timer, true); +} + /* * The jiffies value which is added to now, when there is no timer * in the timer wheel: @@ -183,12 +216,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); +} + +/** + * timer_shutdown_sync - 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 timer_shutdown_sync(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..7c224766065e 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); @@ -1237,18 +1240,7 @@ void add_timer_on(struct timer_list *timer, int cpu) } EXPORT_SYMBOL_GPL(add_timer_on); -/** - * del_timer - deactivate a timer. - * @timer: the timer to be deactivated - * - * del_timer() deactivates a timer - this works on both active and inactive - * timers. - * - * The function returns whether it has deactivated a pending timer or not. - * (ie. del_timer() of an inactive timer returns 0, del_timer() of an - * active timer returns 1.) - */ -int del_timer(struct timer_list *timer) +int __del_timer(struct timer_list *timer, bool free) { struct timer_base *base; unsigned long flags; @@ -1259,21 +1251,20 @@ int del_timer(struct timer_list *timer) if (timer_pending(timer)) { base = lock_timer_base(timer, &flags); ret = detach_if_pending(timer, base, true); + if (free) + timer->function = NULL; + raw_spin_unlock_irqrestore(&base->lock, flags); + } else if (free) { + base = lock_timer_base(timer, &flags); + timer->function = NULL; raw_spin_unlock_irqrestore(&base->lock, flags); } return ret; } -EXPORT_SYMBOL(del_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 +1276,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 +1370,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 +1407,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 +1437,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 +1447,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 *),