Messages in this thread | | | From | Bartosz Golaszewski <> | Date | Fri, 15 Sep 2023 21:27:11 +0200 | Subject | Re: Buggy __free(kfree) usage pattern already in tree |
| |
On Fri, 15 Sept 2023 at 21:06, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Fri, 15 Sept 2023 at 10:22, Bartosz Golaszewski > <bartosz.golaszewski@linaro.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
Understood. I'll go with a modified version of maybe-sane. I'll send a v2 tomorrow and make sure to Cc you.
WRT the silly diff: we have a bunch of helpers centered around string arrays in the kernel. I think it would make sense to package a string array into a class because right now we have functions like kfree_strarray() which takes the pointer to the array AND the number of strings to free. So it may not be that silly in the end. But that's a different story.
Bartosz
| |