linux-kernel - Re: [PATCH v5a 5/5] treewide: Convert del_timer*() to timer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 6 Nov 2022 16:09:56 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Anna-Maria Gleixner <anna-maria@...utronix.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Julia Lawall <Julia.Lawall@...ia.fr>
Subject: Re: [PATCH v5a 5/5] treewide: Convert del_timer*() to
 timer_shutdown*()

On Sun, 6 Nov 2022 12:51:46 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> Thanks, this looks reasonable.
> 
> On Sat, Nov 5, 2022 at 10:46 PM Steven Rostedt <rostedt@...dmis.org> wrote:
> >
> > The coccinelle script:
> >
> > @@
> > expression E;
> > identifier ptr, timer, rfield, slab;  
> 
> I think Julia suggested making 'ptr' be an expression too, and I think

And I forgot to add Julia to the Cc :-/

> she's right. Probably 'slab' should be too - there's no reason to
> limit it to just one identifier.
> 
> >   ... when strict
> >       when != ptr->timer.function = E;  
> 
> I suspect any "ptr->timer" access anywhere between the
> del_timer_sync() and the freeing should disable things.
> 
> Although hopefully there aren't any other odd cases than that one
> "clear timer function by hand" one.

OK, I did the following with this new script:

$ cat timer.cocci
@@
expression E,ptr,slab;
identifier timer, rfield;
@@
(
-       del_timer(&ptr->timer);
+       timer_shutdown(&ptr->timer);
|
-       del_timer_sync(&ptr->timer);
+       timer_shutdown_sync(&ptr->timer);
)
  ... when strict
      when != ptr->timer.function = E;
(
        kfree_rcu(ptr, rfield);
|
        kmem_cache_free(slab, ptr);
|
        kfree(ptr);
)


$ git show | patch -p1 -R
$ spatch --dir timer.cocci . > /tmp/t.patch
$ patch -p1 < /tmp/t.patch

And it caught some more:

$ git diff
diff --git a/drivers/atm/idt77252.c b/drivers/atm/idt77252.c
index 99cae174d558..eec0cc2144e0 100644
--- a/drivers/atm/idt77252.c
+++ b/drivers/atm/idt77252.c
@@ -2530,7 +2530,7 @@ idt77252_close(struct atm_vcc *vcc)
                vc->tx_vcc = NULL;
 
                if (vc->estimator) {
-                       del_timer(&vc->estimator->timer);
+                       timer_shutdown(&vc->estimator->timer);
                        kfree(vc->estimator);
                        vc->estimator = NULL;
                }
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c
index f9f18ff451ea..7ea2631b8069 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/btcoex.c
@@ -394,7 +394,7 @@ void brcmf_btcoex_detach(struct brcmf_cfg80211_info *cfg)
 
        if (cfg->btcoex->timer_on) {
                cfg->btcoex->timer_on = false;
-               del_timer_sync(&cfg->btcoex->timer);
+               timer_shutdown_sync(&cfg->btcoex->timer);
        }
 
        cancel_work_sync(&cfg->btcoex->work);
diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c
index 0f8bb0bf558f..8d36303f3935 100644
--- a/net/netfilter/xt_IDLETIMER.c
+++ b/net/netfilter/xt_IDLETIMER.c
@@ -413,7 +413,7 @@ static void idletimer_tg_destroy(const struct xt_tgdtor_param *par)
                pr_debug("deleting timer %s\n", info->label);
 
                list_del(&info->timer->entry);
-               del_timer_sync(&info->timer->timer);
+               timer_shutdown_sync(&info->timer->timer);
                cancel_work_sync(&info->timer->work);
                sysfs_remove_file(idletimer_tg_kobj, &info->timer->attr.attr);
                kfree(info->timer->attr.attr.name);
@@ -441,7 +441,7 @@ static void idletimer_tg_destroy_v1(const struct xt_tgdtor_param *par)
                if (info->timer->timer_type & XT_IDLETIMER_ALARM) {
                        alarm_cancel(&info->timer->alarm);
                } else {
-                       del_timer_sync(&info->timer->timer);
+                       timer_shutdown_sync(&info->timer->timer);
                }
                cancel_work_sync(&info->timer->work);
                sysfs_remove_file(idletimer_tg_kobj, &info->timer->attr.attr);


Which all look legit.

I'll post a v6a using the new script.

-- Steve