[patch 00/16] timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

[patch 00/16] timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

[Thomas Gleixner]

Timers on stack must be initialized with init_timer_on_stack() which
creates a tracking object if DEBUG_OBJECTS is enabled. Quite some
places miss the complementary destroy_timer_on_stack(), which frees the
tracking object. So we leak the tracking object in the debug objects
store.

While looking at that, one affected driver also used del_timer()
instead of del_timer_sync() before returning, which is wrong as well
as del_timer() meriliy dequeues an armed timer, but does not wait for
a callback which is executed on a different cpu.

So I started to look through del_timer() sites and fixed a bunch of
obvious teardown, module exit pathes which must use del_timer_sync().

As I'm about to travel, I gave up on reviewing all the usage sites of
del_timer().

Maybe Julia has an idea to automate the scan a bit. The paring for
init_timer_on_stack/destroy_timer_on_stack should be easy enough, but
the del_timer() one is pretty complex and definitly needs manual
auditing, but having a scan assistant to identify potential places
would be definitely helpful.

Thanks,

	tglx
---
 drivers/atm/firestream.c           |    2 +-
 drivers/atm/idt77105.c             |    6 +++---
 drivers/block/cpqarray.c           |    2 +-
 drivers/block/umem.c               |    2 +-
 drivers/block/xsysace.c            |    2 ++
 drivers/bluetooth/bluecard_cs.c    |    2 +-
 drivers/bluetooth/hci_bcsp.c       |    2 +-
 drivers/bluetooth/hci_h5.c         |    2 +-
 drivers/cpufreq/intel_pstate.c     |    2 +-
 drivers/input/serio/hp_sdc.c       |    2 +-
 drivers/s390/char/tape_std.c       |    3 ++-
 drivers/s390/net/lcs.c             |    1 +
 drivers/scsi/qla1280.c             |    1 +
 drivers/thermal/intel_powerclamp.c |    1 +
 kernel/rcu/torture.c               |    4 +++-
 15 files changed, 21 insertions(+), 13 deletions(-)

[patch 01/16] s390: tape: Use del_timer_sync()

[Thomas Gleixner]

