linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCH v2 00/31] timers: Use del_timer_shutdown() before freeing timers
@ 2022-10-27 15:05 Steven Rostedt
  2022-10-27 15:05 ` [RFC][PATCH v2 01/31] timers: Add del_timer_shutdown() to be called " Steven Rostedt
                   ` (34 more replies)
  0 siblings, 35 replies; 109+ messages in thread
From: Steven Rostedt @ 2022-10-27 15:05 UTC (permalink / raw)
  To: linux-kernel; +Cc: Linus Torvalds, Thomas Gleixner, Stephen Boyd, Guenter Roeck


Back in April, I posted an RFC patch set to help mitigate a common issue
where a timer gets armed just before it is freed, and when the timer
goes off, it crashes in the timer code without any evidence of who the
culprit was. I got side tracked and never finished up on that patch set.
Since this type of crash is still our #1 crash we are seeing in the field,
it has become a priority again to finish it.

This is v2 of that patch set. Thomas Gleixner posted an untested version
that makes timer->function NULL as the flag that it is shutdown. I took that
code, tested it (fixed it up), added more comments, and changed the
name to del_timer_shutdown() as Linus had asked. I also converted it to use
WARN_ON_ONCE() instead of just WARN_ON() as Linus asked for that too.

(Thomas, you never added a SoB, so I only added a link to your email
 in that commit. But as this will likely go through your tree anyway,
 I'm sure you'll have your SoB on all these).

I then created a trivial coccinelle script to find where del_timer*()
is called before being freed, and converted them all to del_timer_shutdown()
(There was a couple that still used del_timer() instead of del_timer_sync()).

I also updated DEBUG_OBJECTS_TIMERS to check from where the timer is ever
armed, to calling of del_timer_shutdown(), and it will trigger if a timer
is freed in between. The current way is to only check if the timer is armed,
but that means it only triggers if the race condition is hit, and with
experience, it's not run on enough machines to catch all of them. By triggering
it from the time the timer is armed to the time it is shutdown, it catches
all potential cases even if the race condition is not hit.

I went though the result of the cocinelle script, and updated the locations.
Some locations were caught by DEBUG_OBJECTS_TIMERS as the coccinelle script
only checked for timers being freed in the same function as the del_timer*().

V1 is found here: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home/

Here's the original text of that version:

   [
     This is an RFC patch. As we hit a few bugs were del_timer() is called
     instead of del_timer_sync() before the timer is freed, and there could
     be bugs where even del_timer_sync() is used, but the timer gets rearmed,
     I decided to introduce a "del_timer_free()" function that can be used
     instead. This will at least educate developers on what to call before they
     free a structure that holds a timer.

     In this RFC, I modified hci_qca.c as a use case, even though that change
     needs some work, because the workqueue could still rearm it (I'm looking
     to see if I can trigger the warning).

     If this approach is acceptable, then I will remove the hci_qca.c portion
     from this patch, and create a series of patches to use the
     del_timer_free() in all the locations in the kernel that remove the timer
     before freeing.
   ]

   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_free() that not only does a del_timer_sync() but will mark
   the timer as freed in case it gets rearmed, it will trigger a WARN_ON. The
   del_timer_free() 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 "free" 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.

You can pull this series down from here:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
timers


Head SHA1: 581931395e77326bb76d7648be080ce302244dd5


