Re: [PATCH v5a 5/5] treewide: Convert del_timer*() to timer_shutdown*()

[Julia Lawall]

On Sun, 6 Nov 2022, Steven Rostedt wrote:

> On Sun, 6 Nov 2022 12:51:46 -0800
> Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> > Thanks, this looks reasonable.
> >
> > On Sat, Nov 5, 2022 at 10:46 PM Steven Rostedt <rostedt@xxxxxxxxxxx> 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;

Maybe change the second when line to when != ptr->timer to see what are
the differences in the results?  That could show if there is anything
important that has not been taken into account.

julia

> (
>         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
>
>