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]

Message-ID: <20221106211518.GA162439@roeck-us.net>
Date:   Sun, 6 Nov 2022 13:15:18 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        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, Nov 06, 2022 at 04:09:56PM -0500, Steven Rostedt wrote:
> 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);
> )
> 

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