Re: Buggy __free(kfree) usage pattern already in tree

[Linus Torvalds]

On Fri, 15 Sept 2023 at 10:22, Bartosz Golaszewski
<bartosz.golaszewski@xxxxxxxxxx> 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
 drivers/gpio/gpio-sim.c | 62 +++++++++++++++++++------------------------------
 1 file changed, 24 insertions(+), 38 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 271db3639a78..d506d5d34c09 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -685,52 +685,35 @@ gpio_sim_device_config_live_show(struct config_item *item, char *page)
 	return sprintf(page, "%c\n", live ? '1' : '0');
 }
 
-static char **gpio_sim_make_line_names(struct gpio_sim_bank *bank,
-				       unsigned int *line_names_size)
+static unsigned int gpio_sim_count_lines(struct gpio_sim_bank *bank)
 {
 	unsigned int max_offset = 0;
-	bool has_line_names = false;
 	struct gpio_sim_line *line;
-	char **line_names;
 
 	list_for_each_entry(line, &bank->line_list, siblings) {
 		if (line->offset >= bank->num_lines)
 			continue;
-
-		if (line->name) {
-			if (line->offset > max_offset)
-				max_offset = line->offset;
-
-			/*
-			 * max_offset can stay at 0 so it's not an indicator
-			 * of whether line names were configured at all.
-			 */
-			has_line_names = true;
-		}
+		if (!line->name)
+			continue;
+		if (line->offset < max_offset)
+			continue;
+		max_offset = line->offset + 1;
 	}
 
-	if (!has_line_names)
-		/*
-		 * This is not an error - NULL means, there are no line
-		 * names configured.
-		 */
-		return NULL;
+	return max_offset;
+}
 
-	*line_names_size = max_offset + 1;
-
-	line_names = kcalloc(*line_names_size, sizeof(*line_names), GFP_KERNEL);
-	if (!line_names)
-		return ERR_PTR(-ENOMEM);
+static void gpio_sim_fill_line_names(struct gpio_sim_bank *bank, char **line_names)
+{
+	struct gpio_sim_line *line;
 
 	list_for_each_entry(line, &bank->line_list, siblings) {
 		if (line->offset >= bank->num_lines)
 			continue;
-
-		if (line->name && (line->offset <= max_offset))
-			line_names[line->offset] = line->name;
+		if (!line->name)
+			continue;
+		line_names[line->offset] = line->name;
 	}
-
-	return line_names;
 }
 
 static void gpio_sim_remove_hogs(struct gpio_sim_device *dev)
@@ -834,8 +817,7 @@ gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
 			  struct fwnode_handle *parent)
 {
 	struct property_entry properties[GPIO_SIM_PROP_MAX];
-	unsigned int prop_idx = 0, line_names_size = 0;
-	char **line_names __free(kfree) = NULL;
+	unsigned int prop_idx = 0, lines;
 
 	memset(properties, 0, sizeof(properties));
 
@@ -845,14 +827,18 @@ gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
 		properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
 							       bank->label);
 
-	line_names = gpio_sim_make_line_names(bank, &line_names_size);
-	if (IS_ERR(line_names))
-		return ERR_CAST(line_names);
+	lines = gpio_sim_count_lines(bank);
+	char **line_names __free(kfree) = kcalloc(lines, sizeof(*line_names), GFP_KERNEL);
 
-	if (line_names)
+	if (!line_names)
+		return ERR_PTR(-ENOMEM);
+
+	gpio_sim_fill_line_names(bank, line_names);
+
+	if (lines)
 		properties[prop_idx++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
 						"gpio-line-names",
-						line_names, line_names_size);
+						line_names, lines);
 
 	return fwnode_create_software_node(properties, parent);
 }
 drivers/gpio/gpio-sim.c | 56 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 271db3639a78..ac8416cd96c3 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -685,13 +685,36 @@ gpio_sim_device_config_live_show(struct config_item *item, char *page)
 	return sprintf(page, "%c\n", live ? '1' : '0');
 }
 
-static char **gpio_sim_make_line_names(struct gpio_sim_bank *bank,
-				       unsigned int *line_names_size)
+struct line_names {
+	unsigned int size;
+	char **lines;
+};
+
+static struct line_names make_line_names(unsigned int size)
+{
+	char **lines = kcalloc(size, sizeof(*lines), GFP_KERNEL);
+	if (!lines)
+		return (struct line_names) { 0, ERR_PTR(-ENOMEM) };
+
+	return (struct line_names) { size, lines };
+}
+
+static void free_line_names(struct line_names names)
+{
+	if (!IS_ERR_OR_NULL(names.lines))
+		kfree(names.lines);
+}
+
+DEFINE_CLASS(line_names,
+	struct line_names,
+	free_line_names(_T),
+	make_line_names(size), unsigned int size)
+
+static struct line_names gpio_sim_make_line_names(struct gpio_sim_bank *bank)
 {
 	unsigned int max_offset = 0;
 	bool has_line_names = false;
 	struct gpio_sim_line *line;
-	char **line_names;
 
 	list_for_each_entry(line, &bank->line_list, siblings) {
 		if (line->offset >= bank->num_lines)
@@ -714,23 +737,21 @@ static char **gpio_sim_make_line_names(struct gpio_sim_bank *bank,
 		 * This is not an error - NULL means, there are no line
 		 * names configured.
 		 */
-		return NULL;
+		return (struct line_names) { 0, NULL };
 
-	*line_names_size = max_offset + 1;
-
-	line_names = kcalloc(*line_names_size, sizeof(*line_names), GFP_KERNEL);
-	if (!line_names)
-		return ERR_PTR(-ENOMEM);
+	CLASS(line_names, names)(max_offset + 1);
+	if (IS_ERR(names.lines))
+		return names;
 
 	list_for_each_entry(line, &bank->line_list, siblings) {
 		if (line->offset >= bank->num_lines)
 			continue;
 
 		if (line->name && (line->offset <= max_offset))
-			line_names[line->offset] = line->name;
+			names.lines[line->offset] = line->name;
 	}
 
-	return line_names;
+	return (struct line_names) { names.size, no_free_ptr(names.lines) };
 }
 
 static void gpio_sim_remove_hogs(struct gpio_sim_device *dev)
@@ -834,8 +855,7 @@ gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
 			  struct fwnode_handle *parent)
 {
 	struct property_entry properties[GPIO_SIM_PROP_MAX];
-	unsigned int prop_idx = 0, line_names_size = 0;
-	char **line_names __free(kfree) = NULL;
+	unsigned int prop_idx = 0;
 
 	memset(properties, 0, sizeof(properties));
 
@@ -845,14 +865,14 @@ gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
 		properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
 							       bank->label);
 
-	line_names = gpio_sim_make_line_names(bank, &line_names_size);
-	if (IS_ERR(line_names))
-		return ERR_CAST(line_names);
+	class_line_names_t names __cleanup(class_line_names_destructor) = gpio_sim_make_line_names(bank);
+	if (IS_ERR(names.lines))
+		return ERR_CAST(names.lines);
 
-	if (line_names)
+	if (names.size)
 		properties[prop_idx++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
 						"gpio-line-names",
-						line_names, line_names_size);
+						names.lines, names.size);
 
 	return fwnode_create_software_node(properties, parent);
 }