Steven Rostedt (Google) (31):
      timers: Add del_timer_shutdown() to be called before freeing timers
      timers: s390/cmm: Use del_timer_shutdown() before freeing timer
      timers: sh: Use del_timer_shutdown() before freeing timer
      timers: block: Use del_timer_shutdown() before freeing timer
      timers: ACPI: Use del_timer_shutdown() before freeing timer
      timers: atm: Use del_timer_shutdown() before freeing timer
      timers: PM: Use del_timer_shutdown()
      timers: Bluetooth: Use del_timer_shutdown() before freeing timer
      timers: hangcheck: Use del_timer_shutdown() before freeing timer
      timers: ipmi: Use del_timer_shutdown() before freeing timer
      timers: random: Use del_timer_shutdown() before freeing timer
      timers: dma-buf: Use del_timer_shutdown() before freeing timer
      timers: drm: Use del_timer_shutdown() before freeing timer
      timers: HID: Use del_timer_shutdown() before freeing timer
      timers: Input: Use del_timer_shutdown() before freeing timer
      timers: mISDN: Use del_timer_shutdown() before freeing timer
      timers: leds: Use del_timer_shutdown() before freeing timer
      timers: media: Use del_timer_shutdown() before freeing timer
      timers: net: Use del_timer_shutdown() before freeing timer
      timers: usb: Use del_timer_shutdown() before freeing timer
      timers: cgroup: Use del_timer_shutdown() before freeing timer
      timers: workqueue: Use del_timer_shutdown() before freeing timer
      timers: nfc: pn533: Use del_timer_shutdown() before freeing timer
      timers: pcmcia: Use del_timer_shutdown() before freeing timer
      timers: scsi: Use del_timer_shutdown() before freeing timer
      timers: tty: Use del_timer_shutdown() before freeing timer
      timers: ext4: Use del_timer_shutdown() before freeing timer
      timers: fs/nilfs2: Use del_timer_shutdown() before freeing timer
      timers: ALSA: Use del_timer_shutdown() before freeing timer
      timers: x86/mce: Use __init_timer() for resetting timers
      timers: Expand DEBUG_OBJECTS_TIMER to check if it ever was used

