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

[Guenter Roeck <linux@roeck-us.net>]

On Sun, Nov 06, 2022 at 04:09:56PM -0500, Steven Rostedt wrote:
> On Sun, 6 Nov 2022 12:51:46 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > Thanks, this looks reasonable.
> > 
> > On Sat, Nov 5, 2022 at 10:46 PM Steven Rostedt <rostedt@goodmis.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);
> )
> 

Same modified script, same results. I am curious, though:

> --- 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);
>  
...
> @@ -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);

Does that mean something similar may be needed for alarms ?

>                 } 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 checked as well and agree.

Guenter