[-- Attachment #1: s390-tape-use-del-timer-sync.patch --]
[-- Type: text/plain, Size: 846 bytes --]

del_timer() does not wait for a possible running callback to
complete. So the call side might free request and the associated
objects while on another cpu the timer handler runs.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: s390 <linux-s390@vger.kernel.org>
---
 drivers/s390/char/tape_std.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/s390/char/tape_std.c
===================================================================
--- tip.orig/drivers/s390/char/tape_std.c
+++ tip/drivers/s390/char/tape_std.c
@@ -78,7 +78,7 @@ tape_std_assign(struct tape_device *devi
 
 	rc = tape_do_io_interruptible(device, request);
 
-	del_timer(&timeout);
+	del_timer_sync(&timeout);
 
 	if (rc != 0) {
 		DBF_EVENT(3, "%08x: assign failed - device might be busy\n",

Re: [patch 01/16] s390: tape: Use del_timer_sync()

[Heiko Carstens]

On Sun, Mar 23, 2014 at 03:09:24PM -0000, Thomas Gleixner wrote:
> del_timer() does not wait for a possible running callback to
> complete. So the call side might free request and the associated
> objects while on another cpu the timer handler runs.

[...]

>  
> -	del_timer(&timeout);
> +	del_timer_sync(&timeout);

Applied, thanks.

[patch 03/16] s390: net: lcs: Add missing destroy_timer_on_stack()

[Thomas Gleixner]

[-- Attachment #1: s390-net-lcs-add-missing-destroy-timer-on-stack.patch --]
[-- Type: text/plain, Size: 718 bytes --]

Otherwise we leak a tracking object when DEBUG_OBJECTS is enabled.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Ursula Braun <ursula.braun@de.ibm.com>
Cc: s390 <linux-s390@vger.kernel.org>
---
 drivers/s390/net/lcs.c |    1 +
 1 file changed, 1 insertion(+)

Index: tip/drivers/s390/net/lcs.c
===================================================================
--- tip.orig/drivers/s390/net/lcs.c
+++ tip/drivers/s390/net/lcs.c
@@ -899,6 +899,7 @@ lcs_send_lancmd(struct lcs_card *card, s
 	add_timer(&timer);
 	wait_event(reply->wait_q, reply->received);
 	del_timer_sync(&timer);
+	destroy_timer_on_stack(&timer);
 	LCS_DBF_TEXT_(4, trace, "rc:%d",reply->rc);
 	rc = reply->rc;
 	lcs_put_reply(reply);

Re: [patch 03/16] s390: net: lcs: Add missing destroy_timer_on_stack()

[Heiko Carstens]

On Sun, Mar 23, 2014 at 03:09:25PM -0000, Thomas Gleixner wrote:
> Otherwise we leak a tracking object when DEBUG_OBJECTS is enabled.


>  drivers/s390/net/lcs.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: tip/drivers/s390/net/lcs.c
> ===================================================================
> --- tip.orig/drivers/s390/net/lcs.c
> +++ tip/drivers/s390/net/lcs.c
> @@ -899,6 +899,7 @@ lcs_send_lancmd(struct lcs_card *card, s
>  	add_timer(&timer);
>  	wait_event(reply->wait_q, reply->received);
>  	del_timer_sync(&timer);
> +	destroy_timer_on_stack(&timer);

Applied, thanks.

[patch 02/16] s390: tape: Add missing destroy_timer_on_stack()

[Thomas Gleixner]

[-- Attachment #1: s390-tape-add-missing-destroy-timer.patch --]
[-- Type: text/plain, Size: 729 bytes --]

Otherwise we leak a tracking object when DEBUG_OBJECTS is enabled.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: s390 <linux-s390@vger.kernel.org>
---
 drivers/s390/char/tape_std.c |    1 +
 1 file changed, 1 insertion(+)

Index: tip/drivers/s390/char/tape_std.c
===================================================================
--- tip.orig/drivers/s390/char/tape_std.c
+++ tip/drivers/s390/char/tape_std.c
@@ -79,6 +79,7 @@ tape_std_assign(struct tape_device *devi
 	rc = tape_do_io_interruptible(device, request);
 
 	del_timer_sync(&timeout);
+	destroy_timer_on_stack(&timeout);
 
 	if (rc != 0) {
 		DBF_EVENT(3, "%08x: assign failed - device might be busy\n",

Re: [patch 02/16] s390: tape: Add missing destroy_timer_on_stack()

[Heiko Carstens]

On Sun, Mar 23, 2014 at 03:09:25PM -0000, Thomas Gleixner wrote:
> Otherwise we leak a tracking object when DEBUG_OBJECTS is enabled.
> 

[...]

>  	del_timer_sync(&timeout);
> +	destroy_timer_on_stack(&timeout);

Applied, thanks.

[patch 04/16] scsi: qla1280: Add missing destroy_timer_on_stack()

[Thomas Gleixner]

[-- Attachment #1: scsi-qla1280-add-missing-destroy-timer-on-stack.patch --]
[-- Type: text/plain, Size: 718 bytes --]

Otherwise we leak a tracking object when DEBUG_OBJECTS is enabled.

This code should use wait_for_completion_timeout() instead, but that's
a different issue.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Michael Reed <mdr@sgi.com>
Cc: scsi <linux-scsi@vger.kernel.org>
---
 drivers/scsi/qla1280.c |    1 +
 1 file changed, 1 insertion(+)

Index: tip/drivers/scsi/qla1280.c
===================================================================
--- tip.orig/drivers/scsi/qla1280.c
+++ tip/drivers/scsi/qla1280.c
@@ -2514,6 +2514,7 @@ qla1280_mailbox_command(struct scsi_qla_
 
 	wait_for_completion(&wait);
 	del_timer_sync(&timer);
+	destroy_timer_on_stack(&timer);
 
 	spin_lock_irq(ha->host->host_lock);
 

[patch 05/16] thermal: powerclamp: Add missing destroy_timer_on_stack()

[Thomas Gleixner]

[-- Attachment #1: thermal-powerclamp-add-missing-destroy_timer_on_stack.patch --]
[-- Type: text/plain, Size: 652 bytes --]

Otherwise we leak a tracking object when DEBOG_OBJECTS is enabled.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/intel_powerclamp.c |    1 +
 1 file changed, 1 insertion(+)

Index: tip/drivers/thermal/intel_powerclamp.c
===================================================================
--- tip.orig/drivers/thermal/intel_powerclamp.c
+++ tip/drivers/thermal/intel_powerclamp.c
@@ -455,6 +455,7 @@ static int clamp_thread(void *arg)
 		preempt_enable();
 	}
 	del_timer_sync(&wakeup_timer);
+	destroy_timer_on_stack(&wakeup_timer);
 	clear_bit(cpunr, cpu_clamping_mask);
 
 	return 0;

[patch 06/16] rcu: torture: Add missing destroy_timer_on_stack()

[Thomas Gleixner]

[-- Attachment #1: rcu-torture-add-missing-destroy_timer_on_stack.patch --]
[-- Type: text/plain, Size: 925 bytes --]

Otherwise we leak a tracking object if DEBUG_OBJECTS is enabled.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Josh Triplett <josh@freedesktop.org>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
---
 kernel/rcu/torture.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: tip/kernel/rcu/torture.c
===================================================================
--- tip.orig/kernel/rcu/torture.c
+++ tip/kernel/rcu/torture.c
@@ -1038,8 +1038,10 @@ rcu_torture_reader(void *arg)
 	} while (!kthread_should_stop() && fullstop == FULLSTOP_DONTSTOP);
 	VERBOSE_PRINTK_STRING("rcu_torture_reader task stopping");
 	rcutorture_shutdown_absorb("rcu_torture_reader");
-	if (irqreader && cur_ops->irq_capable)
+	if (irqreader && cur_ops->irq_capable) {
 		del_timer_sync(&t);
+		destroy_timer_on_stack(&t);
+	}
 	while (!kthread_should_stop())
 		schedule_timeout_uninterruptible(1);
 	return 0;

Re: [patch 06/16] rcu: torture: Add missing destroy_timer_on_stack()

[Paul E. McKenney]

On Sun, Mar 23, 2014 at 03:09:27PM -0000, Thomas Gleixner wrote:
> Otherwise we leak a tracking object if DEBUG_OBJECTS is enabled.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Good catch!  Queued for 3.16.

							Thanx, Paul

> Cc: Josh Triplett <josh@freedesktop.org>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> ---
>  kernel/rcu/torture.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Index: tip/kernel/rcu/torture.c
> ===================================================================
> --- tip.orig/kernel/rcu/torture.c
> +++ tip/kernel/rcu/torture.c
> @@ -1038,8 +1038,10 @@ rcu_torture_reader(void *arg)
>  	} while (!kthread_should_stop() && fullstop == FULLSTOP_DONTSTOP);
>  	VERBOSE_PRINTK_STRING("rcu_torture_reader task stopping");
>  	rcutorture_shutdown_absorb("rcu_torture_reader");
> -	if (irqreader && cur_ops->irq_capable)
> +	if (irqreader && cur_ops->irq_capable) {
>  		del_timer_sync(&t);
> +		destroy_timer_on_stack(&t);
> +	}
>  	while (!kthread_should_stop())
>  		schedule_timeout_uninterruptible(1);
>  	return 0;
> 
> 

Re: [patch 06/16] rcu: torture: Add missing destroy_timer_on_stack()

[Josh Triplett]

On Sun, Mar 23, 2014 at 03:09:27PM -0000, Thomas Gleixner wrote:
> Otherwise we leak a tracking object if DEBUG_OBJECTS is enabled.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Josh Triplett <josh@freedesktop.org>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  kernel/rcu/torture.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Index: tip/kernel/rcu/torture.c
> ===================================================================
> --- tip.orig/kernel/rcu/torture.c
> +++ tip/kernel/rcu/torture.c
> @@ -1038,8 +1038,10 @@ rcu_torture_reader(void *arg)
>  	} while (!kthread_should_stop() && fullstop == FULLSTOP_DONTSTOP);
>  	VERBOSE_PRINTK_STRING("rcu_torture_reader task stopping");
>  	rcutorture_shutdown_absorb("rcu_torture_reader");
> -	if (irqreader && cur_ops->irq_capable)
> +	if (irqreader && cur_ops->irq_capable) {
>  		del_timer_sync(&t);
> +		destroy_timer_on_stack(&t);
> +	}
>  	while (!kthread_should_stop())
>  		schedule_timeout_uninterruptible(1);
>  	return 0;
> 
> 

[patch 08/16] atm: idt77105: Use del_timer_sync() in exit path

[Thomas Gleixner]

[-- Attachment #1: atm-idt77x-use-del-timer-sync.patch --]
[-- Type: text/plain, Size: 860 bytes --]

The module is about to go away. Make sure everything is stopped safely
before we pull the plug.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Chas Williams <chas@cmf.nrl.navy.mil>
Cc: atm <linux-atm-general@lists.sourceforge.net>
Cc: netdev <netdev@vger.kernel.org>
---
 drivers/atm/idt77105.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: tip/drivers/atm/idt77105.c
===================================================================
--- tip.orig/drivers/atm/idt77105.c
+++ tip/drivers/atm/idt77105.c
@@ -368,9 +368,9 @@ EXPORT_SYMBOL(idt77105_init);
 
 static void __exit idt77105_exit(void)
 {
-        /* turn off timers */
-        del_timer(&stats_timer);
-        del_timer(&restart_timer);
+	/* turn off timers */
+	del_timer_sync(&stats_timer);
+	del_timer_sync(&restart_timer);
 }
 
 module_exit(idt77105_exit);

Re: [patch 08/16] atm: idt77105: Use del_timer_sync() in exit path

[David Miller]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Sun, 23 Mar 2014 15:09:28 -0000

> The module is about to go away. Make sure everything is stopped safely
> before we pull the plug.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Applied.

[patch 07/16] atm: firestream: Use del_timer_sync() in teardown path

[Thomas Gleixner]

[-- Attachment #1: atm-firestream-use-del-timer-sync.patch --]
[-- Type: text/plain, Size: 879 bytes --]

The device is about to vanish. So we need to make sure that the timer
is completely stopped and the callback is not running on another CPU.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Chas Williams <chas@cmf.nrl.navy.mil>
Cc: atm <linux-atm-general@lists.sourceforge.net>
Cc: netdev <netdev@vger.kernel.org>
---
 drivers/atm/firestream.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/atm/firestream.c
===================================================================
--- tip.orig/drivers/atm/firestream.c
+++ tip/drivers/atm/firestream.c
@@ -2000,7 +2000,7 @@ static void firestream_remove_one(struct
 
 		fs_dprintk (FS_DEBUG_CLEANUP, "Freeing irq%d.\n", dev->irq);
 		free_irq (dev->irq, dev);
-		del_timer (&dev->timer);
+		del_timer_sync (&dev->timer);
 
 		atm_dev_deregister(dev->atm_dev);
 		free_queue (dev, &dev->hp_txq);

Re: [patch 07/16] atm: firestream: Use del_timer_sync() in teardown path

[David Miller]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Sun, 23 Mar 2014 15:09:28 -0000

> The device is about to vanish. So we need to make sure that the timer
> is completely stopped and the callback is not running on another CPU.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Applied.

[patch 10/16] block: umem: Use del_timer_sync() in exit path

[Thomas Gleixner]

[-- Attachment #1: block-umem-use-del_timer_sync.patch --]
[-- Type: text/plain, Size: 574 bytes --]

Make sure no callback is running before removing the module.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Kent Overstreet <kmo@daterainc.com>
---
 drivers/block/umem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/block/umem.c
===================================================================
--- tip.orig/drivers/block/umem.c
+++ tip/drivers/block/umem.c
@@ -746,7 +746,7 @@ static void init_battery_timer(void)
 
 static void del_battery_timer(void)
 {
-	del_timer(&battery_timer);
+	del_timer_sync(&battery_timer);
 }
 
 /*

[patch 09/16] block: cpqarray: Use del_timer_sync() in teardown

[Thomas Gleixner]

[-- Attachment #1: block-cpqarray-use-del_timer_sync.patch --]
[-- Type: text/plain, Size: 925 bytes --]

The data structure which contains the timer is about to be freed. Make
sure the timer is completely stopped and no callback running anymore.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Chirag Kantharia <chirag.kantharia@hp.com>
Cc: hpiss <iss_storagedev@hp.com>
---
 drivers/block/cpqarray.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/block/cpqarray.c
===================================================================
--- tip.orig/drivers/block/cpqarray.c
+++ tip/drivers/block/cpqarray.c
@@ -336,7 +336,7 @@ static void cpqarray_remove_one(int i)
 	free_irq(hba[i]->intr, hba[i]);
 	iounmap(hba[i]->vaddr);
 	unregister_blkdev(COMPAQ_SMART2_MAJOR+i, hba[i]->devname);
-	del_timer(&hba[i]->timer);
+	del_timer_sync(&hba[i]->timer);
 	remove_proc_entry(hba[i]->devname, proc_array);
 	pci_free_consistent(hba[i]->pci_dev,
 			NR_CMDS * sizeof(cmdlist_t), (hba[i]->cmd_pool),

[patch 11/16] block: xsysace; Use del_timer_sync() in exit path

[Thomas Gleixner]

[-- Attachment #1: block-xsysace-teardown-timer-on-exit.patch --]
[-- Type: text/plain, Size: 622 bytes --]

The data structure which contains the timer is about to be freed. Make
sure no callback is running.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Grant Likely <grant.likely@linaro.org>
---
 drivers/block/xsysace.c |    2 ++
 1 file changed, 2 insertions(+)

Index: tip/drivers/block/xsysace.c
===================================================================
--- tip.orig/drivers/block/xsysace.c
+++ tip/drivers/block/xsysace.c
@@ -1088,6 +1088,8 @@ static void ace_teardown(struct ace_devi
 	if (ace->irq)
 		free_irq(ace->irq, ace);
 
+	del_timer_sync(&ace->stall_timer);
+
 	iounmap(ace->baseaddr);
 }
 

[patch 12/16] bt: bluecard: Use del_timer_sync() in teardown path

[Thomas Gleixner]

[-- Attachment #1: bt-bluecard-use-del_timer_sync.patch --]
[-- Type: text/plain, Size: 710 bytes --]

Make sure no timer callback is running before releasing the
datastructure which contains it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: bt <linux-bluetooth@vger.kernel.org>
---
 drivers/bluetooth/bluecard_cs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/bluetooth/bluecard_cs.c
===================================================================
--- tip.orig/drivers/bluetooth/bluecard_cs.c
+++ tip/drivers/bluetooth/bluecard_cs.c
@@ -898,7 +898,7 @@ static void bluecard_release(struct pcmc
 
 	bluecard_close(info);
 
-	del_timer(&(info->timer));
+	del_timer_sync(&(info->timer));
 
 	pcmcia_disable_device(link);
 }

[patch 12/16] bt: bluecard: Use del_timer_sync() in teardown path

[Thomas Gleixner]

Make sure no timer callback is running before releasing the
datastructure which contains it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: bt <linux-bluetooth@vger.kernel.org>
---
 drivers/bluetooth/bluecard_cs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/bluetooth/bluecard_cs.c
===================================================================
--- tip.orig/drivers/bluetooth/bluecard_cs.c
+++ tip/drivers/bluetooth/bluecard_cs.c
@@ -898,7 +898,7 @@ static void bluecard_release(struct pcmc
 
 	bluecard_close(info);
 
-	del_timer(&(info->timer));
+	del_timer_sync(&(info->timer));
 
 	pcmcia_disable_device(link);
 }

Re: [patch 12/16] bt: bluecard: Use del_timer_sync() in teardown path

[Marcel Holtmann]

Hi Thomas,

> Make sure no timer callback is running before releasing the
> datastructure which contains it.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: bt <linux-bluetooth@vger.kernel.org>
> ---
> drivers/bluetooth/bluecard_cs.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[patch 13/16] bt: hci-bcsp: Use del_timer_sync() in teardown path

[Thomas Gleixner]

[-- Attachment #1: bt-hci-bcsp-use-del_timer_sync.patch --]
[-- Type: text/plain, Size: 754 bytes --]

Make sure no timer callback is running before freeing the
datastructure which contains it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: bt <linux-bluetooth@vger.kernel.org>
---
 drivers/bluetooth/hci_bcsp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/bluetooth/hci_bcsp.c
===================================================================
--- tip.orig/drivers/bluetooth/hci_bcsp.c
+++ tip/drivers/bluetooth/hci_bcsp.c
@@ -722,7 +722,7 @@ static int bcsp_close(struct hci_uart *h
 	skb_queue_purge(&bcsp->unack);
 	skb_queue_purge(&bcsp->rel);
 	skb_queue_purge(&bcsp->unrel);
-	del_timer(&bcsp->tbcsp);
+	del_timer_sync(&bcsp->tbcsp);
 
 	kfree(bcsp);
 	return 0;

[patch 13/16] bt: hci-bcsp: Use del_timer_sync() in teardown path

[Thomas Gleixner]

Make sure no timer callback is running before freeing the
datastructure which contains it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: bt <linux-bluetooth@vger.kernel.org>
---
 drivers/bluetooth/hci_bcsp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/bluetooth/hci_bcsp.c
===================================================================
--- tip.orig/drivers/bluetooth/hci_bcsp.c
+++ tip/drivers/bluetooth/hci_bcsp.c
@@ -722,7 +722,7 @@ static int bcsp_close(struct hci_uart *h
 	skb_queue_purge(&bcsp->unack);
 	skb_queue_purge(&bcsp->rel);
 	skb_queue_purge(&bcsp->unrel);
-	del_timer(&bcsp->tbcsp);
+	del_timer_sync(&bcsp->tbcsp);
 
 	kfree(bcsp);
 	return 0;

Re: [patch 13/16] bt: hci-bcsp: Use del_timer_sync() in teardown path

[Marcel Holtmann]

Hi Thomas,

> Make sure no timer callback is running before freeing the
> datastructure which contains it.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: bt <linux-bluetooth@vger.kernel.org>
> ---
> drivers/bluetooth/hci_bcsp.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

we already fixed something similar in bluetooth-next tree. I think your patch is no longer needed.

Regards

Marcel

[patch 14/16] bt: hci-h5: Use del_timer_sync() in exit path

[Thomas Gleixner]

[-- Attachment #1: bt-hci-h5-use-del_timer_sync.patch --]
[-- Type: text/plain, Size: 697 bytes --]

Make sure no timer callback is running before releasing the
datastructure which contains it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: bt <linux-bluetooth@vger.kernel.org>
---
 drivers/bluetooth/hci_h5.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/bluetooth/hci_h5.c
===================================================================
--- tip.orig/drivers/bluetooth/hci_h5.c
+++ tip/drivers/bluetooth/hci_h5.c
@@ -210,7 +210,7 @@ static int h5_close(struct hci_uart *hu)
 	skb_queue_purge(&h5->rel);
 	skb_queue_purge(&h5->unrel);
 
-	del_timer(&h5->timer);
+	del_timer_sync(&h5->timer);
 
 	kfree(h5);
 

[patch 14/16] bt: hci-h5: Use del_timer_sync() in exit path

[Thomas Gleixner]

Make sure no timer callback is running before releasing the
datastructure which contains it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: bt <linux-bluetooth@vger.kernel.org>
---
 drivers/bluetooth/hci_h5.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/bluetooth/hci_h5.c
===================================================================
--- tip.orig/drivers/bluetooth/hci_h5.c
+++ tip/drivers/bluetooth/hci_h5.c
@@ -210,7 +210,7 @@ static int h5_close(struct hci_uart *hu)
 	skb_queue_purge(&h5->rel);
 	skb_queue_purge(&h5->unrel);
 
-	del_timer(&h5->timer);
+	del_timer_sync(&h5->timer);
 
 	kfree(h5);
 

Re: [patch 14/16] bt: hci-h5: Use del_timer_sync() in exit path

[Marcel Holtmann]

Hi Thomas,

> Make sure no timer callback is running before releasing the
> datastructure which contains it.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: bt <linux-bluetooth@vger.kernel.org>
> ---
> drivers/bluetooth/hci_h5.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

we already fixed something similar in bluetooth-next tree. I think your patch is no longer needed.

Regards

Marcel

[patch 15/16] cpufreq: intel-pstate: Use del_timer_sync in intel_pstate_cpu_exit()

[Thomas Gleixner]

[-- Attachment #1: cpufreq-intel-pstate-use-del_timer_sync.patch --]
[-- Type: text/plain, Size: 1154 bytes --]

We are about to free the data structure. Make sure no timer callback
is running. I might be paranoid, but the ->exit callback can be
invoked from so many places, that it is not entirely clear whether
del_timer is always called on the cpu on which it is enqueued.

While looking through the call sites I noticed, that
cpufreq_init_policy() can fail and invoke cpufreq_driver->exit() but
it does not return the failure and the callsite happily proceeds.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: cpufreq <cpufreq@vger.kernel.org>
Cc: pm <linux-pm@vger.kernel.org>
---

 drivers/cpufreq/intel_pstate.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/cpufreq/intel_pstate.c
===================================================================
--- tip.orig/drivers/cpufreq/intel_pstate.c
+++ tip/drivers/cpufreq/intel_pstate.c
@@ -777,7 +777,7 @@ static int intel_pstate_cpu_exit(struct
 {
 	int cpu = policy->cpu;
 
-	del_timer(&all_cpu_data[cpu]->timer);
+	del_timer_sync(&all_cpu_data[cpu]->timer);
 	kfree(all_cpu_data[cpu]);
 	all_cpu_data[cpu] = NULL;
 	return 0;

Re: [patch 15/16] cpufreq: intel-pstate: Use del_timer_sync in intel_pstate_cpu_exit()

[Rafael J. Wysocki]

On Sunday, March 23, 2014 03:09:32 PM Thomas Gleixner wrote:
> We are about to free the data structure. Make sure no timer callback
> is running. I might be paranoid, but the ->exit callback can be
> invoked from so many places, that it is not entirely clear whether
> del_timer is always called on the cpu on which it is enqueued.
> 
> While looking through the call sites I noticed, that
> cpufreq_init_policy() can fail and invoke cpufreq_driver->exit() but
> it does not return the failure and the callsite happily proceeds.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: cpufreq <cpufreq@vger.kernel.org>
> Cc: pm <linux-pm@vger.kernel.org>

Dirk?

> ---
> 
>  drivers/cpufreq/intel_pstate.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: tip/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- tip.orig/drivers/cpufreq/intel_pstate.c
> +++ tip/drivers/cpufreq/intel_pstate.c
> @@ -777,7 +777,7 @@ static int intel_pstate_cpu_exit(struct
>  {
>  	int cpu = policy->cpu;
>  
> -	del_timer(&all_cpu_data[cpu]->timer);
> +	del_timer_sync(&all_cpu_data[cpu]->timer);
>  	kfree(all_cpu_data[cpu]);
>  	all_cpu_data[cpu] = NULL;
>  	return 0;
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [patch 15/16] cpufreq: intel-pstate: Use del_timer_sync in intel_pstate_cpu_exit()

[Dirk Brandewie]

Hi Thomas,
On 03/23/2014 06:56 PM, Rafael J. Wysocki wrote:
> On Sunday, March 23, 2014 03:09:32 PM Thomas Gleixner wrote:
>> We are about to free the data structure. Make sure no timer callback
>> is running. I might be paranoid, but the ->exit callback can be
>> invoked from so many places, that it is not entirely clear whether
>> del_timer is always called on the cpu on which it is enqueued.
>>
>> While looking through the call sites I noticed, that
>> cpufreq_init_policy() can fail and invoke cpufreq_driver->exit() but
>> it does not return the failure and the callsite happily proceeds.
>>

The call to del_timer() has been moved to a new callback in material
in Rafaels pull request for v3.15.

I will send a patch adding this change to the v3.15 material.

--Dirk
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: cpufreq <cpufreq@vger.kernel.org>
>> Cc: pm <linux-pm@vger.kernel.org>
>
> Dirk?
>
>> ---
>>
>>   drivers/cpufreq/intel_pstate.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Index: tip/drivers/cpufreq/intel_pstate.c
>> ===================================================================
>> --- tip.orig/drivers/cpufreq/intel_pstate.c
>> +++ tip/drivers/cpufreq/intel_pstate.c
>> @@ -777,7 +777,7 @@ static int intel_pstate_cpu_exit(struct
>>   {
>>   	int cpu = policy->cpu;
>>
>> -	del_timer(&all_cpu_data[cpu]->timer);
>> +	del_timer_sync(&all_cpu_data[cpu]->timer);
>>   	kfree(all_cpu_data[cpu]);
>>   	all_cpu_data[cpu] = NULL;
>>   	return 0;
>>
>>
>

Re: [patch 15/16] cpufreq: intel-pstate: Use del_timer_sync in intel_pstate_cpu_exit()

[Viresh Kumar]

On Sun, Mar 23, 2014 at 8:39 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> We are about to free the data structure. Make sure no timer callback
> is running. I might be paranoid, but the ->exit callback can be
> invoked from so many places, that it is not entirely clear whether
> del_timer is always called on the cpu on which it is enqueued.

In normal cases it will be called only when CPU goes offline and I
hope that isn't called on the dying CPU.

> While looking through the call sites I noticed, that
> cpufreq_init_policy() can fail and invoke cpufreq_driver->exit() but
> it does not return the failure and the callsite happily proceeds.

Thanks for reporting this, will get it fixed.

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: cpufreq <cpufreq@vger.kernel.org>
> Cc: pm <linux-pm@vger.kernel.org>
> ---
>
>  drivers/cpufreq/intel_pstate.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: tip/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- tip.orig/drivers/cpufreq/intel_pstate.c
> +++ tip/drivers/cpufreq/intel_pstate.c
> @@ -777,7 +777,7 @@ static int intel_pstate_cpu_exit(struct
>  {
>         int cpu = policy->cpu;
>
> -       del_timer(&all_cpu_data[cpu]->timer);
> +       del_timer_sync(&all_cpu_data[cpu]->timer);

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

[patch 16/16] input: serio: hp_sdc: Use del_timer_sync() in exit path

[Thomas Gleixner]

[-- Attachment #1: input-serio-hp_sdc-use-del_timer_sync.patch --]
[-- Type: text/plain, Size: 707 bytes --]

Make sure that no callback is running before we teardown the module.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: input <linux-input@vger.kernel.org>
---
 drivers/input/serio/hp_sdc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: tip/drivers/input/serio/hp_sdc.c
===================================================================
--- tip.orig/drivers/input/serio/hp_sdc.c
+++ tip/drivers/input/serio/hp_sdc.c
@@ -984,7 +984,7 @@ static void hp_sdc_exit(void)
 	free_irq(hp_sdc.irq, &hp_sdc);
 	write_unlock_irq(&hp_sdc.lock);
 
-	del_timer(&hp_sdc.kicker);
+	del_timer_sync(&hp_sdc.kicker);
 
 	tasklet_kill(&hp_sdc.task);
 

Re: [patch 16/16] input: serio: hp_sdc: Use del_timer_sync() in exit path

[Dmitry Torokhov]

On Sun, Mar 23, 2014 at 03:09:33PM -0000, Thomas Gleixner wrote:
> Make sure that no callback is running before we teardown the module.
> 

Applied, thank you Thomas.

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: input <linux-input@vger.kernel.org>
> ---
>  drivers/input/serio/hp_sdc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: tip/drivers/input/serio/hp_sdc.c
> ===================================================================
> --- tip.orig/drivers/input/serio/hp_sdc.c
> +++ tip/drivers/input/serio/hp_sdc.c
> @@ -984,7 +984,7 @@ static void hp_sdc_exit(void)
>  	free_irq(hp_sdc.irq, &hp_sdc);
>  	write_unlock_irq(&hp_sdc.lock);
>  
> -	del_timer(&hp_sdc.kicker);
> +	del_timer_sync(&hp_sdc.kicker);
>  
>  	tasklet_kill(&hp_sdc.task);
>  
> 
> 

-- 
Dmitry

Re: [patch 00/16] timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

[Julia Lawall]

As far as I could tell, (part of) the issue is that any kind of exit or 
close function should use del_timer_sync, because they could be called 
from a different CPU than the one that started up the timer?

Here is a semantic patch that takes care of the case of simple module_exit 
functions:

@r@
declarer name module_exit;
identifier ex;
@@

module_exit(ex);

@@
identifier r.ex;
@@

ex(...) {
  <...
- del_timer
+ del_timer_sync
    (...)
  ...>
}

The transformations it makes are below.  I haven't had a chance to check 
which results overlap with what Thomas has already sent, but I could look 
into it if this is the right idea.  I guess other kinds of close/exit 
functions would have to be identified manually, to make more rules.

In what circumstance can one be sure that two instructions are executed on 
the same CPU?

julia

diff -u -p a/arch/mips/lasat/picvue_proc.c b/arch/mips/lasat/picvue_proc.c
--- a/arch/mips/lasat/picvue_proc.c
+++ b/arch/mips/lasat/picvue_proc.c
@@ -175,7 +175,7 @@ static void pvc_proc_cleanup(void)
 	remove_proc_entry("scroll", pvc_display_dir);
 	remove_proc_entry(DISPLAY_DIR_NAME, NULL);
 
-	del_timer(&timer);
+	del_timer_sync(&timer);
 }
 
 static int __init pvc_proc_init(void)
diff -u -p a/drivers/tty/serial/mux.c b/drivers/tty/serial/mux.c
--- a/drivers/tty/serial/mux.c
+++ b/drivers/tty/serial/mux.c
@@ -613,7 +613,7 @@ static void __exit mux_exit(void)
 {
 	/* Delete the Mux timer. */
 	if(port_cnt > 0) {
-		del_timer(&mux_timer);
+		del_timer_sync(&mux_timer);
 #ifdef CONFIG_SERIAL_MUX_CONSOLE
 		unregister_console(&mux_console);
 #endif
diff -u -p a/drivers/input/serio/hp_sdc.c b/drivers/input/serio/hp_sdc.c
--- a/drivers/input/serio/hp_sdc.c
+++ b/drivers/input/serio/hp_sdc.c
@@ -984,7 +984,7 @@ static void hp_sdc_exit(void)
 	free_irq(hp_sdc.irq, &hp_sdc);
 	write_unlock_irq(&hp_sdc.lock);
 
-	del_timer(&hp_sdc.kicker);
+	del_timer_sync(&hp_sdc.kicker);
 
 	tasklet_kill(&hp_sdc.task);
 
diff -u -p a/drivers/isdn/sc/init.c b/drivers/isdn/sc/init.c
--- a/drivers/isdn/sc/init.c
+++ b/drivers/isdn/sc/init.c
@@ -390,8 +390,8 @@ static void __exit sc_exit(void)
 		/*
 		 * kill the timers
 		 */
-		del_timer(&(sc_adapter[i]->reset_timer));
-		del_timer(&(sc_adapter[i]->stat_timer));
+		del_timer_sync(&(sc_adapter[i]->reset_timer));
+		del_timer_sync(&(sc_adapter[i]->stat_timer));
 
 		/*
 		 * Tell I4L we're toast
diff -u -p a/drivers/isdn/i4l/isdn_common.c b/drivers/isdn/i4l/isdn_common.c
--- a/drivers/isdn/i4l/isdn_common.c
+++ b/drivers/isdn/i4l/isdn_common.c
@@ -2381,7 +2381,7 @@ static void __exit isdn_exit(void)
 	}
 	isdn_tty_exit();
 	unregister_chrdev(ISDN_MAJOR, "isdn");
-	del_timer(&dev->timer);
+	del_timer_sync(&dev->timer);
 	/* call vfree with interrupts enabled, else it will hang */
 	vfree(dev);
 	printk(KERN_NOTICE "ISDN-subsystem unloaded\n");
diff -u -p a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
--- a/drivers/isdn/hardware/mISDN/hfcpci.c
+++ b/drivers/isdn/hardware/mISDN/hfcpci.c
@@ -2352,7 +2352,7 @@ static void __exit
 HFC_cleanup(void)
 {
 	if (timer_pending(&hfc_tl))
-		del_timer(&hfc_tl);
+		del_timer_sync(&hfc_tl);
 
 	pci_unregister_driver(&hfc_driver);
 }
diff -u -p a/drivers/isdn/act2000/module.c b/drivers/isdn/act2000/module.c
--- a/drivers/isdn/act2000/module.c
+++ b/drivers/isdn/act2000/module.c
@@ -796,7 +796,7 @@ static void __exit act2000_exit(void)
 	act2000_card *last;
 	while (card) {
 		unregister_card(card);
-		del_timer(&card->ptimer);
+		del_timer_sync(&card->ptimer);
 		card = card->next;
 	}
 	card = cards;
diff -u -p a/drivers/net/hamradio/yam.c b/drivers/net/hamradio/yam.c
--- a/drivers/net/hamradio/yam.c
+++ b/drivers/net/hamradio/yam.c
@@ -1184,7 +1184,7 @@ static void __exit yam_cleanup_driver(vo
 	struct yam_mcs *p;
 	int i;
 
-	del_timer(&yam_timer);
+	del_timer_sync(&yam_timer);
 	for (i = 0; i < NR_PORTS; i++) {
 		struct net_device *dev = yam_devs[i];
 		if (dev) {
diff -u -p a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -2218,7 +2218,7 @@ static void __exit speakup_exit(void)
 	unregister_keyboard_notifier(&keyboard_notifier_block);
 	unregister_vt_notifier(&vt_notifier_block);
 	speakup_unregister_devsynth();
-	del_timer(&cursor_timer);
+	del_timer_sync(&cursor_timer);
 	kthread_stop(speakup_task);
 	speakup_task = NULL;
 	mutex_lock(&spk_mutex);
diff -u -p a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2298,7 +2298,7 @@ static void __exit panel_cleanup_module(
 	unregister_reboot_notifier(&panel_notifier);
 
 	if (scan_timer.function != NULL)
-		del_timer(&scan_timer);
+		del_timer_sync(&scan_timer);
 
 	if (pprt != NULL) {
 		if (keypad_enabled) {
diff -u -p a/drivers/staging/lustre/lustre/llite/super25.c b/drivers/staging/lustre/lustre/llite/super25.c
--- a/drivers/staging/lustre/lustre/llite/super25.c
+++ b/drivers/staging/lustre/lustre/llite/super25.c
@@ -197,7 +197,7 @@ static void __exit exit_lustre_lite(void
 {
 	ll_xattr_fini();
 	vvp_global_fini();
-	del_timer(&ll_capa_timer);
+	del_timer_sync(&ll_capa_timer);
 	ll_capa_thread_stop();
 	LASSERTF(capa_count[CAPA_SITE_CLIENT] == 0,
 		 "client remaining capa count %d\n",
diff -u -p a/drivers/atm/idt77105.c b/drivers/atm/idt77105.c
--- a/drivers/atm/idt77105.c
+++ b/drivers/atm/idt77105.c
@@ -369,8 +369,8 @@ EXPORT_SYMBOL(idt77105_init);
 static void __exit idt77105_exit(void)
 {
         /* turn off timers */
-        del_timer(&stats_timer);
-        del_timer(&restart_timer);
+        del_timer_sync(&stats_timer);
+        del_timer_sync(&restart_timer);
 }
 
 module_exit(idt77105_exit);
diff -u -p a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
--- a/drivers/atm/nicstar.c
+++ b/drivers/atm/nicstar.c
@@ -306,7 +306,7 @@ static void __exit nicstar_cleanup(void)
 {
 	XPRINTK("nicstar: nicstar_cleanup() called.\n");
 
-	del_timer(&ns_timer);
+	del_timer_sync(&ns_timer);
 
 	pci_unregister_driver(&nicstar_driver);
 
diff -u -p a/drivers/atm/iphase.c b/drivers/atm/iphase.c
--- a/drivers/atm/iphase.c
+++ b/drivers/atm/iphase.c
@@ -3289,7 +3289,7 @@ static void __exit ia_module_exit(void)
 {
 	pci_unregister_driver(&ia_driver);
 
-        del_timer(&ia_timer);
+        del_timer_sync(&ia_timer);
 }
 
 module_init(ia_module_init);
diff -u -p a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
--- a/net/hsr/hsr_main.c
+++ b/net/hsr/hsr_main.c
@@ -459,7 +459,7 @@ static int __init hsr_init(void)
 static void __exit hsr_exit(void)
 {
 	unregister_netdevice_notifier(&hsr_nb);
-	del_timer(&prune_timer);
+	del_timer_sync(&prune_timer);
 	hsr_netlink_exit();
 	dev_remove_pack(&hsr_pt);
 }
diff -u -p a/net/atm/mpc.c b/net/atm/mpc.c
--- a/net/atm/mpc.c
+++ b/net/atm/mpc.c
@@ -1492,7 +1492,7 @@ static void __exit atm_mpoa_cleanup(void
 
 	mpc_proc_clean();
 
-	del_timer(&mpc_timer);
+	del_timer_sync(&mpc_timer);
 	unregister_netdevice_notifier(&mpoa_notifier);
 	deregister_atm_ioctl(&atm_ioctl_ops);
 

Re: [patch 00/16] timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

[Thomas Gleixner]

On Sun, 23 Mar 2014, Julia Lawall wrote:
> As far as I could tell, (part of) the issue is that any kind of exit or 
> close function should use del_timer_sync, because they could be called 
> from a different CPU than the one that started up the timer?
> 
> Here is a semantic patch that takes care of the case of simple module_exit 
> functions:
> 
> @r@
> declarer name module_exit;
> identifier ex;
> @@
> 
> module_exit(ex);
> 
> @@
> identifier r.ex;
> @@
> 
> ex(...) {
>   <...
> - del_timer
> + del_timer_sync
>     (...)
>   ...>
> }
> 
> The transformations it makes are below.  I haven't had a chance to check 
> which results overlap with what Thomas has already sent, but I could look 

Minimal overlap, but as I said I did just a few conversions.

> into it if this is the right idea.  I guess other kinds of close/exit 
> functions would have to be identified manually, to make more rules.

If you look through the examples I sent, you'll find the close()
callbacks of various devices. So everything which is a function
pointer to a ops->close(), ops->remove(), ops_>teardown() is dangerous
if using del_timer(). There are a few exceptions, but....

Another thing I saw is 

	del_timer(&bla->timer);
	....
	kfree(&bla);

That's also a potential source of trouble. You can't tell without
analyzing the code, whether it's a problem or not. But making the
responsible people to look at it is definitely a good thing.
 
> In what circumstance can one be sure that two instructions are executed on 
> the same CPU?

If interrupts or preemption are disabled. But that's not the issue at
hand.

The del_timer vs. del_timer_sync problem is:

CPU0	      	  		 CPU1

add_timer(&bla->timer);

				 close(bla)
timer expires		   	   del_timer(&bla->timer);
  callback is invoked
				   kfree(bla);
    derefernces bla

I'll look at your findings on Tuesday, but feel free to send them to
the relevant maintainers for review.

Thanks,

	tglx

Re: [patch 00/16] timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

[Julia Lawall]

On Mon, 24 Mar 2014, Thomas Gleixner wrote:

> On Sun, 23 Mar 2014, Julia Lawall wrote:
> > As far as I could tell, (part of) the issue is that any kind of exit or 
> > close function should use del_timer_sync, because they could be called 
> > from a different CPU than the one that started up the timer?
> > 
> > Here is a semantic patch that takes care of the case of simple module_exit 
> > functions:
> > 
> > @r@
> > declarer name module_exit;
> > identifier ex;
> > @@
> > 
> > module_exit(ex);
> > 
> > @@
> > identifier r.ex;
> > @@
> > 
> > ex(...) {
> >   <...
> > - del_timer
> > + del_timer_sync
> >     (...)
> >   ...>
> > }
> > 
> > The transformations it makes are below.  I haven't had a chance to check 
> > which results overlap with what Thomas has already sent, but I could look 
> 
> Minimal overlap, but as I said I did just a few conversions.
> 
> > into it if this is the right idea.  I guess other kinds of close/exit 
> > functions would have to be identified manually, to make more rules.
> 
> If you look through the examples I sent, you'll find the close()
> callbacks of various devices. So everything which is a function
> pointer to a ops->close(), ops->remove(), ops_>teardown() is dangerous
> if using del_timer(). There are a few exceptions, but....
> 
> Another thing I saw is 
> 
> 	del_timer(&bla->timer);
> 	....
> 	kfree(&bla);
> 
> That's also a potential source of trouble. You can't tell without
> analyzing the code, whether it's a problem or not. But making the
> responsible people to look at it is definitely a good thing.
>  
> > In what circumstance can one be sure that two instructions are executed on 
> > the same CPU?
> 
> If interrupts or preemption are disabled. But that's not the issue at
> hand.
> 
> The del_timer vs. del_timer_sync problem is:
> 
> CPU0	      	  		 CPU1
> 
> add_timer(&bla->timer);
> 
> 				 close(bla)
> timer expires		   	   del_timer(&bla->timer);
>   callback is invoked
> 				   kfree(bla);
>     derefernces bla
> 
> I'll look at your findings on Tuesday, but feel free to send them to
> the relevant maintainers for review.

Thanks for all of the suggestions!

julia

Re: [patch 00/16] timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

[Julia Lawall]

> Another thing I saw is 
> 
> 	del_timer(&bla->timer);
> 	....
> 	kfree(&bla);

In one case I saw the following:

        if (isac->dch.timer.function != NULL) {
                del_timer(&isac->dch.timer);
                isac->dch.timer.function = NULL;
        }
        kfree(isac->mon_rx);
        isac->mon_rx = NULL;

Is the assignment isac->dch.timer.function = NULL good enough to solve 
the problem?

julia

Re: [patch 00/16] timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

[Thomas Gleixner]

On 2014-03-24 08:29, Julia Lawall wrote:
>> Another thing I saw is
>>
>> 	del_timer(&bla->timer);
>> 	....
>> 	kfree(&bla);
>
> In one case I saw the following:
>
>         if (isac->dch.timer.function != NULL) {
>                 del_timer(&isac->dch.timer);
>                 isac->dch.timer.function = NULL;
>         }
>         kfree(isac->mon_rx);
>         isac->mon_rx = NULL;
>
> Is the assignment isac->dch.timer.function = NULL good enough to 
> solve
> the problem?

No. It might lead to a NULL dereference when the other core wants
to call the callback. Same situation as in the other picture.

Thanks,

        tglx

Re: [patch 00/16] timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

[Thomas Gleixner]

On Sun, 23 Mar 2014, Julia Lawall wrote:
> diff -u -p a/arch/mips/lasat/picvue_proc.c b/arch/mips/lasat/picvue_proc.c
> --- a/arch/mips/lasat/picvue_proc.c
> +++ b/arch/mips/lasat/picvue_proc.c
> @@ -175,7 +175,7 @@ static void pvc_proc_cleanup(void)
>  	remove_proc_entry("scroll", pvc_display_dir);
>  	remove_proc_entry(DISPLAY_DIR_NAME, NULL);
>  
> -	del_timer(&timer);
> +	del_timer_sync(&timer);

Correct.

>  }
>  
>  static int __init pvc_proc_init(void)
> diff -u -p a/drivers/tty/serial/mux.c b/drivers/tty/serial/mux.c
> --- a/drivers/tty/serial/mux.c
> +++ b/drivers/tty/serial/mux.c
> @@ -613,7 +613,7 @@ static void __exit mux_exit(void)
>  {
>  	/* Delete the Mux timer. */
>  	if(port_cnt > 0) {
> -		del_timer(&mux_timer);
> +		del_timer_sync(&mux_timer);

Correct.

>  #ifdef CONFIG_SERIAL_MUX_CONSOLE
>  		unregister_console(&mux_console);
>  #endif
> diff -u -p a/drivers/input/serio/hp_sdc.c b/drivers/input/serio/hp_sdc.c
> --- a/drivers/input/serio/hp_sdc.c
> +++ b/drivers/input/serio/hp_sdc.c
> @@ -984,7 +984,7 @@ static void hp_sdc_exit(void)
>  	free_irq(hp_sdc.irq, &hp_sdc);
>  	write_unlock_irq(&hp_sdc.lock);
>  
> -	del_timer(&hp_sdc.kicker);
> +	del_timer_sync(&hp_sdc.kicker);

Correct.
  
>  	tasklet_kill(&hp_sdc.task);
>  
> diff -u -p a/drivers/isdn/sc/init.c b/drivers/isdn/sc/init.c
> --- a/drivers/isdn/sc/init.c
> +++ b/drivers/isdn/sc/init.c
> @@ -390,8 +390,8 @@ static void __exit sc_exit(void)
>  		/*
>  		 * kill the timers
>  		 */
> -		del_timer(&(sc_adapter[i]->reset_timer));
> -		del_timer(&(sc_adapter[i]->stat_timer));
> +		del_timer_sync(&(sc_adapter[i]->reset_timer));
> +		del_timer_sync(&(sc_adapter[i]->stat_timer));

Correct.
  
>  		/*
>  		 * Tell I4L we're toast
> diff -u -p a/drivers/isdn/i4l/isdn_common.c b/drivers/isdn/i4l/isdn_common.c
> --- a/drivers/isdn/i4l/isdn_common.c
> +++ b/drivers/isdn/i4l/isdn_common.c
> @@ -2381,7 +2381,7 @@ static void __exit isdn_exit(void)
>  	}
>  	isdn_tty_exit();
>  	unregister_chrdev(ISDN_MAJOR, "isdn");
> -	del_timer(&dev->timer);
> +	del_timer_sync(&dev->timer);

Correct.

>  	/* call vfree with interrupts enabled, else it will hang */
>  	vfree(dev);
>  	printk(KERN_NOTICE "ISDN-subsystem unloaded\n");
> diff -u -p a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
> --- a/drivers/isdn/hardware/mISDN/hfcpci.c
> +++ b/drivers/isdn/hardware/mISDN/hfcpci.c
> @@ -2352,7 +2352,7 @@ static void __exit
>  HFC_cleanup(void)
>  {
>  	if (timer_pending(&hfc_tl))
> -		del_timer(&hfc_tl);
> +		del_timer_sync(&hfc_tl);

That's incomplete the timer_pending() check is racy as well and the
timer is rearming itself from the callback.
  
>  	pci_unregister_driver(&hfc_driver);
>  }
> diff -u -p a/drivers/isdn/act2000/module.c b/drivers/isdn/act2000/module.c
> --- a/drivers/isdn/act2000/module.c
> +++ b/drivers/isdn/act2000/module.c
> @@ -796,7 +796,7 @@ static void __exit act2000_exit(void)
>  	act2000_card *last;
>  	while (card) {
>  		unregister_card(card);
> -		del_timer(&card->ptimer);
> +		del_timer_sync(&card->ptimer);

Correct.

>  		card = card->next;
>  	}
>  	card = cards;
> diff -u -p a/drivers/net/hamradio/yam.c b/drivers/net/hamradio/yam.c
> --- a/drivers/net/hamradio/yam.c
> +++ b/drivers/net/hamradio/yam.c
> @@ -1184,7 +1184,7 @@ static void __exit yam_cleanup_driver(vo
>  	struct yam_mcs *p;
>  	int i;
>  
> -	del_timer(&yam_timer);
> +	del_timer_sync(&yam_timer);

Correct.

>  	for (i = 0; i < NR_PORTS; i++) {
>  		struct net_device *dev = yam_devs[i];
>  		if (dev) {
> diff -u -p a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> --- a/drivers/staging/speakup/main.c
> +++ b/drivers/staging/speakup/main.c
> @@ -2218,7 +2218,7 @@ static void __exit speakup_exit(void)
>  	unregister_keyboard_notifier(&keyboard_notifier_block);
>  	unregister_vt_notifier(&vt_notifier_block);
>  	speakup_unregister_devsynth();
> -	del_timer(&cursor_timer);
> +	del_timer_sync(&cursor_timer);

Not sure about that one as the timer might be rearmed, so the
invocation might need to be further down the cleanup path.

>  	kthread_stop(speakup_task);
>  	speakup_task = NULL;
>  	mutex_lock(&spk_mutex);
> diff -u -p a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -2298,7 +2298,7 @@ static void __exit panel_cleanup_module(
>  	unregister_reboot_notifier(&panel_notifier);
>  
>  	if (scan_timer.function != NULL)
> -		del_timer(&scan_timer);
> +		del_timer_sync(&scan_timer);

Correct.

>  	if (pprt != NULL) {
>  		if (keypad_enabled) {
> diff -u -p a/drivers/staging/lustre/lustre/llite/super25.c b/drivers/staging/lustre/lustre/llite/super25.c
> --- a/drivers/staging/lustre/lustre/llite/super25.c
> +++ b/drivers/staging/lustre/lustre/llite/super25.c
> @@ -197,7 +197,7 @@ static void __exit exit_lustre_lite(void
>  {
>  	ll_xattr_fini();
>  	vvp_global_fini();
> -	del_timer(&ll_capa_timer);
> +	del_timer_sync(&ll_capa_timer);

Looks wrong, as the timer might be rearmed by the thread which is
not yet stopped.

>  	ll_capa_thread_stop();
>  	LASSERTF(capa_count[CAPA_SITE_CLIENT] == 0,
>  		 "client remaining capa count %d\n",
> diff -u -p a/drivers/atm/idt77105.c b/drivers/atm/idt77105.c
> --- a/drivers/atm/idt77105.c
> +++ b/drivers/atm/idt77105.c
> @@ -369,8 +369,8 @@ EXPORT_SYMBOL(idt77105_init);
>  static void __exit idt77105_exit(void)
>  {
>          /* turn off timers */
> -        del_timer(&stats_timer);
> -        del_timer(&restart_timer);
> +        del_timer_sync(&stats_timer);
> +        del_timer_sync(&restart_timer);

Yep.

>  }
>  
>  module_exit(idt77105_exit);
> diff -u -p a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
> --- a/drivers/atm/nicstar.c
> +++ b/drivers/atm/nicstar.c
> @@ -306,7 +306,7 @@ static void __exit nicstar_cleanup(void)
>  {
>  	XPRINTK("nicstar: nicstar_cleanup() called.\n");
>  
> -	del_timer(&ns_timer);
> +	del_timer_sync(&ns_timer);
>  

The driver might still be in use, as the devices are shut down via the
pci_unregister_driver() call below. So while it's correct to use
del_timer_sync() it's not clear w/o digging deeper into the code
whether it's actually sufficient.

>  	pci_unregister_driver(&nicstar_driver);
>  
> diff -u -p a/drivers/atm/iphase.c b/drivers/atm/iphase.c
> --- a/drivers/atm/iphase.c
> +++ b/drivers/atm/iphase.c
> @@ -3289,7 +3289,7 @@ static void __exit ia_module_exit(void)
>  {
>  	pci_unregister_driver(&ia_driver);
>  
> -        del_timer(&ia_timer);
> +        del_timer_sync(&ia_timer);

The timer is self rearming and has protection against the removal of
the devices. So it should be fine.

>  }
>  
>  module_init(ia_module_init);
> diff -u -p a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
> --- a/net/hsr/hsr_main.c
> +++ b/net/hsr/hsr_main.c
> @@ -459,7 +459,7 @@ static int __init hsr_init(void)
>  static void __exit hsr_exit(void)
>  {
>  	unregister_netdevice_notifier(&hsr_nb);
> -	del_timer(&prune_timer);
> +	del_timer_sync(&prune_timer);

Correct as the only rearm is from the timer callback itself

>  	hsr_netlink_exit();
>  	dev_remove_pack(&hsr_pt);
>  }
> diff -u -p a/net/atm/mpc.c b/net/atm/mpc.c
> --- a/net/atm/mpc.c
> +++ b/net/atm/mpc.c
> @@ -1492,7 +1492,7 @@ static void __exit atm_mpoa_cleanup(void
>  
>  	mpc_proc_clean();
>  
> -	del_timer(&mpc_timer);
> +	del_timer_sync(&mpc_timer);

Looks correct as well. Timer is rearming no other dependencies.

>  	unregister_netdevice_notifier(&mpoa_notifier);
>  	deregister_atm_ioctl(&atm_ioctl_ops);

Thanks,

	tglx

Re: [patch 00/16] timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

[Julia Lawall]

Thanks.  I haven't had time to send them out, but I could do so tomorrow 
afternoon.

julia

On Tue, 25 Mar 2014, Thomas Gleixner wrote:

> On Sun, 23 Mar 2014, Julia Lawall wrote:
> > diff -u -p a/arch/mips/lasat/picvue_proc.c b/arch/mips/lasat/picvue_proc.c
> > --- a/arch/mips/lasat/picvue_proc.c
> > +++ b/arch/mips/lasat/picvue_proc.c
> > @@ -175,7 +175,7 @@ static void pvc_proc_cleanup(void)
> >  	remove_proc_entry("scroll", pvc_display_dir);
> >  	remove_proc_entry(DISPLAY_DIR_NAME, NULL);
> >  
> > -	del_timer(&timer);
> > +	del_timer_sync(&timer);
> 
> Correct.
> 
> >  }
> >  
> >  static int __init pvc_proc_init(void)
> > diff -u -p a/drivers/tty/serial/mux.c b/drivers/tty/serial/mux.c
> > --- a/drivers/tty/serial/mux.c
> > +++ b/drivers/tty/serial/mux.c
> > @@ -613,7 +613,7 @@ static void __exit mux_exit(void)
> >  {
> >  	/* Delete the Mux timer. */
> >  	if(port_cnt > 0) {
> > -		del_timer(&mux_timer);
> > +		del_timer_sync(&mux_timer);
> 
> Correct.
> 
> >  #ifdef CONFIG_SERIAL_MUX_CONSOLE
> >  		unregister_console(&mux_console);
> >  #endif
> > diff -u -p a/drivers/input/serio/hp_sdc.c b/drivers/input/serio/hp_sdc.c
> > --- a/drivers/input/serio/hp_sdc.c
> > +++ b/drivers/input/serio/hp_sdc.c
> > @@ -984,7 +984,7 @@ static void hp_sdc_exit(void)
> >  	free_irq(hp_sdc.irq, &hp_sdc);
> >  	write_unlock_irq(&hp_sdc.lock);
> >  
> > -	del_timer(&hp_sdc.kicker);
> > +	del_timer_sync(&hp_sdc.kicker);
> 
> Correct.
>   
> >  	tasklet_kill(&hp_sdc.task);
> >  
> > diff -u -p a/drivers/isdn/sc/init.c b/drivers/isdn/sc/init.c
> > --- a/drivers/isdn/sc/init.c
> > +++ b/drivers/isdn/sc/init.c
> > @@ -390,8 +390,8 @@ static void __exit sc_exit(void)
> >  		/*
> >  		 * kill the timers
> >  		 */
> > -		del_timer(&(sc_adapter[i]->reset_timer));
> > -		del_timer(&(sc_adapter[i]->stat_timer));
> > +		del_timer_sync(&(sc_adapter[i]->reset_timer));
> > +		del_timer_sync(&(sc_adapter[i]->stat_timer));
> 
> Correct.
>   
> >  		/*
> >  		 * Tell I4L we're toast
> > diff -u -p a/drivers/isdn/i4l/isdn_common.c b/drivers/isdn/i4l/isdn_common.c
> > --- a/drivers/isdn/i4l/isdn_common.c
> > +++ b/drivers/isdn/i4l/isdn_common.c
> > @@ -2381,7 +2381,7 @@ static void __exit isdn_exit(void)
> >  	}
> >  	isdn_tty_exit();
> >  	unregister_chrdev(ISDN_MAJOR, "isdn");
> > -	del_timer(&dev->timer);
> > +	del_timer_sync(&dev->timer);
> 
> Correct.
> 
> >  	/* call vfree with interrupts enabled, else it will hang */
> >  	vfree(dev);
> >  	printk(KERN_NOTICE "ISDN-subsystem unloaded\n");
> > diff -u -p a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
> > --- a/drivers/isdn/hardware/mISDN/hfcpci.c
> > +++ b/drivers/isdn/hardware/mISDN/hfcpci.c
> > @@ -2352,7 +2352,7 @@ static void __exit
> >  HFC_cleanup(void)
> >  {
> >  	if (timer_pending(&hfc_tl))
> > -		del_timer(&hfc_tl);
> > +		del_timer_sync(&hfc_tl);
> 
> That's incomplete the timer_pending() check is racy as well and the
> timer is rearming itself from the callback.
>   
> >  	pci_unregister_driver(&hfc_driver);
> >  }
> > diff -u -p a/drivers/isdn/act2000/module.c b/drivers/isdn/act2000/module.c
> > --- a/drivers/isdn/act2000/module.c
> > +++ b/drivers/isdn/act2000/module.c
> > @@ -796,7 +796,7 @@ static void __exit act2000_exit(void)
> >  	act2000_card *last;
> >  	while (card) {
> >  		unregister_card(card);
> > -		del_timer(&card->ptimer);
> > +		del_timer_sync(&card->ptimer);
> 
> Correct.
> 
> >  		card = card->next;
> >  	}
> >  	card = cards;
> > diff -u -p a/drivers/net/hamradio/yam.c b/drivers/net/hamradio/yam.c
> > --- a/drivers/net/hamradio/yam.c
> > +++ b/drivers/net/hamradio/yam.c
> > @@ -1184,7 +1184,7 @@ static void __exit yam_cleanup_driver(vo
> >  	struct yam_mcs *p;
> >  	int i;
> >  
> > -	del_timer(&yam_timer);
> > +	del_timer_sync(&yam_timer);
> 
> Correct.
> 
> >  	for (i = 0; i < NR_PORTS; i++) {
> >  		struct net_device *dev = yam_devs[i];
> >  		if (dev) {
> > diff -u -p a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> > --- a/drivers/staging/speakup/main.c
> > +++ b/drivers/staging/speakup/main.c
> > @@ -2218,7 +2218,7 @@ static void __exit speakup_exit(void)
> >  	unregister_keyboard_notifier(&keyboard_notifier_block);
> >  	unregister_vt_notifier(&vt_notifier_block);
> >  	speakup_unregister_devsynth();
> > -	del_timer(&cursor_timer);
> > +	del_timer_sync(&cursor_timer);
> 
> Not sure about that one as the timer might be rearmed, so the
> invocation might need to be further down the cleanup path.
> 
> >  	kthread_stop(speakup_task);
> >  	speakup_task = NULL;
> >  	mutex_lock(&spk_mutex);
> > diff -u -p a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> > --- a/drivers/staging/panel/panel.c
> > +++ b/drivers/staging/panel/panel.c
> > @@ -2298,7 +2298,7 @@ static void __exit panel_cleanup_module(
> >  	unregister_reboot_notifier(&panel_notifier);
> >  
> >  	if (scan_timer.function != NULL)
> > -		del_timer(&scan_timer);
> > +		del_timer_sync(&scan_timer);
> 
> Correct.
> 
> >  	if (pprt != NULL) {
> >  		if (keypad_enabled) {
> > diff -u -p a/drivers/staging/lustre/lustre/llite/super25.c b/drivers/staging/lustre/lustre/llite/super25.c
> > --- a/drivers/staging/lustre/lustre/llite/super25.c
> > +++ b/drivers/staging/lustre/lustre/llite/super25.c
> > @@ -197,7 +197,7 @@ static void __exit exit_lustre_lite(void
> >  {
> >  	ll_xattr_fini();
> >  	vvp_global_fini();
> > -	del_timer(&ll_capa_timer);
> > +	del_timer_sync(&ll_capa_timer);
> 
> Looks wrong, as the timer might be rearmed by the thread which is
> not yet stopped.
> 
> >  	ll_capa_thread_stop();
> >  	LASSERTF(capa_count[CAPA_SITE_CLIENT] == 0,
> >  		 "client remaining capa count %d\n",
> > diff -u -p a/drivers/atm/idt77105.c b/drivers/atm/idt77105.c
> > --- a/drivers/atm/idt77105.c
> > +++ b/drivers/atm/idt77105.c
> > @@ -369,8 +369,8 @@ EXPORT_SYMBOL(idt77105_init);
> >  static void __exit idt77105_exit(void)
> >  {
> >          /* turn off timers */
> > -        del_timer(&stats_timer);
> > -        del_timer(&restart_timer);
> > +        del_timer_sync(&stats_timer);
> > +        del_timer_sync(&restart_timer);
> 
> Yep.
> 
> >  }
> >  
> >  module_exit(idt77105_exit);
> > diff -u -p a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
> > --- a/drivers/atm/nicstar.c
> > +++ b/drivers/atm/nicstar.c
> > @@ -306,7 +306,7 @@ static void __exit nicstar_cleanup(void)
> >  {
> >  	XPRINTK("nicstar: nicstar_cleanup() called.\n");
> >  
> > -	del_timer(&ns_timer);
> > +	del_timer_sync(&ns_timer);
> >  
> 
> The driver might still be in use, as the devices are shut down via the
> pci_unregister_driver() call below. So while it's correct to use
> del_timer_sync() it's not clear w/o digging deeper into the code
> whether it's actually sufficient.
> 
> >  	pci_unregister_driver(&nicstar_driver);
> >  
> > diff -u -p a/drivers/atm/iphase.c b/drivers/atm/iphase.c
> > --- a/drivers/atm/iphase.c
> > +++ b/drivers/atm/iphase.c
> > @@ -3289,7 +3289,7 @@ static void __exit ia_module_exit(void)
> >  {
> >  	pci_unregister_driver(&ia_driver);
> >  
> > -        del_timer(&ia_timer);
> > +        del_timer_sync(&ia_timer);
> 
> The timer is self rearming and has protection against the removal of
> the devices. So it should be fine.
> 
> >  }
> >  
> >  module_init(ia_module_init);
> > diff -u -p a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
> > --- a/net/hsr/hsr_main.c
> > +++ b/net/hsr/hsr_main.c
> > @@ -459,7 +459,7 @@ static int __init hsr_init(void)
> >  static void __exit hsr_exit(void)
> >  {
> >  	unregister_netdevice_notifier(&hsr_nb);
> > -	del_timer(&prune_timer);
> > +	del_timer_sync(&prune_timer);
> 
> Correct as the only rearm is from the timer callback itself
> 
> >  	hsr_netlink_exit();
> >  	dev_remove_pack(&hsr_pt);
> >  }
> > diff -u -p a/net/atm/mpc.c b/net/atm/mpc.c
> > --- a/net/atm/mpc.c
> > +++ b/net/atm/mpc.c
> > @@ -1492,7 +1492,7 @@ static void __exit atm_mpoa_cleanup(void
> >  
> >  	mpc_proc_clean();
> >  
> > -	del_timer(&mpc_timer);
> > +	del_timer_sync(&mpc_timer);
> 
> Looks correct as well. Timer is rearming no other dependencies.
> 
> >  	unregister_netdevice_notifier(&mpoa_notifier);
> >  	deregister_atm_ioctl(&atm_ioctl_ops);
> 
> Thanks,
> 
> 	tglx
> 
> 

Re: [patch 00/16] timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

[Julia Lawall]

I was looking at some cases that are marked as possibly insufficient.  Is
the issue that mod_timer or add_timer is called in some function that is
not the timer interrupt handler (or the function that starts the timer
running in the first place)?

julia


On Tue, 25 Mar 2014, Thomas Gleixner wrote:

> On Sun, 23 Mar 2014, Julia Lawall wrote:
> > diff -u -p a/arch/mips/lasat/picvue_proc.c b/arch/mips/lasat/picvue_proc.c
> > --- a/arch/mips/lasat/picvue_proc.c
> > +++ b/arch/mips/lasat/picvue_proc.c
> > @@ -175,7 +175,7 @@ static void pvc_proc_cleanup(void)
> >  	remove_proc_entry("scroll", pvc_display_dir);
> >  	remove_proc_entry(DISPLAY_DIR_NAME, NULL);
> >
> > -	del_timer(&timer);
> > +	del_timer_sync(&timer);
>
> Correct.
>
> >  }
> >
> >  static int __init pvc_proc_init(void)
> > diff -u -p a/drivers/tty/serial/mux.c b/drivers/tty/serial/mux.c
> > --- a/drivers/tty/serial/mux.c
> > +++ b/drivers/tty/serial/mux.c
> > @@ -613,7 +613,7 @@ static void __exit mux_exit(void)
> >  {
> >  	/* Delete the Mux timer. */
> >  	if(port_cnt > 0) {
> > -		del_timer(&mux_timer);
> > +		del_timer_sync(&mux_timer);
>
> Correct.
>
> >  #ifdef CONFIG_SERIAL_MUX_CONSOLE
> >  		unregister_console(&mux_console);
> >  #endif
> > diff -u -p a/drivers/input/serio/hp_sdc.c b/drivers/input/serio/hp_sdc.c
> > --- a/drivers/input/serio/hp_sdc.c
> > +++ b/drivers/input/serio/hp_sdc.c
> > @@ -984,7 +984,7 @@ static void hp_sdc_exit(void)
> >  	free_irq(hp_sdc.irq, &hp_sdc);
> >  	write_unlock_irq(&hp_sdc.lock);
> >
> > -	del_timer(&hp_sdc.kicker);
> > +	del_timer_sync(&hp_sdc.kicker);
>
> Correct.
>
> >  	tasklet_kill(&hp_sdc.task);
> >
> > diff -u -p a/drivers/isdn/sc/init.c b/drivers/isdn/sc/init.c
> > --- a/drivers/isdn/sc/init.c
> > +++ b/drivers/isdn/sc/init.c
> > @@ -390,8 +390,8 @@ static void __exit sc_exit(void)
> >  		/*
> >  		 * kill the timers
> >  		 */
> > -		del_timer(&(sc_adapter[i]->reset_timer));
> > -		del_timer(&(sc_adapter[i]->stat_timer));
> > +		del_timer_sync(&(sc_adapter[i]->reset_timer));
> > +		del_timer_sync(&(sc_adapter[i]->stat_timer));
>
> Correct.
>
> >  		/*
> >  		 * Tell I4L we're toast
> > diff -u -p a/drivers/isdn/i4l/isdn_common.c b/drivers/isdn/i4l/isdn_common.c
> > --- a/drivers/isdn/i4l/isdn_common.c
> > +++ b/drivers/isdn/i4l/isdn_common.c
> > @@ -2381,7 +2381,7 @@ static void __exit isdn_exit(void)
> >  	}
> >  	isdn_tty_exit();
> >  	unregister_chrdev(ISDN_MAJOR, "isdn");
> > -	del_timer(&dev->timer);
> > +	del_timer_sync(&dev->timer);
>
> Correct.
>
> >  	/* call vfree with interrupts enabled, else it will hang */
> >  	vfree(dev);
> >  	printk(KERN_NOTICE "ISDN-subsystem unloaded\n");
> > diff -u -p a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
> > --- a/drivers/isdn/hardware/mISDN/hfcpci.c
> > +++ b/drivers/isdn/hardware/mISDN/hfcpci.c
> > @@ -2352,7 +2352,7 @@ static void __exit
> >  HFC_cleanup(void)
> >  {
> >  	if (timer_pending(&hfc_tl))
> > -		del_timer(&hfc_tl);
> > +		del_timer_sync(&hfc_tl);
>
> That's incomplete the timer_pending() check is racy as well and the
> timer is rearming itself from the callback.
>
> >  	pci_unregister_driver(&hfc_driver);
> >  }
> > diff -u -p a/drivers/isdn/act2000/module.c b/drivers/isdn/act2000/module.c
> > --- a/drivers/isdn/act2000/module.c
> > +++ b/drivers/isdn/act2000/module.c
> > @@ -796,7 +796,7 @@ static void __exit act2000_exit(void)
> >  	act2000_card *last;
> >  	while (card) {
> >  		unregister_card(card);
> > -		del_timer(&card->ptimer);
> > +		del_timer_sync(&card->ptimer);
>
> Correct.
>
> >  		card = card->next;
> >  	}
> >  	card = cards;
> > diff -u -p a/drivers/net/hamradio/yam.c b/drivers/net/hamradio/yam.c
> > --- a/drivers/net/hamradio/yam.c
> > +++ b/drivers/net/hamradio/yam.c
> > @@ -1184,7 +1184,7 @@ static void __exit yam_cleanup_driver(vo
> >  	struct yam_mcs *p;
> >  	int i;
> >
> > -	del_timer(&yam_timer);
> > +	del_timer_sync(&yam_timer);
>
> Correct.
>
> >  	for (i = 0; i < NR_PORTS; i++) {
> >  		struct net_device *dev = yam_devs[i];
> >  		if (dev) {
> > diff -u -p a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> > --- a/drivers/staging/speakup/main.c
> > +++ b/drivers/staging/speakup/main.c
> > @@ -2218,7 +2218,7 @@ static void __exit speakup_exit(void)
> >  	unregister_keyboard_notifier(&keyboard_notifier_block);
> >  	unregister_vt_notifier(&vt_notifier_block);
> >  	speakup_unregister_devsynth();
> > -	del_timer(&cursor_timer);
> > +	del_timer_sync(&cursor_timer);
>
> Not sure about that one as the timer might be rearmed, so the
> invocation might need to be further down the cleanup path.
>
> >  	kthread_stop(speakup_task);
> >  	speakup_task = NULL;
> >  	mutex_lock(&spk_mutex);
> > diff -u -p a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> > --- a/drivers/staging/panel/panel.c
> > +++ b/drivers/staging/panel/panel.c
> > @@ -2298,7 +2298,7 @@ static void __exit panel_cleanup_module(
> >  	unregister_reboot_notifier(&panel_notifier);
> >
> >  	if (scan_timer.function != NULL)
> > -		del_timer(&scan_timer);
> > +		del_timer_sync(&scan_timer);
>
> Correct.
>
> >  	if (pprt != NULL) {
> >  		if (keypad_enabled) {
> > diff -u -p a/drivers/staging/lustre/lustre/llite/super25.c b/drivers/staging/lustre/lustre/llite/super25.c
> > --- a/drivers/staging/lustre/lustre/llite/super25.c
> > +++ b/drivers/staging/lustre/lustre/llite/super25.c
> > @@ -197,7 +197,7 @@ static void __exit exit_lustre_lite(void
> >  {
> >  	ll_xattr_fini();
> >  	vvp_global_fini();
> > -	del_timer(&ll_capa_timer);
> > +	del_timer_sync(&ll_capa_timer);
>
> Looks wrong, as the timer might be rearmed by the thread which is
> not yet stopped.
>
> >  	ll_capa_thread_stop();
> >  	LASSERTF(capa_count[CAPA_SITE_CLIENT] == 0,
> >  		 "client remaining capa count %d\n",
> > diff -u -p a/drivers/atm/idt77105.c b/drivers/atm/idt77105.c
> > --- a/drivers/atm/idt77105.c
> > +++ b/drivers/atm/idt77105.c
> > @@ -369,8 +369,8 @@ EXPORT_SYMBOL(idt77105_init);
> >  static void __exit idt77105_exit(void)
> >  {
> >          /* turn off timers */
> > -        del_timer(&stats_timer);
> > -        del_timer(&restart_timer);
> > +        del_timer_sync(&stats_timer);
> > +        del_timer_sync(&restart_timer);
>
> Yep.
>
> >  }
> >
> >  module_exit(idt77105_exit);
> > diff -u -p a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
> > --- a/drivers/atm/nicstar.c
> > +++ b/drivers/atm/nicstar.c
> > @@ -306,7 +306,7 @@ static void __exit nicstar_cleanup(void)
> >  {
> >  	XPRINTK("nicstar: nicstar_cleanup() called.\n");
> >
> > -	del_timer(&ns_timer);
> > +	del_timer_sync(&ns_timer);
> >
>
> The driver might still be in use, as the devices are shut down via the
> pci_unregister_driver() call below. So while it's correct to use
> del_timer_sync() it's not clear w/o digging deeper into the code
> whether it's actually sufficient.
>
> >  	pci_unregister_driver(&nicstar_driver);
> >
> > diff -u -p a/drivers/atm/iphase.c b/drivers/atm/iphase.c
> > --- a/drivers/atm/iphase.c
> > +++ b/drivers/atm/iphase.c
> > @@ -3289,7 +3289,7 @@ static void __exit ia_module_exit(void)
> >  {
> >  	pci_unregister_driver(&ia_driver);
> >
> > -        del_timer(&ia_timer);
> > +        del_timer_sync(&ia_timer);
>
> The timer is self rearming and has protection against the removal of
> the devices. So it should be fine.
>
> >  }
> >
> >  module_init(ia_module_init);
> > diff -u -p a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
> > --- a/net/hsr/hsr_main.c
> > +++ b/net/hsr/hsr_main.c
> > @@ -459,7 +459,7 @@ static int __init hsr_init(void)
> >  static void __exit hsr_exit(void)
> >  {
> >  	unregister_netdevice_notifier(&hsr_nb);
> > -	del_timer(&prune_timer);
> > +	del_timer_sync(&prune_timer);
>
> Correct as the only rearm is from the timer callback itself
>
> >  	hsr_netlink_exit();
> >  	dev_remove_pack(&hsr_pt);
> >  }
> > diff -u -p a/net/atm/mpc.c b/net/atm/mpc.c
> > --- a/net/atm/mpc.c
> > +++ b/net/atm/mpc.c
> > @@ -1492,7 +1492,7 @@ static void __exit atm_mpoa_cleanup(void
> >
> >  	mpc_proc_clean();
> >
> > -	del_timer(&mpc_timer);
> > +	del_timer_sync(&mpc_timer);
>
> Looks correct as well. Timer is rearming no other dependencies.
>
> >  	unregister_netdevice_notifier(&mpoa_notifier);
> >  	deregister_atm_ioctl(&atm_ioctl_ops);
>
> Thanks,
>
> 	tglx
>
>