timers: Use del_timer_shutdown() before freeing timers
Subsystems (79)
Steven Rostedt
reviewerv2 cover timers: Use del_timer_shutdown() before freeing timers
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/[email protected]/
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(-)
RFC PATCH v2 4/31 timers: block: Use del_timer_shutdown() before freeing timer
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- 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/block/drbd/drbd_main.c | 2 +- drivers/block/loop.c | 2 +- drivers/block/sunvdc.c | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-)
block/blk-iocost.c +1 -1
@@ -2814,7 +2814,7 @@ static void ioc_rqos_exit(struct rq_qos *rqos) ioc->running =IOC_STOP ;spin_unlock_irq (&ioc->lock); -del_timer_sync (&ioc->timer); +del_timer_shutdown (&ioc->timer);free_percpu (ioc->pcpu_stat);kfree (ioc); }
block/blk-iolatency.c +1 -1
@@ -645,7 +645,7 @@ static void blkcg_iolatency_exit(struct rq_qos *rqos) {struct blk_iolatency *blkiolat =BLKIOLATENCY (rqos); -del_timer_sync (&blkiolat->timer); +del_timer_shutdown (&blkiolat->timer);flush_work (&blkiolat->enable_work);blkcg_deactivate_policy (rqos->q, &blkcg_policy_iolatency);kfree (blkiolat);
block/blk-stat.c +1 -1
@@ -165,7 +165,7 @@ void blk_stat_remove_callback(struct request_queue *q,blk_queue_flag_clear (QUEUE_FLAG_STATS , q);spin_unlock_irqrestore (&q->stats->lock, flags); -del_timer_sync (&cb->timer); +del_timer_shutdown (&cb->timer); }static void blk_stat_free_callback_rcu (struct rcu_head *head)
block/blk-throttle.c +1 -1
@@ -490,7 +490,7 @@ static void throtl_pd_free(struct blkg_policy_data *pd) {struct throtl_grp *tg =pd_to_tg (pd); -del_timer_sync (&tg->service_queue.pending_timer); +del_timer_shutdown (&tg->service_queue.pending_timer);blkg_rwstat_exit (&tg->stat_bytes);blkg_rwstat_exit (&tg->stat_ios);kfree (tg);
block/kyber-iosched.c +1 -1
@@ -434,7 +434,7 @@ static void kyber_exit_sched(struct elevator_queue *e)struct kyber_queue_data *kqd = e->elevator_data;int i; -del_timer_sync (&kqd->timer); +del_timer_shutdown (&kqd->timer);blk_stat_disable_accounting (kqd->q);for (i =0 ; i <KYBER_NUM_DOMAINS ; i++)
drivers/block/drbd/drbd_main.c +1 -1
@@ -2184,7 +2184,7 @@ void drbd_destroy_device(struct kref *kref)struct drbd_resource *resource = device->resource;struct drbd_peer_device *peer_device, *tmp_peer_device; -del_timer_sync (&device->request_timer); +del_timer_shutdown (&device->request_timer);/* paranoia asserts */ D_ASSERT (device, device->open_cnt ==0 );
drivers/block/loop.c +1 -1
@@ -1755,7 +1755,7 @@ static void lo_free_disk(struct gendisk *disk)if (lo->workqueue)destroy_workqueue (lo->workqueue);loop_free_idle_workers (lo, true); -del_timer_sync (&lo->timer); +del_timer_shutdown (&lo->timer);mutex_destroy (&lo->lo_mutex);kfree (lo); }
drivers/block/sunvdc.c +1 -1
@@ -1067,7 +1067,7 @@ static void vdc_port_remove(struct vio_dev *vdev)flush_work (&port->ldc_reset_work);cancel_delayed_work_sync (&port->ldc_reset_timer_work); -del_timer_sync (&port->vio.timer); +del_timer_shutdown (&port->vio.timer);del_gendisk (port->disk);put_disk (port->disk);
Signature · 5 lines
2.35.1 _______________________________________________ drbd-dev mailing list [email protected] https://lists.linbit.com/mailman/listinfo/drbd-dev
Steven Rostedt
reviewerRe: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer
[
quilt mail --send still can't handle unicode characters.
Here's the patch again
]
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- 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/block/drbd/drbd_main.c | 2 +- drivers/block/loop.c | 2 +- drivers/block/sunvdc.c | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-)
block/blk-iocost.c +1 -1
@@ -2814,7 +2814,7 @@ static void ioc_rqos_exit(struct rq_qos *rqos) ioc->running =IOC_STOP ;spin_unlock_irq (&ioc->lock); -del_timer_sync (&ioc->timer); +del_timer_shutdown (&ioc->timer);free_percpu (ioc->pcpu_stat);kfree (ioc); }
block/blk-iolatency.c +1 -1
@@ -645,7 +645,7 @@ static void blkcg_iolatency_exit(struct rq_qos *rqos) {struct blk_iolatency *blkiolat =BLKIOLATENCY (rqos); -del_timer_sync (&blkiolat->timer); +del_timer_shutdown (&blkiolat->timer);flush_work (&blkiolat->enable_work);blkcg_deactivate_policy (rqos->q, &blkcg_policy_iolatency);kfree (blkiolat);
block/blk-stat.c +1 -1
@@ -165,7 +165,7 @@ void blk_stat_remove_callback(struct request_queue *q,blk_queue_flag_clear (QUEUE_FLAG_STATS , q);spin_unlock_irqrestore (&q->stats->lock, flags); -del_timer_sync (&cb->timer); +del_timer_shutdown (&cb->timer); }static void blk_stat_free_callback_rcu (struct rcu_head *head)
block/blk-throttle.c +1 -1
@@ -490,7 +490,7 @@ static void throtl_pd_free(struct blkg_policy_data *pd) {struct throtl_grp *tg =pd_to_tg (pd); -del_timer_sync (&tg->service_queue.pending_timer); +del_timer_shutdown (&tg->service_queue.pending_timer);blkg_rwstat_exit (&tg->stat_bytes);blkg_rwstat_exit (&tg->stat_ios);kfree (tg);
block/kyber-iosched.c +1 -1
@@ -434,7 +434,7 @@ static void kyber_exit_sched(struct elevator_queue *e)struct kyber_queue_data *kqd = e->elevator_data;int i; -del_timer_sync (&kqd->timer); +del_timer_shutdown (&kqd->timer);blk_stat_disable_accounting (kqd->q);for (i =0 ; i <KYBER_NUM_DOMAINS ; i++)
drivers/block/drbd/drbd_main.c +1 -1
@@ -2184,7 +2184,7 @@ void drbd_destroy_device(struct kref *kref)struct drbd_resource *resource = device->resource;struct drbd_peer_device *peer_device, *tmp_peer_device; -del_timer_sync (&device->request_timer); +del_timer_shutdown (&device->request_timer);/* paranoia asserts */ D_ASSERT (device, device->open_cnt ==0 );
drivers/block/loop.c +1 -1
@@ -1755,7 +1755,7 @@ static void lo_free_disk(struct gendisk *disk)if (lo->workqueue)destroy_workqueue (lo->workqueue);loop_free_idle_workers (lo, true); -del_timer_sync (&lo->timer); +del_timer_shutdown (&lo->timer);mutex_destroy (&lo->lo_mutex);kfree (lo); }
drivers/block/sunvdc.c +1 -1
@@ -1067,7 +1067,7 @@ static void vdc_port_remove(struct vio_dev *vdev)flush_work (&port->ldc_reset_work);cancel_delayed_work_sync (&port->ldc_reset_timer_work); -del_timer_sync (&port->vio.timer); +del_timer_shutdown (&port->vio.timer);del_gendisk (port->disk);put_disk (port->disk);
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer
This is just a single patch out of apparently 31, which claims that something that doesn't even exist in mainline must be used without any explanation. How do you expect anyone to be able to review it?
Steven Rostedt
reviewerRe: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer
Quoted 3 lines · On Fri, 28 Oct 2022 01:26:03 -0700 Christoph Hellwig <[email protected]> wrote:
This is just a single patch out of apparently 31, which claims that something that doesn't even exist in mainline must be used without any explanation. How do you expect anyone to be able to review it?
https://lore.kernel.org/all/[email protected]/
Only the first patch is relevant to you. I guess the Cc list would have been too big to Cc everyone that was Cc'd in the series.
It not being in mainline is the reason I marked it RFC. As it's more of an FYI than a pull it in request.
-- Steve
Jens Axboe
maintainerRe: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer
Quoted 11 lines · Steven Rostedt
Quoted 3 lines · On Fri, 28 Oct 2022 01:26:03 -0700 Christoph Hellwig…
This is just a single patch out of apparently 31, which claims that something that doesn't even exist in mainline must be used without any explanation. How do you expect anyone to be able to review it?
https://lore.kernel.org/all/[email protected]/
Only the first patch is relevant to you. I guess the Cc list would have been too big to Cc everyone that was Cc'd in the series.
No it's not, because how on earth would anyone know what the change does if you only see the simple s/name/newname change? The patch is useless by itself.
Signature · 1 line
Jens Axboe
Steven Rostedt
reviewerRe: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer
Quoted 8 lines · On Fri, 28 Oct 2022 07:56:50 -0600 Jens Axboe <[email protected]> wrote:
Quoted 7 lines · Steven Rostedt
Quoted 3 lines · On Fri, 28 Oct 2022 01:26:03 -0700 Christoph Hellwig <[email protected]> wrote:
This is just a single patch out of apparently 31, which claims that something that doesn't even exist in mainline must be used without any explanation. How do you expect anyone to be able to review it?
Quoted 9 lines
Quoted 4 lines
https://lore.kernel.org/all/[email protected]/
Only the first patch is relevant to you. I guess the Cc list would have been too big to Cc everyone that was Cc'd in the series.
No it's not, because how on earth would anyone know what the change does if you only see the simple s/name/newname change? The patch is useless by itself.
I meant this as the first patch:
https://lore.kernel.org/all/[email protected]/
Which was what the link above was suppose to point to.
It's the only patch relevant to the rest of the series, as the rest is just converting over to the shutdown API, and the last patch changes DEBUG_OBJECTS_TIMERS to catch if this was done properly.
That is, patch 01/31 and the patch you were Cc'd on is relevant, and for those that want to look deeper, see patch 31 as well.
But if I included the Cc list for patch 01 for all those Cc'd in the entire series, it would be a huge Cc list, so I avoided doing so.
Also, this is still RFC as the changes may still change. That is, this patch set is a heads up to what is to come. Ideally, I'd like to get just the API possibly in the kernel before the merge window without anyone using it. Then I can ask all the sub systems to pull in these individual patches as well.
-- Steve
Jens Axboe
maintainerRe: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer
Quoted 25 lines · Steven Rostedt
Quoted 8 lines · On Fri, 28 Oct 2022 07:56:50 -0600 Jens Axboe…
Quoted 7 lines · Steven Rostedt
Quoted 3 lines · On Fri, 28 Oct 2022 01:26:03 -0700 Christoph Hellwig…
This is just a single patch out of apparently 31, which claims that something that doesn't even exist in mainline must be used without any explanation. How do you expect anyone to be able to review it?
Quoted 9 lines
Quoted 4 lines
https://lore.kernel.org/all/[email protected]/
Only the first patch is relevant to you. I guess the Cc list would have been too big to Cc everyone that was Cc'd in the series.
No it's not, because how on earth would anyone know what the change does if you only see the simple s/name/newname change? The patch is useless by itself.
I meant this as the first patch:
Quoted 8 lines
Which was what the link above was suppose to point to.
It's the only patch relevant to the rest of the series, as the rest is just converting over to the shutdown API, and the last patch changes
DEBUG_OBJECTS_TIMERSto catch if this was done properly.That is, patch 01/31 and the patch you were Cc'd on is relevant, and for those that want to look deeper, see patch 31 as well.
So we got half of what was needed to make any kind of sense of judgement on the patch.
Quoted 2 lines
But if I included the Cc list for patch 01 for all those Cc'd in the entire series, it would be a huge Cc list, so I avoided doing so.
And my point is that just CC'ing the relevant list for patch 4/31 is useless. Do we need to see the whole series? No. Does everyone need to see patch 1/31? Yes, very much so. Without that, 4/31 means nothing.
This is pretty common for tree wide changes. The relevant lists need to see the full context, patch 4/31 by itself is useless and may as well not be sent at this point then.
Signature · 1 line
Jens Axboe
Steven Rostedt
reviewerRe: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer
Quoted 3 lines · On Fri, 28 Oct 2022 08:11:27 -0600 Jens Axboe <[email protected]> wrote:
This is pretty common for tree wide changes. The relevant lists need to see the full context, patch 4/31 by itself is useless and may as well not be sent at this point then.
Ah, I didn't think about just including the mailing lists. The Cc lists were auto-generated, and I didn't think about just taking out the lists.
Will do that for v2.
Thanks,
-- Steve
Re: [RFC][PATCH v2 04/31] timers: block: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtQuoted 4 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.
I also had to add the following, as you had already suggested.
Just changing blk_sync_queue() was insufficient; I had to add the call from blk_release_queue() because otherwise blk_sync_queue() was not always called.
Thanks,
Guenter
---
block/blk-core.c +1 -1
@@ -227,7 +227,7 @@ const char *blk_status_to_str(blk_status_t status)*/ void blk_sync_queue (struct request_queue *q) { -del_timer_sync (&q->timeout); +del_timer_shutdown (&q->timeout);cancel_work_sync (&q->timeout_work); }EXPORT_SYMBOL (blk_sync_queue);
block/blk-sysfs.c +2 -0
@@ -769,6 +769,8 @@ static void blk_release_queue(struct kobject *kobj)percpu_ref_exit (&q->q_usage_counter); +blk_sync_queue (q); +if (q->poll_stat)blk_stat_remove_callback (q, q->poll_cb);blk_stat_free_callback (q->poll_cb);
Steven Rostedt
reviewerRFC PATCH v2 1/31 timers: Add del_timer_shutdown() to be called before freeing timers
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
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.
--- .../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(-)
Documentation/RCU/Design/Requirements/Requirements.rst +1 -1
@@ -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
Documentation/core-api/local_ops.rst +1 -1
@@ -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);
Documentation/kernel-hacking/locking.rst +4 -0
@@ -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 =============
include/linux/timer.h +22 -5
@@ -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)
kernel/time/timer.c +26 -17
104 lines not on this page · raw message
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 2/31 timers: s390/cmm: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- arch/s390/mm/cmm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/s390/mm/cmm.c +2 -2
@@ -419,7 +419,7 @@ static int __init cmm_init(void)#endif unregister_sysctl_table (cmm_sysctl_header); out_sysctl: -del_timer_sync (&cmm_timer); +del_timer_shutdown (&cmm_timer);return rc; }module_init (cmm_init);
@@ -432,7 +432,7 @@ static void __exit cmm_exit(void)#endif unregister_oom_notifier (&cmm_oom_nb);kthread_stop (cmm_thread_ptr); -del_timer_sync (&cmm_timer); +del_timer_shutdown (&cmm_timer);cmm_free_pages (cmm_pages, &cmm_pages, &cmm_page_list);cmm_free_pages (cmm_timed_pages, &cmm_timed_pages, &cmm_timed_page_list); }
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 3/31 timers: sh: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- arch/sh/drivers/push-switch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/sh/drivers/push-switch.c +1 -1
@@ -102,7 +102,7 @@ static int switch_drv_remove(struct platform_device *pdev)platform_set_drvdata (pdev,NULL );flush_work (&psw->work); -del_timer_sync (&psw->debounce); +del_timer_shutdown (&psw->debounce);free_irq (irq, pdev);kfree (psw);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 5/31 timers: ACPI: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/acpi/apei/ghes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/acpi/apei/ghes.c +1 -1
@@ -1405,7 +1405,7 @@ static int ghes_remove(struct platform_device *ghes_dev) ghes->flags |=GHES_EXITING ;switch (generic->notify.type) {case ACPI_HEST_NOTIFY_POLLED : -del_timer_sync (&ghes->timer); +del_timer_shutdown (&ghes->timer);break ;case ACPI_HEST_NOTIFY_EXTERNAL :free_irq (ghes->irq, ghes);
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 05/31] timers: ACPI: Use del_timer_shutdown() before freeing timer
Quoted 7 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.
Quoted 10 lines
Cc "Rafael J. Wysocki" <[email protected]>Cc Len Brown <[email protected]>Cc James Morse <[email protected]>Cc Tony Luck <[email protected]>Cc Borislav Petkov <[email protected]>Cc Shuai Xue <[email protected]>Cc Dave Hansen <[email protected]>Cc Jarkko Sakkinen <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>
Please add "APEI: ghes:" to the subject after "ACPI:".
Apart from this
Quoted 19 lines
--- drivers/acpi/apei/ghes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/acpi/apei/ghes.c +1
-2@@ -1405,7 +1405,7 @@ static int ghes_remove(struct platform_device *ghes_dev) ghes->flags |=GHES_EXITING ;switch (generic->notify.type) {case ACPI_HEST_NOTIFY_POLLED : -del_timer_sync (&ghes->timer); +del_timer_shutdown (&ghes->timer);break ;case ACPI_HEST_NOTIFY_EXTERNAL :free_irq (ghes->irq, ghes); --2.35.1
Re: [RFC][PATCH v2 05/31] timers: ACPI: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtQuoted 35 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.Cc "Rafael J. Wysocki" <[email protected]>Cc Len Brown <[email protected]>Cc James Morse <[email protected]>Cc Tony Luck <[email protected]>Cc Borislav Petkov <[email protected]>Cc Shuai Xue <[email protected]>Cc Dave Hansen <[email protected]>Cc Jarkko Sakkinen <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>--- drivers/acpi/apei/ghes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/acpi/apei/ghes.c +1
-1@@ -1405,7 +1405,7 @@ static int ghes_remove(struct platform_device *ghes_dev) ghes->flags |=GHES_EXITING ;switch (generic->notify.type) {case ACPI_HEST_NOTIFY_POLLED : -del_timer_sync (&ghes->timer); +del_timer_shutdown (&ghes->timer);break ;case ACPI_HEST_NOTIFY_EXTERNAL :free_irq (ghes->irq, ghes);Signature · 1 line
2.35.1
BR, Jarkko
Steven Rostedt
reviewerRFC PATCH v2 6/31 timers: atm: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/atm/idt77105.c | 4 ++-- drivers/atm/idt77252.c | 4 ++-- drivers/atm/iphase.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-)
drivers/atm/idt77105.c +2 -2
@@ -366,8 +366,8 @@ EXPORT_SYMBOL(idt77105_init); static void __exit idt77105_exit(void) {/* turn off timers */ -del_timer_sync (&stats_timer); -del_timer_sync (&restart_timer); +del_timer_shutdown (&stats_timer); +del_timer_shutdown (&restart_timer); }module_exit (idt77105_exit);
drivers/atm/idt77252.c +2 -2
@@ -2213,7 +2213,7 @@ idt77252_init_ubr(struct idt77252_dev *card, struct vc_map *vc, }spin_unlock_irqrestore (&vc->lock, flags);if (est) { -del_timer_sync (&est->timer); +del_timer_shutdown (&est->timer);kfree (est); }
@@ -3752,7 +3752,7 @@ static void __exit idt77252_exit(void) card = idt77252_chain; dev = card->atmdev; idt77252_chain = card->next; -del_timer_sync (&card->tst_timer); +del_timer_shutdown (&card->tst_timer);if (dev->phy->stop) dev->phy->stop (dev);
drivers/atm/iphase.c +1 -1
@@ -3280,7 +3280,7 @@ static void __exit ia_module_exit(void) {pci_unregister_driver (&ia_driver); -del_timer_sync (&ia_timer); +del_timer_shutdown (&ia_timer); }module_init (ia_module_init);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 7/31 timers: PM: Use del_timer_shutdown()
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Instead of open coding making the timer look like it was not registered by setting the function pointer to NULL, call del_timer_shutdown() that does the same thing.
--- drivers/base/power/wakeup.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/base/power/wakeup.c +1 -6
@@ -202,12 +202,7 @@ void wakeup_source_remove(struct wakeup_source *ws)raw_spin_unlock_irqrestore (&events_lock, flags);synchronize_srcu (&wakeup_srcu); -del_timer_sync (&ws->timer); -/* -* Clear timer.function to make wakeup_source_not_registered() treat -* this wakeup source as not registered. -*/ - ws->timer.function =NULL ; +del_timer_shutdown (&ws->timer); }EXPORT_SYMBOL_GPL (wakeup_source_remove);
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 07/31] timers: PM: Use del_timer_shutdown()
Quoted 7 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Instead of open coding making the timer look like it was not registered by setting the function pointer to NULL, call
del_timer_shutdown()that does the same thing.
Quoted 8 lines
Cc "Rafael J. Wysocki" <[email protected]>Cc Len Brown <[email protected]>Cc Pavel Machek <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>
Please add "wakeup:" to the subject after "PM:".
Apart from this
Quoted 24 lines
--- drivers/base/power/wakeup.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)drivers/base/power/wakeup.c +1
-7@@ -202,12 +202,7 @@ void wakeup_source_remove(struct wakeup_source *ws)raw_spin_unlock_irqrestore (&events_lock, flags);synchronize_srcu (&wakeup_srcu); -del_timer_sync (&ws->timer); -/* -* Clear timer.function to make wakeup_source_not_registered() treat -* this wakeup source as not registered. -*/ - ws->timer.function =NULL ; +del_timer_shutdown (&ws->timer); }EXPORT_SYMBOL_GPL (wakeup_source_remove); --2.35.1
Steven Rostedt
reviewerRFC PATCH v2 8/31 timers: Bluetooth: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/bluetooth/hci_bcsp.c | 2 +- drivers/bluetooth/hci_h5.c | 2 +- drivers/bluetooth/hci_qca.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
drivers/bluetooth/hci_bcsp.c +1 -1
@@ -737,7 +737,7 @@ static int bcsp_close(struct hci_uart *hu) {struct bcsp_struct *bcsp = hu->priv; -del_timer_sync (&bcsp->tbcsp); +del_timer_shutdown (&bcsp->tbcsp); hu->priv =NULL ;
drivers/bluetooth/hci_h5.c +1 -1
@@ -253,7 +253,7 @@ static int h5_close(struct hci_uart *hu) {struct h5 *h5 = hu->priv; -del_timer_sync (&h5->timer); +del_timer_shutdown (&h5->timer);skb_queue_purge (&h5->unack);skb_queue_purge (&h5->rel);
drivers/bluetooth/hci_qca.c +2 -2
@@ -697,8 +697,8 @@ static int qca_close(struct hci_uart *hu)skb_queue_purge (&qca->txq);skb_queue_purge (&qca->rx_memdump_q);destroy_workqueue (qca->workqueue); -del_timer_sync (&qca->tx_idle_timer); -del_timer_sync (&qca->wake_retrans_timer); +del_timer_shutdown (&qca->tx_idle_timer); +del_timer_shutdown (&qca->wake_retrans_timer); qca->hu =NULL ;kfree_skb (qca->rx_skb);
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 08/31] timers: Bluetooth: Use del_timer_shutdown() before freeing timer
Hi Steven,
Quoted 5 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.
Quoted 7 lines
Cc Marcel Holtmann <[email protected]>Cc Johan Hedberg <[email protected]>Cc Luiz Augusto von Dentz <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>
Btw, we do seem to have similar problems with the likes of cancel_delayed_work/cancel_delayed_work_sync so wonder if it wouldn't be the case to introduce something like shutdown_delayed_work before freeing the struct/object containing it?
Quoted 49 lines
--- drivers/bluetooth/hci_bcsp.c | 2 +- drivers/bluetooth/hci_h5.c | 2 +- drivers/bluetooth/hci_qca.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)drivers/bluetooth/hci_bcsp.c +1
-1@@ -737,7 +737,7 @@ static int bcsp_close(struct hci_uart *hu) {struct bcsp_struct *bcsp = hu->priv; -del_timer_sync (&bcsp->tbcsp); +del_timer_shutdown (&bcsp->tbcsp); hu->priv =NULL ;drivers/bluetooth/hci_h5.c +1
-1@@ -253,7 +253,7 @@ static int h5_close(struct hci_uart *hu) {struct h5 *h5 = hu->priv; -del_timer_sync (&h5->timer); +del_timer_shutdown (&h5->timer);skb_queue_purge (&h5->unack);skb_queue_purge (&h5->rel);drivers/bluetooth/hci_qca.c +2
-3@@ -697,8 +697,8 @@ static int qca_close(struct hci_uart *hu)skb_queue_purge (&qca->txq);skb_queue_purge (&qca->rx_memdump_q);destroy_workqueue (qca->workqueue); -del_timer_sync (&qca->tx_idle_timer); -del_timer_sync (&qca->wake_retrans_timer); +del_timer_shutdown (&qca->tx_idle_timer); +del_timer_shutdown (&qca->wake_retrans_timer); qca->hu =NULL ;kfree_skb (qca->rx_skb); --2.35.1
Signature · 1 line
Luiz Augusto von Dentz
Steven Rostedt
reviewerRe: [RFC][PATCH v2 08/31] timers: Bluetooth: Use del_timer_shutdown() before freeing timer
Quoted 1 line · On Fri, 28 Oct 2022 17:12:44 -0700 Luiz Augusto von Dentz…
Thanks, but I will be sending a v2 as the naming is going to change.
Quoted 5 lines
Btw, we do seem to have similar problems with the likes of cancel_delayed_work/cancel_delayed_work_sync so wonder if it wouldn't be the case to introduce something like
shutdown_delayed_workbefore freeing the struct/object containing it?
As I stated here:
https://lore.kernel.org/all/[email protected]/
That may be the next endevour I partake in.
Cheers,
-- Steve
Steven Rostedt
reviewerRFC PATCH v2 9/31 timers: hangcheck: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/char/hangcheck-timer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/char/hangcheck-timer.c +1 -1
@@ -167,7 +167,7 @@ static int __init hangcheck_init(void) static void __exit hangcheck_exit(void) { -del_timer_sync (&hangcheck_ticktock); +del_timer_shutdown (&hangcheck_ticktock);printk ("Hangcheck: Stopped hangcheck timer.\n" ); }
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 10/31 timers: ipmi: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/char/ipmi/ipmi_msghandler.c | 2 +- drivers/char/ipmi/ipmi_ssif.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/char/ipmi/ipmi_msghandler.c +1 -1
@@ -5540,7 +5540,7 @@ static void __exit cleanup_ipmi(void)* here. */ atomic_set (&stop_operation,1 ); -del_timer_sync (&ipmi_timer); +del_timer_shutdown (&ipmi_timer); initialized = false;
drivers/char/ipmi/ipmi_ssif.c +2 -2
@@ -1273,8 +1273,8 @@ static void shutdown_ssif(void *send_info)schedule_timeout (1 ); ssif_info->stopping = true; -del_timer_sync (&ssif_info->watch_timer); -del_timer_sync (&ssif_info->retry_timer); +del_timer_shutdown (&ssif_info->watch_timer); +del_timer_shutdown (&ssif_info->retry_timer);if (ssif_info->thread) {complete (&ssif_info->wake_thread);kthread_stop (ssif_info->thread);
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 10/31] timers: ipmi: Use del_timer_shutdown() before freeing timer
Quoted 3 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.
Thanks, this is in my queue, or:
if you prefer that.
-corey
Quoted 41 lines
Cc Corey Minyard <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>--- drivers/char/ipmi/ipmi_msghandler.c | 2 +- drivers/char/ipmi/ipmi_ssif.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)drivers/char/ipmi/ipmi_msghandler.c +1
-1@@ -5540,7 +5540,7 @@ static void __exit cleanup_ipmi(void)* here. */ atomic_set (&stop_operation,1 ); -del_timer_sync (&ipmi_timer); +del_timer_shutdown (&ipmi_timer); initialized = false;drivers/char/ipmi/ipmi_ssif.c +2
-2@@ -1273,8 +1273,8 @@ static void shutdown_ssif(void *send_info)schedule_timeout (1 ); ssif_info->stopping = true; -del_timer_sync (&ssif_info->watch_timer); -del_timer_sync (&ssif_info->retry_timer); +del_timer_shutdown (&ssif_info->watch_timer); +del_timer_shutdown (&ssif_info->retry_timer);if (ssif_info->thread) {complete (&ssif_info->wake_thread);kthread_stop (ssif_info->thread);Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 10/31] timers: ipmi: Use del_timer_shutdown() before freeing timer
Quoted 10 lines · Corey Minyard
Quoted 3 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.Thanks, this is in my queue, or:
Acked-by Corey Minyard <[email protected]>if you prefer that.
Well, del_timer_shutdown() isn't there yet, so I guess the Ack is what you need.
-corey
Quoted 44 lines
-corey
Quoted 41 lines
Cc Corey Minyard <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>--- drivers/char/ipmi/ipmi_msghandler.c | 2 +- drivers/char/ipmi/ipmi_ssif.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)drivers/char/ipmi/ipmi_msghandler.c +1
-1@@ -5540,7 +5540,7 @@ static void __exit cleanup_ipmi(void)* here. */ atomic_set (&stop_operation,1 ); -del_timer_sync (&ipmi_timer); +del_timer_shutdown (&ipmi_timer); initialized = false;drivers/char/ipmi/ipmi_ssif.c +2
-2@@ -1273,8 +1273,8 @@ static void shutdown_ssif(void *send_info)schedule_timeout (1 ); ssif_info->stopping = true; -del_timer_sync (&ssif_info->watch_timer); -del_timer_sync (&ssif_info->retry_timer); +del_timer_shutdown (&ssif_info->watch_timer); +del_timer_shutdown (&ssif_info->retry_timer);if (ssif_info->thread) {complete (&ssif_info->wake_thread);kthread_stop (ssif_info->thread);Signature · 1 line
2.35.1
Steven Rostedt
reviewerRe: [RFC][PATCH v2 10/31] timers: ipmi: Use del_timer_shutdown() before freeing timer
Quoted 2 lines · On Thu, 27 Oct 2022 10:22:54 -0500 Corey Minyard <[email protected]> wrote:
Well,
del_timer_shutdown()isn't there yet, so I guess the Ack is what you need.
Yep :-)
I need Thomas and Linus's OK on that too, hence the RFC.
-- Steve
Steven Rostedt
reviewerRFC PATCH v2 11/31 timers: random: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/char/random.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/char/random.c +1 -1
@@ -1233,7 +1233,7 @@ static void __cold try_to_generate_entropy(void) stack.entropy =random_get_entropy (); } -del_timer_sync (&stack.timer); +del_timer_shutdown (&stack.timer);destroy_timer_on_stack (&stack.timer);mix_pool_bytes (&stack.entropy,sizeof (stack.entropy)); }
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 11/31] timers: random: Use del_timer_shutdown() before freeing timer
Quoted 9 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.Cc "Theodore Ts'o" <[email protected]>Cc "Jason A. Donenfeld" <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>
If this patchset lands,
However, please keep with the uniform subject convention for commits to that file. Namely, title this:
random: use del_timer_shutdown() before freeing timer
Jason
Steven Rostedt
reviewerRFC PATCH v2 12/31 timers: dma-buf: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/dma-buf/st-dma-fence.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/dma-buf/st-dma-fence.c +1 -1
@@ -412,7 +412,7 @@ static int test_wait_timeout(void *arg) err =0 ; err_free: -del_timer_sync (&wt.timer); +del_timer_shutdown (&wt.timer);destroy_timer_on_stack (&wt.timer);dma_fence_signal (wt.f);dma_fence_put (wt.f);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRe: [RFC][PATCH v2 12/31] timers: dma-buf: Use del_timer_shutdown() before freeing timer
[
quilt mail --send still can't handle unicode characters.
Here's the patch again
]
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/dma-buf/st-dma-fence.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/dma-buf/st-dma-fence.c +1 -1
@@ -412,7 +412,7 @@ static int test_wait_timeout(void *arg) err =0 ; err_free: -del_timer_sync (&wt.timer); +del_timer_shutdown (&wt.timer);destroy_timer_on_stack (&wt.timer);dma_fence_signal (wt.f);dma_fence_put (wt.f);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 13/31 timers: drm: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/gpu/drm/gud/gud_pipe.c | 2 +- drivers/gpu/drm/i915/i915_sw_fence.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/gud/gud_pipe.c +1 -1
@@ -272,7 +272,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t len)usb_sg_wait (&ctx.sgr); -if (!del_timer_sync (&ctx.timer)) +if (!del_timer_shutdown (&ctx.timer)) ret = -ETIMEDOUT ;else if (ctx.sgr.status <0 ) ret = ctx.sgr.status;
drivers/gpu/drm/i915/i915_sw_fence.c +1 -1
@@ -465,7 +465,7 @@ static void irq_i915_sw_fence_work(struct irq_work *wrk)struct i915_sw_dma_fence_cb_timer *cb =container_of (wrk,typeof (*cb), work); -del_timer_sync (&cb->timer); +del_timer_shutdown (&cb->timer);dma_fence_put (cb->dma);kfree_rcu (cb, rcu);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRe: [RFC][PATCH v2 13/31] timers: drm: Use del_timer_shutdown() before freeing timer
[
quilt mail --send still can't handle unicode characters.
Here's the patch again
]
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/gpu/drm/gud/gud_pipe.c | 2 +- drivers/gpu/drm/i915/i915_sw_fence.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/gud/gud_pipe.c +1 -1
@@ -272,7 +272,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t len)usb_sg_wait (&ctx.sgr); -if (!del_timer_sync (&ctx.timer)) +if (!del_timer_shutdown (&ctx.timer)) ret = -ETIMEDOUT ;else if (ctx.sgr.status <0 ) ret = ctx.sgr.status;
drivers/gpu/drm/i915/i915_sw_fence.c +1 -1
@@ -465,7 +465,7 @@ static void irq_i915_sw_fence_work(struct irq_work *wrk)struct i915_sw_dma_fence_cb_timer *cb =container_of (wrk,typeof (*cb), work); -del_timer_sync (&cb->timer); +del_timer_shutdown (&cb->timer);dma_fence_put (cb->dma);kfree_rcu (cb, rcu);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 14/31 timers: HID: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/hid/hid-wiimote-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/hid/hid-wiimote-core.c +1 -1
@@ -1764,7 +1764,7 @@ static void wiimote_destroy(struct wiimote_data *wdata)spin_unlock_irqrestore (&wdata->state.lock, flags);cancel_work_sync (&wdata->init_worker); -del_timer_sync (&wdata->timer); +del_timer_shutdown (&wdata->timer);device_remove_file (&wdata->hdev->dev, &dev_attr_devtype);device_remove_file (&wdata->hdev->dev, &dev_attr_extension);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 16/31 timers: mISDN: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/isdn/hardware/mISDN/hfcmulti.c | 2 +- drivers/isdn/mISDN/l1oip_core.c | 4 ++-- drivers/isdn/mISDN/timerdev.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-)
drivers/isdn/hardware/mISDN/hfcmulti.c +1 -1
@@ -4544,7 +4544,7 @@ release_port(struct hfc_multi *hc, struct dchannel *dch)spin_lock_irqsave (&hc->lock, flags);if (dch->timer.function) { -del_timer (&dch->timer); +del_timer_shutdown (&dch->timer); dch->timer.function =NULL ; }
drivers/isdn/mISDN/l1oip_core.c +2 -2
@@ -1236,8 +1236,8 @@ release_card(struct l1oip *hc) hc->shutdown = true; -del_timer_sync (&hc->keep_tl); -del_timer_sync (&hc->timeout_tl); +del_timer_shutdown (&hc->keep_tl); +del_timer_shutdown (&hc->timeout_tl);cancel_work_sync (&hc->workq);
drivers/isdn/mISDN/timerdev.c +2 -2
@@ -74,7 +74,7 @@ mISDN_close(struct inode *ino, struct file *filep)while (!list_empty (list)) { timer = list_first_entry(list, struct mISDNtimer, list);spin_unlock_irq (&dev->lock); -del_timer_sync (&timer->tl); +del_timer_shutdown (&timer->tl);spin_lock_irq (&dev->lock);/* it might have been moved to ->expired */ list_del (&timer->list);
@@ -204,7 +204,7 @@ misdn_del_timer(struct mISDNtimerdev *dev, int id)list_del_init (&timer->list); timer->id =-1 ;spin_unlock_irq (&dev->lock); -del_timer_sync (&timer->tl); +del_timer_shutdown (&timer->tl);kfree (timer);return id; }
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 17/31 timers: leds: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- 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 +- 4 files changed, 4 insertions(+), 4 deletions(-)
drivers/leds/trigger/ledtrig-activity.c +1 -1
@@ -208,7 +208,7 @@ static void activity_deactivate(struct led_classdev *led_cdev) {struct activity_data *activity_data =led_get_trigger_data (led_cdev); -del_timer_sync (&activity_data->timer); +del_timer_shutdown (&activity_data->timer);kfree (activity_data);clear_bit (LED_BLINK_SW , &led_cdev->work_flags); }
drivers/leds/trigger/ledtrig-heartbeat.c +1 -1
@@ -151,7 +151,7 @@ static void heartbeat_trig_deactivate(struct led_classdev *led_cdev)struct heartbeat_trig_data *heartbeat_data =led_get_trigger_data (led_cdev); -del_timer_sync (&heartbeat_data->timer); +del_timer_shutdown (&heartbeat_data->timer);kfree (heartbeat_data);clear_bit (LED_BLINK_SW , &led_cdev->work_flags); }
drivers/leds/trigger/ledtrig-pattern.c +1 -1
@@ -430,7 +430,7 @@ static void pattern_trig_deactivate(struct led_classdev *led_cdev)if (led_cdev->pattern_clear) led_cdev->pattern_clear (led_cdev); -del_timer_sync (&data->timer); +del_timer_shutdown (&data->timer);led_set_brightness (led_cdev,LED_OFF );kfree (data);
drivers/leds/trigger/ledtrig-transient.c +1 -1
@@ -180,7 +180,7 @@ static void transient_trig_deactivate(struct led_classdev *led_cdev) {struct transient_trig_data *transient_data =led_get_trigger_data (led_cdev); -del_timer_sync (&transient_data->timer); +del_timer_shutdown (&transient_data->timer);led_set_brightness_nosleep (led_cdev, transient_data->restore_state);kfree (transient_data); }
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 18/31 timers: media: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/media/pci/ivtv/ivtv-driver.c | 2 +- drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 18 +++++++++--------- drivers/media/usb/s2255/s2255drv.c | 4 ++-- .../staging/media/atomisp/i2c/atomisp-lm3554.c | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-)
drivers/media/pci/ivtv/ivtv-driver.c +1 -1
@@ -1425,7 +1425,7 @@ static void ivtv_remove(struct pci_dev *pdev)/* Interrupts */ ivtv_set_irq_mask (itv,0xffffffff ); -del_timer_sync (&itv->dma_timer); +del_timer_shutdown (&itv->dma_timer);/* Kill irq worker */ kthread_flush_worker (&itv->irq_worker);
drivers/media/usb/pvrusb2/pvrusb2-hdw.c +9 -9
@@ -2605,10 +2605,10 @@ struct pvr2_hdw *pvr2_hdw_create(struct usb_interface *intf,return hdw; fail:if (hdw) { -del_timer_sync (&hdw->quiescent_timer); -del_timer_sync (&hdw->decoder_stabilization_timer); -del_timer_sync (&hdw->encoder_run_timer); -del_timer_sync (&hdw->encoder_wait_timer); +del_timer_shutdown (&hdw->quiescent_timer); +del_timer_shutdown (&hdw->decoder_stabilization_timer); +del_timer_shutdown (&hdw->encoder_run_timer); +del_timer_shutdown (&hdw->encoder_wait_timer);flush_work (&hdw->workpoll);v4l2_device_unregister (&hdw->v4l2_dev);usb_free_urb (hdw->ctl_read_urb);
@@ -2668,10 +2668,10 @@ void pvr2_hdw_destroy(struct pvr2_hdw *hdw)if (!hdw)return ;pvr2_trace (PVR2_TRACE_INIT ,"pvr2_hdw_destroy: hdw=%p" ,hdw);flush_work (&hdw->workpoll); -del_timer_sync (&hdw->quiescent_timer); -del_timer_sync (&hdw->decoder_stabilization_timer); -del_timer_sync (&hdw->encoder_run_timer); -del_timer_sync (&hdw->encoder_wait_timer); +del_timer_shutdown (&hdw->quiescent_timer); +del_timer_shutdown (&hdw->decoder_stabilization_timer); +del_timer_shutdown (&hdw->encoder_run_timer); +del_timer_shutdown (&hdw->encoder_wait_timer);if (hdw->fw_buffer) {kfree (hdw->fw_buffer); hdw->fw_buffer =NULL ;
@@ -3722,7 +3722,7 @@ status); hdw->cmd_debug_state =5 ;/* Stop timer */ -del_timer_sync (&timer.timer); +del_timer_shutdown (&timer.timer); hdw->cmd_debug_state =6 ; status =0 ;
drivers/media/usb/s2255/s2255drv.c +2 -2
@@ -1487,7 +1487,7 @@ static void s2255_destroy(struct s2255_dev *dev)/* board shutdown stops the read pipe if it is running */ s2255_board_shutdown (dev);/* make sure firmware still not trying to load */ -del_timer_sync (&dev->timer);/* only started in .probe and .open */ +del_timer_shutdown (&dev->timer);/* only started in .probe and .open */ if (dev->fw_data->fw_urb) {usb_kill_urb (dev->fw_data->fw_urb);usb_free_urb (dev->fw_data->fw_urb);
@@ -2322,7 +2322,7 @@ static int s2255_probe(struct usb_interface *interface, errorFWDATA2:usb_free_urb (dev->fw_data->fw_urb); errorFWURB: -del_timer_sync (&dev->timer); +del_timer_shutdown (&dev->timer); errorEP:usb_put_dev (dev->udev); errorUDEV:
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c +1 -1
@@ -921,7 +921,7 @@ static void lm3554_remove(struct i2c_client *client)atomisp_gmin_remove_subdev (sd); -del_timer_sync (&flash->flash_off_delay); +del_timer_shutdown (&flash->flash_off_delay);lm3554_gpio_uninit (client);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 19/31 timers: net: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- 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 ++++---- 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 +- 27 files changed, 35 insertions(+), 35 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_main.c +3 -3
@@ -15528,7 +15528,7 @@ static int i40e_init_recovery_mode(struct i40e_pf *pf, struct i40e_hw *hw) err_switch_setup:i40e_reset_interrupt_capability (pf); -del_timer_sync (&pf->service_timer); +del_timer_shutdown (&pf->service_timer);i40e_shutdown_adminq (hw);iounmap (hw->hw_addr);pci_disable_pcie_error_reporting (pf->pdev);
@@ -16147,7 +16147,7 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)kfree (pf->vsi); err_switch_setup:i40e_reset_interrupt_capability (pf); -del_timer_sync (&pf->service_timer); +del_timer_shutdown (&pf->service_timer); err_mac_addr: err_configure_lan_hmc: (void )i40e_shutdown_lan_hmc (hw);
@@ -16209,7 +16209,7 @@ static void i40e_remove(struct pci_dev *pdev)set_bit (__I40E_SUSPENDED, pf->state);set_bit (__I40E_DOWN, pf->state);if (pf->service_timer.function) -del_timer_sync (&pf->service_timer); +del_timer_shutdown (&pf->service_timer);if (pf->service_task.func)cancel_work_sync (&pf->service_task);
drivers/net/ethernet/marvell/sky2.c +1 -1
@@ -5013,7 +5013,7 @@ static void sky2_remove(struct pci_dev *pdev)if (!hw)return ; -del_timer_sync (&hw->watchdog_timer); +del_timer_shutdown (&hw->watchdog_timer);cancel_work_sync (&hw->restart_work);for (i = hw->ports-1 ; i >=0 ; --i)
drivers/net/ethernet/sun/sunvnet.c +1 -1
@@ -524,7 +524,7 @@ static void vnet_port_remove(struct vio_dev *vdev)hlist_del_rcu (&port->hash);synchronize_rcu (); -del_timer_sync (&port->clean_timer); +del_timer_shutdown (&port->clean_timer);sunvnet_port_rm_txq_common (port);netif_napi_del (&port->napi);sunvnet_port_free_tx_bufs_common (port);
drivers/net/usb/sierra_net.c +1 -1
@@ -759,7 +759,7 @@ static void sierra_net_unbind(struct usbnet *dev, struct usb_interface *intf)dev_dbg (&dev->udev->dev,"%s" , __func__);/* kill the timer and work */ -del_timer_sync (&priv->sync_timer); +del_timer_shutdown (&priv->sync_timer);cancel_work_sync (&priv->sierra_net_kevent);/* tell modem we are going away */
drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c +1 -1
@@ -371,7 +371,7 @@ void iwl_dbg_tlv_del_timers(struct iwl_trans *trans)struct iwl_dbg_tlv_timer_node *node, *tmp;list_for_each_entry_safe (node, tmp, timer_list, list) { -del_timer_sync (&node->timer); +del_timer_shutdown (&node->timer);list_del (&node->list);kfree (node); }
drivers/net/wireless/intersil/hostap/hostap_ap.c +1 -1
@@ -135,7 +135,7 @@ static void ap_free_sta(struct ap_data *ap, struct sta_info *sta)if (!sta->ap)kfree (sta->u.sta.challenge); -del_timer_sync (&sta->timer); +del_timer_shutdown (&sta->timer);#endif /* PRISM2_NO_KERNEL_IEEE80211_MGMT */ kfree (sta);
drivers/net/wireless/marvell/mwifiex/main.c +1 -1
@@ -123,7 +123,7 @@ static int mwifiex_unregister(struct mwifiex_adapter *adapter)if (adapter->if_ops.cleanup_if) adapter->if_ops.cleanup_if (adapter); -del_timer_sync (&adapter->cmd_timer); +del_timer_shutdown (&adapter->cmd_timer);/* Free private structures */ for (i =0 ; i < adapter->priv_num; i++) {
drivers/net/wireless/microchip/wilc1000/hif.c +4 -4
@@ -1520,10 +1520,10 @@ int wilc_deinit(struct wilc_vif *vif)mutex_lock (&vif->wilc->deinit_lock); -del_timer_sync (&hif_drv->scan_timer); -del_timer_sync (&hif_drv->connect_timer); -del_timer_sync (&vif->periodic_rssi); -del_timer_sync (&hif_drv->remain_on_ch_timer); +del_timer_shutdown (&hif_drv->scan_timer); +del_timer_shutdown (&hif_drv->connect_timer); +del_timer_shutdown (&vif->periodic_rssi); +del_timer_shutdown (&hif_drv->remain_on_ch_timer);if (hif_drv->usr_scan_req.scan_result) { hif_drv->usr_scan_req.scan_result(SCAN_EVENT_ABORTED, NULL,
net/802/garp.c +1 -1
@@ -618,7 +618,7 @@ void garp_uninit_applicant(struct net_device *dev, struct garp_application *appl/* Delete timer and generate a final TRANSMIT_PDU event to flush out * all pending messages before the applicant is gone. */ -del_timer_sync (&app->join_timer); +del_timer_shutdown (&app->join_timer);spin_lock_bh (&app->lock);garp_gid_event (app,GARP_EVENT_TRANSMIT_PDU );
net/802/mrp.c +1 -1
@@ -904,7 +904,7 @@ void mrp_uninit_applicant(struct net_device *dev, struct mrp_application *appl)* all pending messages before the applicant is gone. */ del_timer_sync (&app->join_timer); -del_timer_sync (&app->periodic_timer); +del_timer_shutdown (&app->periodic_timer);spin_lock_bh (&app->lock);mrp_mad_event (app,MRP_EVENT_TX );
net/bridge/br_multicast.c +3 -3
@@ -605,7 +605,7 @@ static void br_multicast_destroy_mdb_entry(struct net_bridge_mcast_gc *gc)WARN_ON (!hlist_unhashed (&mp->mdb_node));WARN_ON (mp->ports); -del_timer_sync (&mp->timer); +del_timer_shutdown (&mp->timer);kfree_rcu (mp, rcu); }
@@ -646,7 +646,7 @@ static void br_multicast_destroy_group_src(struct net_bridge_mcast_gc *gc) src = container_of(gc, struct net_bridge_group_src, mcast_gc);WARN_ON (!hlist_unhashed (&src->node)); -del_timer_sync (&src->timer); +del_timer_shutdown (&src->timer);kfree_rcu (src, rcu); }
@@ -671,7 +671,7 @@ static void br_multicast_destroy_port_group(struct net_bridge_mcast_gc *gc)WARN_ON (!hlist_empty (&pg->src_list));del_timer_sync (&pg->rexmit_timer); -del_timer_sync (&pg->timer); +del_timer_shutdown (&pg->timer);kfree_rcu (pg, rcu); }
net/bridge/br_multicast_eht.c +2 -2
@@ -142,7 +142,7 @@ static void br_multicast_destroy_eht_set_entry(struct net_bridge_mcast_gc *gc) set_h = container_of(gc, struct net_bridge_group_eht_set_entry, mcast_gc);WARN_ON (!RB_EMPTY_NODE (&set_h->rb_node)); -del_timer_sync (&set_h->timer); +del_timer_shutdown (&set_h->timer);kfree (set_h); }
@@ -154,7 +154,7 @@ static void br_multicast_destroy_eht_set(struct net_bridge_mcast_gc *gc)WARN_ON (!RB_EMPTY_NODE (&eht_set->rb_node));WARN_ON (!RB_EMPTY_ROOT (&eht_set->entry_tree)); -del_timer_sync (&eht_set->timer); +del_timer_shutdown (&eht_set->timer);kfree (eht_set); }
net/core/gen_estimator.c +1 -1
@@ -208,7 +208,7 @@ void gen_kill_estimator(struct net_rate_estimator __rcu **rate_est) est =xchg ((__force struct net_rate_estimator **)rate_est,NULL );if (est) { -del_timer_sync (&est->timer); +del_timer_shutdown (&est->timer);kfree_rcu (est, rcu); } }
net/core/sock.c +1 -1
@@ -3352,7 +3352,7 @@ EXPORT_SYMBOL(sk_stop_timer);void sk_stop_timer_sync (struct sock *sk,struct timer_list *timer) { -if (del_timer_sync (timer)) +if (del_timer_shutdown (timer))__sock_put (sk); }EXPORT_SYMBOL (sk_stop_timer_sync);
net/ipv4/inet_timewait_sock.c +1 -1
@@ -208,7 +208,7 @@ EXPORT_SYMBOL_GPL(inet_twsk_alloc);*/ void inet_twsk_deschedule_put (struct inet_timewait_sock *tw) { -if (del_timer_sync (&tw->tw_timer)) +if (del_timer_shutdown (&tw->tw_timer))inet_twsk_kill (tw);inet_twsk_put (tw); }
net/ipv4/ipmr.c +1 -1
@@ -412,7 +412,7 @@ static struct mr_table *ipmr_new_table(struct net *net, u32 id)static void ipmr_free_table (struct mr_table *mrt) { -del_timer_sync (&mrt->ipmr_expire_timer); +del_timer_shutdown (&mrt->ipmr_expire_timer);mroute_clean_tables (mrt,MRT_FLUSH_VIFS |MRT_FLUSH_VIFS_STATIC |MRT_FLUSH_MFC |MRT_FLUSH_MFC_STATIC );rhltable_destroy (&mrt->mfc_hash);
net/ipv6/ip6mr.c +1 -1
@@ -392,7 +392,7 @@ static struct mr_table *ip6mr_new_table(struct net *net, u32 id)static void ip6mr_free_table (struct mr_table *mrt) { -del_timer_sync (&mrt->ipmr_expire_timer); +del_timer_shutdown (&mrt->ipmr_expire_timer);mroute_clean_tables (mrt,MRT6_FLUSH_MIFS |MRT6_FLUSH_MIFS_STATIC |MRT6_FLUSH_MFC |MRT6_FLUSH_MFC_STATIC );rhltable_destroy (&mrt->mfc_hash);
net/mac80211/mesh_pathtbl.c +1 -1
@@ -512,7 +512,7 @@ static void mesh_path_free_rcu(struct mesh_table *tbl, mpath->flags |=MESH_PATH_RESOLVING |MESH_PATH_DELETED ;mesh_gate_del (tbl, mpath);spin_unlock_bh (&mpath->state_lock); -del_timer_sync (&mpath->timer); +del_timer_shutdown (&mpath->timer);atomic_dec (&sdata->u.mesh.mpaths);atomic_dec (&tbl->entries);mesh_path_flush_pending (mpath);
net/netfilter/ipset/ip_set_list_set.c +1 -1
@@ -427,7 +427,7 @@ list_set_destroy(struct ip_set *set)struct set_elem *e, *n;if (SET_WITH_TIMEOUT (set)) -del_timer_sync (&map->gc); +del_timer_shutdown (&map->gc);list_for_each_entry_safe (e, n, &map->members, list) {list_del (&e->list);
net/netfilter/ipvs/ip_vs_lblc.c +1 -1
@@ -384,7 +384,7 @@ static void ip_vs_lblc_done_svc(struct ip_vs_service *svc)struct ip_vs_lblc_table *tbl = svc->sched_data;/* remove periodic timer */ -del_timer_sync (&tbl->periodic_timer); +del_timer_shutdown (&tbl->periodic_timer);/* got to clean up table entries here */ ip_vs_lblc_flush (svc);
net/netfilter/ipvs/ip_vs_lblcr.c +1 -1
@@ -547,7 +547,7 @@ static void ip_vs_lblcr_done_svc(struct ip_vs_service *svc)struct ip_vs_lblcr_table *tbl = svc->sched_data;/* remove periodic timer */ -del_timer_sync (&tbl->periodic_timer); +del_timer_shutdown (&tbl->periodic_timer);/* got to clean up table entries here */ ip_vs_lblcr_flush (svc);
net/netfilter/xt_LED.c +1 -1
@@ -166,7 +166,7 @@ static void led_tg_destroy(const struct xt_tgdtor_param *par)list_del (&ledinternal->list); -del_timer_sync (&ledinternal->timer); +del_timer_shutdown (&ledinternal->timer);led_trigger_unregister (&ledinternal->netfilter_led_trigger);
net/rxrpc/conn_object.c +1 -1
@@ -358,7 +358,7 @@ static void rxrpc_destroy_connection(struct rcu_head *rcu)_net ("DESTROY CONN %d" , conn->debug_id); -del_timer_sync (&conn->timer); +del_timer_shutdown (&conn->timer);rxrpc_purge_queue (&conn->rx_queue); conn->security->clear (conn);
net/sched/cls_flow.c +1 -1
@@ -367,7 +367,7 @@ static const struct nla_policy flow_policy[TCA_FLOW_MAX + 1] = {static void __flow_destroy_filter (struct flow_filter *f) { -del_timer_sync (&f->perturb_timer); +del_timer_shutdown (&f->perturb_timer);tcf_exts_destroy (&f->exts);tcf_em_tree_destroy (&f->ematches);tcf_exts_put_net (&f->exts);
net/sunrpc/svc.c +1 -1
@@ -567,7 +567,7 @@ svc_destroy(struct kref *ref) struct svc_serv *serv = container_of(ref, struct svc_serv, sv_refcnt);dprintk ("svc: svc_destroy(%s)\n" , serv->sv_program->pg_name); -del_timer_sync (&serv->sv_temptimer); +del_timer_shutdown (&serv->sv_temptimer);/* * The last user is gone and thus all sockets have to be destroyed to
net/tipc/discover.c +1 -1
@@ -385,7 +385,7 @@ int tipc_disc_create(struct net *net, struct tipc_bearer *b,*/ void tipc_disc_delete (struct tipc_discoverer *d) { -del_timer_sync (&d->timer); +del_timer_shutdown (&d->timer);kfree_skb (d->skb);kfree (d); }
net/tipc/monitor.c +1 -1
@@ -700,7 +700,7 @@ void tipc_mon_delete(struct net *net, int bearer_id) } mon->self =NULL ;write_unlock_bh (&mon->lock); -del_timer_sync (&mon->timer); +del_timer_shutdown (&mon->timer);kfree (self->domain);kfree (self);kfree (mon);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRe: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 50 lines · On Thu, 27 Oct 2022 12:38:16 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 7 lines · Steven Rostedt
Quoted 4 lines · On Thu, 27 Oct 2022 15:20:58 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 3 lines
(many more of those)
...[ 16.329989] timer_fixup_free+0x40/0x54Quoted 8 lines
Quoted 5 lines
Ah, I see the issue here. Looks like the
timer_fixup_free()is calling itself and crashing.Let me take a look into that. I didn't touch the fixup code, and there could be an assumption there that it's behaving with the old approach.
Can you add this and see if it makes this issue go away?
Yes, that fixes the crash. However, it still reports
[ 12.235054] ------------[ cut here ]------------ [ 12.235240] ODEBUG: free active (active state 0) object type: timer_list hint: tcp_write_timer+0x0/0x190 [ 12.237331] WARNING: CPU: 0 PID: 310 at lib/debugobjects.c:502 debug_print_object+0xb8/0x100 ... [ 12.255251] Call trace: [ 12.255305] debug_print_object+0xb8/0x100 [ 12.255385] __debug_check_no_obj_freed+0x1d0/0x25c [ 12.255474] debug_check_no_obj_freed+0x20/0x90 [ 12.255555] slab_free_freelist_hook.constprop.0+0xac/0x1b0 [ 12.255650] kmem_cache_free+0x1ac/0x500 [ 12.255728] __sk_destruct+0x140/0x2a0 [ 12.255805] sk_destruct+0x54/0x64 [ 12.255877] __sk_free+0x74/0x120 [ 12.255944] sk_free+0x64/0x8c [ 12.256009] tcp_close+0x94/0xc0 [ 12.256076] inet_release+0x50/0xb0 [ 12.256145] __sock_release+0x44/0xbc [ 12.256219] sock_close+0x18/0x30 [ 12.256292] __fput+0x84/0x270 [ 12.256361] ____fput+0x10/0x20 [ 12.256426] task_work_run+0x88/0xf0 [ 12.256499] do_exit+0x334/0xafc [ 12.256566] do_group_exit+0x34/0x90 [ 12.256634] __arm64_sys_exit_group+0x18/0x20 [ 12.256713] invoke_syscall+0x48/0x114 [ 12.256789] el0_svc_common.constprop.0+0x60/0x11c [ 12.256874] do_el0_svc+0x30/0xd0 [ 12.256943] el0_svc+0x48/0xc0 [ 12.257008] el0t_64_sync_handler+0xbc/0x13c [ 12.257086] el0t_64_sync+0x18c/0x190
Quoted 4 lines
Is that a real problem or a false positive ? I didn't see that without your patch series (which of course might be the whole point of the series).
I think this is indeed an issue, and I'm replying to the net patch as it has the necessary folks Cc'd.
The ipv4 tcp code has:
void tcp_init_xmit_timers(struct sock *sk)
{
inet_csk_init_xmit_timers(sk, &tcp_write_timer, &tcp_delack_timer,
&tcp_keepalive_timer);And from the above back trace:
tcp_close() where I'm assuming that tcp_disconnect() or tcp_done() was called that both calls:
tcp_clear_xmit_timers(sk);
That calls:
inet_csk_clear_xmit_timers(sk);
That has:
void inet_csk_clear_xmit_timers(struct sock *sk)
{
struct inet_connection_sock *icsk = inet_csk(sk);
icsk->icsk_pending = icsk->icsk_ack.pending = 0;
sk_stop_timer(sk, &icsk->icsk_retransmit_timer);
sk_stop_timer(sk, &icsk->icsk_delack_timer);
sk_stop_timer(sk, &sk->sk_timer);
}Where:
void sk_stop_timer(struct sock *sk, struct timer_list* timer)
{
if (del_timer(timer))
__sock_put(sk);
}Hence, this is a case where we have timers that have been disabled with only del_timer() before the timers are freed.
I think we need to update this code to squeeze in a del_timer_shutdown() to make sure that the timers are never restarted.
There is a sk_stop_timer_sync() that I changed to use del_timer_shutdown() but that's only used in one file: net/mptcp/pm_netlink.c
-- Steve
Re: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 3 lines · Steven Rostedt
I think we need to update this code to squeeze in a
del_timer_shutdown()to make sure that the timers are never restarted.
So the reason the networking code does this is that it can't just do the old 'sync()' thing, the timers are deleted in contexts where that isn't valid.
Which is also afaik why the networking code does that whole "timer implies a refcount to the socket" and then does the
if (del_timer(timer))
sock_put()thing (ie if the del_timer failed - possibly because it was already running - you leave the refcount alone).
So the networking code cannot do the del_timer_shutdown() for the same reason it cannot do the del_timer_sync(): it can't afford to wait for the timer to stop running.
I suspect it needs something like a new "del_timer_shutdown_async()" that isn't synchronous, but does that
- acts as del_timer in that it doesn't wait, and returns a success if
it could just remove the pending case
- does that "mark timer for shutdown" in that success case
or something similar.
But the networking people will know better.
Linus
Steven Rostedt
reviewerRe: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 9 lines · On Thu, 27 Oct 2022 13:15:23 -0700 Linus Torvalds…
Quoted 3 lines · Steven Rostedt
I think we need to update this code to squeeze in a
del_timer_shutdown()to make sure that the timers are never restarted.So the reason the networking code does this is that it can't just do the old '
sync()' thing, the timers are deleted in contexts where that isn't valid.
Quoted 8 lines
Which is also afaik why the networking code does that whole "timer implies a refcount to the socket" and then does the
if (del_timer(timer)) sock_put()thing (ie if the
del_timerfailed - possibly because it was already running - you leave the refcount alone).
OK, so the above is assuming that the timer is always active, and del_timer() returns if it successfully removed it (where it can call sock_put()), but if del_timer() returns 0, that means the timer is currently running (or about to be), so it doesn't call sock_put().
Quoted 8 lines
So the networking code cannot do the
del_timer_shutdown()for the same reason it cannot do thedel_timer_sync(): it can't afford to wait for the timer to stop running.I suspect it needs something like a new "
del_timer_shutdown_async()" that isn't synchronous, but does that
Quoted 7 lines
- acts as
del_timerin that it doesn't wait, and returns a success if
it could just remove the pending case- does that "mark timer for shutdown" in that success case
or something similar.
What about del_timer_try_shutdown(), that if it removes the timer, it sets the function to NULL (making it equivalent to a successful shutdown), otherwise it does nothing. Allowing the the timer to be rearmed.
I think this would work in this case.
-- Steve
Re: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 4 lines · Steven Rostedt
What about
del_timer_try_shutdown(), that if it removes the timer, it sets the function to NULL (making it equivalent to a successful shutdown), otherwise it does nothing. Allowing the the timer to be rearmed.
Sounds sane to me and should work, but as mentioned, I think the networking people need to say "yeah" too.
And maybe that function can also disallow any future re-arming even for the case where the timer couldn't be actively removed.
So any *currently* active timer wouldn't be waited for (either because locking may make that a deadlock situation, or simply due to performance issues), but at least it would guarantee that no new timer activations can happen.
Because I do like the whole notion of "timer has been shutdown and cannot be used as a timer any more without re-initializing it" being a real state - even for a timer that may be "currently in flight".
So this all sounds very worthwhile to me, but I'm not surprised that we have code that then knows about all the subtleties of "del_timer() might still have a running timer" and actually take advantage of it (where "advantage" is likely more of a "deal with the complexities" rather than anything really positive ;)
And those existing subtle users might want particular semantics to at least make said complexities easier.
Linus
Steven Rostedt
reviewerRe: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 11 lines · On Thu, 27 Oct 2022 13:48:54 -0700 Linus Torvalds…
Quoted 4 lines · Steven Rostedt
What about
del_timer_try_shutdown(), that if it removes the timer, it sets the function to NULL (making it equivalent to a successful shutdown), otherwise it does nothing. Allowing the the timer to be rearmed.Sounds sane to me and should work, but as mentioned, I think the networking people need to say "yeah" too.
And maybe that function can also disallow any future re-arming even for the case where the timer couldn't be actively removed.
Well, I think this current use case will break if we prevent the timer from being rearmed or run again if it's not found. But as you said, the networking folks need to confirm or deny it.
The fact that it does the sock_put() when it removes the timer makes me think that it can be called again, and we shouldn't prevent that from happening.
The debug code will let us know too, as it only "frees" it for freeing if it deactivated the timer and shut it down.
Quoted 10 lines
So any *currently* active timer wouldn't be waited for (either because locking may make that a deadlock situation, or simply due to performance issues), but at least it would guarantee that no new timer activations can happen.
Because I do like the whole notion of "timer has been shutdown and cannot be used as a timer any more without re-initializing it" being a real state - even for a timer that may be "currently in flight".
Quoted 5 lines
So this all sounds very worthwhile to me, but I'm not surprised that we have code that then knows about all the subtleties of "
del_timer()might still have a running timer" and actually take advantage of it (where "advantage" is likely more of a "deal with the complexities" rather than anything really positive ;)
Good to hear. This has been a thorn in our side as we keep hitting these crashes in the timer code that look like a timer was freed before it triggered.
Quoted 4 lines
And those existing subtle users might want particular semantics to at least make said complexities easier.
Yeah, as someone told me recently, "If you let them play long enough without setting out the rules, they will take advantage of everything and it will be extremely hard to get them back in order".
-- Steve
Steven Rostedt
reviewerRe: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 2 lines · On Thu, 27 Oct 2022 17:07:20 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 2 lines
And maybe that function can also disallow any future re-arming even for the case where the timer couldn't be actively removed.
The naming of the functions will depend on this.
If the async version always shuts down the timer, then we should have the interface be:
del_timer_shutdown() <- async
del_timer_shutdown_sync <- sync
As it would match the del_timer() and del_timer_sync() semantics.
If shutdown only happens if the timer is removed, then I believe the current approach of del_timer_shutdown() being synchronous and del_timer_try_shutdown() being async is the way to go, as it follows more the semantics of mutex_lock() and mutex_trylock().
-- Steve
Steven Rostedt
reviewerRe: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtQuoted 10 lines · On Thu, 27 Oct 2022 17:07:20 -0400 Steven Rostedt <[email protected]> wrote:
Well, I think this current use case will break if we prevent the timer from being rearmed or run again if it's not found. But as you said, the networking folks need to confirm or deny it.
The fact that it does the
sock_put()when it removes the timer makes me think that it can be called again, and we shouldn't prevent that from happening.The debug code will let us know too, as it only "frees" it for freeing if it deactivated the timer and shut it down.
I think we have our answer from Guenter's report:
Linux version 6.1.0-rc2-00138-gced58c742836 (groeck@jupiter) (aarch64-linux-gcc (GCC) 11.3.0, GNU ld (GNU Binutils) 2.39) #1 SMP PREEMPT Thu Oct 27 14:53:17 PDT 2022
[ 17.258727] ------------[ cut here ]------------ [ 17.259079] ODEBUG: free active (active state 0) object type: timer_list hint: tcp_write_timer+0x0/0x190 [ 17.259723] WARNING: CPU: 0 PID: 309 at lib/debugobjects.c:502 debug_print_object+0xb8/0x100 [ 17.259951] Modules linked in: [ 17.260249] CPU: 0 PID: 309 Comm: telnet Tainted: G N 6.1.0-rc2-00138-gced58c742836 #1 [ 17.260518] Hardware name: linux,dummy-virt (DT) [ 17.260779] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 17.260967] pc : debug_print_object+0xb8/0x100 [ 17.261096] lr : debug_print_object+0xb8/0x100 [ 17.261223] sp : ffff8000086539e0 [ 17.261324] x29: ffff8000086539e0 x28: 0000000000000004 x27: ffff0d2ac2168000 [ 17.261574] x26: 0000000000000000 x25: ffffa241e2b9de18 x24: ffffa241e4f8fcd8 [ 17.261772] x23: ffffa241e336b370 x22: ffffa241e2b9de18 x21: ffff0d2ac20c5710 [ 17.261967] x20: ffffa241e4ea2568 x19: ffffa241e3ea3c00 x18: 00000000ffffffff [ 17.262161] x17: 6c6973742068696e x16: 3a2074696d65725f x15: 6563742074797065 [ 17.262375] x14: 65203029206f626a x13: ffffa241e3ec7640 x12: 0000000000000d50 [ 17.262591] x11: 0000000000000470 x10: ffffa241e3f1f640 x9 : ffffa241e3ec7640 [ 17.262821] x8 : 00000000ffffefff x7 : ffffa241e3f1f640 x6 : 0000000000000000 [ 17.263028] x5 : ffff0d2adfebba68 x4 : 0000000000000000 x3 : 0000000000000027 [ 17.263235] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0d2ac658b340 [ 17.263528] Call trace: [ 17.263646] debug_print_object+0xb8/0x100 [ 17.263795] __debug_check_no_obj_freed+0x1d0/0x25c [ 17.263927] debug_check_no_obj_freed+0x20/0x90 [ 17.264051] slab_free_freelist_hook.constprop.0+0xac/0x1b0 [ 17.264197] kmem_cache_free+0x1ac/0x500 [ 17.264311] __sk_destruct+0x140/0x2a0 [ 17.264425] sk_destruct+0x54/0x64 [ 17.264531] __sk_free+0x74/0x120 [ 17.264636] sk_free+0x64/0x8c [ 17.264736] tcp_close+0x94/0xc0 [ 17.264840] inet_release+0x50/0xb0 [ 17.264949] __sock_release+0x44/0xbc [ 17.265061] sock_close+0x18/0x30 [ 17.265166] __fput+0x84/0x270 [ 17.265266] ____fput+0x10/0x20 [ 17.265366] task_work_run+0x88/0xf0 [ 17.265491] do_exit+0x334/0xafc [ 17.265596] do_group_exit+0x34/0x90 [ 17.265705] __arm64_sys_exit_group+0x18/0x20 [ 17.265826] invoke_syscall+0x48/0x114 [ 17.265941] el0_svc_common.constprop.0+0x60/0x11c [ 17.266070] do_el0_svc+0x30/0xd0 [ 17.266175] el0_svc+0x48/0xc0 [ 17.266276] el0t_64_sync_handler+0xbc/0x13c [ 17.266396] el0t_64_sync+0x18c/0x190 [ 17.266565] irq event stamp: 5192 [ 17.266676] hardirqs last enabled at (5191): [<ffffa241e1926a18>] __up_console_sem+0x78/0x84 [ 17.266903] hardirqs last disabled at (5192): [<ffffa241e2b4d504>] el1_dbg+0x24/0x90 [ 17.267093] softirqs last enabled at (5170): [<ffffa241e181050c>] __do_softirq+0x46c/0x5d8 [ 17.267305] softirqs last disabled at (5163): [<ffffa241e1816750>] ____do_softirq+0x10/0x20 [ 17.267506] ---[ end trace 0000000000000000 ]--- [ 17.275715] ------------[ cut here ]------------
I'll go modify that code to make it shutdown even if it returns zero.
I thinks this means we'll need to change the name to:
del_timer_shutdown()
del_timer_shutdown_sync()
But I want to confirm this first.
-- Steve
Steven Rostedt
reviewerRe: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Could someone from networking confirm (or deny) that the timer being removed in sk_stop_timer() will no longer be used even if del_timer() returns false?
net/core/sock.c:
void sk_stop_timer(struct sock *sk, struct timer_list* timer)
{
if (del_timer(timer))
__sock_put(sk);
}If this is the case, then I'll add the following interface:
del_timer_sync_shutdown() // the common case which syncs
del_timer_shutdown() // the uncommon case, that returns immediately
// used for those cases that add extra code to
// handle it, like sk_stop_timer()Which has the same semantics as del_timer_sync() and del_timer() respectively, but will prevent the timer from being rearmed again.
This way we can convert the sk_stop_timer() to:
void sk_stop_timer(struct sock *sk, struct timer_list* timer)
{
if (del_timer_shutdown(timer))
__sock_put(sk);
}We can also add the del_timer_shutdown() to other locations that need to put a timer into a shutdown state before freeing, and where it's in a context that can not call del_timer_sync_shutdown().
-- Steve
Jakub Kicinski
maintainerRe: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 12 lines · On Fri, 28 Oct 2022 18:31:49 -0400 Steven Rostedt wrote:
Could someone from networking confirm (or deny) that the timer being removed in
sk_stop_timer()will no longer be used even ifdel_timer()returns false?
net/core/sock.c:void
sk_stop_timer(struct sock *sk, struct timer_list* timer){ if (del_timer(timer)) __sock_put(sk); }
Quoted 7 lines
If this is the case, then I'll add the following interface:
del_timer_sync_shutdown()// the common case which syncsdel_timer_shutdown() // the uncommon case, that returns immediately // used for those cases that add extra code to // handle it, like sk_stop_timer()
Sorry too many bugs at once :)
FWIW Paolo was saying privately earlier today that he spotted some cases of reuse, he gave an example of ccid2_hc_tx_packet_recv()
So we can't convert all cases of sk_stop_timer() in one fell swoop :(
Quoted 15 lines
Which has the same semantics as
del_timer_sync()anddel_timer()respectively, but will prevent the timer from being rearmed again.This way we can convert the
sk_stop_timer()to:void
sk_stop_timer(struct sock *sk, struct timer_list* timer){ if (del_timer_shutdown(timer)) __sock_put(sk); }We can also add the
del_timer_shutdown()to other locations that need to put a timer into a shutdown state before freeing, and where it's in a context that can not calldel_timer_sync_shutdown().
Paolo Abeni
maintainerRe: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 17 lines · Jakub Kicinski
Quoted 16 lines · On Fri, 28 Oct 2022 18:31:49 -0400 Steven Rostedt wrote:
Could someone from networking confirm (or deny) that the timer being removed in
sk_stop_timer()will no longer be used even ifdel_timer()returns false?
net/core/sock.c:void
sk_stop_timer(struct sock *sk, struct timer_list* timer){ if (del_timer(timer)) __sock_put(sk); }If this is the case, then I'll add the following interface:
del_timer_sync_shutdown()// the common case which syncs
Quoted 8 lines
Quoted 3 lines
del_timer_shutdown() // the uncommon case, that returns immediately // used for those cases that add extra code to // handle it, like sk_stop_timer()Sorry too many bugs at once :)
FWIW Paolo was saying privately earlier today that he spotted some cases of reuse, he gave an example of
ccid2_hc_tx_packet_recv()
For the records, there are other cases, e.g. after sk_stop_timer() in clear_3rdack_retransmission() (mptcp code) the timer can be-rearmed without re-initializing. I *think* there are more of such use in the in ax25/rose code.
Quoted 1 line
So we can't convert all cases of
sk_stop_timer()in one fell swoop :(
On the positive side, I think converting the sk_stop_timer in inet_csk_clear_xmit_timers() should be safe and should cover the issue reported by Guenter
Cheers,
Paolo
Steven Rostedt
reviewerRe: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 3 lines · On Sun, 30 Oct 2022 18:22:03 +0100 Paolo Abeni <[email protected]> wrote:
On the positive side, I think converting the
sk_stop_timerininet_csk_clear_xmit_timers()should be safe and should cover the issue reported by Guenter
Would something like this be OK?
[ Note, talking with Thomas Gleixner, we agreed that we are changing the
name to: time_shutdown_sync() and timer_shutdown() (no wait version). I'll be posting new patches soon. ]
-- Steve
include/net/sock.h +2 -0
@@ -2439,6 +2439,8 @@ void sk_stop_timer(struct sock *sk, struct timer_list *timer);void sk_stop_timer_sync (struct sock *sk,struct timer_list *timer); +void sk_shutdown_timer (struct sock *sk,struct timer_list *timer); + int __sk_queue_drop_skb(struct sock *sk, struct sk_buff_head *sk_queue, struct sk_buff *skb, unsigned int flags, void (*destructor)(struct sock *sk,
net/core/sock.c +7 -0
@@ -3357,6 +3357,13 @@ void sk_stop_timer_sync(struct sock *sk, struct timer_list *timer) }EXPORT_SYMBOL (sk_stop_timer_sync); +void sk_shutdown_timer (struct sock *sk,struct timer_list * timer) +{ +if (timer_shutdown (timer)) +__sock_put (sk); +} +EXPORT_SYMBOL (sk_shutdown_timer); +void sock_init_data (struct socket *sock,struct sock *sk) {sk_init_common (sk);
net/ipv4/inet_connection_sock.c +4 -4
@@ -722,15 +722,15 @@ void inet_csk_clear_xmit_timers(struct sock *sk) icsk->icsk_pending = icsk->icsk_ack.pending =0 ; -sk_stop_timer (sk, &icsk->icsk_retransmit_timer); -sk_stop_timer (sk, &icsk->icsk_delack_timer); -sk_stop_timer (sk, &sk->sk_timer); +sk_shutdown_timer (sk, &icsk->icsk_retransmit_timer); +sk_shutdown_timer (sk, &icsk->icsk_delack_timer); +sk_shutdown_timer (sk, &sk->sk_timer); }EXPORT_SYMBOL (inet_csk_clear_xmit_timers);void inet_csk_delete_keepalive_timer (struct sock *sk) { -sk_stop_timer (sk, &sk->sk_timer); +sk_shutdown_timer (sk, &sk->sk_timer); }EXPORT_SYMBOL (inet_csk_delete_keepalive_timer);
Re: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 53 lines · Steven Rostedt
Quoted 3 lines · On Sun, 30 Oct 2022 18:22:03 +0100 Paolo Abeni <[email protected]> wrote:
On the positive side, I think converting the
sk_stop_timerininet_csk_clear_xmit_timers()should be safe and should cover the issue reported by GuenterWould something like this be OK?
[ Note, talking with Thomas Gleixner, we agreed that we are changing the
name to:time_shutdown_sync()andtimer_shutdown()(no wait version). I'll be posting new patches soon. ]-- Steve
include/net/sock.h +2
-0@@ -2439,6 +2439,8 @@ void sk_stop_timer(struct sock *sk, struct timer_list *timer);void sk_stop_timer_sync (struct sock *sk,struct timer_list *timer); +void sk_shutdown_timer (struct sock *sk,struct timer_list *timer); + int __sk_queue_drop_skb(struct sock *sk, struct sk_buff_head *sk_queue, struct sk_buff *skb, unsigned int flags, void (*destructor)(struct sock *sk,net/core/sock.c +7
-0@@ -3357,6 +3357,13 @@ void sk_stop_timer_sync(struct sock *sk, struct timer_list *timer) }EXPORT_SYMBOL (sk_stop_timer_sync); +void sk_shutdown_timer (struct sock *sk,struct timer_list * timer) +{ +if (timer_shutdown (timer)) +__sock_put (sk); +} +EXPORT_SYMBOL (sk_shutdown_timer); +void sock_init_data (struct socket *sock,struct sock *sk) {sk_init_common (sk);net/ipv4/inet_connection_sock.c +0
-0@@ -722,15 +722,15 @@ void inet_csk_clear_xmit_timers(struct sock *sk)
net/ipv4/inet_connection_sock.c void inet_csk_clear_xmit_timers(struct sock *sk) · line 729
Quoted 10 lines
net/ipv4/inet_connection_sock.c +3
-3icsk->icsk_pending = icsk->icsk_ack.pending =0 ; -sk_stop_timer (sk, &icsk->icsk_retransmit_timer); -sk_stop_timer (sk, &icsk->icsk_delack_timer); -sk_stop_timer (sk, &sk->sk_timer); +sk_shutdown_timer (sk, &icsk->icsk_retransmit_timer); +sk_shutdown_timer (sk, &icsk->icsk_delack_timer); +sk_shutdown_timer (sk, &sk->sk_timer); }EXPORT_SYMBOL (inet_csk_clear_xmit_timers);
inet_csk_clear_xmit_timers() can be called multiple times during TCP
socket lifetime.
(See tcp_disconnect(), which can be followed by another connect() ... and loop)
Maybe add a second parameter, or add a new inet_csk_shutdown_xmit_timers() only called from tcp_v4_destroy_sock() ?
net/ipv4/inet_connection_sock.c void inet_csk_clear_xmit_timers(struct sock *sk) · line 733
Quoted 5 lines
net/ipv4/inet_connection_sock.c +1
-1void inet_csk_delete_keepalive_timer (struct sock *sk) { -sk_stop_timer (sk, &sk->sk_timer); +sk_shutdown_timer (sk, &sk->sk_timer);
SO_KEEPALIVE can be called multiple times in a TCP socket lifetime, on/off/on/off/...
I suggest leaving sk_stop_timer() here.
Eventually inet_csk_clear_xmit_timers( sk, destroy=true) (or inet_csk_shutdown_xmit_timers(())
will be called before the socket is destroyed.
Steven Rostedt
reviewerRe: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 8 lines · On Thu, 3 Nov 2022 17:00:20 -0700 Eric Dumazet <[email protected]> wrote:
inet_csk_clear_xmit_timers()can be called multiple times during TCP
socket lifetime.(See
tcp_disconnect(), which can be followed by anotherconnect()... and loop)Maybe add a second parameter, or add a new
inet_csk_shutdown_xmit_timers()only called fromtcp_v4_destroy_sock()?
I guess.
Quoted 6 lines
Review
Quoted 5 lines
void
inet_csk_delete_keepalive_timer(struct sock *sk){(quoted diff) +1
-1- sk_stop_timer(sk, &sk->sk_timer); + sk_shutdown_timer(sk, &sk->sk_timer);
Quoted 8 lines
SO_KEEPALIVEcan be called multiple times in a TCP socket lifetime, on/off/on/off/...I suggest leaving
sk_stop_timer()here.Eventually
inet_csk_clear_xmit_timers( sk, destroy=true)(or inet_csk_shutdown_xmit_timers(())
will be called before the socket is destroyed.
OK.
Guenter,
I posted a new series, but did not include this change. If you want to test that other series, I would suggest to at least add the first part of this patch, otherwise it will trigger. But we want to see if there's other locations of issue that we should care about.
-- Steve
Re: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 29 lines · Steven Rostedt
Quoted 8 lines · On Thu, 3 Nov 2022 17:00:20 -0700 Eric Dumazet <[email protected]> wrote:
inet_csk_clear_xmit_timers()can be called multiple times during TCP
socket lifetime.(See
tcp_disconnect(), which can be followed by anotherconnect()... and loop)Maybe add a second parameter, or add a new
inet_csk_shutdown_xmit_timers()only called fromtcp_v4_destroy_sock()?I guess.
Quoted 6 lines
Review
Quoted 5 lines
void
inet_csk_delete_keepalive_timer(struct sock *sk){(quoted diff) +1
-1- sk_stop_timer(sk, &sk->sk_timer); + sk_shutdown_timer(sk, &sk->sk_timer);Quoted 8 lines
SO_KEEPALIVEcan be called multiple times in a TCP socket lifetime, on/off/on/off/...I suggest leaving
sk_stop_timer()here.Eventually
inet_csk_clear_xmit_timers( sk, destroy=true)(or inet_csk_shutdown_xmit_timers(())
will be called before the socket is destroyed.
Quoted 9 lines
OK.
Guenter,
I posted a new series, but did not include this change. If you want to test that other series, I would suggest to at least add the first part of this patch, otherwise it will trigger. But we want to see if there's other locations of issue that we should care about.
I'll run a test on the other series without change first. We'll see what happens. If necessary I'll add [parts of] this patch and re-test, but before doing that I would like to get a sense for the status of your series as-is.
Thanks,
Guenter
Steven Rostedt
reviewerRe: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtQuoted 5 lines · On Thu, 27 Oct 2022 16:34:53 -0400 Steven Rostedt <[email protected]> wrote:
What about
del_timer_try_shutdown(), that if it removes the timer, it sets the function to NULL (making it equivalent to a successful shutdown), otherwise it does nothing. Allowing the the timer to be rearmed.I think this would work in this case.
Guenter,
Can you apply this patch on top of the series, and see if it makes the warning go away?
include/linux/timer.h +12 -1
@@ -184,12 +184,23 @@ 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); +static inline int del_timer_try_shutdown (struct timer_list *timer) +{ +return __del_timer (timer, true); +} + +static inline int del_timer (struct timer_list *timer) +{ +return __del_timer (timer, false); +} +/* * The jiffies value which is added to now, when there is no timer * in the timer wheel:
kernel/time/timer.c +6 -2
@@ -1255,7 +1255,7 @@ EXPORT_SYMBOL_GPL(add_timer_on);* (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;
@@ -1266,12 +1266,16 @@ 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 && ret) { + timer->function =NULL ; +debug_timer_deactivate (timer); + }raw_spin_unlock_irqrestore (&base->lock, flags); }return ret; } -EXPORT_SYMBOL (del_timer); +EXPORT_SYMBOL (__del_timer);static int __try_to_del_timer_sync (struct timer_list *timer,bool free) {
net/core/sock.c +1 -1
@@ -3345,7 +3345,7 @@ EXPORT_SYMBOL(sk_reset_timer);void sk_stop_timer (struct sock *sk,struct timer_list * timer) { -if (del_timer (timer)) +if (del_timer_try_shutdown (timer))__sock_put (sk); }EXPORT_SYMBOL (sk_stop_timer);
Re: [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
Quoted 7 lines · Steven Rostedt
Quoted 4 lines · On Thu, 27 Oct 2022 16:34:53 -0400 Steven Rostedt <[email protected]> wrote:
What about
del_timer_try_shutdown(), that if it removes the timer, it sets the function to NULL (making it equivalent to a successful shutdown), otherwise it does nothing. Allowing the the timer to be rearmed.
Quoted 7 lines
Quoted 1 line
I think this would work in this case.
Guenter,
Can you apply this patch on top of the series, and see if it makes the warning go away?
That patch not only helps, it also fixes the crash seen with openrisc. For that crash, I was able to collect some useful data; see the log below.
Thanks,
Guenter
---
WARNING: CPU: 0 PID: 7 at lib/debugobjects.c:502 debug_print_object+0xc0/0xe8 ODEBUG: free active (active state 0) object type: timer_list hint: rcu_lock_map+0x0/0x14 Modules linked in: CPU: 0 PID: 7 Comm: ksoftirqd/0 Not tainted 6.1.0-rc2-00145-g2c4e85e9ac93 #1 Call trace: [<048ecc8e>] dump_stack_lvl+0x44/0x80 [<c6a7029c>] dump_stack+0x1c/0x2c [<b225e4eb>] __warn+0xdc/0x118 [<1070b766>] ? debug_print_object+0xc0/0xe8 [<57923a76>] warn_slowpath_fmt+0x78/0x90 [<1070b766>] debug_print_object+0xc0/0xe8 [<b3abbcb0>] __debug_check_no_obj_freed+0x230/0x2b8 [<508d9b5a>] ? delayed_put_task_struct+0x0/0x84 [<30f5a2a0>] ? _s_kernel_ro+0x0/0x200 [<403ab082>] debug_check_no_obj_freed+0x30/0x40 [<82702c56>] free_pcp_prepare+0xc4/0x2b0 [<508d9b5a>] ? delayed_put_task_struct+0x0/0x84 [<7798b190>] free_unref_page+0x44/0x210 [<d73717e5>] __free_pages+0x108/0x124 [<a32de4eb>] slob_free_pages+0x9c/0xac [<bd51c171>] slob_free+0x40c/0x62c [<a2d26e0e>] ? thread_stack_free_rcu+0x0/0x44 [<24b2df6c>] ? rcu_process_callbacks+0x114/0x224 [<24b2df6c>] ? rcu_process_callbacks+0x114/0x224 [<7794ec75>] ? rcu_process_callbacks+0xdc/0x224 [<7794ec75>] ? rcu_process_callbacks+0xdc/0x224 [<d76fe88f>] kmem_cache_free+0x64/0xa0 [<46d25dac>] free_task+0x7c/0xe0 [<2df25813>] __put_task_struct+0xe8/0x194 [<64f9675b>] delayed_put_task_struct+0x58/0x84 [<8755437e>] rcu_process_callbacks+0xf0/0x224 [<24b2df6c>] ? rcu_process_callbacks+0x114/0x224 [<020db442>] ? rcu_process_callbacks+0x178/0x224 [<87626af4>] __do_softirq+0x11c/0x2f8 [<c3f89a50>] ? smpboot_thread_fn+0x4c/0x304 [<c3f89a50>] ? smpboot_thread_fn+0x4c/0x304 [<021b0175>] ? smpboot_thread_fn+0x188/0x304 [<f2e79ebd>] ? smpboot_thread_fn+0x158/0x304 [<966be0e6>] run_ksoftirqd+0x4c/0x80 [<4bf65f60>] smpboot_thread_fn+0x180/0x304 [<3f914d93>] ? _raw_spin_unlock_irqrestore+0x50/0x84 [<bef37779>] ? __kthread_parkme+0x60/0xdc [<b0798e10>] ? smpboot_thread_fn+0x0/0x304 [<c463cd92>] kthread+0x11c/0x144 [<3eaef0b7>] ? kthread+0x0/0x144 [<ef2f6228>] ret_from_fork+0x1c/0x84 ---[ end trace 0000000000000000 ]--- Unable to handle kernel access at virtual address 0xbd6ed6a4 Oops#: 0000
CPU #: 0
PC: c0056c78 SR: 00008679 SP: c1027c24
GPR00: 00000000 GPR01: c1027c24 GPR02: c1027c78 GPR03: 00008279
GPR04: 00000000 GPR05: 00000000 GPR06: 00000000 GPR07: 00000001
GPR08: 00000000 GPR09: c0056c64 GPR10: c1026000 GPR11: 00000000
GPR12: 00000000 GPR13: 00000001 GPR14: c05c0000 GPR15: 00000000
GPR16: 00000001 GPR17: bd6ed6a4 GPR18: ff4517b0 GPR19: fd145f00
GPR20: 00000000 GPR21: 00000000 GPR22: 00000000 GPR23: c0760000
GPR24: c10232a0 GPR25: 00000003 GPR26: 00000000 GPR27: 00000000
GPR28: c1a00458 GPR29: 00000000 GPR30: c0790000 GPR31: 00000000
RES: 00000000 oGPR11: ffffffff
Process ksoftirqd/0 (pid: 7, stackpage=c10232a0)
Stack: Call trace: [<6ce5cfad>] __lock_acquire.constprop.0+0xa8/0x914 [<4bc14e12>] ? __del_timer_sync+0x0/0x128 [<da915c87>] lock_acquire.part.0.isra.0+0xd4/0x1ac [<4bc14e12>] ? __del_timer_sync+0x0/0x128 [<9b341df3>] lock_acquire+0x2c/0x44 [<233b5cbc>] __del_timer_sync+0x64/0x128 [<4bc14e12>] ? __del_timer_sync+0x0/0x128 [<05cd2741>] timer_fixup_free+0x34/0x5c [<3fa496ad>] __debug_check_no_obj_freed+0x250/0x2b8 [<508d9b5a>] ? delayed_put_task_struct+0x0/0x84 [<30f5a2a0>] ? _s_kernel_ro+0x0/0x200 [<403ab082>] debug_check_no_obj_freed+0x30/0x40 [<82702c56>] free_pcp_prepare+0xc4/0x2b0 [<508d9b5a>] ? delayed_put_task_struct+0x0/0x84 [<7798b190>] free_unref_page+0x44/0x210 [<d73717e5>] __free_pages+0x108/0x124 [<a32de4eb>] slob_free_pages+0x9c/0xac [<bd51c171>] slob_free+0x40c/0x62c [<a2d26e0e>] ? thread_stack_free_rcu+0x0/0x44 [<24b2df6c>] ? rcu_process_callbacks+0x114/0x224 [<24b2df6c>] ? rcu_process_callbacks+0x114/0x224 [<7794ec75>] ? rcu_process_callbacks+0xdc/0x224 [<7794ec75>] ? rcu_process_callbacks+0xdc/0x224 [<d76fe88f>] kmem_cache_free+0x64/0xa0 [<46d25dac>] free_task+0x7c/0xe0 [<2df25813>] __put_task_struct+0xe8/0x194 [<64f9675b>] delayed_put_task_struct+0x58/0x84 [<8755437e>] rcu_process_callbacks+0xf0/0x224 [<24b2df6c>] ? rcu_process_callbacks+0x114/0x224 [<020db442>] ? rcu_process_callbacks+0x178/0x224 [<87626af4>] __do_softirq+0x11c/0x2f8 [<c3f89a50>] ? smpboot_thread_fn+0x4c/0x304 [<c3f89a50>] ? smpboot_thread_fn+0x4c/0x304 [<021b0175>] ? smpboot_thread_fn+0x188/0x304 [<f2e79ebd>] ? smpboot_thread_fn+0x158/0x304 [<966be0e6>] run_ksoftirqd+0x4c/0x80 [<4bf65f60>] smpboot_thread_fn+0x180/0x304 [<3f914d93>] ? _raw_spin_unlock_irqrestore+0x50/0x84 [<bef37779>] ? __kthread_parkme+0x60/0xdc [<b0798e10>] ? smpboot_thread_fn+0x0/0x304 [<c463cd92>] kthread+0x11c/0x144 [<3eaef0b7>] ? kthread+0x0/0x144 [<ef2f6228>] ret_from_fork+0x1c/0x84
Steven Rostedt
reviewerRFC PATCH v2 20/31 timers: usb: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- 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 +- 4 files changed, 6 insertions(+), 3 deletions(-)
drivers/usb/core/hub.c +3 -0
@@ -1261,6 +1261,9 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)/* Don't do a long sleep inside a workqueue routine */ if (type ==HUB_INIT2 ) { +/* Timers must be shutdown before they are re-initialized */ +if (hub->init_work.work.func) +del_timer_shutdown (&hub->init_work.timer);INIT_DELAYED_WORK (&hub->init_work, hub_init_func3); queue_delayed_work(system_power_efficient_wq, &hub->init_work,
drivers/usb/gadget/udc/m66592-udc.c +1 -1
@@ -1519,7 +1519,7 @@ static int m66592_remove(struct platform_device *pdev)usb_del_gadget_udc (&m66592->gadget); -del_timer_sync (&m66592->timer); +del_timer_shutdown (&m66592->timer);iounmap (m66592->reg);free_irq (platform_get_irq (pdev,0 ), m66592);m66592_free_request (&m66592->ep[0 ].ep, m66592->ep0_req);
drivers/usb/serial/garmin_gps.c +1 -1
@@ -1405,7 +1405,7 @@ static void garmin_port_remove(struct usb_serial_port *port)usb_kill_anchored_urbs (&garmin_data_p->write_urbs);usb_kill_urb (port->interrupt_in_urb); -del_timer_sync (&garmin_data_p->timer); +del_timer_shutdown (&garmin_data_p->timer);kfree (garmin_data_p); }
drivers/usb/serial/mos7840.c +1 -1
@@ -1726,7 +1726,7 @@ static void mos7840_port_remove(struct usb_serial_port *port)mos7840_set_led_sync (port,MODEM_CONTROL_REGISTER ,0x0300 );del_timer_sync (&mos7840_port->led_timer1); -del_timer_sync (&mos7840_port->led_timer2); +del_timer_shutdown (&mos7840_port->led_timer2);usb_kill_urb (mos7840_port->led_urb);usb_free_urb (mos7840_port->led_urb);
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 3 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.
Is this supposed to be true for all timers? Because the USB subsystem contains an awful lot more timers than just the two you touched in this patch.
Alan Stern
Quoted 48 lines
Cc Greg Kroah-Hartman <[email protected]>Cc Felipe Balbi <[email protected]>Cc Johan Hovold <[email protected]>Cc Alan Stern <[email protected]>Cc Mathias Nyman <[email protected]>Cc Kai-Heng Feng <[email protected]>Cc Matthias Kaehlcke <[email protected]>Cc Michael Grzeschik <[email protected]>Cc Bhuvanesh Surachari <[email protected]>Cc Dan Carpenter <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>--- 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 +- 4 files changed, 6 insertions(+), 3 deletions(-)drivers/usb/core/hub.c +3
-0@@ -1261,6 +1261,9 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)/* Don't do a long sleep inside a workqueue routine */ if (type ==HUB_INIT2 ) { +/* Timers must be shutdown before they are re-initialized */ +if (hub->init_work.work.func) +del_timer_shutdown (&hub->init_work.timer);INIT_DELAYED_WORK (&hub->init_work, hub_init_func3); queue_delayed_work(system_power_efficient_wq, &hub->init_work,drivers/usb/gadget/udc/m66592-udc.c +1
-1@@ -1519,7 +1519,7 @@ static int m66592_remove(struct platform_device *pdev)usb_del_gadget_udc (&m66592->gadget); -del_timer_sync (&m66592->timer); +del_timer_shutdown (&m66592->timer);iounmap (m66592->reg);free_irq (platform_get_irq (pdev,0 ), m66592);m66592_free_request (&m66592->ep[0 ].ep, m66592->ep0_req);
Steven Rostedt
reviewerRe: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 8 lines · On Thu, 27 Oct 2022 16:38:19 -0400 Alan Stern <[email protected]> wrote:
Quoted 3 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.Is this supposed to be true for all timers? Because the USB subsystem contains an awful lot more timers than just the two you touched in this patch.
Yes, and this does mean that we are going to have to painstakingly find and fix ever one of them. This is why the last patch updates DEBUG_OBJECTS_TIMERS to detect cases where I miss.
-- Steve
Steven Rostedt
reviewerRe: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 8 lines · On Thu, 27 Oct 2022 16:42:27 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 5 lines · On Thu, 27 Oct 2022 16:38:19 -0400 Alan Stern <[email protected]> wrote:
Quoted 3 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.
Quoted 7 lines
Quoted 3 lines
Is this supposed to be true for all timers? Because the USB subsystem contains an awful lot more timers than just the two you touched in this patch.
Yes, and this does mean that we are going to have to painstakingly find and fix ever one of them. This is why the last patch updates
DEBUG_OBJECTS_TIMERSto detect cases where I miss.
BTW, as del_timer_shutdown() prevents the timer from being re-armed, there are lots of timers in the kernel where I did not touch, because I could not tell if the del_timer_sync() or the buggy del_timer() calls were for it to be freed, or for some other legitimate reason, and I just stayed well enough alone.
-- Steve
Re: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtQuoted 30 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.Cc Greg Kroah-Hartman <[email protected]>Cc Felipe Balbi <[email protected]>Cc Johan Hovold <[email protected]>Cc Alan Stern <[email protected]>Cc Mathias Nyman <[email protected]>Cc Kai-Heng Feng <[email protected]>Cc Matthias Kaehlcke <[email protected]>Cc Michael Grzeschik <[email protected]>Cc Bhuvanesh Surachari <[email protected]>Cc Dan Carpenter <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>--- 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 +- 4 files changed, 6 insertions(+), 3 deletions(-)drivers/usb/core/hub.c +0
-0
drivers/usb/core/hub.c static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) · line 1267
Quoted 7 lines
drivers/usb/core/hub.c +3
-0@@ -1261,6 +1261,9 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)/* Don't do a long sleep inside a workqueue routine */ if (type ==HUB_INIT2 ) { +/* Timers must be shutdown before they are re-initialized */ +if (hub->init_work.work.func) +del_timer_shutdown (&hub->init_work.timer);INIT_DELAYED_WORK (&hub->init_work, hub_init_func3);
A similar call to INIT_DELAYED_WORK() around line 1085 needs the same change.
It would be great if that can somehow be hidden in INIT_DELAYED_WORK().
Thanks,
Guenter
Quoted 41 lines
drivers/usb/core/hub.c +0
-0queue_delayed_work(system_power_efficient_wq, &hub->init_work,drivers/usb/gadget/udc/m66592-udc.c +1
-1@@ -1519,7 +1519,7 @@ static int m66592_remove(struct platform_device *pdev)usb_del_gadget_udc (&m66592->gadget); -del_timer_sync (&m66592->timer); +del_timer_shutdown (&m66592->timer);iounmap (m66592->reg);free_irq (platform_get_irq (pdev,0 ), m66592);m66592_free_request (&m66592->ep[0 ].ep, m66592->ep0_req);drivers/usb/serial/garmin_gps.c +1
-1@@ -1405,7 +1405,7 @@ static void garmin_port_remove(struct usb_serial_port *port)usb_kill_anchored_urbs (&garmin_data_p->write_urbs);usb_kill_urb (port->interrupt_in_urb); -del_timer_sync (&garmin_data_p->timer); +del_timer_shutdown (&garmin_data_p->timer);kfree (garmin_data_p); }drivers/usb/serial/mos7840.c +1
-1@@ -1726,7 +1726,7 @@ static void mos7840_port_remove(struct usb_serial_port *port)mos7840_set_led_sync (port,MODEM_CONTROL_REGISTER ,0x0300 );del_timer_sync (&mos7840_port->led_timer1); -del_timer_sync (&mos7840_port->led_timer2); +del_timer_shutdown (&mos7840_port->led_timer2);usb_kill_urb (mos7840_port->led_urb);usb_free_urb (mos7840_port->led_urb);
Steven Rostedt
reviewerRe: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 15 lines · On Thu, 27 Oct 2022 22:23:06 -0700 Guenter Roeck <[email protected]> wrote:
drivers/usb/core/hub.c static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) · line 1267
Quoted 12 lines
drivers/usb/core/hub.c +3
-0@@ -1261,6 +1261,9 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)/* Don't do a long sleep inside a workqueue routine */ if (type ==HUB_INIT2 ) { +/* Timers must be shutdown before they are re-initialized */ +if (hub->init_work.work.func) +del_timer_shutdown (&hub->init_work.timer);INIT_DELAYED_WORK (&hub->init_work, hub_init_func3);A similar call to
INIT_DELAYED_WORK()around line 1085 needs the same change.
Quoted 1 line
It would be great if that can somehow be hidden in
INIT_DELAYED_WORK().
I agree, but the delayed work is such a special case, I'm struggling to find something that works sensibly. :-/
-- Steve
Steven Rostedt
reviewerRe: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 10 lines · On Fri, 28 Oct 2022 06:14:22 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 3 lines · On Thu, 27 Oct 2022 22:23:06 -0700 Guenter Roeck <[email protected]> wrote:
A similar call to
INIT_DELAYED_WORK()around line 1085 needs the same change.It would be great if that can somehow be hidden in
INIT_DELAYED_WORK().I agree, but the delayed work is such a special case, I'm struggling to find something that works sensibly. :-/
OK, I diagnosed the issue here. The problem is that delayed work also has no "shutdown" method when it's done. Which means there's no generic way to call the work->timer shutdown method. So we have two options to handle delayed work timers:
1) Add special initialization for delayed work so that it can just go back to the old checking (activating on arming, deactivating by any del_timer*).
2) Implement a shutdown state for the work queues as well. There could definitely be the same types of bugs as with timers, where a delayed work could be pending on something that's been freed. That's probably why there's a DEBUG_OBJECTS_WORK too.
Ideally, I would like to have #2, but realistically, I'm going for #1 for now. We could always add the work queue shutdown state later if we want.
The problem with timers with respect to delayed work queues, is that there's no place to add the "shutdown" before its no longer in use. Worse yet, there's code that caches descriptors that have delayed work instead of freeing them. (See block/blk-mq.c and drivers/scsi/scsi_lib.c with the queuelist). Where it just calls del_timer(), and then sends it back to the free store for reuse later. Perhaps we should add DEBUG_OBJECTS checking to these too?
Anyway, I'll make it where the INIT_DELAYED_WORK will call __timer_init_work() that will set the debug state in the timer to TIMER_DEBUG_WORK, were it will activate and deactivate the debug object on add_timer() and del_timer() and hope that it's not one of the bugs we are hitting :-/
-- Steve
Steven Rostedt
reviewerRe: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
In reply to Guenter RoeckQuoted 14 lines · On Thu, 27 Oct 2022 22:23:06 -0700 Guenter Roeck <[email protected]> wrote:
drivers/usb/core/hub.c static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) · line 1267
Quoted 11 lines
index
bbab424b0d55..397f263ab7da100644drivers/usb/core/hub.c +3
-0@@ -1261,6 +1261,9 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)/* Don't do a long sleep inside a workqueue routine */ if (type ==HUB_INIT2 ) { +/* Timers must be shutdown before they are re-initialized */ +if (hub->init_work.work.func) +del_timer_shutdown (&hub->init_work.timer);INIT_DELAYED_WORK (&hub->init_work, hub_init_func3);A similar call to
INIT_DELAYED_WORK()around line 1085 needs the same change.
Quoted 1 line
It would be great if that can somehow be hidden in
INIT_DELAYED_WORK().
I've decided to treat INIT_DELAYED_WORK() like it was before. It only checks from the time the timer is added to the time it is removed without needing a shutdown call. That's because there's no API in the workqueue code that allows for us to require a shutdown on the INIT_DELAYED_WORK's timer.
Guenter,
Can you remove all the extra patches that touched the timer.h and timer.c code, and replace the last patch with this, and then try again?
-- Steve
include/linux/timer.h | 38 +++++++++++++++++++++++++++-- include/linux/workqueue.h | 4 ++-- kernel/time/timer.c | 50 ++++++++++++++++++++++++++++++++++----- kernel/workqueue.c | 12 ++++++++++ 4 files changed, 94 insertions(+), 10 deletions(-)
include/linux/timer.h +36 -2
@@ -8,6 +8,12 @@#include <linux/debugobjects.h> #include <linux/stringify.h> +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))
@@ -243,7 +277,7 @@ 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) +#define del_singleshot_timer_sync (t) del_timer_shutdown(t)extern void init_timers (void );struct hrtimer ;
include/linux/workqueue.h +2 -2
@@ -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)
kernel/time/timer.c +44 -6
104 lines not on this page · raw message
kernel/workqueue.c +12 -0
@@ -1225,6 +1225,16 @@ static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, unsigned long work_put_pwq (pwq); } +static void deactivate_timer (struct work_struct *work,bool is_dwork) +{ +struct delayed_work *dwork; + +if (!is_dwork) +return ; + + dwork =to_delayed_work (work); +} +/** * try_to_grab_pending - steal work item from worklist and disable irq * @work: work item to steal
@@ -3148,6 +3158,8 @@ static bool __cancel_work_timer(struct work_struct *work, bool is_dwork) } }while (unlikely (ret <0 )); +deactivate_timer (work, is_dwork); +/* tell other tasks trying to grab @work to back off */ mark_work_canceling (work);local_irq_restore (flags);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRe: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 5 lines · On Fri, 28 Oct 2022 14:01:29 -0400 Steven Rostedt <[email protected]> wrote:
(quoted diff) +1
-0@@ -813,6 +839,14 @@ void destroy_timer_on_stack(struct timer_list *timer) } EXPORT_SYMBOL_GPL(destroy_timer_on_stack); +static struct timer_base *lock_timer_base(struct timer_list *timer,
Review void destroy_timer_on_stack(struct timer_list *timer) · line 852
Quoted 10 lines
(quoted diff) +7
-0+ unsigned long *flags); + +void __timer_reinit_debug_objects(struct timer_list *timer) +{ + return; +} + #else static inline void debug_timer_init(struct timer_list *timer) { } static inline void debug_timer_activate(struct timer_list *timer) { }
Bah, the above chunk was leftover from some debugging.
Updated patch:
-- Steve
include/linux/timer.h | 38 +++++++++++++++++++++++++++++++++-- include/linux/workqueue.h | 4 ++-- kernel/time/timer.c | 42 +++++++++++++++++++++++++++++++++------ kernel/workqueue.c | 12 +++++++++++ 4 files changed, 86 insertions(+), 10 deletions(-)
include/linux/timer.h +36 -2
@@ -8,6 +8,12 @@#include <linux/debugobjects.h> #include <linux/stringify.h> +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))
@@ -243,7 +277,7 @@ 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) +#define del_singleshot_timer_sync (t) del_timer_shutdown(t)extern void init_timers (void );struct hrtimer ;
include/linux/workqueue.h +2 -2
@@ -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)
kernel/time/timer.c +36 -6
90 lines not on this page · raw message
kernel/workqueue.c +12 -0
@@ -1225,6 +1225,16 @@ static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, unsigned long work_put_pwq (pwq); } +static void deactivate_timer (struct work_struct *work,bool is_dwork) +{ +struct delayed_work *dwork; + +if (!is_dwork) +return ; + + dwork =to_delayed_work (work); +} +/** * try_to_grab_pending - steal work item from worklist and disable irq * @work: work item to steal
@@ -3148,6 +3158,8 @@ static bool __cancel_work_timer(struct work_struct *work, bool is_dwork) } }while (unlikely (ret <0 )); +deactivate_timer (work, is_dwork); +/* tell other tasks trying to grab @work to back off */ mark_work_canceling (work);local_irq_restore (flags);
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 7 lines · Steven Rostedt
Quoted 4 lines · On Fri, 28 Oct 2022 14:01:29 -0400 Steven Rostedt <[email protected]> wrote:
(quoted diff) +0
-0@@ -813,6 +839,14 @@ void destroy_timer_on_stack(struct timer_list *timer) } EXPORT_SYMBOL_GPL(destroy_timer_on_stack);
Quoted 14 lines
Review void destroy_timer_on_stack(struct timer_list *timer) · line 852
Quoted 11 lines
(quoted diff) +8
-0+static struct timer_base *lock_timer_base(struct timer_list *timer, + unsigned long *flags); + +void __timer_reinit_debug_objects(struct timer_list *timer) +{ + return; +} + #else static inline void debug_timer_init(struct timer_list *timer) { } static inline void debug_timer_activate(struct timer_list *timer) { }Bah, the above chunk was leftover from some debugging.
I'll test again with the following changes on top of your published patch series. I hope this is the current status, but I may have lost something.
Looking into it ... deactivate_timer() doesn't do anything and seems wrong. Did I miss something ?
Thanks,
Guenter
---
block/blk-core.c +1 -1
@@ -227,7 +227,7 @@ const char *blk_status_to_str(blk_status_t status)*/ void blk_sync_queue (struct request_queue *q) { -del_timer_sync (&q->timeout); +del_timer_shutdown (&q->timeout);cancel_work_sync (&q->timeout_work); }EXPORT_SYMBOL (blk_sync_queue);
block/blk-sysfs.c +2 -0
@@ -769,6 +769,8 @@ static void blk_release_queue(struct kobject *kobj)percpu_ref_exit (&q->q_usage_counter); +blk_sync_queue (q); +if (q->poll_stat)blk_stat_remove_callback (q, q->poll_cb);blk_stat_free_callback (q->poll_cb);
drivers/net/ethernet/dec/tulip/tulip_core.c +4 -2
@@ -770,8 +770,6 @@ static void tulip_down (struct net_device *dev)spin_unlock_irqrestore (&tp->lock, flags); -timer_setup (&tp->timer, tulip_tbl[tp->chip_id].media_timer,0 ); - dev->if_port = tp->saved_if_port;/* Leave the driver in snooze, not sleep, mode. */
@@ -1869,10 +1867,14 @@ static int __maybe_unused tulip_resume(struct device *dev_d)static void tulip_remove_one (struct pci_dev *pdev) {struct net_device *dev =pci_get_drvdata (pdev); +struct tulip_private *tp;if (!dev)return ; + tp =netdev_priv (dev); +del_timer_shutdown (&tp->timer); +unregister_netdev (dev); }
drivers/parport/ieee1284.c +1 -1
@@ -73,7 +73,7 @@ int parport_wait_event (struct parport *port, signed long timeout)timer_setup (&port->timer, timeout_waiting_on_port,0 );mod_timer (&port->timer, jiffies + timeout); ret =down_interruptible (&port->physport->ieee1284.irq); -if (!del_timer_sync (&port->timer) && !ret) +if (!del_timer_shutdown (&port->timer) && !ret)/* Timed out. */ ret =1 ;
drivers/scsi/ibmvscsi/ibmvscsi.c +3 -3
@@ -789,7 +789,7 @@ static void purge_requests(struct ibmvscsi_host_data *hostdata, int error_code)while (!list_empty (&hostdata->sent)) { evt = list_first_entry(&hostdata->sent, struct srp_event_struct, list);list_del (&evt->list); -del_timer (&evt->timer); +del_timer_try_shutdown (&evt->timer);spin_unlock_irqrestore (hostdata->host->host_lock, flags);if (evt->cmnd) {
@@ -944,7 +944,7 @@ static int ibmvscsi_send_srp_event(struct srp_event_struct *evt_struct, be64_to_cpu(crq_as_u64[1]));if (rc !=0 ) {list_del (&evt_struct->list); -del_timer (&evt_struct->timer); +del_timer_shutdown (&evt_struct->timer);/* If send_crq returns H_CLOSED, return SCSI_MLQUEUE_HOST_BUSY. * Firmware will send a CRQ with a transport event (0xFF) to
@@ -1840,7 +1840,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,atomic_add (be32_to_cpu (evt_struct->xfer_iu->srp.rsp.req_lim_delta), &hostdata->request_limit); -del_timer (&evt_struct->timer); +del_timer_shutdown (&evt_struct->timer);if ((crq->status !=VIOSRP_OK && crq->status !=VIOSRP_OK2 ) && evt_struct->cmnd) evt_struct->cmnd->result =DID_ERROR <<16 ;
drivers/usb/core/hub.c +3 -0
@@ -1082,6 +1082,9 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) delay =hub_power_on_good_delay (hub);hub_power_on (hub, false); +/* Timers must be shutdown before they are re-initialized */ +if (hub->init_work.work.func) +del_timer_shutdown (&hub->init_work.timer);INIT_DELAYED_WORK (&hub->init_work, hub_init_func2); queue_delayed_work(system_power_efficient_wq, &hub->init_work,
include/linux/timer.h +34 -5
@@ -8,6 +8,12 @@#include <linux/debugobjects.h> #include <linux/stringify.h> +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
@@ -19,7 +25,7 @@ struct timer_list {u32 flags;#ifdef CONFIG_DEBUG_OBJECTS_TIMERS -u32 enabled; +enum timer_debug_state enabled;#endif #ifdef CONFIG_LOCKDEP struct lockdep_map lockdep_map;
@@ -134,14 +140,26 @@ static inline void init_timer_on_stack_key(struct timer_list *timer,#ifdef CONFIG_DEBUG_OBJECTS_TIMERS #define __init_timer_debug (_timer, _fn, _flags) \ do { \ - (_timer)->enabled = 0; \ + (_timer)->enabled = TIMER_DEBUG_DISABLED; \ __init_timer((_timer), (_fn), (_flags)); \ } while (0) -#else -#define __init_timer_debug (_timer, _fn, _flags) \ +#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 /**
@@ -184,12 +202,23 @@ 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); +static inline int del_timer_try_shutdown (struct timer_list *timer) +{ +return __del_timer (timer, true); +} + +static inline int del_timer (struct timer_list *timer) +{ +return __del_timer (timer, false); +} +/* * The jiffies value which is added to now, when there is no timer * in the timer wheel:
include/linux/workqueue.h +2 -2
@@ -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)
kernel/time/timer.c +40 -14
109 lines not on this page · raw message
kernel/workqueue.c +12 -0
@@ -1225,6 +1225,16 @@ static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, unsigned long work_put_pwq (pwq); } +static void deactivate_timer (struct work_struct *work,bool is_dwork) +{ +struct delayed_work *dwork; + +if (!is_dwork) +return ; + + dwork =to_delayed_work (work); +} +/** * try_to_grab_pending - steal work item from worklist and disable irq * @work: work item to steal
@@ -3148,6 +3158,8 @@ static bool __cancel_work_timer(struct work_struct *work, bool is_dwork) } }while (unlikely (ret <0 )); +deactivate_timer (work, is_dwork); +/* tell other tasks trying to grab @work to back off */ mark_work_canceling (work);local_irq_restore (flags);
net/core/sock.c +1 -1
@@ -3345,7 +3345,7 @@ EXPORT_SYMBOL(sk_reset_timer);void sk_stop_timer (struct sock *sk,struct timer_list * timer) { -if (del_timer (timer)) +if (del_timer_try_shutdown (timer))__sock_put (sk); }EXPORT_SYMBOL (sk_stop_timer);
Steven Rostedt
reviewerRe: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 7 lines · On Fri, 28 Oct 2022 12:59:59 -0700 Guenter Roeck <[email protected]> wrote:
I'll test again with the following changes on top of your published patch series. I hope this is the current status, but I may have lost something.
Looking into it ...
deactivate_timer()doesn't do anything and seems wrong. Did I miss something ?
You mean debug_deactivate_timer() or debug_deactivate?
kernel/time/timer.c static const struct debug_obj_descr timer_debug_descr = { · line 803
Quoted 12 lines
kernel/time/timer.c +3
-3-static inline void debug_timer_deactivate (struct timer_list *timer) +static inline void debug_timer_deactivate (struct timer_list *timer,bool free) { -if (timer->enabled) -debug_object_deactivate (timer, &timer_debug_descr); +switch (timer->enabled) { +case TIMER_DEBUG_DISABLED :
DISABLE is set before an activate happens (before it is ever armed).
kernel/time/timer.c static const struct debug_obj_descr timer_debug_descr = { · line 807
Quoted 4 lines
kernel/time/timer.c +4
-0+return ; +case TIMER_DEBUG_ENABLED : +if (!free) +return ;
This is called by del_timer{,_sync}() where free is false, or del_timer_shutdown() where free is true.
We only want to deactivate when free is true.
kernel/time/timer.c
Quoted 1 line
kernel/time/timer.c +1
-0+ timer->enabled =TIMER_DEBUG_DISABLED ;
And we allow for initialization of a "freed" timer again.
kernel/time/timer.c static const struct debug_obj_descr timer_debug_descr = { · line 810
Quoted 2 lines
kernel/time/timer.c +2
-0+break ; +case TIMER_DEBUG_WORK :
This is part of the delayed_work timers, were we keep the old behavior (del_timer() and del_timer_sync() both deactivate the timer.
kernel/time/timer.c static const struct debug_obj_descr timer_debug_descr = { · line 813
Quoted 3 lines
kernel/time/timer.c +3
-0+break ; + } +debug_object_deactivate (timer, &timer_debug_descr);
Here we call the debug object code to deactivate it.
kernel/time/timer.c static inline void debug_init(struct timer_list *timer) · line 857
Quoted 8 lines
}
static inline void
debug_timer_assert_init(struct timer_list *timer)kernel/time/timer.c +1
-0@@ -833,6 +854,7 @@ static inline void debug_init(struct timer_list *timer)static inline void debug_deactivate (struct timer_list *timer) { +debug_timer_deactivate (timer, false);
This calls the above code.
kernel/time/timer.c static inline void debug_init(struct timer_list *timer) · line 859
Quoted 2 lines
kernel/time/timer.c +0
-0trace_timer_cancel (timer); }
Or am I confused and you meant something else?
-- Steve
Re: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 12 lines · Steven Rostedt
Quoted 7 lines · On Fri, 28 Oct 2022 12:59:59 -0700 Guenter Roeck <[email protected]> wrote:
I'll test again with the following changes on top of your published patch series. I hope this is the current status, but I may have lost something.
Looking into it ...
deactivate_timer()doesn't do anything and seems wrong. Did I miss something ?You mean
debug_deactivate_timer()ordebug_deactivate?
This:
+static void deactivate_timer(struct work_struct *work, bool is_dwork) +{
+ struct delayed_work *dwork;
+
+ if (!is_dwork)
+ return;
+
+ dwork = to_delayed_work(work);
+}
Guenter
Steven Rostedt
reviewerRe: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 16 lines · On Fri, 28 Oct 2022 16:25:32 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 12 lines · Steven Rostedt
Quoted 7 lines · On Fri, 28 Oct 2022 12:59:59 -0700 Guenter Roeck <[email protected]> wrote:
I'll test again with the following changes on top of your published patch series. I hope this is the current status, but I may have lost something.
Looking into it ...
deactivate_timer()doesn't do anything and seems wrong. Did I miss something ?You mean
debug_deactivate_timer()ordebug_deactivate?This:
kernel/workqueue.c static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, unsigned long work_ · line 1236
Quoted 9 lines
kernel/workqueue.c +9
-0+static void deactivate_timer (struct work_struct *work,bool is_dwork) +{ +struct delayed_work *dwork; + +if (!is_dwork) +return ; + + dwork =to_delayed_work (work); +}
Oh, that was part of my trying to figure out WTF delayed work was doing with its timers. You can delete it's existence.
Thanks (and I'll go remove it from my tree).
-- Steve
Re: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
In reply to Guenter RoeckQuoted 23 lines · Guenter Roeck
Quoted 7 lines · Steven Rostedt
Quoted 4 lines · On Fri, 28 Oct 2022 14:01:29 -0400 Steven Rostedt <[email protected]> wrote:
(quoted diff) +0
-0@@ -813,6 +839,14 @@ void destroy_timer_on_stack(struct timer_list *timer) } EXPORT_SYMBOL_GPL(destroy_timer_on_stack);Quoted 14 lines
Review void destroy_timer_on_stack(struct timer_list *timer) · line 852
Quoted 11 lines
(quoted diff) +8
-0+static struct timer_base *lock_timer_base(struct timer_list *timer, + unsigned long *flags); + +void __timer_reinit_debug_objects(struct timer_list *timer) +{ + return; +} + #else static inline void debug_timer_init(struct timer_list *timer) { } static inline void debug_timer_activate(struct timer_list *timer) { }Bah, the above chunk was leftover from some debugging.
Quoted 4 lines
I'll test again with the following changes on top of your published patch series. I hope this is the current status, but I may have lost something.
With the diffs I sent earlier applied, the warning still seen is
WARNING: CPU: 0 PID: 9 at lib/debugobjects.c:502 debug_print_object+0xd0/0x100 ODEBUG: free active (active state 0) object type: timer_list hint: neigh_timer_handler+0x0/0x480
That happens with almost every test, so I may have missed some others in the noise.
Guenter
Steven Rostedt
reviewerRe: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 7 lines · On Sat, 29 Oct 2022 07:52:41 -0700 Guenter Roeck <[email protected]> wrote:
With the diffs I sent earlier applied, the warning still seen is
WARNING: CPU: 0 PID: 9 at lib/debugobjects.c:502 debug_print_object+0xd0/0x100 ODEBUG: free active (active state 0) object type: timer_list hint: neigh_timer_handler+0x0/0x480That happens with almost every test, so I may have missed some others in the noise.
Can you add this?
-- Steve
net/core/neighbour.c +2 -0
@@ -895,6 +895,8 @@ void neigh_destroy(struct neighbour *neigh)if (neigh_del_timer (neigh))pr_warn ("Impossible event\n" ); +del_timer_try_shutdown (&neigh->timer); +write_lock_bh (&neigh->lock);__skb_queue_purge (&neigh->arp_queue);write_unlock_bh (&neigh->lock);
Re: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 5 lines · Steven Rostedt
Quoted 2 lines · On Sat, 29 Oct 2022 07:52:41 -0700 Guenter Roeck <[email protected]> wrote:
With the diffs I sent earlier applied, the warning still seen is
Quoted 8 lines
Quoted 5 lines
WARNING: CPU: 0 PID: 9 at lib/debugobjects.c:502 debug_print_object+0xd0/0x100 ODEBUG: free active (active state 0) object type: timer_list hint: neigh_timer_handler+0x0/0x480That happens with almost every test, so I may have missed some others in the noise.
Can you add this?
It doesn't make a difference.
Guenter
Quoted 15 lines
-- Steve
net/core/neighbour.c +2
-0@@ -895,6 +895,8 @@ void neigh_destroy(struct neighbour *neigh)if (neigh_del_timer (neigh))pr_warn ("Impossible event\n" ); +del_timer_try_shutdown (&neigh->timer); +write_lock_bh (&neigh->lock);__skb_queue_purge (&neigh->arp_queue);write_unlock_bh (&neigh->lock);
Steven Rostedt
reviewerRe: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 10 lines · On Sat, 29 Oct 2022 15:56:25 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 8 lines
Quoted 5 lines
WARNING: CPU: 0 PID: 9 at lib/debugobjects.c:502 debug_print_object+0xd0/0x100 ODEBUG: free active (active state 0) object type: timer_list hint: neigh_timer_handler+0x0/0x480That happens with almost every test, so I may have missed some others in the noise.
Can you add this?
It doesn't make a difference.
Ah, it also requires this (I have other debugging in that file, so it may only apply with some fuzzing):
-- Steve
kernel/time/timer.c +5 -0
@@ -1282,6 +1296,11 @@ int __del_timer(struct timer_list *timer, bool free)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); }return ret;
Re: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 9 lines · Steven Rostedt
Quoted 6 lines · On Sat, 29 Oct 2022 15:56:25 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 6 lines
Quoted 5 lines
WARNING: CPU: 0 PID: 9 at lib/debugobjects.c:502 debug_print_object+0xd0/0x100 ODEBUG: free active (active state 0) object type: timer_list hint: neigh_timer_handler+0x0/0x480That happens with almost every test, so I may have missed some others in the noise.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Can you add this?
It doesn't make a difference.
Ah, it also requires this (I have other debugging in that file, so it may only apply with some fuzzing):
Almost good, except for the attached backtrace. That seems to happen on shutdown after bootting from a usb drive, but not on all platforms.
The warning is in __mod_timer():
if (WARN_ON_ONCE(!timer->function))
return -EINVAL;This may be due to the change in blk_sync_queue() which I suspect may be called prior to the last mod_timer() call. I'll add some debug code to verify.
Guenter
------------[ cut here ]------------ WARNING: CPU: 0 PID: 283 at kernel/time/timer.c:1046 mod_timer+0x294/0x34c Modules linked in: CPU: 0 PID: 283 Comm: init Tainted: G N 6.1.0-rc2-00397-g18ccc9f8a778 #1 Hardware name: Freescale i.MX25 (Device Tree Support) unwind_backtrace from show_stack+0x10/0x18 show_stack from dump_stack_lvl+0x34/0x54 dump_stack_lvl from __warn+0xc0/0x1f0 __warn from warn_slowpath_fmt+0x5c/0xc4 warn_slowpath_fmt from mod_timer+0x294/0x34c mod_timer from blk_add_timer+0xa4/0xb4 blk_add_timer from blk_mq_start_request+0x84/0x1f4 blk_mq_start_request from scsi_queue_rq+0x4a8/0xb84 scsi_queue_rq from blk_mq_dispatch_rq_list+0x320/0x9d0 blk_mq_dispatch_rq_list from __blk_mq_sched_dispatch_requests+0xb0/0x158 __blk_mq_sched_dispatch_requests from blk_mq_sched_dispatch_requests+0x34/0x64 blk_mq_sched_dispatch_requests from __blk_mq_run_hw_queue+0x8c/0x234 __blk_mq_run_hw_queue from blk_mq_sched_insert_request+0xe8/0x15c blk_mq_sched_insert_request from blk_execute_rq+0xa4/0x1d0 blk_execute_rq from __scsi_execute+0xb4/0x19c __scsi_execute from sd_sync_cache+0xac/0x1ec sd_sync_cache from sd_shutdown+0x5c/0xc8 sd_shutdown from sd_remove+0x30/0x44 sd_remove from device_release_driver_internal+0xd0/0x16c device_release_driver_internal from bus_remove_device+0xd0/0x100 bus_remove_device from device_del+0x190/0x464 device_del from __scsi_remove_device+0x130/0x184 __scsi_remove_device from scsi_forget_host+0x60/0x64 scsi_forget_host from scsi_remove_host+0x6c/0x188 scsi_remove_host from usb_stor_disconnect+0x40/0xf4 usb_stor_disconnect from usb_unbind_interface+0x68/0x230 usb_unbind_interface from device_release_driver_internal+0xd0/0x16c device_release_driver_internal from bus_remove_device+0xd0/0x100 bus_remove_device from device_del+0x190/0x464 device_del from usb_disable_device+0x88/0x130 usb_disable_device from usb_disconnect+0xb4/0x234 usb_disconnect from usb_disconnect+0x9c/0x234 usb_disconnect from usb_remove_hcd+0xd0/0x16c usb_remove_hcd from host_stop+0x38/0xa8 host_stop from ci_hdrc_remove+0x40/0x11c ci_hdrc_remove from platform_remove+0x24/0x54 platform_remove from device_release_driver_internal+0xd0/0x16c device_release_driver_internal from bus_remove_device+0xd0/0x100 bus_remove_device from device_del+0x190/0x464 device_del from platform_device_del.part.0+0x10/0x78 platform_device_del.part.0 from platform_device_unregister+0x18/0x28 platform_device_unregister from ci_hdrc_remove_device+0xc/0x24 ci_hdrc_remove_device from ci_hdrc_imx_remove+0x28/0xfc ci_hdrc_imx_remove from device_shutdown+0x178/0x230 device_shutdown from kernel_restart_prepare+0x2c/0x3c kernel_restart_prepare from kernel_restart+0xc/0x68 kernel_restart from __do_sys_reboot+0xc0/0x204 __do_sys_reboot from ret_fast_syscall+0x0/0x1c Exception stack(0xc8ca1fa8 to 0xc8ca1ff0) 1fa0: 01234567 0000000f fee1dead 28121969 01234567 00000000 1fc0: 01234567 0000000f 00000001 00000058 000e05c0 00000000 00000000 00000000 1fe0: 000e0298 bea82de4 000994bc b6f6d2c0 irq event stamp: 3443 hardirqs last enabled at (3451): [<c0074590>] __up_console_sem+0x64/0x88 hardirqs last disabled at (3458): [<c007457c>] __up_console_sem+0x50/0x88 softirqs last enabled at (3438): [<c000988c>] __do_softirq+0x2fc/0x5d0 softirqs last disabled at (3433): [<c0022518>] __irq_exit_rcu+0x170/0x1ec ---[ end trace 0000000000000000 ]---
sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result: hostbyte=0x01 driverbyte=DRIVER_OK ci_hdrc ci_hdrc.0: USB bus 1 deregistered
reboot: Restarting system
------------
Re: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 22 lines · Guenter Roeck
Quoted 9 lines · Steven Rostedt
Quoted 6 lines · On Sat, 29 Oct 2022 15:56:25 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 6 lines
Quoted 5 lines
WARNING: CPU: 0 PID: 9 at lib/debugobjects.c:502 debug_print_object+0xd0/0x100 ODEBUG: free active (active state 0) object type: timer_list hint: neigh_timer_handler+0x0/0x480That happens with almost every test, so I may have missed some others in the noise.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Can you add this?
It doesn't make a difference.
Ah, it also requires this (I have other debugging in that file, so it may only apply with some fuzzing):
Almost good, except for the attached backtrace. That seems to happen on shutdown after bootting from a usb drive, but not on all platforms.
Quoted 9 lines
The warning is in
__mod_timer():if (WARN_ON_ONCE(!timer->function)) return -EINVAL;This may be due to the change in
blk_sync_queue()which I suspect may be called prior to the lastmod_timer()call. I'll add some debug code to verify.
I see that additional requests are sent to the scsi device after the call to blk_sync_queue(). The description of this function suggests that this may happen. Overall it does not seem to be appropriate to call del_timer_shutdown() from blk_sync_queue(). I'll change my test code accordingly.
Guenter
Steven Rostedt
reviewerRFC PATCH v2 21/31 timers: cgroup: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- kernel/cgroup/cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/cgroup/cgroup.c +1 -1
@@ -1669,7 +1669,7 @@ static void cgroup_rm_file(struct cgroup *cgrp, const struct cftype *cft) cfile->kn =NULL ;spin_unlock_irq (&cgroup_file_kn_lock); -del_timer_sync (&cfile->notify_timer); +del_timer_shutdown (&cfile->notify_timer); }kernfs_remove_by_name (cgrp->kn,cgroup_file_name (cgrp, cft, name));
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 22/31 timers: workqueue: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- kernel/workqueue.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/workqueue.c +2 -2
@@ -3608,8 +3608,8 @@ static void put_unbound_pool(struct worker_pool *pool)wait_for_completion (pool->detach_completion);/* shut down the timers */ -del_timer_sync (&pool->idle_timer); -del_timer_sync (&pool->mayday_timer); +del_timer_shutdown (&pool->idle_timer); +del_timer_shutdown (&pool->mayday_timer);/* RCU protected to allow dereferences from get_work_pool() */ call_rcu (&pool->rcu, rcu_free_pool);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 23/31 timers: nfc: pn533: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/nfc/pn533/pn533.c | 2 +- drivers/nfc/pn533/uart.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/nfc/pn533/pn533.c +1 -1
@@ -2788,7 +2788,7 @@ void pn53x_common_clean(struct pn533 *priv)struct pn533_cmd *cmd, *n;/* delete the timer before cleanup the worker */ -del_timer_sync (&priv->listen_timer); +del_timer_shutdown (&priv->listen_timer);flush_delayed_work (&priv->poll_work);destroy_workqueue (priv->wq);
drivers/nfc/pn533/uart.c +1 -1
@@ -310,7 +310,7 @@ static void pn532_uart_remove(struct serdev_device *serdev)pn53x_unregister_nfc (pn532->priv);serdev_device_close (serdev);pn53x_common_clean (pn532->priv); -del_timer_sync (&pn532->cmd_timeout); +del_timer_shutdown (&pn532->cmd_timeout);kfree_skb (pn532->recv_skb);kfree (pn532); }
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 24/31 timers: pcmcia: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- 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 ++-- 5 files changed, 7 insertions(+), 7 deletions(-)
drivers/pcmcia/bcm63xx_pcmcia.c +1 -1
@@ -443,7 +443,7 @@ static int bcm63xx_drv_pcmcia_remove(struct platform_device *pdev)struct resource *res; skt =platform_get_drvdata (pdev); -del_timer_sync (&skt->timer); +del_timer_shutdown (&skt->timer);iounmap (skt->base);iounmap (skt->io_base); res = skt->reg_res;
drivers/pcmcia/electra_cf.c +1 -1
@@ -317,7 +317,7 @@ static int electra_cf_remove(struct platform_device *ofdev) cf->active =0 ;pcmcia_unregister_socket (&cf->socket);free_irq (cf->irq, cf); -del_timer_sync (&cf->timer); +del_timer_shutdown (&cf->timer);iounmap (cf->io_virt);iounmap (cf->mem_base);
drivers/pcmcia/omap_cf.c +1 -1
@@ -296,7 +296,7 @@ static int __exit omap_cf_remove(struct platform_device *pdev) cf->active =0 ;pcmcia_unregister_socket (&cf->socket); -del_timer_sync (&cf->timer); +del_timer_shutdown (&cf->timer);release_mem_region (cf->phys_cf,SZ_8K );free_irq (cf->irq, cf);kfree (cf);
drivers/pcmcia/pd6729.c +2 -2
@@ -727,7 +727,7 @@ static int pd6729_pci_probe(struct pci_dev *dev,if (irq_mode ==1 )free_irq (dev->irq, socket);else -del_timer_sync (&socket->poll_timer); +del_timer_shutdown (&socket->poll_timer); err_out_free_res:pci_release_regions (dev); err_out_disable:
@@ -754,7 +754,7 @@ static void pd6729_pci_remove(struct pci_dev *dev)if (irq_mode ==1 )free_irq (dev->irq, socket);else -del_timer_sync (&socket->poll_timer); +del_timer_shutdown (&socket->poll_timer);pci_release_regions (dev);pci_disable_device (dev);
drivers/pcmcia/yenta_socket.c +2 -2
@@ -814,7 +814,7 @@ static void yenta_close(struct pci_dev *dev)if (sock->cb_irq)free_irq (sock->cb_irq, sock);else -del_timer_sync (&sock->poll_timer); +del_timer_shutdown (&sock->poll_timer);iounmap (sock->base);yenta_free_resources (sock);
@@ -1285,7 +1285,7 @@ static int yenta_probe(struct pci_dev *dev, const struct pci_device_id *id)if (socket->cb_irq)free_irq (socket->cb_irq, socket);else -del_timer_sync (&socket->poll_timer); +del_timer_shutdown (&socket->poll_timer); unmap:iounmap (socket->base);yenta_free_resources (socket);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 25/31 timers: scsi: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/scsi/qla2xxx/qla_edif.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/qla2xxx/qla_edif.c +2 -2
@@ -416,7 +416,7 @@ static void __qla2x00_release_all_sadb(struct scsi_qla_host *vha,*/ if (edif_entry->delete_sa_index !=INVALID_EDIF_SA_INDEX ) { -del_timer (&edif_entry->timer); +del_timer_shutdown (&edif_entry->timer);/* build and send the aen */ fcport->edif.rx_sa_set =1 ;
@@ -2799,7 +2799,7 @@ qla28xx_sa_update_iocb_entry(scsi_qla_host_t *v, struct req_que *req,"%s: removing edif_entry %p, new sa_index: 0x%x\n" , __func__, edif_entry, pkt->sa_index);qla_edif_list_delete_sa_index (sp->fcport, edif_entry); -del_timer (&edif_entry->timer); +del_timer_shutdown (&edif_entry->timer); ql_dbg(ql_dbg_edif, vha, 0x5033, "%s: releasing edif_entry %p, new sa_index: 0x%x\n",
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 26/31 timers: tty: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- drivers/tty/n_gsm.c | 2 +- drivers/tty/sysrq.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/tty/n_gsm.c +1 -1
@@ -2098,7 +2098,7 @@ static void gsm_dlci_free(struct tty_port *port) { struct gsm_dlci *dlci = container_of(port, struct gsm_dlci, port); -del_timer_sync (&dlci->t1); +del_timer_shutdown (&dlci->t1); dlci->gsm->dlci[dlci->addr] =NULL ;kfifo_free (&dlci->fifo);while ((dlci->skb =skb_dequeue (&dlci->skb_list)))
drivers/tty/sysrq.c +1 -1
@@ -1003,7 +1003,7 @@ static void sysrq_disconnect(struct input_handle *handle)input_close_device (handle);cancel_work_sync (&sysrq->reinject_work); -del_timer_sync (&sysrq->keyreset_timer); +del_timer_shutdown (&sysrq->keyreset_timer);input_unregister_handle (handle);kfree (sysrq); }
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 26/31] timers: tty: Use del_timer_shutdown() before freeing timer
Quoted 9 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.Cc Greg Kroah-Hartman <[email protected]>Cc Jiri Slaby <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>
Quoted 23 lines
--- drivers/tty/n_gsm.c | 2 +- drivers/tty/sysrq.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)drivers/tty/n_gsm.c +1
-1@@ -2098,7 +2098,7 @@ static void gsm_dlci_free(struct tty_port *port) { struct gsm_dlci *dlci = container_of(port, struct gsm_dlci, port); -del_timer_sync (&dlci->t1); +del_timer_shutdown (&dlci->t1); dlci->gsm->dlci[dlci->addr] =NULL ;kfifo_free (&dlci->fifo);while ((dlci->skb =skb_dequeue (&dlci->skb_list)))drivers/tty/sysrq.c +0
-0
drivers/tty/sysrq.c static void sysrq_disconnect(struct input_handle *handle) · line 1009
Quoted 8 lines
drivers/tty/sysrq.c +1
-1@@ -1003,7 +1003,7 @@ static void sysrq_disconnect(struct input_handle *handle)input_close_device (handle);cancel_work_sync (&sysrq->reinject_work); -del_timer_sync (&sysrq->keyreset_timer); +del_timer_shutdown (&sysrq->keyreset_timer);input_unregister_handle (handle);kfree (sysrq); }
thanks,
Signature · 2 lines
js suse labs
Steven Rostedt
reviewerRFC PATCH v2 27/31 timers: ext4: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- fs/ext4/super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ext4/super.c +1 -1
@@ -1225,7 +1225,7 @@ static void ext4_put_super(struct super_block *sb) }ext4_es_unregister_shrinker (sbi); -del_timer_sync (&sbi->s_err_report); +del_timer_shutdown (&sbi->s_err_report);ext4_release_system_zone (sb);ext4_mb_release (sb);ext4_ext_release (sb);
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 28/31 timers: fs/nilfs2: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- fs/nilfs2/segment.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/nilfs2/segment.c +1 -1
@@ -2752,7 +2752,7 @@ static void nilfs_segctor_destroy(struct nilfs_sc_info *sci)down_write (&nilfs->ns_segctor_sem); -del_timer_sync (&sci->sc_timer); +del_timer_shutdown (&sci->sc_timer);kfree (sci); }
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 28/31] timers: fs/nilfs2: Use del_timer_shutdown() before freeing timer
Quoted 28 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.Cc Ryusuke Konishi <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>--- fs/nilfs2/segment.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/nilfs2/segment.c +1
-2@@ -2752,7 +2752,7 @@ static void nilfs_segctor_destroy(struct nilfs_sc_info *sci)down_write (&nilfs->ns_segctor_sem); -del_timer_sync (&sci->sc_timer); +del_timer_shutdown (&sci->sc_timer);kfree (sci); } --
Quoted 1 line
2.35.1
del_timer_shutdown() is not yet in the mainline, so I reply with:
in the sense that I agree with the purpose of introducing the new function and the place to apply it is correct in nilfs2.
Thanks,
Ryusuke Konishi
Steven Rostedt
reviewerRFC PATCH v2 29/31 timers: ALSA: Use del_timer_shutdown() before freeing timer
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- sound/i2c/other/ak4117.c | 2 +- sound/synth/emux/emux.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
sound/i2c/other/ak4117.c +1 -1
@@ -47,7 +47,7 @@ static void reg_dump(struct ak4117 *ak4117)static void snd_ak4117_free (struct ak4117 *chip) { -del_timer_sync (&chip->timer); +del_timer_shutdown (&chip->timer);kfree (chip); }
sound/synth/emux/emux.c +1 -1
@@ -129,7 +129,7 @@ int snd_emux_free(struct snd_emux *emu)if (! emu)return -EINVAL ; -del_timer_sync (&emu->tlist); +del_timer_shutdown (&emu->tlist);snd_emux_proc_free (emu);snd_emux_delete_virmidi (emu);
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 29/31] timers: ALSA: Use del_timer_shutdown() before freeing timer
Quoted 5 lines · Steven Rostedt
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.
Quoted 7 lines
Cc Jaroslav Kysela <[email protected]>Cc Takashi Iwai <[email protected]>Cc Austin Kim <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>
I think there are a few other codes in sound/* that should use del_timer_shutdown() (although it works practically as is for now). I'll convert them once when the API is included.
thanks,
Takashi
Quoted 34 lines
--- sound/i2c/other/ak4117.c | 2 +- sound/synth/emux/emux.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)sound/i2c/other/ak4117.c +1
-1@@ -47,7 +47,7 @@ static void reg_dump(struct ak4117 *ak4117)static void snd_ak4117_free (struct ak4117 *chip) { -del_timer_sync (&chip->timer); +del_timer_shutdown (&chip->timer);kfree (chip); }sound/synth/emux/emux.c +1
-1@@ -129,7 +129,7 @@ int snd_emux_free(struct snd_emux *emu)if (! emu)return -EINVAL ; -del_timer_sync (&emu->tlist); +del_timer_shutdown (&emu->tlist);snd_emux_proc_free (emu);snd_emux_delete_virmidi (emu);Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 30/31 timers: x86/mce: Use __init_timer() for resetting timers
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
DEBUG_OBJECTS_TIMERS is now checking if a timer is ever enqueued, and if so, it must call del_timer_shutdown() before freeing, otherwise debug objects will trigger. This requires that once a timer is initialized (and initialized for debug objects) it must not be re-initialized using timer_setup(), as that will call the debug objects initialization code again and trigger a bug if it was ever used.
As the mce reinitializes its timers on CPU hotplug, it must use __init_timer() instead of timer_setup(), which will only initialize the debug objects once.
--- arch/x86/kernel/cpu/mce/core.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/mce/core.c +12 -2
@@ -2051,14 +2051,24 @@ static void __mcheck_cpu_setup_timer(void) {struct timer_list *t =this_cpu_ptr (&mce_timer); -timer_setup (t, mce_timer_fn,TIMER_PINNED ); +/* +* timer_setup() may only be used on a timer for the +* first time it is initialized. This resets the +* timer on CPU hotplug, so use __init_timer() instead. +*/ +__init_timer (t, mce_timer_fn,TIMER_PINNED ); }static void __mcheck_cpu_init_timer (void ) {struct timer_list *t =this_cpu_ptr (&mce_timer); -timer_setup (t, mce_timer_fn,TIMER_PINNED ); +/* +* timer_setup() may only be used on a timer for the +* first time it is initialized. This resets the +* timer on CPU hotplug, so use __init_timer() instead. +*/ +__init_timer (t, mce_timer_fn,TIMER_PINNED );mce_start_timer (t); }
Signature · 1 line
2.35.1
Steven Rostedt
reviewerRFC PATCH v2 31/31 timers: Expand DEBUG_OBJECTS_TIMER to check if it ever was used
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
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 del_timer_shutdown() is to be called before freeing, move the checks of DEBUG_OBJECTS_TIMER to if it ever gets armed to where del_timer_shutdown() is called. If there's a case where a timer is armed, and then freed without calling del_timer_shutdown() DEBUG_OBJECTS_TIMER will now trigger on it.
This catches cases that are potential issues instead of just catching when the race condition occurs.
--- include/linux/timer.h | 20 ++++++++++++++++++-- kernel/time/timer.c | 16 +++++++++++----- 2 files changed, 29 insertions(+), 7 deletions(-)
include/linux/timer.h +18 -2
@@ -18,6 +18,9 @@ struct timer_list {void (*function)(struct timer_list *);u32 flags; +#ifdef CONFIG_DEBUG_OBJECTS_TIMERS +u32 enabled; +#endif #ifdef CONFIG_LOCKDEP struct lockdep_map lockdep_map;#endif
@@ -128,6 +131,19 @@ 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 = 0; \ + __init_timer((_timer), (_fn), (_flags)); \ + } while (0) +#else +#define __init_timer_debug (_timer, _fn, _flags) \ + do { \ + __init_timer((_timer), (_fn), (_flags)); \ + } while (0) +#endif +/** * timer_setup - prepare a timer for first use * @timer: the timer in question
@@ -139,7 +155,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))
@@ -207,7 +223,7 @@ 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) +#define del_singleshot_timer_sync (t) del_timer_shutdown(t)extern void init_timers (void );struct hrtimer ;
kernel/time/timer.c +11 -5
@@ -774,17 +774,22 @@ static const struct debug_obj_descr timer_debug_descr = {static inline void debug_timer_init (struct timer_list *timer) { -debug_object_init (timer, &timer_debug_descr); +if (!timer->enabled) +debug_object_init (timer, &timer_debug_descr); }static inline void debug_timer_activate (struct timer_list *timer) { -debug_object_activate (timer, &timer_debug_descr); +if (!timer->enabled) { + timer->enabled =1 ; +debug_object_activate (timer, &timer_debug_descr); + } }static inline void debug_timer_deactivate (struct timer_list *timer) { -debug_object_deactivate (timer, &timer_debug_descr); +if (timer->enabled) +debug_object_deactivate (timer, &timer_debug_descr); }static inline void debug_timer_assert_init (struct timer_list *timer)
@@ -828,7 +833,6 @@ static inline void debug_init(struct timer_list *timer)static inline void debug_deactivate (struct timer_list *timer) { -debug_timer_deactivate (timer);trace_timer_cancel (timer); }
@@ -1281,8 +1285,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); + }raw_spin_unlock_irqrestore (&base->lock, flags);
Signature · 1 line
2.35.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Steven Rostedt
reviewerRe: [RFC][PATCH v2 15/31] timers: Input: Use del_timer_shutdown() before freeing timer
[
quilt mail --send still can't handle unicode characters.
Here's the patch again
]
From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed, del_timer_shutdown() must be called.
--- 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 +- 5 files changed, 5 insertions(+), 5 deletions(-)
drivers/input/keyboard/locomokbd.c +1 -1
@@ -310,7 +310,7 @@ static void locomokbd_remove(struct locomo_dev *dev)free_irq (dev->irq[0 ], locomokbd); -del_timer_sync (&locomokbd->timer); +del_timer_shutdown (&locomokbd->timer);input_unregister_device (locomokbd->input);locomo_set_drvdata (dev,NULL );
drivers/input/keyboard/omap-keypad.c +1 -1
@@ -296,7 +296,7 @@ static int omap_kp_remove(struct platform_device *pdev)omap_writew (1 ,OMAP1_MPUIO_BASE +OMAP_MPUIO_KBD_MASKIT );free_irq (omap_kp->irq, omap_kp); -del_timer_sync (&omap_kp->timer); +del_timer_shutdown (&omap_kp->timer);tasklet_kill (&kp_tasklet);/* unregister everything */
drivers/input/mouse/alps.c +1 -1
@@ -2970,7 +2970,7 @@ static void alps_disconnect(struct psmouse *psmouse)struct alps_data *priv = psmouse->private;psmouse_reset (psmouse); -del_timer_sync (&priv->timer); +del_timer_shutdown (&priv->timer);if (priv->dev2)input_unregister_device (priv->dev2);if (!IS_ERR_OR_NULL (priv->dev3))
drivers/input/serio/hil_mlc.c +1 -1
@@ -1017,7 +1017,7 @@ static int __init hil_mlc_init(void) static void __exit hil_mlc_exit(void) { -del_timer_sync (&hil_mlcs_kicker); +del_timer_shutdown (&hil_mlcs_kicker);tasklet_kill (&hil_mlcs_tasklet); }
drivers/input/serio/hp_sdc.c +1 -1
@@ -980,7 +980,7 @@ static void hp_sdc_exit(void)free_irq (hp_sdc.irq, &hp_sdc);write_unlock_irq (&hp_sdc.lock); -del_timer_sync (&hp_sdc.kicker); +del_timer_shutdown (&hp_sdc.kicker);tasklet_kill (&hp_sdc.task);
Signature · 1 line
2.35.1
Re: [RFC][PATCH v2 15/31] timers: Input: Use del_timer_shutdown() before freeing timer
Quoted 11 lines · Steven Rostedt
[
quilt mail --send still can't handle unicode characters.
Here's the patch again
]From: "Steven Rostedt (Google)" <[email protected]>
Before a timer is freed,
del_timer_shutdown()must be called.
Quoted 9 lines
Cc Dmitry Torokhov <[email protected]>Cc "Pali Rohár" <[email protected]>Cc "James E.J. Bottomley" <[email protected]>Cc Helge Deller <[email protected]>Cc Tony Lindgren <[email protected]>Cc Arnd Bergmann <[email protected]>Signed-off-by Steven Rostedt (Google) <[email protected]>
Please feel free to merge with the rest of the series.
Thanks.
Signature · 1 line
Dmitry
Jason A. Donenfeld
maintainerQuoted 14 lines · Steven Rostedt
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 whendel_timer_sync()should have been.Add a
del_timer_free()that not only does adel_timer_sync()but will mark the timer as freed in case it gets rearmed, it will trigger aWARN_ON. Thedel_timer_free()is more likely to be used by developers that are about to free a timer, then usingdel_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.
Quoted 4 lines
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.
FYI, there's a related issue with add_timer_on(), currently without a straight forward solution, in case you're curious, discussed with Sebastian and Sultan a few weeks ago. Pasting from that thread, the issue is:
1 while (conditions) {
2 if (!timer_pending(&stack.timer))
3 add_timer_on(&stack.timer, some_next_cpu);
4 }
5 del_timer_sync(&stack.timer);
a) add_timer_on() on line 3 is called from CPU 1 and pends the timer on CPU 2.
b) Just before the timer callback runs, not after, timer_pending() is made false, so the condition on line 2 holds true again.
c) add_timer_on() on line 3 is called from CPU 1 and pends the timer on CPU 3.
d) The conditions on line 1 are made false, and the loop breaks.
e) del_timer_sync() on line 5 is called, and its base->running_timer != timer check is false, because of step (c).
f) stack.timer gets freed / goes out of scope.
g) The callback scheduled from step (b) runs, and we have a UaF.
Here's a reproducer of this flow, which prints out:
[ 4.157610] wireguard: Stack on cpu 1 is corrupt
drivers/net/wireguard/main.c +30 -0
@@ -17,10 +17,40 @@#include <linux/genetlink.h> #include <net/rtnetlink.h> +struct state { +struct timer_list timer; +char valid[8 ]; +}; + +static void fire (struct timer_list *timer) +{ + struct state *stack = container_of(timer, struct state, timer); +mdelay (1000 ); +pr_err ("Stack on cpu %d is %s\n" ,raw_smp_processor_id (), stack->valid); +} + +static void do_the_thing (struct work_struct *work) +{ +struct state stack = { .valid ="valid" }; +timer_setup_on_stack (&stack.timer, fire,0 ); + stack.timer.expires = jiffies; +add_timer_on (&stack.timer,1 ); +while (timer_pending (&stack.timer)) +cpu_relax (); + stack.timer.expires = jiffies; +add_timer_on (&stack.timer,2 ); +del_timer_sync (&stack.timer); +memcpy (&stack.valid,"corrupt" ,8 ); +} + +static DECLARE_DELAYED_WORK(reproducer, do_the_thing); +static int __init wg_mod_init (void ) {int ret; +schedule_delayed_work_on (0 , &reproducer,HZ *3 ); + ret =wg_allowedips_slab_init ();if (ret <0 ) goto err_allowedips;
It would be interesting if your patch fixed this case too. But maybe the above is unfixable (and rather niche anyway).
Jason
Quoted 1 line · Jason A. Donenfeld
On Thu, Oct 27, 2022 at 11:05:25AM -0400, Steven Rostedt wrote:
…
Quoted 5 lines
FYI, there's a related issue with
add_timer_on(), currently without a straight forward solution, in case you're curious, discussed with Sebastian and Sultan a few weeks ago. Pasting from that thread, the issue is:
…
…
Quoted 3 lines
It would be interesting if your patch fixed this case too. But maybe the above is unfixable (and rather niche anyway).
Haven't read the thread, yet, but the issue we talked about a few weeks ago is unfixable. I plan to document this.
Eitherway CONFIG_DEBUG_OBJECTS_TIMERS should warn you here.
There are watchdogs (clocksource_watchdog()) which rotate CPUs and invoke add_timer_on() from within the callback. This complicates things.
Quoted 1 line
Jason
Sebastian
Steven Rostedt
reviewerQuoted 3 lines · On Thu, 27 Oct 2022 18:01:21 +0200 Sebastian Andrzej Siewior…
Haven't read the thread, yet, but the issue we talked about a few weeks ago is unfixable. I plan to document this.
EitherwayCONFIG_DEBUG_OBJECTS_TIMERSshould warn you here.
Not sure if my change affects that in this patch series.
https://lore.kernel.org/all/[email protected]/
-- Steve
Quoted 7 lines · Steven Rostedt
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.
Quoted 6 lines
This is v2 of that patch set. Thomas Gleixner posted an untested version that makes
timer->functionNULL as the flag that it is shutdown. I took that code, tested it (fixed it up), added more comments, and changed the name todel_timer_shutdown()as Linus had asked. I also converted it to useWARN_ON_ONCE()instead of justWARN_ON()as Linus asked for that too.
I rebased the series on top of v6.1-rc2 and gave it a try. Unfortunately it blows up in my face, first with
[ 16.212535] ------------[ cut here ]------------ [ 16.212887] ODEBUG: free active (active state 0) object type: timer_list hint: tcp_write_timer+0x0/0x190 [ 16.213725] WARNING: CPU: 0 PID: 310 at lib/debugobjects.c:502 debug_print_object+0xb8/0x100
which, of course, might be real, but after a couple of those I get
[ 16.325257] Insufficient stack space to handle exception! [ 16.325326] ESR: 0x0000000096000047 -- DABT (current EL) [ 16.325355] FAR: 0xffff800008677fe0 [ 16.325366] Task stack: [0xffff800008678000..0xffff80000867c000] [ 16.325376] IRQ stack: [0xffff800008000000..0xffff800008004000] [ 16.325387] Overflow stack: [0xffff51e19feab300..0xffff51e19feac300] [ 16.325406] CPU: 0 PID: 310 Comm: telnet Tainted: G W N 6.1.0-rc2-00032-g895257c4037a #1 [ 16.325425] Hardware name: linux,dummy-virt (DT) [ 16.325434] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 16.325450] pc : mark_lock+0x4/0x620 [ 16.325482] lr : __lock_acquire+0x3e8/0x1950 [ 16.325493] sp : ffff800008678090 [ 16.325499] x29: ffff800008678090 x28: ffff51e18651ccc0 x27: ffffc690a5583000 [ 16.325530] x26: ffff51e18651d590 x25: 0000000000000001 x24: 0000000000000001 [ 16.325552] x23: 0000000000000001 x22: 0000000000000028 x21: ffff51e18651d568 [ 16.325571] x20: ffffc690a61b5710 x19: 0000000000000000 x18: ffffffffffffffff [ 16.325589] x17: 6e6968207473696c x16: 5f72656d6974203a x15: 6570797420746365 [ 16.325608] x14: 6a626f2029302065 x13: ffffc690a55a78c0 x12: 00000000000c02e7 [ 16.325626] x11: 0000000030fa562e x10: ffffc690a5a9f208 x9 : ffffc690a5a9f200 [ 16.325645] x8 : ffff51e18651ccc0 x7 : 0000000000000000 x6 : 000000000002e9fa [ 16.325663] x5 : 0000000000000028 x4 : 0000000000000002 x3 : 00000000000c0003 [ 16.325681] x2 : 0000000000000008 x1 : ffff51e18651d590 x0 : ffff51e18651ccc0 [ 16.325902] Kernel panic - not syncing: kernel stack overflow [ 16.325992] CPU: 0 PID: 310 Comm: telnet Tainted: G W N 6.1.0-rc2-00032-g895257c4037a #1 [ 16.326008] Hardware name: linux,dummy-virt (DT) [ 16.326026] Call trace: [ 16.326031] dump_backtrace.part.0+0xe0/0xf0 [ 16.326048] show_stack+0x18/0x40 [ 16.326058] dump_stack_lvl+0x9c/0xd8 [ 16.326073] dump_stack+0x18/0x34 [ 16.326083] panic+0x194/0x38c [ 16.326093] nmi_panic+0xac/0xb0 [ 16.326103] panic_bad_stack+0x104/0x124 [ 16.326113] handle_bad_stack+0x34/0xe0 [ 16.326124] __bad_stack+0x78/0x7c [ 16.326134] mark_lock+0x4/0x620 [ 16.326144] lock_acquire.part.0+0xf0/0x26c [ 16.326155] lock_acquire+0x68/0x84 [ 16.326166] _raw_spin_lock_irqsave+0x70/0x150 [ 16.326178] debug_object_assert_init+0xa0/0x1a4 [ 16.326193] __try_to_del_timer_sync+0x40/0xdc [ 16.326207] __del_timer_sync+0xa4/0x100 [ 16.326218] timer_fixup_free+0x2c/0x54 [ 16.326229] debug_object_free.part.0+0x188/0x1b0 ... (many more of those) ... [ 16.329989] timer_fixup_free+0x40/0x54 [ 16.329996] __debug_check_no_obj_freed+0x1ec/0x25c [ 16.330003] debug_check_no_obj_freed+0x20/0x90 [ 16.330009] slab_free_freelist_hook.constprop.0+0xac/0x1b0 [ 16.330019] kmem_cache_free+0x1ac/0x500 [ 16.330026] __sk_destruct+0x140/0x2a0 [ 16.330035] sk_destruct+0x54/0x64 [ 16.330042] __sk_free+0x74/0x120 [ 16.330048] sk_free+0x64/0x8c [ 16.330055] tcp_close+0x94/0xc0 [ 16.330064] inet_release+0x50/0xb0 [ 16.330071] __sock_release+0x44/0xbc [ 16.330076] sock_close+0x18/0x30 [ 16.330081] __fput+0x84/0x270 [ 16.330088] ____fput+0x10/0x20 [ 16.330094] task_work_run+0x88/0xf0 [ 16.330102] do_exit+0x334/0xafc [ 16.330108] do_group_exit+0x34/0x90 [ 16.330115] __arm64_sys_exit_group+0x18/0x20 [ 16.330121] invoke_syscall+0x48/0x114 [ 16.330133] el0_svc_common.constprop.0+0x60/0x11c [ 16.330146] do_el0_svc+0x30/0xd0 [ 16.330157] el0_svc+0x48/0xc0 [ 16.330170] el0t_64_sync_handler+0xbc/0x13c [ 16.330179] el0t_64_sync+0x18c/0x190 [ 16.330645] Kernel Offset: 0x46909ae00000 from 0xffff800008000000 [ 16.330664] PHYS_OFFSET: 0xffffae1ec0000000 [ 16.330686] CPU features: 0x22000,2033c080,0000421b [ 16.330752] Memory Limit: none
This is with arm64_defconfig plus various debug options. Running the same test with v6.0.4 produces no errors, and neither does running it with v6.1-rc2-105-gb229b6ca5abb (current mainline).
Guenter
Steven Rostedt
reviewerQuoted 3 lines · On Thu, 27 Oct 2022 11:58:59 -0700 Guenter Roeck <[email protected]> wrote:
This is with
arm64_defconfigplus various debug options. Running the same test with v6.0.4 produces no errors, and neither does running it with v6.1-rc2-105-gb229b6ca5abb (current mainline).
So it works on current stable and current mainline? Does that mean we need to worry about this?
-- Steve
Quoted 10 lines · Steven Rostedt
Quoted 3 lines · On Thu, 27 Oct 2022 11:58:59 -0700 Guenter Roeck <[email protected]> wrote:
This is with
arm64_defconfigplus various debug options. Running the same test with v6.0.4 produces no errors, and neither does running it with v6.1-rc2-105-gb229b6ca5abb (current mainline).So it works on current stable and current mainline? Does that mean we need to worry about this?
Sorry, I wasn't clear. The test on v6.0.4 and v6.1-rc2-105-gb229b6ca5abb was without your patch set but with all ODEBUG options enabled, to be sure that there is no pre-existing condition.
Guenter
Quoted 3 lines · Steven Rostedt
So it works on current stable and current mainline? Does that mean we need to worry about this?
No, I think Guenter is saying that current mainline *without* the patches works fine.
But v6.1-rc2 (which is quite close to that current mainline) *with* the patches blows up.
So it's almost certainly the patches that break. There are no appreciable timer changes in those 105 commits (there's some added irq_work_sync but that looks very unlikely to be related.
Linus
Steven Rostedt
reviewerQuoted 8 lines · On Thu, 27 Oct 2022 12:11:43 -0700 Linus Torvalds…
Quoted 3 lines · Steven Rostedt
So it works on current stable and current mainline? Does that mean we need to worry about this?
No, I think Guenter is saying that current mainline *without* the patches works fine.
Quoted 7 lines
But v6.1-rc2 (which is quite close to that current mainline) *with* the patches blows up.
So it's almost certainly the patches that break. There are no appreciable timer changes in those 105 commits (there's some added
irq_work_syncbut that looks very unlikely to be related.
Got it. I'll need to setup an arm64 VM to see if I can reproduce it.
-- Steve
Quoted 14 lines · Steven Rostedt
Quoted 11 lines · On Thu, 27 Oct 2022 12:11:43 -0700 Linus Torvalds…
Quoted 3 lines · Steven Rostedt
So it works on current stable and current mainline? Does that mean we need to worry about this?
No, I think Guenter is saying that current mainline *without* the patches works fine.
But v6.1-rc2 (which is quite close to that current mainline) *with* the patches blows up.
Quoted 7 lines
Quoted 4 lines
So it's almost certainly the patches that break. There are no appreciable timer changes in those 105 commits (there's some added
irq_work_syncbut that looks very unlikely to be related.Got it. I'll need to setup an arm64 VM to see if I can reproduce it.
Try http://server.roeck-us.net/qemu/aarch64/
Guenter
Steven Rostedt
reviewerQuoted 64 lines · On Thu, 27 Oct 2022 11:58:59 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 7 lines · Steven Rostedt
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.
Quoted 6 lines
This is v2 of that patch set. Thomas Gleixner posted an untested version that makes
timer->functionNULL as the flag that it is shutdown. I took that code, tested it (fixed it up), added more comments, and changed the name todel_timer_shutdown()as Linus had asked. I also converted it to useWARN_ON_ONCE()instead of justWARN_ON()as Linus asked for that too.I rebased the series on top of v6.1-rc2 and gave it a try. Unfortunately it blows up in my face, first with
[ 16.212535] ------------[ cut here ]------------ [ 16.212887] ODEBUG: free active (active state 0) object type: timer_list hint: tcp_write_timer+0x0/0x190 [ 16.213725] WARNING: CPU: 0 PID: 310 at lib/debugobjects.c:502 debug_print_object+0xb8/0x100which, of course, might be real, but after a couple of those I get
[ 16.325257] Insufficient stack space to handle exception! [ 16.325326] ESR: 0x0000000096000047 -- DABT (current EL) [ 16.325355] FAR: 0xffff800008677fe0 [ 16.325366] Task stack: [0xffff800008678000..0xffff80000867c000] [ 16.325376] IRQ stack: [0xffff800008000000..0xffff800008004000] [ 16.325387] Overflow stack: [0xffff51e19feab300..0xffff51e19feac300] [ 16.325406] CPU: 0 PID: 310 Comm: telnet Tainted: G W N 6.1.0-rc2-00032-g895257c4037a #1 [ 16.325425] Hardware name: linux,dummy-virt (DT) [ 16.325434] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 16.325450] pc : mark_lock+0x4/0x620 [ 16.325482] lr : __lock_acquire+0x3e8/0x1950 [ 16.325493] sp : ffff800008678090 [ 16.325499] x29: ffff800008678090 x28: ffff51e18651ccc0 x27: ffffc690a5583000 [ 16.325530] x26: ffff51e18651d590 x25: 0000000000000001 x24: 0000000000000001 [ 16.325552] x23: 0000000000000001 x22: 0000000000000028 x21: ffff51e18651d568 [ 16.325571] x20: ffffc690a61b5710 x19: 0000000000000000 x18: ffffffffffffffff [ 16.325589] x17: 6e6968207473696c x16: 5f72656d6974203a x15: 6570797420746365 [ 16.325608] x14: 6a626f2029302065 x13: ffffc690a55a78c0 x12: 00000000000c02e7 [ 16.325626] x11: 0000000030fa562e x10: ffffc690a5a9f208 x9 : ffffc690a5a9f200 [ 16.325645] x8 : ffff51e18651ccc0 x7 : 0000000000000000 x6 : 000000000002e9fa [ 16.325663] x5 : 0000000000000028 x4 : 0000000000000002 x3 : 00000000000c0003 [ 16.325681] x2 : 0000000000000008 x1 : ffff51e18651d590 x0 : ffff51e18651ccc0 [ 16.325902] Kernel panic - not syncing: kernel stack overflow [ 16.325992] CPU: 0 PID: 310 Comm: telnet Tainted: G W N 6.1.0-rc2-00032-g895257c4037a #1 [ 16.326008] Hardware name: linux,dummy-virt (DT) [ 16.326026] Call trace: [ 16.326031] dump_backtrace.part.0+0xe0/0xf0 [ 16.326048] show_stack+0x18/0x40 [ 16.326058] dump_stack_lvl+0x9c/0xd8 [ 16.326073] dump_stack+0x18/0x34 [ 16.326083] panic+0x194/0x38c [ 16.326093] nmi_panic+0xac/0xb0 [ 16.326103] panic_bad_stack+0x104/0x124 [ 16.326113] handle_bad_stack+0x34/0xe0 [ 16.326124] __bad_stack+0x78/0x7c [ 16.326134] mark_lock+0x4/0x620 [ 16.326144] lock_acquire.part.0+0xf0/0x26c [ 16.326155] lock_acquire+0x68/0x84 [ 16.326166] _raw_spin_lock_irqsave+0x70/0x150 [ 16.326178] debug_object_assert_init+0xa0/0x1a4
Quoted 8 lines
[ 16.326193] __try_to_del_timer_sync+0x40/0xdc [ 16.326207] __del_timer_sync+0xa4/0x100 [ 16.326218] timer_fixup_free+0x2c/0x54 [ 16.326229] debug_object_free.part.0+0x188/0x1b0 ... (many more of those) ... [ 16.329989] timer_fixup_free+0x40/0x54
Ah, I see the issue here. Looks like the timer_fixup_free() is calling itself and crashing.
Let me take a look into that. I didn't touch the fixup code, and there could be an assumption there that it's behaving with the old approach.
-- Steve
Quoted 34 lines
[ 16.329996] __debug_check_no_obj_freed+0x1ec/0x25c [ 16.330003] debug_check_no_obj_freed+0x20/0x90 [ 16.330009] slab_free_freelist_hook.constprop.0+0xac/0x1b0 [ 16.330019] kmem_cache_free+0x1ac/0x500 [ 16.330026] __sk_destruct+0x140/0x2a0 [ 16.330035] sk_destruct+0x54/0x64 [ 16.330042] __sk_free+0x74/0x120 [ 16.330048] sk_free+0x64/0x8c [ 16.330055] tcp_close+0x94/0xc0 [ 16.330064] inet_release+0x50/0xb0 [ 16.330071] __sock_release+0x44/0xbc [ 16.330076] sock_close+0x18/0x30 [ 16.330081] __fput+0x84/0x270 [ 16.330088] ____fput+0x10/0x20 [ 16.330094] task_work_run+0x88/0xf0 [ 16.330102] do_exit+0x334/0xafc [ 16.330108] do_group_exit+0x34/0x90 [ 16.330115] __arm64_sys_exit_group+0x18/0x20 [ 16.330121] invoke_syscall+0x48/0x114 [ 16.330133] el0_svc_common.constprop.0+0x60/0x11c [ 16.330146] do_el0_svc+0x30/0xd0 [ 16.330157] el0_svc+0x48/0xc0 [ 16.330170] el0t_64_sync_handler+0xbc/0x13c [ 16.330179] el0t_64_sync+0x18c/0x190 [ 16.330645] Kernel Offset: 0x46909ae00000 from 0xffff800008000000 [ 16.330664] PHYS_OFFSET: 0xffffae1ec0000000 [ 16.330686] CPU features: 0x22000,2033c080,0000421b [ 16.330752] Memory Limit: noneThis is with
arm64_defconfigplus various debug options. Running the same test with v6.0.4 produces no errors, and neither does running it with v6.1-rc2-105-gb229b6ca5abb (current mainline).Guenter
Steven Rostedt
reviewerQuoted 9 lines · On Thu, 27 Oct 2022 15:20:58 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 3 lines
(many more of those)
...[ 16.329989] timer_fixup_free+0x40/0x54Ah, I see the issue here. Looks like the
timer_fixup_free()is calling itself and crashing.Let me take a look into that. I didn't touch the fixup code, and there could be an assumption there that it's behaving with the old approach.
Can you add this and see if it makes this issue go away?
-- Steve
kernel/time/timer.c +2 -2
@@ -691,7 +691,7 @@ static bool timer_fixup_init(void *addr, enum debug_obj_state state)switch (state) {case ODEBUG_STATE_ACTIVE : -del_timer_sync (timer); +del_timer_shutdown (timer);debug_object_init (timer, &timer_debug_descr);return true;default :
@@ -737,7 +737,7 @@ static bool timer_fixup_free(void *addr, enum debug_obj_state state)switch (state) {case ODEBUG_STATE_ACTIVE : -del_timer_sync (timer); +del_timer_shutdown (timer);debug_object_free (timer, &timer_debug_descr);return true;default :
Quoted 7 lines · Steven Rostedt
Quoted 4 lines · On Thu, 27 Oct 2022 15:20:58 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 3 lines
(many more of those)
...[ 16.329989] timer_fixup_free+0x40/0x54
Quoted 8 lines
Quoted 5 lines
Ah, I see the issue here. Looks like the
timer_fixup_free()is calling itself and crashing.Let me take a look into that. I didn't touch the fixup code, and there could be an assumption there that it's behaving with the old approach.
Can you add this and see if it makes this issue go away?
Yes, that fixes the crash. However, it still reports
[ 12.235054] ------------[ cut here ]------------ [ 12.235240] ODEBUG: free active (active state 0) object type: timer_list hint: tcp_write_timer+0x0/0x190 [ 12.237331] WARNING: CPU: 0 PID: 310 at lib/debugobjects.c:502 debug_print_object+0xb8/0x100 ... [ 12.255251] Call trace: [ 12.255305] debug_print_object+0xb8/0x100 [ 12.255385] __debug_check_no_obj_freed+0x1d0/0x25c [ 12.255474] debug_check_no_obj_freed+0x20/0x90 [ 12.255555] slab_free_freelist_hook.constprop.0+0xac/0x1b0 [ 12.255650] kmem_cache_free+0x1ac/0x500 [ 12.255728] __sk_destruct+0x140/0x2a0 [ 12.255805] sk_destruct+0x54/0x64 [ 12.255877] __sk_free+0x74/0x120 [ 12.255944] sk_free+0x64/0x8c [ 12.256009] tcp_close+0x94/0xc0 [ 12.256076] inet_release+0x50/0xb0 [ 12.256145] __sock_release+0x44/0xbc [ 12.256219] sock_close+0x18/0x30 [ 12.256292] __fput+0x84/0x270 [ 12.256361] ____fput+0x10/0x20 [ 12.256426] task_work_run+0x88/0xf0 [ 12.256499] do_exit+0x334/0xafc [ 12.256566] do_group_exit+0x34/0x90 [ 12.256634] __arm64_sys_exit_group+0x18/0x20 [ 12.256713] invoke_syscall+0x48/0x114 [ 12.256789] el0_svc_common.constprop.0+0x60/0x11c [ 12.256874] do_el0_svc+0x30/0xd0 [ 12.256943] el0_svc+0x48/0xc0 [ 12.257008] el0t_64_sync_handler+0xbc/0x13c [ 12.257086] el0t_64_sync+0x18c/0x190
Is that a real problem or a false positive ? I didn't see that without your patch series (which of course might be the whole point of the series).
Thanks,
Guenter
Quoted 8 lines · Steven Rostedt
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.
Quoted 6 lines
This is v2 of that patch set. Thomas Gleixner posted an untested version that makes
timer->functionNULL as the flag that it is shutdown. I took that code, tested it (fixed it up), added more comments, and changed the name todel_timer_shutdown()as Linus had asked. I also converted it to useWARN_ON_ONCE()instead of justWARN_ON()as Linus asked for that too.
Here are various warnings and crashes. Complete logs are at
https://kerneltests.org/builders
in the "testing" column of the qemu test results.
This is with the published patch set plus the fixups in timer_fixup_init() and timer_fixup_free().
Guenter
---
Block code:
WARNING: CPU: 0 PID: 8 at lib/debugobjects.c:502 debug_print_object+0x66/0x7a ODEBUG: free active (active state 0) object type: timer_list hint: blk_rq_timed_out_timer+0x0/0xc
---
In tcp:
WARNING: CPU: 0 PID: 255 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: tcp_keepalive_timer+0x0/0x3a8 WARNING: CPU: 0 PID: 255 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: tcp_write_timer+0x0/0x1a8
---
Other networking:
WARNING: CPU: 0 PID: 8 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: neigh_timer_handler+0x0/0x468
---
WARNING: CPU: 0 PID: 280 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: init active (active state 0) object type: timer_list hint: tulip_timer+0x0/0x38
That one is often followed by:
[ 29.833216] ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 [ 29.833723] WARNING: CPU: 0 PID: 365 at lib/debugobjects.c:502 debug_print_object+0xb8/0x100
in the same code line, suggesting that the timer may be shut down more than once (?). Then there is another crash with
[ 29.863890] Insufficient stack space to handle exception! [ 29.863939] ESR: 0x0000000096000047 -- DABT (current EL) [ 29.863952] FAR: 0xffff800008b17f80 [ 29.863959] Task stack: [0xffff800008b18000..0xffff800008b1c000] [ 29.863967] IRQ stack: [0xffff800008000000..0xffff800008004000] [ 29.863975] Overflow stack: [0xffff30a35fe7a0f0..0xffff30a35fe7b0f0] [ 29.863987] CPU: 0 PID: 365 Comm: ip Tainted: G W N 6.1.0-rc2-00138-gced58c742836 #1 [ 29.863999] Hardware name: linux,dummy-virt (DT)
followed by a sequence of
[ 29.864572] __try_to_del_timer_sync+0x40/0xdc [ 29.864582] __del_timer_sync+0xa4/0x100 [ 29.864590] timer_fixup_init+0x2c/0x5c [ 29.864599] __debug_object_init+0x248/0x53c [ 29.864606] debug_object_init+0x24/0x30 [ 29.864614] timer_fixup_init+0x40/0x5c [ 29.864622] __debug_object_init+0x248/0x53c [ 29.864630] debug_object_init+0x24/0x30 [ 29.864637] timer_fixup_init+0x40/0x5c [ 29.864645] __debug_object_init+0x248/0x53c [ 29.864658] debug_object_init+0x24/0x30 [ 29.864666] timer_fixup_init+0x40/0x5c ... [ 29.866492] debug_object_init+0x24/0x30 [ 29.866500] init_timer_key+0x7c/0x80 [ 29.866508] tulip_down+0x1d0/0x24c [ 29.866518] tulip_close+0x3c/0xc0
This crash is seen with variants on several platforms/architectures.
---
WARNING: CPU: 0 PID: 8 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: addrconf_dad_work+0x0/0x628
---
mips64:
WARNING: CPU: 0 PID: 280 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8
ODEBUG: init active (active state 0) object type: timer_list hint: 0x0
Modules linked in:
CPU: 0 PID: 280 Comm: ip Tainted: G W N 6.1.0-rc2-00138-gced58c742836 #1
Stack : 0000000000000056 ffffffffffffffff 0000000000000008 00b65275abba8126
00b65275abba8126 0000000000000000 9000000004193728 ffffffff80f29f38
ffffffff8105c9d8 ffffffffffff8880 9000000004193668 0000000000000000
0000000000000005 0000000000000010 ffffffff80c731a0 ffffffff81030000
2020205720202020 ffffffff81030000 0000000000000000 ffffffff80f29f38
0000000000000009 00000000000001f6 ffffffff8063d45c ffffffff81cb94c0
ffffffff81d76240 0000000000000000 ffffffff807575a8 0000000000000000
ffffffff81230000 9000000004190000 9000000004193720 9000000018002030
ffffffff80c8f528 0000000000000000 0000000000000000 00b65275abba8126
ffffffff8122e050 000000001000a4e1 ffffffff8010a884 00b65275abba8126
...
Call Trace:
[<ffffffff8010a884>] show_stack+0x3c/0x120
[<ffffffff80c8f528>] dump_stack_lvl+0x4c/0x90
[<ffffffff80130e54>] __warn+0xdc/0x1c8
[<ffffffff80c75a08>] warn_slowpath_fmt+0x98/0xc4
[<ffffffff8063d45c>] debug_print_object+0xa4/0xd8
[<ffffffff8063e590>] __debug_object_init+0x2d0/0x670
[<ffffffff801b44b8>] timer_fixup_init+0x40/0x58
irq event stamp: 1537
hardirqs last enabled at (1549): [<ffffffff8018e67c>] __up_console_sem+0x9c/0xc8
hardirqs last disabled at (1560): [<ffffffff8018e658>] __up_console_sem+0x78/0xc8
softirqs last enabled at (1324): [<ffffffff80a572bc>] dev_deactivate_many+0x32c/0x458
softirqs last disabled at (1322): [<ffffffff80a57288>] dev_deactivate_many+0x2f8/0x458
---[ end trace 0000000000000000 ]---=====================================
WARNING: bad unlock balance detected!
6.1.0-rc2-00138-gced58c742836 #1 Tainted: G W N
-------------------------------------
�������␈␂/-1 is trying to release lock (&obj_hash[i].lock) at:
[<ffffffff801b41e0>] __try_to_del_timer_sync+0x48/0x140 but there are no more locks to release!
other info that might help us debug this:
qemu-system-mips64: terminating on signal 15 from pid 2525787 (/bin/bash)
---
Openrisc fails completely. Lots of the following, then boot stalls.
WARNING: CPU: 0 PID: 88 at lib/debugobjects.c:502 debug_print_object+0xc0/0xe8 ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 Modules linked in: CPU: 0 PID: 88 Comm: udhcpc Not tainted 6.1.0-rc2-00138-gced58c742836 #1 Call trace: [<8eaa3f51>] dump_stack_lvl+0x44/0x80 [<4da6c5ef>] dump_stack+0x1c/0x2c [<1b9f58b7>] __warn+0xdc/0x118 [<86d4d066>] ? debug_print_object+0xc0/0xe8 [<689beae8>] warn_slowpath_fmt+0x78/0x90 [<86d4d066>] debug_print_object+0xc0/0xe8 [<872f4074>] __debug_object_init+0x2bc/0x7f4 [<c30d6214>] ? _raw_spin_unlock_irqrestore+0x50/0x84 [<c3f3be91>] ? debug_check_no_locks_freed+0xb8/0x194 [<558675c1>] ? slob_alloc+0xe8/0x350 [<26225550>] ? lockdep_init_map_type+0x68/0x38c [<c3f3be91>] ? debug_check_no_locks_freed+0xb8/0x194 [<2c9cf7b5>] ? inet_create+0x2e8/0x404 [<bab055df>] debug_object_init+0x30/0x40 [<6d2e468f>] init_timer_key+0xb4/0x110 [<60c0d3e7>] ? sk_init_common+0x1a0/0x1c0 [<b52686c2>] sock_init_data+0x60/0x2a4 [<1ff904c3>] ? sk_alloc+0xe8/0x138 [<ca3e6ef9>] inet_create+0x1e0/0x404 [<623d726a>] ? inet_create+0x80/0x404 [<a3256728>] ? lock_release+0x1c0/0x30c [<960a454e>] __sock_create+0x140/0x288 [<0290cd64>] ? __sock_create+0x98/0x288 [<b62bc649>] __sys_socket+0x7c/0x128 [<fa003224>] ? do_work_pending+0x4c/0x118 [<705147e9>] sys_socket+0x14/0x24 [<9c4e015d>] ? _syscall_return+0x0/0x4 ---[ end trace 0000000000000000 ]---
---
parisc crashes.
[ 3.015186] ------------[ cut here ]------------ [ 3.015580] ODEBUG: init active (active state 0) object type: timer_list hint: timeout_waiting_on_port+0x0/0x2c [ 3.016945] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 debug_print_object+0x98/0xc8 ... [ 3.026117] ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 [ 3.026355] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 debug_print_object+0x98/0xc8
...
[ many more of the same ]
[ 3.054542] stackcheck: swapper/0 will most likely overflow irq stack (sp:11e51800, stk bottom-top:11460004-11468004) [ 3.054592] Kernel panic - not syncing: low stack detected by irq handler - check messages
and boom.
---
usb:
[ 23.993737] WARNING: CPU: 0 PID: 343 at lib/debugobjects.c:502 debug_print_object+0xac/0xc8 [ 23.993953] ODEBUG: free active (active state 0) object type: timer_list hint: hub_init_func2+0x0/0xc
---
ppc:
[ 6.607478][ T1] ODEBUG: init active (active state 0) object type: timer_list hint: .ibmvscsi_timeout+0x0/0x58 [ 6.608536][ T1] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 .debug_print_object+0xbc/0xf0
followed by several of
[ 6.623661][ T1] ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 [ 6.624155][ T1] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 .debug_print_object+0xbc/0xf0
and finally:
[ 6.694785][ C0] BUG: Kernel NULL pointer dereference on read at 0x000000b8 [ 6.694796][ C0] Faulting instruction address: 0xc0000000001ece44 [ 6.694807][ C0] Thread overran stack, or stack corrupted [ 6.694967][ C0] Oops: Kernel access of bad area, sig: 7 [#1]
Steven Rostedt
reviewerQuoted 25 lines · On Thu, 27 Oct 2022 15:24:04 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 8 lines · Steven Rostedt
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.
Quoted 6 lines
This is v2 of that patch set. Thomas Gleixner posted an untested version that makes
timer->functionNULL as the flag that it is shutdown. I took that code, tested it (fixed it up), added more comments, and changed the name todel_timer_shutdown()as Linus had asked. I also converted it to useWARN_ON_ONCE()instead of justWARN_ON()as Linus asked for that too.Here are various warnings and crashes. Complete logs are at
https://kerneltests.org/builders
in the "testing" column of the qemu test results.
This is with the published patch set plus the fixups in
timer_fixup_init()andtimer_fixup_free().
Quoted 7 lines
Guenter
---
Block code:WARNING: CPU: 0 PID: 8 at lib/debugobjects.c:502 debug_print_object+0x66/0x7a ODEBUG: free active (active state 0) object type: timer_list hint: blk_rq_timed_out_timer+0x0/0xc
This is probably just missing a switch to shutdown.
Quoted 9 lines
---
In tcp:WARNING: CPU: 0 PID: 255 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: tcp_keepalive_timer+0x0/0x3a8 WARNING: CPU: 0 PID: 255 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: tcp_write_timer+0x0/0x1a8
Looking at these, I think the del_timer_try_shutdown() needs to always shutdown (let's try that, and see if that fixes things before we rename anything).
Quoted 49 lines
---
Other networking:WARNING: CPU: 0 PID: 8 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: neigh_timer_handler+0x0/0x468---
WARNING: CPU: 0 PID: 280 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: init active (active state 0) object type: timer_list hint: tulip_timer+0x0/0x38That one is often followed by:
[ 29.833216] ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 [ 29.833723] WARNING: CPU: 0 PID: 365 at lib/debugobjects.c:502 debug_print_object+0xb8/0x100in the same code line, suggesting that the timer may be shut down more than once (?). Then there is another crash with
[ 29.863890] Insufficient stack space to handle exception! [ 29.863939] ESR: 0x0000000096000047 -- DABT (current EL) [ 29.863952] FAR: 0xffff800008b17f80 [ 29.863959] Task stack: [0xffff800008b18000..0xffff800008b1c000] [ 29.863967] IRQ stack: [0xffff800008000000..0xffff800008004000] [ 29.863975] Overflow stack: [0xffff30a35fe7a0f0..0xffff30a35fe7b0f0] [ 29.863987] CPU: 0 PID: 365 Comm: ip Tainted: G W N 6.1.0-rc2-00138-gced58c742836 #1 [ 29.863999] Hardware name: linux,dummy-virt (DT)followed by a sequence of
[ 29.864572] __try_to_del_timer_sync+0x40/0xdc [ 29.864582] __del_timer_sync+0xa4/0x100 [ 29.864590] timer_fixup_init+0x2c/0x5c [ 29.864599] __debug_object_init+0x248/0x53c [ 29.864606] debug_object_init+0x24/0x30 [ 29.864614] timer_fixup_init+0x40/0x5c [ 29.864622] __debug_object_init+0x248/0x53c [ 29.864630] debug_object_init+0x24/0x30 [ 29.864637] timer_fixup_init+0x40/0x5c [ 29.864645] __debug_object_init+0x248/0x53c [ 29.864658] debug_object_init+0x24/0x30 [ 29.864666] timer_fixup_init+0x40/0x5c ... [ 29.866492] debug_object_init+0x24/0x30 [ 29.866500] init_timer_key+0x7c/0x80 [ 29.866508] tulip_down+0x1d0/0x24c [ 29.866518] tulip_close+0x3c/0xc0
Quoted 1 line
This crash is seen with variants on several platforms/architectures.
Hmm, I'll have to take a deeper look here.
Quoted 5 lines
---
WARNING: CPU: 0 PID: 8 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: addrconf_dad_work+0x0/0x628
Hmm, I was afraid of this one.
Looks to be from the workqueue code for delayed work. I don't see a place that frees besides just flushing it. And we can't determine if a flush is permanent or will be armed again.
May need to add a helper function that resets the debugging so that it doesn't trigger on delayed work timers. There's a DEBUG_OBJECTS_WORK that handles bad work queue issues.
Quoted 6 lines
---
mips64:WARNING: CPU: 0 PID: 280 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8ODEBUG: init active (active state 0) object type:
timer_listhint:0x0
Not a very good hint.
Quoted 92 lines
Modules linked in: CPU: 0 PID: 280 Comm: ip Tainted: G W N 6.1.0-rc2-00138-gced58c742836 #1 Stack : 0000000000000056 ffffffffffffffff 0000000000000008 00b65275abba8126 00b65275abba8126 0000000000000000 9000000004193728 ffffffff80f29f38 ffffffff8105c9d8 ffffffffffff8880 9000000004193668 0000000000000000 0000000000000005 0000000000000010 ffffffff80c731a0 ffffffff81030000 2020205720202020 ffffffff81030000 0000000000000000 ffffffff80f29f38 0000000000000009 00000000000001f6 ffffffff8063d45c ffffffff81cb94c0 ffffffff81d76240 0000000000000000 ffffffff807575a8 0000000000000000 ffffffff81230000 9000000004190000 9000000004193720 9000000018002030 ffffffff80c8f528 0000000000000000 0000000000000000 00b65275abba8126 ffffffff8122e050 000000001000a4e1 ffffffff8010a884 00b65275abba8126 ... Call Trace: [<ffffffff8010a884>] show_stack+0x3c/0x120 [<ffffffff80c8f528>] dump_stack_lvl+0x4c/0x90 [<ffffffff80130e54>] __warn+0xdc/0x1c8 [<ffffffff80c75a08>] warn_slowpath_fmt+0x98/0xc4 [<ffffffff8063d45c>] debug_print_object+0xa4/0xd8 [<ffffffff8063e590>] __debug_object_init+0x2d0/0x670 [<ffffffff801b44b8>] timer_fixup_init+0x40/0x58 irq event stamp: 1537 hardirqs last enabled at (1549): [<ffffffff8018e67c>] __up_console_sem+0x9c/0xc8 hardirqs last disabled at (1560): [<ffffffff8018e658>] __up_console_sem+0x78/0xc8 softirqs last enabled at (1324): [<ffffffff80a572bc>] dev_deactivate_many+0x32c/0x458 softirqs last disabled at (1322): [<ffffffff80a57288>] dev_deactivate_many+0x2f8/0x458 ---[ end trace 0000000000000000 ]---=====================================
WARNING: bad unlock balance detected!
6.1.0-rc2-00138-gced58c742836 #1 Tainted: G W N
-------------------------------------
�������␈␂/-1 is trying to release lock (&obj_hash[i].lock) at:
[<ffffffff801b41e0>]__try_to_del_timer_sync+0x48/0x140but there are no more locks to release!other info that might help us debug this:
qemu-system-mips64: terminating on signal 15 from pid 2525787 (/bin/bash)---
Openrisc fails completely. Lots of the following, then boot stalls.WARNING: CPU: 0 PID: 88 at lib/debugobjects.c:502 debug_print_object+0xc0/0xe8 ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 Modules linked in: CPU: 0 PID: 88 Comm: udhcpc Not tainted 6.1.0-rc2-00138-gced58c742836 #1 Call trace: [<8eaa3f51>] dump_stack_lvl+0x44/0x80 [<4da6c5ef>] dump_stack+0x1c/0x2c [<1b9f58b7>] __warn+0xdc/0x118 [<86d4d066>] ? debug_print_object+0xc0/0xe8 [<689beae8>] warn_slowpath_fmt+0x78/0x90 [<86d4d066>] debug_print_object+0xc0/0xe8 [<872f4074>] __debug_object_init+0x2bc/0x7f4 [<c30d6214>] ? _raw_spin_unlock_irqrestore+0x50/0x84 [<c3f3be91>] ? debug_check_no_locks_freed+0xb8/0x194 [<558675c1>] ? slob_alloc+0xe8/0x350 [<26225550>] ? lockdep_init_map_type+0x68/0x38c [<c3f3be91>] ? debug_check_no_locks_freed+0xb8/0x194 [<2c9cf7b5>] ? inet_create+0x2e8/0x404 [<bab055df>] debug_object_init+0x30/0x40 [<6d2e468f>] init_timer_key+0xb4/0x110 [<60c0d3e7>] ? sk_init_common+0x1a0/0x1c0 [<b52686c2>] sock_init_data+0x60/0x2a4 [<1ff904c3>] ? sk_alloc+0xe8/0x138 [<ca3e6ef9>] inet_create+0x1e0/0x404 [<623d726a>] ? inet_create+0x80/0x404 [<a3256728>] ? lock_release+0x1c0/0x30c [<960a454e>] __sock_create+0x140/0x288 [<0290cd64>] ? __sock_create+0x98/0x288 [<b62bc649>] __sys_socket+0x7c/0x128 [<fa003224>] ? do_work_pending+0x4c/0x118 [<705147e9>] sys_socket+0x14/0x24 [<9c4e015d>] ? _syscall_return+0x0/0x4 ---[ end trace 0000000000000000 ]------
parisc crashes.[ 3.015186] ------------[ cut here ]------------ [ 3.015580] ODEBUG: init active (active state 0) object type: timer_list hint: timeout_waiting_on_port+0x0/0x2c [ 3.016945] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 debug_print_object+0x98/0xc8 ... [ 3.026117] ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 [ 3.026355] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 debug_print_object+0x98/0xc8...
[ many more of the same ][ 3.054542] stackcheck: swapper/0 will most likely overflow irq stack (sp:11e51800, stk bottom-top:11460004-11468004) [ 3.054592] Kernel panic - not syncing: low stack detected by irq handler - check messages
Quoted 7 lines
and boom.
---
usb:[ 23.993737] WARNING: CPU: 0 PID: 343 at lib/debugobjects.c:502 debug_print_object+0xac/0xc8 [ 23.993953] ODEBUG: free active (active state 0) object type: timer_list hint: hub_init_func2+0x0/0xc
Yeah, there's going to be lots of these.
Quoted 12 lines
---
ppc:[ 6.607478][ T1] ODEBUG: init active (active state 0) object type: timer_list hint: .ibmvscsi_timeout+0x0/0x58 [ 6.608536][ T1] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 .debug_print_object+0xbc/0xf0followed by several of
[ 6.623661][ T1] ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 [ 6.624155][ T1] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 .debug_print_object+0xbc/0xf0
Quoted 6 lines
and finally:
[ 6.694785][ C0] BUG: Kernel NULL pointer dereference on read at 0x000000b8 [ 6.694796][ C0] Faulting instruction address: 0xc0000000001ece44 [ 6.694807][ C0] Thread overran stack, or stack corrupted [ 6.694967][ C0] Oops: Kernel access of bad area, sig: 7 [#1]
Anyway, this is all an issue with the last patch that makes calling shutdown a requirement before freeing and not just a hint.
You may want to test all but the last patch, as only the last patch is what's going to trigger all the above, as it modifies the semantics of DEBUG_OBJECTS_TIMER.
But for the tcp one, add this to the queue and see if the tcp one goes away?
-- Steve
kernel/time/timer.c +1 -1
@@ -1255,7 +1255,7 @@ 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 && ret) { +if (free) { timer->function =NULL ;debug_timer_deactivate (timer); }
Quoted 30 lines · Steven Rostedt
Quoted 27 lines · On Thu, 27 Oct 2022 15:24:04 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 8 lines · Steven Rostedt
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.
Quoted 6 lines
This is v2 of that patch set. Thomas Gleixner posted an untested version that makes
timer->functionNULL as the flag that it is shutdown. I took that code, tested it (fixed it up), added more comments, and changed the name todel_timer_shutdown()as Linus had asked. I also converted it to useWARN_ON_ONCE()instead of justWARN_ON()as Linus asked for that too.Here are various warnings and crashes. Complete logs are at
https://kerneltests.org/builders
in the "testing" column of the qemu test results.
This is with the published patch set plus the fixups in
timer_fixup_init()andtimer_fixup_free().Guenter
Quoted 8 lines
Quoted 5 lines
---
Block code:WARNING: CPU: 0 PID: 8 at lib/debugobjects.c:502 debug_print_object+0x66/0x7a ODEBUG: free active (active state 0) object type: timer_list hint: blk_rq_timed_out_timer+0x0/0xcThis is probably just missing a switch to shutdown.
Wild shot, and I don't really know what I am doing, but I'll try
iff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index e71b3b43927c..6c1cb4a219f0 100644
block/blk-sysfs.c +2 -0
@@ -767,6 +767,8 @@ static void blk_release_queue(struct kobject *kobj)might_sleep (); +del_timer_shutdown (&q->timeout); +percpu_ref_exit (&q->q_usage_counter);if (q->poll_stat)
Guenter
Steven Rostedt
reviewerQuoted 10 lines · On Thu, 27 Oct 2022 16:24:42 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 2 lines
This is probably just missing a switch to shutdown.
Wild shot, and I don't really know what I am doing, but I'll try
iff --git
a/block/blk-sysfs.cb/block/blk-sysfs.c
indexe71b3b43927c..6c1cb4a219f0100644
block/blk-sysfs.c static void blk_release_queue(struct kobject *kobj) · line 774
Quoted 8 lines
block/blk-sysfs.c +2
-0@@ -767,6 +767,8 @@ static void blk_release_queue(struct kobject *kobj)might_sleep (); +del_timer_shutdown (&q->timeout); +percpu_ref_exit (&q->q_usage_counter);if (q->poll_stat)
Or perhaps this:
-- Steve
block/blk-core.c +1 -1
@@ -227,7 +227,7 @@ const char *blk_status_to_str(blk_status_t status)*/ void blk_sync_queue (struct request_queue *q) { -del_timer_sync (&q->timeout); +del_timer_shutdown (&q->timeout);cancel_work_sync (&q->timeout_work); }EXPORT_SYMBOL (blk_sync_queue);
block/blk-sysfs.c +2 -0
@@ -769,6 +769,8 @@ static void blk_release_queue(struct kobject *kobj)percpu_ref_exit (&q->q_usage_counter); +blk_sync_queue (q); +if (q->poll_stat)blk_stat_remove_callback (q, q->poll_cb);blk_stat_free_callback (q->poll_cb);
Quoted 16 lines · Steven Rostedt
Quoted 13 lines · On Thu, 27 Oct 2022 16:24:42 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 2 lines
This is probably just missing a switch to shutdown.
Wild shot, and I don't really know what I am doing, but I'll try
iff --git
a/block/blk-sysfs.cb/block/blk-sysfs.c
indexe71b3b43927c..6c1cb4a219f0100644block/blk-sysfs.c +0
-0@@ -767,6 +767,8 @@ static void blk_release_queue(struct kobject *kobj)might_sleep ();
Quoted 8 lines
block/blk-sysfs.c static void blk_release_queue(struct kobject *kobj) · line 774
Quoted 5 lines
block/blk-sysfs.c +2
-0+del_timer_shutdown (&q->timeout); +percpu_ref_exit (&q->q_usage_counter);if (q->poll_stat)Or perhaps this:
I think you are correct. Let me give it a try.
Quoted 20 lines
-- Steve
block/blk-core.c +1
-1@@ -227,7 +227,7 @@ const char *blk_status_to_str(blk_status_t status)*/ void blk_sync_queue (struct request_queue *q) { -del_timer_sync (&q->timeout); +del_timer_shutdown (&q->timeout);cancel_work_sync (&q->timeout_work); }EXPORT_SYMBOL (blk_sync_queue);block/blk-sysfs.c +0
-0
block/blk-sysfs.c static void blk_release_queue(struct kobject *kobj) · line 773
Quoted 5 lines
block/blk-sysfs.c +2
-0@@ -769,6 +769,8 @@ static void blk_release_queue(struct kobject *kobj)percpu_ref_exit (&q->q_usage_counter); +blk_sync_queue (q); +
This might not even be needed. I'll try without it first.
Thanks,
Guenter
Quoted 3 lines
block/blk-sysfs.c +0
-0if (q->poll_stat)blk_stat_remove_callback (q, q->poll_cb);blk_stat_free_callback (q->poll_cb);
Quoted 31 lines · Steven Rostedt
Quoted 28 lines · On Thu, 27 Oct 2022 15:24:04 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 8 lines · Steven Rostedt
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.
Quoted 6 lines
This is v2 of that patch set. Thomas Gleixner posted an untested version that makes
timer->functionNULL as the flag that it is shutdown. I took that code, tested it (fixed it up), added more comments, and changed the name todel_timer_shutdown()as Linus had asked. I also converted it to useWARN_ON_ONCE()instead of justWARN_ON()as Linus asked for that too.Here are various warnings and crashes. Complete logs are at
https://kerneltests.org/builders
in the "testing" column of the qemu test results.
This is with the published patch set plus the fixups in
timer_fixup_init()andtimer_fixup_free().Guenter
---
[ ... ]
Quoted 4 lines
Quoted 4 lines
WARNING: CPU: 0 PID: 280 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: init active (active state 0) object type: timer_list hint: tulip_timer+0x0/0x38
The problem is that the tulip code calls timer_setup() repeatedly (and unnecessarily). Apparently either the new timer code and/or the associated ODEBUG code doesn't like that. The patch below fixes the problem.
I think there needs to be a means to handle that situation gracefully. The parport code has the same problem (see second patch below), and I am sure there are others.
Thanks,
Guenter
---
tulip:
drivers/net/ethernet/dec/tulip/tulip_core.c +4 -2
@@ -770,8 +770,6 @@ static void tulip_down (struct net_device *dev)spin_unlock_irqrestore (&tp->lock, flags); -timer_setup (&tp->timer, tulip_tbl[tp->chip_id].media_timer,0 ); - dev->if_port = tp->saved_if_port;/* Leave the driver in snooze, not sleep, mode. */
@@ -1869,10 +1867,14 @@ static int __maybe_unused tulip_resume(struct device *dev_d)static void tulip_remove_one (struct pci_dev *pdev) {struct net_device *dev =pci_get_drvdata (pdev); +struct tulip_private *tp;if (!dev)return ; + tp =netdev_priv (dev); +del_timer_shutdown (&tp->timer); +unregister_netdev (dev); }
---
parport:
drivers/parport/ieee1284.c +1 -1
@@ -73,7 +73,7 @@ int parport_wait_event (struct parport *port, signed long timeout)timer_setup (&port->timer, timeout_waiting_on_port,0 );mod_timer (&port->timer, jiffies + timeout); ret =down_interruptible (&port->physport->ieee1284.irq); -if (!del_timer_sync (&port->timer) && !ret) +if (!del_timer_shutdown (&port->timer) && !ret)/* Timed out. */ ret =1 ;
Quoted 254 lines · Steven Rostedt
Quoted 25 lines · On Thu, 27 Oct 2022 15:24:04 -0700 Guenter Roeck <[email protected]> wrote:
Quoted 8 lines · Steven Rostedt
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.
Quoted 6 lines
This is v2 of that patch set. Thomas Gleixner posted an untested version that makes
timer->functionNULL as the flag that it is shutdown. I took that code, tested it (fixed it up), added more comments, and changed the name todel_timer_shutdown()as Linus had asked. I also converted it to useWARN_ON_ONCE()instead of justWARN_ON()as Linus asked for that too.Here are various warnings and crashes. Complete logs are at
https://kerneltests.org/builders
in the "testing" column of the qemu test results.
This is with the published patch set plus the fixups in
timer_fixup_init()andtimer_fixup_free().Quoted 7 lines
Guenter
---
Block code:WARNING: CPU: 0 PID: 8 at lib/debugobjects.c:502 debug_print_object+0x66/0x7a ODEBUG: free active (active state 0) object type: timer_list hint: blk_rq_timed_out_timer+0x0/0xcThis is probably just missing a switch to shutdown.
Quoted 9 lines
---
In tcp:WARNING: CPU: 0 PID: 255 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: tcp_keepalive_timer+0x0/0x3a8 WARNING: CPU: 0 PID: 255 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: tcp_write_timer+0x0/0x1a8Looking at these, I think the
del_timer_try_shutdown()needs to always shutdown (let's try that, and see if that fixes things before we rename anything).Quoted 49 lines
---
Other networking:WARNING: CPU: 0 PID: 8 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: neigh_timer_handler+0x0/0x468---
WARNING: CPU: 0 PID: 280 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: init active (active state 0) object type: timer_list hint: tulip_timer+0x0/0x38That one is often followed by:
[ 29.833216] ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 [ 29.833723] WARNING: CPU: 0 PID: 365 at lib/debugobjects.c:502 debug_print_object+0xb8/0x100in the same code line, suggesting that the timer may be shut down more than once (?). Then there is another crash with
[ 29.863890] Insufficient stack space to handle exception! [ 29.863939] ESR: 0x0000000096000047 -- DABT (current EL) [ 29.863952] FAR: 0xffff800008b17f80 [ 29.863959] Task stack: [0xffff800008b18000..0xffff800008b1c000] [ 29.863967] IRQ stack: [0xffff800008000000..0xffff800008004000] [ 29.863975] Overflow stack: [0xffff30a35fe7a0f0..0xffff30a35fe7b0f0] [ 29.863987] CPU: 0 PID: 365 Comm: ip Tainted: G W N 6.1.0-rc2-00138-gced58c742836 #1 [ 29.863999] Hardware name: linux,dummy-virt (DT)followed by a sequence of
[ 29.864572] __try_to_del_timer_sync+0x40/0xdc [ 29.864582] __del_timer_sync+0xa4/0x100 [ 29.864590] timer_fixup_init+0x2c/0x5c [ 29.864599] __debug_object_init+0x248/0x53c [ 29.864606] debug_object_init+0x24/0x30 [ 29.864614] timer_fixup_init+0x40/0x5c [ 29.864622] __debug_object_init+0x248/0x53c [ 29.864630] debug_object_init+0x24/0x30 [ 29.864637] timer_fixup_init+0x40/0x5c [ 29.864645] __debug_object_init+0x248/0x53c [ 29.864658] debug_object_init+0x24/0x30 [ 29.864666] timer_fixup_init+0x40/0x5c ... [ 29.866492] debug_object_init+0x24/0x30 [ 29.866500] init_timer_key+0x7c/0x80 [ 29.866508] tulip_down+0x1d0/0x24c [ 29.866518] tulip_close+0x3c/0xc0Quoted 1 line
This crash is seen with variants on several platforms/architectures.
Hmm, I'll have to take a deeper look here.
Quoted 5 lines
---
WARNING: CPU: 0 PID: 8 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8 ODEBUG: free active (active state 0) object type: timer_list hint: addrconf_dad_work+0x0/0x628Hmm, I was afraid of this one.
Looks to be from the workqueue code for delayed work. I don't see a place that frees besides just flushing it. And we can't determine if a flush is permanent or will be armed again.
May need to add a helper function that resets the debugging so that it doesn't trigger on delayed work timers. There's a
DEBUG_OBJECTS_WORKthat handles bad work queue issues.Quoted 6 lines
---
mips64:WARNING: CPU: 0 PID: 280 at lib/debugobjects.c:502 debug_print_object+0xa4/0xd8ODEBUG: init active (active state 0) object type:
timer_listhint:0x0Not a very good hint.
Quoted 92 lines
Modules linked in: CPU: 0 PID: 280 Comm: ip Tainted: G W N 6.1.0-rc2-00138-gced58c742836 #1 Stack : 0000000000000056 ffffffffffffffff 0000000000000008 00b65275abba8126 00b65275abba8126 0000000000000000 9000000004193728 ffffffff80f29f38 ffffffff8105c9d8 ffffffffffff8880 9000000004193668 0000000000000000 0000000000000005 0000000000000010 ffffffff80c731a0 ffffffff81030000 2020205720202020 ffffffff81030000 0000000000000000 ffffffff80f29f38 0000000000000009 00000000000001f6 ffffffff8063d45c ffffffff81cb94c0 ffffffff81d76240 0000000000000000 ffffffff807575a8 0000000000000000 ffffffff81230000 9000000004190000 9000000004193720 9000000018002030 ffffffff80c8f528 0000000000000000 0000000000000000 00b65275abba8126 ffffffff8122e050 000000001000a4e1 ffffffff8010a884 00b65275abba8126 ... Call Trace: [<ffffffff8010a884>] show_stack+0x3c/0x120 [<ffffffff80c8f528>] dump_stack_lvl+0x4c/0x90 [<ffffffff80130e54>] __warn+0xdc/0x1c8 [<ffffffff80c75a08>] warn_slowpath_fmt+0x98/0xc4 [<ffffffff8063d45c>] debug_print_object+0xa4/0xd8 [<ffffffff8063e590>] __debug_object_init+0x2d0/0x670 [<ffffffff801b44b8>] timer_fixup_init+0x40/0x58 irq event stamp: 1537 hardirqs last enabled at (1549): [<ffffffff8018e67c>] __up_console_sem+0x9c/0xc8 hardirqs last disabled at (1560): [<ffffffff8018e658>] __up_console_sem+0x78/0xc8 softirqs last enabled at (1324): [<ffffffff80a572bc>] dev_deactivate_many+0x32c/0x458 softirqs last disabled at (1322): [<ffffffff80a57288>] dev_deactivate_many+0x2f8/0x458 ---[ end trace 0000000000000000 ]---=====================================
WARNING: bad unlock balance detected!
6.1.0-rc2-00138-gced58c742836 #1 Tainted: G W N
-------------------------------------
�������␈␂/-1 is trying to release lock (&obj_hash[i].lock) at:
[<ffffffff801b41e0>]__try_to_del_timer_sync+0x48/0x140but there are no more locks to release!other info that might help us debug this:
qemu-system-mips64: terminating on signal 15 from pid 2525787 (/bin/bash)---
Openrisc fails completely. Lots of the following, then boot stalls.WARNING: CPU: 0 PID: 88 at lib/debugobjects.c:502 debug_print_object+0xc0/0xe8 ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 Modules linked in: CPU: 0 PID: 88 Comm: udhcpc Not tainted 6.1.0-rc2-00138-gced58c742836 #1 Call trace: [<8eaa3f51>] dump_stack_lvl+0x44/0x80 [<4da6c5ef>] dump_stack+0x1c/0x2c [<1b9f58b7>] __warn+0xdc/0x118 [<86d4d066>] ? debug_print_object+0xc0/0xe8 [<689beae8>] warn_slowpath_fmt+0x78/0x90 [<86d4d066>] debug_print_object+0xc0/0xe8 [<872f4074>] __debug_object_init+0x2bc/0x7f4 [<c30d6214>] ? _raw_spin_unlock_irqrestore+0x50/0x84 [<c3f3be91>] ? debug_check_no_locks_freed+0xb8/0x194 [<558675c1>] ? slob_alloc+0xe8/0x350 [<26225550>] ? lockdep_init_map_type+0x68/0x38c [<c3f3be91>] ? debug_check_no_locks_freed+0xb8/0x194 [<2c9cf7b5>] ? inet_create+0x2e8/0x404 [<bab055df>] debug_object_init+0x30/0x40 [<6d2e468f>] init_timer_key+0xb4/0x110 [<60c0d3e7>] ? sk_init_common+0x1a0/0x1c0 [<b52686c2>] sock_init_data+0x60/0x2a4 [<1ff904c3>] ? sk_alloc+0xe8/0x138 [<ca3e6ef9>] inet_create+0x1e0/0x404 [<623d726a>] ? inet_create+0x80/0x404 [<a3256728>] ? lock_release+0x1c0/0x30c [<960a454e>] __sock_create+0x140/0x288 [<0290cd64>] ? __sock_create+0x98/0x288 [<b62bc649>] __sys_socket+0x7c/0x128 [<fa003224>] ? do_work_pending+0x4c/0x118 [<705147e9>] sys_socket+0x14/0x24 [<9c4e015d>] ? _syscall_return+0x0/0x4 ---[ end trace 0000000000000000 ]------
parisc crashes.[ 3.015186] ------------[ cut here ]------------ [ 3.015580] ODEBUG: init active (active state 0) object type: timer_list hint: timeout_waiting_on_port+0x0/0x2c [ 3.016945] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 debug_print_object+0x98/0xc8 ... [ 3.026117] ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 [ 3.026355] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 debug_print_object+0x98/0xc8...
[ many more of the same ][ 3.054542] stackcheck: swapper/0 will most likely overflow irq stack (sp:11e51800, stk bottom-top:11460004-11468004) [ 3.054592] Kernel panic - not syncing: low stack detected by irq handler - check messagesQuoted 7 lines
and boom.
---
usb:[ 23.993737] WARNING: CPU: 0 PID: 343 at lib/debugobjects.c:502 debug_print_object+0xac/0xc8 [ 23.993953] ODEBUG: free active (active state 0) object type: timer_list hint: hub_init_func2+0x0/0xcYeah, there's going to be lots of these.
Quoted 12 lines
---
ppc:[ 6.607478][ T1] ODEBUG: init active (active state 0) object type: timer_list hint: .ibmvscsi_timeout+0x0/0x58 [ 6.608536][ T1] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 .debug_print_object+0xbc/0xf0followed by several of
[ 6.623661][ T1] ODEBUG: init active (active state 0) object type: timer_list hint: 0x0 [ 6.624155][ T1] WARNING: CPU: 0 PID: 1 at lib/debugobjects.c:502 .debug_print_object+0xbc/0xf0Quoted 6 lines
and finally:
[ 6.694785][ C0] BUG: Kernel NULL pointer dereference on read at 0x000000b8 [ 6.694796][ C0] Faulting instruction address: 0xc0000000001ece44 [ 6.694807][ C0] Thread overran stack, or stack corrupted [ 6.694967][ C0] Oops: Kernel access of bad area, sig: 7 [#1]
Quoted 3 lines
Anyway, this is all an issue with the last patch that makes calling shutdown a requirement before freeing and not just a hint.
I can see this is a real problem. I see it all over the place. The latest is in drivers/scsi/ibmvscsi/ibmvscsi.c where, again, timer_setup() is called repeatedly and the timer is stopped with del_timer(), causing a crash. Replacing those calls with del_timer_shutdown() or, where this isn't possible, with del_timer_try_shutdown() fixes that problem.
Quoted 4 lines
You may want to test all but the last patch, as only the last patch is what's going to trigger all the above, as it modifies the semantics of
DEBUG_OBJECTS_TIMER.
I'll do that after running a test with the change below, to see if there are any other problems.
Quoted 9 lines
But for the tcp one, add this to the queue and see if the tcp one goes away?
-- Steve
kernel/time/timer.c +0
-0
kernel/time/timer.c int __del_timer(struct timer_list *timer, bool free) · line 1261
Quoted 8 lines
kernel/time/timer.c +1
-1@@ -1255,7 +1255,7 @@ 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 && ret) { +if (free) { timer->function =NULL ;debug_timer_deactivate (timer); }
Just noticed that one (too many patches floating around, sorry). Applied, and I'll test again.
Thanks,
Guenter
A message that is not in the archive (sent off-list or not yet received): [email protected]
Steven Rostedt
reviewerRe: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
Quoted 6 lines · On Fri, 28 Oct 2022 10:18:15 +0800 Hillf Danton <[email protected]> wrote:
On 27 Oct 2022 11:05:45 -0400 Steven Rostedt (Google) <[email protected]>
Quoted 5 lines
drivers/usb/core/hub.c +0
-0@@ -1261,6 +1261,9 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
Quoted 8 lines
drivers/usb/core/hub.c static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) · line 1266
Quoted 5 lines
drivers/usb/core/hub.c +3
-0/* Don't do a long sleep inside a workqueue routine */ if (type ==HUB_INIT2 ) { +/* Timers must be shutdown before they are re-initialized */ +if (hub->init_work.work.func) +del_timer_shutdown (&hub->init_work.timer);This is not needed in the workqueue callback as the timer in question is not pending.
This was added because of the updates to DEBUG_OBJECTS_TIMERS that changed it to require a shutdown to remove the activation of the timer. This is to detect the possibility that a timer may become active just before freeing (there's way too many bugs that show that code logic is not enough).
This code in particular is troubling because it re-initializes an already initialized timer with a new function. This causes the debug-objects to trigger an "object activated while initializing" warning.
I originally added the "shutdown" to deactivate the object before you re-initialize it. But I have since updated the code to keep track of if it was ever activated, and if so, not to call the init code again, so this may not be required anymore.
I'm still trying to work out the kinks as the users of timers have become adapted to the implementation, and may need to add some other helpers to make this work.
-- Steve
Quoted 4 lines
Quoted 4 lines
drivers/usb/core/hub.c +0
-0INIT_DELAYED_WORK (&hub->init_work, hub_init_func3); queue_delayed_work(system_power_efficient_wq, &hub->init_work,
Steven Rostedt
reviewerTrond,
I'm looking at a commit from 2005:
0f9dc2b16884b ("RPC: Clean up socket autodisconnect")
Cancel autodisconnect requests inside xprt_transmit() in order to avoid races.
Use more efficient del_singleshot_timer_sync()
I'm working on adding a "shutdown" state to timers, making it required for freeing the timer. This is to address the numerous bugs we hit where timers get rearmed just before freeing and then cause a crash in the timer code, without knowing what timer it was that caused it.
Having a specific shutdown state for timers will remove this problem because if something tries to rearm a shutdown timer, it will fail and a WARN_ON_ONCE() is triggered. See below in the "reply" part for a description of this effort.
The reason for this email, is because that WARN_ON_ONCE() triggered on the mod_timer() from:
static void xprt_schedule_autodisconnect(struct rpc_xprt *xprt)
__must_hold(&xprt->transport_lock)
{
xprt->last_used = jiffies;
if (RB_EMPTY_ROOT(&xprt->recv_queue) && xprt_has_timer(xprt))
mod_timer(&xprt->timer, xprt->last_used + xprt->idle_timeout);
}That's because xptr->timer was shutdown due to:
intxprt_request_enqueue_receive(struct rpc_task *task)
{
[..]
/* Turn off autodisconnect */
del_singleshot_timer_sync(&xprt->timer);
return 0;
}Now singleshot means just that. It's a single shot and calling del_singleshot_timer_sync() will shut it down so that it can be freed. That also means that it can no longer be re-armed.
I'm not sure what you meant by "Use more efficient del_singleshot_timer_sync()" but I'm guessing since that was written in 2005, it is no longer relevant, and del_timer_sync() should now be used.
After replacing that with del_timer_sync(), the warning goes away.
I just want to confirm that's OK with you.
Thanks,
-- Steve
Quoted 74 lines · On Thu, 27 Oct 2022 11:05:25 -0400 Steven Rostedt <[email protected]> wrote:
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->functionNULL as the flag that it is shutdown. I took that code, tested it (fixed it up), added more comments, and changed the name todel_timer_shutdown()as Linus had asked. I also converted it to useWARN_ON_ONCE()instead of justWARN_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 useddel_timer()instead ofdel_timer_sync()).I also updated
DEBUG_OBJECTS_TIMERSto check from where the timer is ever armed, to calling ofdel_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_TIMERSas 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/[email protected]/
Here's the original text of that version:
[
This is an RFC patch. As we hit a few bugs weredel_timer()is called instead ofdel_timer_sync()before the timer is freed, and there could be bugs where evendel_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.cas 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.cportion from this patch, and create a series of patches to use thedel_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 whendel_timer_sync()should have been.Add a
del_timer_free()that not only does adel_timer_sync()but will mark the timer as freed in case it gets rearmed, it will trigger aWARN_ON. Thedel_timer_free()is more likely to be used by developers that are about to free a timer, then usingdel_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.
Hi Steve,
Quoted 50 lines · Steven Rostedt
Trond,
I'm looking at a commit from 2005:
0f9dc2b16884b("RPC: Clean up socket autodisconnect")Cancel autodisconnect requests inside
xprt_transmit()in order to avoid races.
Use more efficientdel_singleshot_timer_sync()I'm working on adding a "shutdown" state to timers, making it required for freeing the timer. This is to address the numerous bugs we hit where timers get rearmed just before freeing and then cause a crash in the timer code, without knowing what timer it was that caused it.
Having a specific shutdown state for timers will remove this problem because if something tries to rearm a shutdown timer, it will fail and a
WARN_ON_ONCE()is triggered. See below in the "reply" part for a description of this effort.The reason for this email, is because that
WARN_ON_ONCE()triggered on themod_timer()from:static void
xprt_schedule_autodisconnect(struct rpc_xprt *xprt)__must_hold(&xprt->transport_lock)
{xprt->last_used= jiffies;
if (RB_EMPTY_ROOT(&xprt->recv_queue)&&xprt_has_timer(xprt))mod_timer(&xprt->timer, xprt->last_used + xprt->idle_timeout); }That's because
xptr->timerwas shutdown due to:int
xprt_request_enqueue_receive(struct rpc_task *task){
[..]
/* Turn off autodisconnect */del_singleshot_timer_sync(&xprt->timer); return 0;
}Now singleshot means just that. It's a single shot and calling
del_singleshot_timer_sync()will shut it down so that it can be freed. That also means that it can no longer be re-armed.
Quoted 7 lines
I'm not sure what you meant by "Use more efficient
del_singleshot_timer_sync()" but I'm guessing since that was written in 2005, it is no longer relevant, anddel_timer_sync()should now be used.After replacing that with
del_timer_sync(), the warning goes away.I just want to confirm that's OK with you.
I seem to vaguely remember that at the time, del_timer_sync() would loop in order to catch re-arming timers, whereas del_singleshot_timer_sync() would not, hence the commit message. The expectation for del_singleshot_timer_sync() was simply that the caller would ensure safety against re-arming, which was indeed the case for this code.
However if that del_singleshot_timer_sync() expectation has been strengthened to mean that you guarantee never to re-arm the timer at all, then I agree that we should switch to del_timer_sync().
Thanks!
Trond
_________________________________
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
Steven Rostedt
reviewerQuoted 5 lines · On Fri, 28 Oct 2022 20:12:30 +0000 Trond Myklebust <[email protected]>…
I seem to vaguely remember that at the time,
del_timer_sync()would loop in order to catch re-arming timers, whereasdel_singleshot_timer_sync()would not, hence the commit message. The expectation fordel_singleshot_timer_sync()was simply that the caller would ensure safety against re-arming, which was indeed the case for this code.
Well, that expectation didn't last long. Your commit was added on June 22, 2005. Then on June 23, 2005 (the next day!) this happened:
55c888d6d09a0 ("timers fixes/improvements")
Which has:
@@ -89,12 +77,12 @@ static inline void add_timer(struct timer_list * timer)
#ifdef CONFIG_SMP
extern int del_timer_sync(struct timer_list *timer);
- extern int del_singleshot_timer_sync(struct timer_list *timer); #else
# define del_timer_sync(t) del_timer(t) -# define del_singleshot_timer_sync(t) del_timer(t) #endif
+#define del_singleshot_timer_sync(t) del_timer_sync(t) +
So much or efficiency! :-)
I guess converting it back to del_timer_sync() is the right thing to do regardless of this patch series. I'll send you a patch.
-- Steve
Quoted 37 lines · Steven Rostedt
Quoted 5 lines · On Fri, 28 Oct 2022 20:12:30 +0000 Trond Myklebust <[email protected]>…
I seem to vaguely remember that at the time,
del_timer_sync()would loop in order to catch re-arming timers, whereasdel_singleshot_timer_sync()would not, hence the commit message. The expectation fordel_singleshot_timer_sync()was simply that the caller would ensure safety against re-arming, which was indeed the case for this code.Well, that expectation didn't last long. Your commit was added on June 22, 2005. Then on June 23, 2005 (the next day!) this happened:
55c888d6d09a0("timers fixes/improvements")Which has:
(quoted diff) +2
-2@@ -89,12 +77,12 @@ static inline void add_timer(struct timer_list * timer) #ifdef CONFIG_SMP extern int del_timer_sync(struct timer_list *timer); - extern int del_singleshot_timer_sync(struct timer_list *timer); #else # define del_timer_sync(t) del_timer(t) -# define del_singleshot_timer_sync(t) del_timer(t) #endif +#define del_singleshot_timer_sync(t) del_timer_sync(t) + So much or efficiency! :-)
Quoted 4 lines
I guess converting it back to
del_timer_sync()is the right thing to do regardless of this patch series. I'll send you a patch.-- Steve
We’re all moving a lot slower now, 17 years later...
_________________________________
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]