----
 .../RCU/Design/Requirements/Requirements.rst       |  2 +-
 Documentation/core-api/local_ops.rst               |  2 +-
 Documentation/kernel-hacking/locking.rst           |  4 ++
 arch/s390/mm/cmm.c                                 |  4 +-
 arch/sh/drivers/push-switch.c                      |  2 +-
 arch/x86/kernel/cpu/mce/core.c                     | 14 +++++-
 block/blk-iocost.c                                 |  2 +-
 block/blk-iolatency.c                              |  2 +-
 block/blk-stat.c                                   |  2 +-
 block/blk-throttle.c                               |  2 +-
 block/kyber-iosched.c                              |  2 +-
 drivers/acpi/apei/ghes.c                           |  2 +-
 drivers/atm/idt77105.c                             |  4 +-
 drivers/atm/idt77252.c                             |  4 +-
 drivers/atm/iphase.c                               |  2 +-
 drivers/base/power/wakeup.c                        |  7 +--
 drivers/block/drbd/drbd_main.c                     |  2 +-
 drivers/block/loop.c                               |  2 +-
 drivers/block/sunvdc.c                             |  2 +-
 drivers/bluetooth/hci_bcsp.c                       |  2 +-
 drivers/bluetooth/hci_h5.c                         |  2 +-
 drivers/bluetooth/hci_qca.c                        |  4 +-
 drivers/char/hangcheck-timer.c                     |  2 +-
 drivers/char/ipmi/ipmi_msghandler.c                |  2 +-
 drivers/char/ipmi/ipmi_ssif.c                      |  4 +-
 drivers/char/random.c                              |  2 +-
 drivers/dma-buf/st-dma-fence.c                     |  2 +-
 drivers/gpu/drm/gud/gud_pipe.c                     |  2 +-
 drivers/gpu/drm/i915/i915_sw_fence.c               |  2 +-
 drivers/hid/hid-wiimote-core.c                     |  2 +-
 drivers/input/keyboard/locomokbd.c                 |  2 +-
 drivers/input/keyboard/omap-keypad.c               |  2 +-
 drivers/input/mouse/alps.c                         |  2 +-
 drivers/input/serio/hil_mlc.c                      |  2 +-
 drivers/input/serio/hp_sdc.c                       |  2 +-
 drivers/isdn/hardware/mISDN/hfcmulti.c             |  2 +-
 drivers/isdn/mISDN/l1oip_core.c                    |  4 +-
 drivers/isdn/mISDN/timerdev.c                      |  4 +-
 drivers/leds/trigger/ledtrig-activity.c            |  2 +-
 drivers/leds/trigger/ledtrig-heartbeat.c           |  2 +-
 drivers/leds/trigger/ledtrig-pattern.c             |  2 +-
 drivers/leds/trigger/ledtrig-transient.c           |  2 +-
 drivers/media/pci/ivtv/ivtv-driver.c               |  2 +-
 drivers/media/usb/pvrusb2/pvrusb2-hdw.c            | 18 +++----
 drivers/media/usb/s2255/s2255drv.c                 |  4 +-
 drivers/net/ethernet/intel/i40e/i40e_main.c        |  6 +--
 drivers/net/ethernet/marvell/sky2.c                |  2 +-
 drivers/net/ethernet/sun/sunvnet.c                 |  2 +-
 drivers/net/usb/sierra_net.c                       |  2 +-
 drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c   |  2 +-
 drivers/net/wireless/intersil/hostap/hostap_ap.c   |  2 +-
 drivers/net/wireless/marvell/mwifiex/main.c        |  2 +-
 drivers/net/wireless/microchip/wilc1000/hif.c      |  8 +--
 drivers/nfc/pn533/pn533.c                          |  2 +-
 drivers/nfc/pn533/uart.c                           |  2 +-
 drivers/pcmcia/bcm63xx_pcmcia.c                    |  2 +-
 drivers/pcmcia/electra_cf.c                        |  2 +-
 drivers/pcmcia/omap_cf.c                           |  2 +-
 drivers/pcmcia/pd6729.c                            |  4 +-
 drivers/pcmcia/yenta_socket.c                      |  4 +-
 drivers/scsi/qla2xxx/qla_edif.c                    |  4 +-
 drivers/staging/media/atomisp/i2c/atomisp-lm3554.c |  2 +-
 drivers/tty/n_gsm.c                                |  2 +-
 drivers/tty/sysrq.c                                |  2 +-
 drivers/usb/core/hub.c                             |  3 ++
 drivers/usb/gadget/udc/m66592-udc.c                |  2 +-
 drivers/usb/serial/garmin_gps.c                    |  2 +-
 drivers/usb/serial/mos7840.c                       |  2 +-
 fs/ext4/super.c                                    |  2 +-
 fs/nilfs2/segment.c                                |  2 +-
 include/linux/timer.h                              | 47 +++++++++++++++---
 kernel/cgroup/cgroup.c                             |  2 +-
 kernel/time/timer.c                                | 57 ++++++++++++++--------
 kernel/workqueue.c                                 |  4 +-
 net/802/garp.c                                     |  2 +-
 net/802/mrp.c                                      |  2 +-
 net/bridge/br_multicast.c                          |  6 +--
 net/bridge/br_multicast_eht.c                      |  4 +-
 net/core/gen_estimator.c                           |  2 +-
 net/core/sock.c                                    |  2 +-
 net/ipv4/inet_timewait_sock.c                      |  2 +-
 net/ipv4/ipmr.c                                    |  2 +-
 net/ipv6/ip6mr.c                                   |  2 +-
 net/mac80211/mesh_pathtbl.c                        |  2 +-
 net/netfilter/ipset/ip_set_list_set.c              |  2 +-
 net/netfilter/ipvs/ip_vs_lblc.c                    |  2 +-
 net/netfilter/ipvs/ip_vs_lblcr.c                   |  2 +-
 net/netfilter/xt_LED.c                             |  2 +-
 net/rxrpc/conn_object.c                            |  2 +-
 net/sched/cls_flow.c                               |  2 +-
 net/sunrpc/svc.c                                   |  2 +-
 net/tipc/discover.c                                |  2 +-
 net/tipc/monitor.c                                 |  2 +-
 sound/i2c/other/ak4117.c                           |  2 +-
 sound/synth/emux/emux.c                            |  2 +-
 95 files changed, 213 insertions(+), 153 deletions(-)

