linux-kernel - Re: [PATCH v4a 00/38] timers: Use 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: <CAHk-=wi95dGkg7DiuOZ27gGW+mxJipn9ykB6LHB-HrbbLG6OMQ@mail.gmail.com>
Date:   Sat, 5 Nov 2022 08:59:36 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Steven Rostedt <rostedt@...dmis.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>, rcu@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-s390@...r.kernel.org,
        linux-sh@...r.kernel.org, linux-edac@...r.kernel.org,
        cgroups@...r.kernel.org, linux-block@...r.kernel.org,
        linux-acpi@...r.kernel.org,
        linux-atm-general@...ts.sourceforge.net, netdev@...r.kernel.org,
        linux-pm@...r.kernel.org, drbd-dev@...ts.linbit.com,
        linux-bluetooth@...r.kernel.org,
        openipmi-developer@...ts.sourceforge.net,
        linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linaro-mm-sig@...ts.linaro.org, intel-gfx@...ts.freedesktop.org,
        linux-input@...r.kernel.org, linux-parisc@...r.kernel.org,
        linux-leds@...r.kernel.org, intel-wired-lan@...ts.osuosl.org,
        linux-usb@...r.kernel.org, linux-wireless@...r.kernel.org,
        linux-scsi@...r.kernel.org, linux-staging@...ts.linux.dev,
        linux-ext4@...r.kernel.org, linux-nilfs@...r.kernel.org,
        bridge@...ts.linux-foundation.org, netfilter-devel@...r.kernel.org,
        coreteam@...filter.org, lvs-devel@...r.kernel.org,
        linux-afs@...ts.infradead.org, linux-nfs@...r.kernel.org,
        tipc-discussion@...ts.sourceforge.net, alsa-devel@...a-project.org
Subject: Re: [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers

On Fri, Nov 4, 2022 at 11:01 PM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> Patch 1 fixes an issue with sunrpc/xprt where it incorrectly uses
> del_singleshot_timer_sync() for something that is not a oneshot timer. As this
> will be converted to shutdown, this needs to be fixed first.

So this is the kind of thing that I would *not* want to get eartly.

I really would want to get just the infrastructure in to let people
start doing conversions.

And then the "mindlessly obvious patches that are done by scripting
and can not possibly matter".

The kinds that do not *need* review, because they are mechanical, and
that just cause pointless noise for the rest of the patches that *do*
want review.

Not this kind of thing that is so subtle that you have to explain it.
That's not a "scripted patch for no semantic change".

So leave the del_singleshot_timer_sync() cases alone, they are
irrelevant for the new infrastructure and for the "mindless scripted
conversion" patches.

> Patches 2-4 changes existing timer_shutdown() functions used locally in ARM and
> some drivers to better namespace names.

Ok, these are relevant.

> Patch 5 implements the new timer_shutdown() and timer_shutdown_sync() functions
> that disable re-arming the timer after they are called.

This is obviously what I'd want early so that people can start doign
this in their trees.

> Patches 6-28 change all the locations where there's a kfree(), kfree_rcu(),
> kmem_cache_free() and one call_rcu() call where the RCU function frees the
> timer (the workqueue patch) in the same function as the del_timer{,_sync}() is
> called on that timer, and there's no extra exit path between the del_timer and
> freeing of the timer.

So honestly, I was literally hoping for a "this is the coccinelle
script" kind of patch.

Now there seems to be a number of patches here that are actualyl
really hard to see that they are "obviously correct" and I can't tell
if they are actually scripted or not.

They don't *look* scripted, but I can't really tell.  I looked at the
patches with ten lines of context, and I didn't see the immediately
following kfree() even in that expanded patch context, so it's fairly
far away.

Others in the series were *definitely* not scripted, doing clearly
manual cleanups:

-    if (dch->timer.function) {
-        del_timer(&dch->timer);
-        dch->timer.function = NULL;
-    }
+    timer_shutdown(&dch->timer);

so no, this does *not* make me feel "ok, this is all trivial".

IOW, I'd really want *just* the infrastructure and *just* the provably
trivial stuff. If it wasn't some scripted really obvious thing that
cannot possibly change anything and that wasn't then edited manually
for some reason, I really don't want it early.

IOW, any early conversions I'd take are literally about removing pure
mindless noise. Not about doing conversions.

And I wouldn't mind it as a single conversion patch that has the
coccinelle script as the explanation.

Really just THAT kind of "100% mindless conversion".

               Linus