Message-ID: <CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@mail.gmail.com>
Date:   Fri, 15 Sep 2023 12:06:25 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Cc:     Alexey Dobriyan <adobriyan@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        akpm@...ux-foundation.org
Subject: Re: Buggy __free(kfree) usage pattern already in tree

On Fri, 15 Sept 2023 at 10:22, Bartosz Golaszewski
<bartosz.golaszewski@...aro.org> wrote:
>
> IMO this feature has much more potential at fixing existing memory
> leaks than introducing new ones. I agree, I should have been more
> careful, but I wouldn't exaggerate the issue. It's a bug, I sent a
> fix, it'll be fine in a few days. I hope it won't be seen as an
> argument against __free(). It just needs some time to mature.

Honestly, I think your "fix" is still wrong.

It may *work*, but it's against the whole spirit of having an
allocation paired with the "this is how you free it".

Your model of is fundamentally fragile, and honestly, it's disgusting.

The fact that you literally have

        char ***line_names

as an argument should have made you wonder. Yes, we have triple
indirect pointers in some other parts of the tree, but it sure isn't a
"feature".

The thing is, your cleanup function should mirror your allocation
function. It didn't, and it caused a bug.

And it STILL DOES NOT, even with your change.

So I claim you are completely mis-using the whole __free thing. What
you are doing is simply WRONG.

And unless you can do it right, I will revert that change of yours to
mis-use the cleanup functions, because I do not want anybody else to
look at your code and get all the wrong ideas.

Seriously.

So look at your code, and DO IT RIGHT. Don't try to claim that
"kfree()" is the cleanup function for gpio_sim_make_line_names().
Because it really isn't. One free's a random pointer. Another returns
a complex data structure *and* a count. They aren't inverses.

I don't care ONE WHIT if you have learnt to use these kinds of things
from GLib/GObject, and if that kind of disgusting behavior is ok
there.

It's not going to fly in the kernel.

So your pattern needs to be something like this:

        struct X *p __free(freeXYZ) = allocXYZ();

and ABSOLUTELY NOTHING ELSE.  So if you use __free(kfree), it looks like

        struct obj *p __free(kfree) = kmalloc(...);

and not some different variation of it.

And if you want to do something more complex, we literally have that
"CLASS()" abstraction to make the above pattern particularly easy to
use. Use it.

But don't abuse the very special 'kmalloc/kfree' class that we have as
an example. That's for kmalloc/kfree pairs, not for your "char
***line_names" thing.

Now, Just to give you a very concrete example, here are two TOTALLY
UNTESTED patches.

I wrote two, because there's two ways to fix this properly as per
above, and use those functions properly.

The *SANE* way is to just re-organize the code to count things
separately, and then you can allocate it properly with a sane

    char **line_names __free(kfree) = kcalloc(lines,
sizeof(*line_names), GFP_KERNEL);

and not have that crazy "count and fill and return both the count and
the lines" model at all. The above pairs the constructor and
destructor correctly and clearly.

So that's the first "maybe-sane.diff" version. It may be untested,
it's probably still buggy due to that, but it is what I *think* you
should model the real fix around.

The second patch is WAY overkill, and actually creates a "class" for
this all, and keeps the old broken "count and fill in one go", and
returns that *one* value that is just the class, and has a destructor
for that class etc etc.

It's completely broken and ridiculously over-complicated for something
like this, but it's trying to document that way of doing things. For
other things that aren't just one-offs, that whole CLASS() model may
be the right one.

Either of these patches *might* work, but honestly, both of them are
likely broken. The second one in particular is almost certainly buggy
just because it's such a crazy overkill solution, but partly *because*
of how crazy overkill it is, I think it might be a useful example of
what *can* be done.

Again: UNTESTED. They both build for me, but that doesn't say much.

                Linus

View attachment "maybe-sane.diff" of type "text/x-patch" (3156 bytes)

View attachment "silly.diff" of type "text/x-patch" (3359 bytes)

Powered by blists - more mailing lists