^ permalink raw reply	[flat|nested] 109+ messages in thread

end of thread, other threads:[~2022-11-04 16:15 UTC | newest]

Thread overview: 109+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-27 15:05 [RFC][PATCH v2 00/31] timers: Use del_timer_shutdown() before freeing timers Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 01/31] timers: Add del_timer_shutdown() to be called " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 02/31] timers: s390/cmm: Use del_timer_shutdown() before freeing timer Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 03/31] timers: sh: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 05/31] timers: ACPI: " Steven Rostedt
2022-10-28 16:56   ` Rafael J. Wysocki
2022-11-01  1:11   ` Jarkko Sakkinen
2022-10-27 15:05 ` [RFC][PATCH v2 06/31] timers: atm: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 07/31] timers: PM: Use del_timer_shutdown() Steven Rostedt
2022-10-28 17:45   ` Rafael J. Wysocki
2022-10-27 15:05 ` [RFC][PATCH v2 08/31] timers: Bluetooth: Use del_timer_shutdown() before freeing timer Steven Rostedt
2022-10-29  0:12   ` Luiz Augusto von Dentz
2022-10-29  0:33     ` Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 09/31] timers: hangcheck: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 10/31] timers: ipmi: " Steven Rostedt
2022-10-27 15:20   ` Corey Minyard
2022-10-27 15:22     ` Corey Minyard
2022-10-27 15:31       ` Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 11/31] timers: random: " Steven Rostedt
2022-10-27 15:55   ` Jason A. Donenfeld
2022-10-27 15:05 ` [RFC][PATCH v2 14/31] timers: HID: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 16/31] timers: mISDN: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 17/31] timers: leds: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 19/31] timers: net: " Steven Rostedt
2022-10-27 19:55   ` Steven Rostedt
2022-10-27 20:15     ` Linus Torvalds
2022-10-27 20:34       ` Steven Rostedt
2022-10-27 20:48         ` Linus Torvalds
2022-10-27 21:07           ` Steven Rostedt
2022-10-27 21:15             ` Steven Rostedt
2022-10-27 22:35             ` Steven Rostedt
2022-10-28 22:31               ` Steven Rostedt
2022-10-28 22:46                 ` Jakub Kicinski
2022-10-30 17:22                   ` Paolo Abeni
2022-11-03 21:51                     ` Steven Rostedt
2022-11-04  0:00                       ` Eric Dumazet
2022-11-04  5:51                         ` Steven Rostedt
2022-11-04 16:14                           ` Guenter Roeck
2022-10-27 21:07         ` Steven Rostedt
2022-10-28 15:16           ` Guenter Roeck
2022-10-27 15:05 ` [RFC][PATCH v2 20/31] timers: usb: " Steven Rostedt
2022-10-27 20:38   ` Alan Stern
2022-10-27 20:42     ` Steven Rostedt
2022-10-27 21:22       ` Steven Rostedt
2022-10-28  5:23   ` Guenter Roeck
2022-10-28 10:14     ` Steven Rostedt
2022-10-28 14:00       ` Steven Rostedt
2022-10-28 18:01     ` Steven Rostedt
2022-10-28 18:10       ` Steven Rostedt
2022-10-28 19:59         ` Guenter Roeck
2022-10-28 20:40           ` Steven Rostedt
2022-10-28 23:25             ` Guenter Roeck
2022-10-28 23:29               ` Steven Rostedt
2022-10-29 14:52           ` Guenter Roeck
2022-10-29 19:19             ` Steven Rostedt
2022-10-29 22:56               ` Guenter Roeck
2022-10-30 15:48                 ` Steven Rostedt
2022-10-31 15:50                   ` Guenter Roeck
2022-10-31 20:14                     ` Guenter Roeck
2022-10-27 15:05 ` [RFC][PATCH v2 21/31] timers: cgroup: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 22/31] timers: workqueue: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 23/31] timers: nfc: pn533: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 24/31] timers: pcmcia: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 25/31] timers: scsi: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 26/31] timers: tty: " Steven Rostedt
2022-10-31  8:34   ` Jiri Slaby
2022-10-27 15:05 ` [RFC][PATCH v2 27/31] timers: ext4: " Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 28/31] timers: fs/nilfs2: " Steven Rostedt
2022-10-28  5:12   ` Ryusuke Konishi
2022-10-27 15:05 ` [RFC][PATCH v2 29/31] timers: ALSA: " Steven Rostedt
2022-10-28  9:17   ` Takashi Iwai
2022-10-27 15:05 ` [RFC][PATCH v2 30/31] timers: x86/mce: Use __init_timer() for resetting timers Steven Rostedt
2022-10-27 15:05 ` [RFC][PATCH v2 31/31] timers: Expand DEBUG_OBJECTS_TIMER to check if it ever was used Steven Rostedt
     [not found] ` <20221027150925.819019339@goodmis.org>
