From patchwork Fri Nov 4 05:41: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: 15352 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp190800wru; Thu, 3 Nov 2022 22:51:08 -0700 (PDT) X-Google-Smtp-Source: AMsMyM73AkbfnApbi+EtuO8NbuF38hwfZMDxJ+CiwNf2jATa2A/kye88FqVm04y0KsdOa7DtOMvX X-Received: by 2002:a17:907:3205:b0:770:8554:c4ee with SMTP id xg5-20020a170907320500b007708554c4eemr33360068ejb.39.1667541068020; Thu, 03 Nov 2022 22:51:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667541068; cv=none; d=google.com; s=arc-20160816; b=WpXn67zQyt36evEEJQY1n6Hml/J9mnAv6K9cq+iRt+iVJ9N8BQWEFfhZDkvnAMs4K1 e8hL/DSr6gxmr1ejnwBZ9rGNa2RAgG+O1fGVCE8ZmBVYTW2jK4vY6Lc7yFqjsEJUIZSI 29Wjs/PAfDcHT7V4ttR86ZOKZGpOutxd7QComWwUEzZe1G3MtaDyC7lBUL2TVjRX7j1L pqYzrG1IaX1GgY2bRgqww+71Cjm8/FOKI/lt1Z5zoK3TUmnO7OWauGSo1vC9T/2dfS88 Ah/HEe2ZelKFWVbXH9K/WLZ1Uc2AyyUDiUrwflOh6PVnAaNLU65b+zuyHkzg0AgrVCo6 EpIQ== 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=807O0YV9D6zu3jPu8y66AAkjW18mVSxvAXhpzi5Ie2Y=; b=emKgN6zAgm3AiPKQHi82nP72jcoHDSly/+AjJskf6az4ZhUv9+VcSfiK2c+A/tWCpN t8f0bh4LitdbRHMepQXPrZpxn5K+e4wFVkhgYoP4z9Z6iAbgJdY7oosiTFXJggln8TfO JODqWEVDn5NZLMEoXyc7CAkf3CswopbD0TkgWCETID9YXQj8qgbLz2w5MSKXmfljwV2G zFaOwhsrgiEwAFMLcw30yr+xYcaQrjfIrCqiGUVFk8zCKtTKH+zWIN/pRE0YVVBa2BJV CoLnM68zAeOYbkvOs1hpljAqkBtCRAGtiUHY0Pjp35KcrieQSIXAYD3Yf8FKGj8Wzk1k YX1g== 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 cr15-20020a170906d54f00b0078d0f88a6e9si4173540ejc.284.2022.11.03.22.50.44; Thu, 03 Nov 2022 22:51:08 -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 S231695AbiKDFuC (ORCPT + 99 others); Fri, 4 Nov 2022 01:50:02 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55804 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231307AbiKDFsz (ORCPT ); Fri, 4 Nov 2022 01:48:55 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 786DF28E11 for ; Thu, 3 Nov 2022 22:48:53 -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 dfw.source.kernel.org (Postfix) with ESMTPS id BF63F620D4 for ; Fri, 4 Nov 2022 05:48:51 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8837EC43144; Fri, 4 Nov 2022 05:48:51 +0000 (UTC) Received: from rostedt by gandalf.local.home with local (Exim 4.96) (envelope-from ) id 1oqpZu-0071GP-0E; Fri, 04 Nov 2022 01:49:18 -0400 Message-ID: <20221104054917.915205356@goodmis.org> User-Agent: quilt/0.66 Date: Fri, 04 Nov 2022 01:41:26 -0400 From: Steven Rostedt To: linux-kernel@vger.kernel.org Cc: Linus Torvalds , Thomas Gleixner , Stephen Boyd , Guenter Roeck , Anna-Maria Gleixner , Andrew Morton , John Stultz Subject: [RFC][PATCH v3 33/33] timers: Expand DEBUG_OBJECTS_TIMER to check if it ever was used References: <20221104054053.431922658@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?1748543542693635989?= X-GMAIL-MSGID: =?utf-8?q?1748543542693635989?= From: "Steven Rostedt (Google)" There's been too many bugs happening where a timer is removed, either by del_timer() or even del_timer_sync() but get's re-armed again by a workqueue or some other task. Then the timer is freed while it's still queued to go off. When the timer eventually goes off, as its content no longer exists, it causes a crash in the timer code. This is very hard to debug because all evidence of who added the timer is gone. Currently, DEBUG_OBJECTS_TIMER will trigger if this happens, but as this only happens rarely (but in the field, thousands of times) and may depend on performing various tasks (USB unplug, CPU hotplug, suspend and resume), not to mention that enabling DEBUG_OBJECTS_TIMER has too much overhead to run in the field, it seldom catches these types of bugs. Now that timer_shutdown_sync() is to be called before freeing, move the checks of DEBUG_OBJECTS_TIMER to if it ever gets armed to where timer_shutdown_sync() is called. If there's a case where a timer is armed, and then freed without calling timer_shutdown_sync() DEBUG_OBJECTS_TIMER will now trigger on it. This catches cases that are potential issues instead of just catching when the race condition occurs. Note, due to delayed workqueues that use timers but they themselves do not supply a shutdown method, there's no way to be able to call timer_shutdown() on delayed work timers correctly. Because of this, the delayed work timers will add a state to inform the DEBUG_OBJECTS_TIMER code that its a timer for a delayed work. The delayed work timers will be treated the old way of only trigging an issue if its timer is active when freed, but does not need to be shutdown first. Work may be needed to make workqueue code also have a shutdown state. Cc: Thomas Gleixner Cc: John Stultz Cc: Stephen Boyd Signed-off-by: Steven Rostedt (Google) --- include/linux/timer.h | 36 +++++++++++++++++++++++++++++- include/linux/workqueue.h | 4 ++-- kernel/time/timer.c | 46 +++++++++++++++++++++++++++++++++------ 3 files changed, 76 insertions(+), 10 deletions(-) diff --git a/include/linux/timer.h b/include/linux/timer.h index 0758b447afd7..ab6148db289e 100644 --- a/include/linux/timer.h +++ b/include/linux/timer.h @@ -8,6 +8,12 @@ #include #include +enum timer_debug_state { + TIMER_DEBUG_DISABLED, + TIMER_DEBUG_ENABLED, + TIMER_DEBUG_WORK, +}; + struct timer_list { /* * All fields that change during normal runtime grouped to the @@ -18,6 +24,9 @@ struct timer_list { void (*function)(struct timer_list *); u32 flags; +#ifdef CONFIG_DEBUG_OBJECTS_TIMERS + enum timer_debug_state enabled; +#endif #ifdef CONFIG_LOCKDEP struct lockdep_map lockdep_map; #endif @@ -128,6 +137,31 @@ static inline void init_timer_on_stack_key(struct timer_list *timer, init_timer_on_stack_key((_timer), (_fn), (_flags), NULL, NULL) #endif +#ifdef CONFIG_DEBUG_OBJECTS_TIMERS +#define __init_timer_debug(_timer, _fn, _flags) \ + do { \ + (_timer)->enabled = TIMER_DEBUG_DISABLED; \ + __init_timer((_timer), (_fn), (_flags)); \ + } while (0) +#define __init_timer_work(_timer, _fn, _flags) \ + do { \ + (_timer)->enabled = TIMER_DEBUG_WORK; \ + __init_timer((_timer), (_fn), (_flags)); \ + } while (0) +#define __init_timer_work_on_stack(_timer, _fn, _flags) \ + do { \ + (_timer)->enabled = TIMER_DEBUG_WORK; \ + __init_timer_on_stack((_timer), (_fn), (_flags)); \ + } while (0) +#else +#define __init_timer_debug(_timer, _fn, _flags) \ + __init_timer((_timer), (_fn), (_flags)) +#define __init_timer_work(_timer, _fn, _flags) \ + __init_timer((_timer), (_fn), (_flags)) +#define __init_timer_work_on_stack(_timer, _fn, _flags) \ + __init_timer_on_stack((_timer), (_fn), (_flags)) +#endif + /** * timer_setup - prepare a timer for first use * @timer: the timer in question @@ -139,7 +173,7 @@ static inline void init_timer_on_stack_key(struct timer_list *timer, * be used and must be balanced with a call to destroy_timer_on_stack(). */ #define timer_setup(timer, callback, flags) \ - __init_timer((timer), (callback), (flags)) + __init_timer_debug((timer), (callback), (flags)) #define timer_setup_on_stack(timer, callback, flags) \ __init_timer_on_stack((timer), (callback), (flags)) diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h index a0143dd24430..290c96429ce1 100644 --- a/include/linux/workqueue.h +++ b/include/linux/workqueue.h @@ -250,7 +250,7 @@ static inline unsigned int work_static(struct work_struct *work) { return 0; } #define __INIT_DELAYED_WORK(_work, _func, _tflags) \ do { \ INIT_WORK(&(_work)->work, (_func)); \ - __init_timer(&(_work)->timer, \ + __init_timer_work(&(_work)->timer, \ delayed_work_timer_fn, \ (_tflags) | TIMER_IRQSAFE); \ } while (0) @@ -258,7 +258,7 @@ static inline unsigned int work_static(struct work_struct *work) { return 0; } #define __INIT_DELAYED_WORK_ONSTACK(_work, _func, _tflags) \ do { \ INIT_WORK_ONSTACK(&(_work)->work, (_func)); \ - __init_timer_on_stack(&(_work)->timer, \ + __init_timer_work_on_stack(&(_work)->timer, \ delayed_work_timer_fn, \ (_tflags) | TIMER_IRQSAFE); \ } while (0) diff --git a/kernel/time/timer.c b/kernel/time/timer.c index 7c224766065e..7596396ce1f6 100644 --- a/kernel/time/timer.c +++ b/kernel/time/timer.c @@ -691,7 +691,10 @@ static bool timer_fixup_init(void *addr, enum debug_obj_state state) switch (state) { case ODEBUG_STATE_ACTIVE: - del_timer_sync(timer); + /* Force the debug deactivate code */ + if (timer->enabled != TIMER_DEBUG_WORK) + timer->enabled = TIMER_DEBUG_ENABLED; + timer_shutdown_sync(timer); debug_object_init(timer, &timer_debug_descr); return true; default: @@ -737,7 +740,7 @@ static bool timer_fixup_free(void *addr, enum debug_obj_state state) switch (state) { case ODEBUG_STATE_ACTIVE: - del_timer_sync(timer); + timer_shutdown_sync(timer); debug_object_free(timer, &timer_debug_descr); return true; default: @@ -774,16 +777,40 @@ static const struct debug_obj_descr timer_debug_descr = { static inline void debug_timer_init(struct timer_list *timer) { + /* Only need to call debug_object_init once if not a work timer */ + if (timer->enabled == TIMER_DEBUG_ENABLED) + return; + debug_object_init(timer, &timer_debug_descr); } static inline void debug_timer_activate(struct timer_list *timer) { + /* Only call debug_timer_activate once if not a work timer */ + if (timer->enabled == TIMER_DEBUG_ENABLED) + return; + + if (timer->enabled == TIMER_DEBUG_DISABLED) + timer->enabled = TIMER_DEBUG_ENABLED; + debug_object_activate(timer, &timer_debug_descr); } -static inline void debug_timer_deactivate(struct timer_list *timer) +static inline void debug_timer_deactivate(struct timer_list *timer, bool free) { + switch (timer->enabled) { + case TIMER_DEBUG_DISABLED: + /* Already disabled, nothing to do */ + return; + case TIMER_DEBUG_ENABLED: + /* free is true when shutting down the timer */ + if (!free) + return; + timer->enabled = TIMER_DEBUG_DISABLED; + break; + case TIMER_DEBUG_WORK: + break; + } debug_object_deactivate(timer, &timer_debug_descr); } @@ -816,7 +843,7 @@ EXPORT_SYMBOL_GPL(destroy_timer_on_stack); #else static inline void debug_timer_init(struct timer_list *timer) { } static inline void debug_timer_activate(struct timer_list *timer) { } -static inline void debug_timer_deactivate(struct timer_list *timer) { } +static inline void debug_timer_deactivate(struct timer_list *timer, bool free) { } static inline void debug_timer_assert_init(struct timer_list *timer) { } #endif @@ -828,7 +855,7 @@ static inline void debug_init(struct timer_list *timer) static inline void debug_deactivate(struct timer_list *timer) { - debug_timer_deactivate(timer); + debug_timer_deactivate(timer, false); trace_timer_cancel(timer); } @@ -1251,12 +1278,15 @@ int __del_timer(struct timer_list *timer, bool free) if (timer_pending(timer)) { base = lock_timer_base(timer, &flags); ret = detach_if_pending(timer, base, true); - if (free) + if (free) { timer->function = NULL; + debug_timer_deactivate(timer, true); + } raw_spin_unlock_irqrestore(&base->lock, flags); } else if (free) { base = lock_timer_base(timer, &flags); timer->function = NULL; + debug_timer_deactivate(timer, true); raw_spin_unlock_irqrestore(&base->lock, flags); } @@ -1276,8 +1306,10 @@ static int __try_to_del_timer_sync(struct timer_list *timer, bool free) if (base->running_timer != timer) ret = detach_if_pending(timer, base, true); - if (free) + if (free) { timer->function = NULL; + debug_timer_deactivate(timer, true); + } raw_spin_unlock_irqrestore(&base->lock, flags);