linux-kernel - Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

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:   Fri, 8 Apr 2022 07:33:53 -1000
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        LKML <linux-kernel@...r.kernel.org>, jstultz@...gle.com,
        Stephen Boyd <sboyd@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Guenter Roeck <linux@...ck-us.net>
Subject: Re: [RFC][PATCH] timers: Add del_time_free() to be called before
 freeing timers

On Fri, Apr 8, 2022 at 12:37 AM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> So this would become:
>
> -       BUG_ON(!timer->function);
> +       if (WARN_ON(!timer->function))
> +               return -EBROKEN;

Yes. But please make it a WARN_ON_ONCE(), just on basic principles. I
can't imagine this happening a lot, but at the same time I don't think
there's any reason _not_ to just always use WARN_ON_ONCE() for these
kinds of "serious bug, but should never happen" situations.

Because we don't want some "user can trigger this and spam the logs"
situation either.

That said, I would actually prefer a name-change: instead of making
this about "del_timer_free()", can we please just make this about it
being "final". Or maybe "del_timer_cancel()" or something like that?

Because the actual _freeing_ will obviously happen later, and the
function does nothing of the sort. In fact, there may be situations
where you don't free it at all, but just want to be in the situation
where you want to make sure there are no pending timers until after
you explicitly re-arm it, even if the timer would otherwise be
self-arming.

(That use-case would actually mean removing the WARN_ON_ONCE(), but I
think that would be a "future use" issue, I'm *not* suggesting it not
be done initially).

I also suspect that 99% of all del_timer_sync() users actually want
that kind of explicit "del_timer_final()" behavior. Some may not
_need_ it (because their re-arming already checks for "have I shut
down?"), but I have this suspicion that we could convert a lot - maybe
all - of the current del_timer_sync() users to this and try to see if
we could just make it the rule.

And then we could actually maybe start removing the explicit "shut
down timer" code. See for example "work->canceling" logic for
kthreads, which now does double duty (it disables re-arming the timer,
_and_ it does some "double cancel work avoidance")

              Linus