2022-10-27 15:19   ` [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer Steven Rostedt
2022-10-28  8:26     ` Christoph Hellwig
2022-10-28 10:24       ` Steven Rostedt
2022-10-28 13:56         ` Jens Axboe
2022-10-28 14:06           ` Steven Rostedt
2022-10-28 14:11             ` Jens Axboe
2022-10-28 14:30               ` Steven Rostedt
2022-10-28 15:11   ` Guenter Roeck
     [not found] ` <20221027150927.371916000@goodmis.org>
2022-10-27 15:20   ` [RFC][PATCH v2 12/31] timers: dma-buf: " Steven Rostedt
     [not found] ` <20221027150927.611233945@goodmis.org>
2022-10-27 15:20   ` [RFC][PATCH v2 13/31] timers: drm: " Steven Rostedt
     [not found] ` <20221027150927.992061541@goodmis.org>
2022-10-27 15:21   ` [RFC][PATCH v2 15/31] timers: Input: " Steven Rostedt
2022-10-27 16:38     ` Dmitry Torokhov
2022-10-27 15:52 ` [RFC][PATCH v2 00/31] timers: Use del_timer_shutdown() before freeing timers Jason A. Donenfeld
2022-10-27 16:01   ` Sebastian Andrzej Siewior
2022-10-27 17:23     ` Steven Rostedt
2022-10-27 18:58 ` Guenter Roeck
2022-10-27 19:02   ` Steven Rostedt
2022-10-27 19:11     ` Guenter Roeck
2022-10-27 19:11     ` Linus Torvalds
2022-10-27 19:16       ` Steven Rostedt
2022-10-27 19:44         ` Guenter Roeck
2022-10-27 19:20   ` Steven Rostedt
2022-10-27 19:27     ` Steven Rostedt
2022-10-27 19:38       ` Guenter Roeck
2022-10-27 22:24 ` Guenter Roeck
2022-10-27 22:58   ` Steven Rostedt
2022-10-27 23:24     ` Guenter Roeck
2022-10-27 23:55       ` Steven Rostedt
2022-10-28  0:54         ` Guenter Roeck
2022-10-28 15:30     ` Guenter Roeck
2022-10-28 16:10     ` Guenter Roeck
     [not found] ` <20221028021815.3130-1-hdanton@sina.com>
2022-10-28  3:17   ` [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer Steven Rostedt
2022-10-28 18:50 ` [RFC][PATCH v2 00/31] timers: Use del_timer_shutdown() before freeing timers Steven Rostedt
2022-10-28 20:12   ` Trond Myklebust
2022-10-28 20:49     ` Steven Rostedt
2022-10-28 21:57       ` Trond Myklebust

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).