Buggy __free(kfree) usage pattern already in tree

Buggy __free(kfree) usage pattern already in tree

[Alexey Dobriyan]

__free() got some usage and some of the usage is buggy:

   832  static struct fwnode_handle *
   833  gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
   834                            struct fwnode_handle *parent)
   835  {
   838          char **line_names __free(kfree) = NULL;
		// returns NULL or ERR_PTR(-E)
   848          line_names = gpio_sim_make_line_names(bank, &line_names_size);
   849          if (IS_ERR(line_names))
   850                  return ERR_CAST(line_names);


This pattern will result in calling kfree() on error value.
And there are no compiler or sparse checking these things.

This test module demonstrates the landmine:

[  812.981089] ------------[ cut here ]------------
[  812.981597] WARNING: CPU: 0 PID: 1326 at mm/slab_common.c:991 free_large_kmalloc+0x50/0x80
[  813.013266] ---[ end trace 0000000000000000 ]---
[  813.013800] object pointer: 0xfffffffffffffff4

#include <linux/module.h>
#include <linux/slab.h>
#include <linux/cleanup.h>

struct S {
	int x;
};

static struct S* f(void)
{
	struct S* s = kmalloc(sizeof(struct S), GFP_KERNEL);
	s = NULL;
	return s ?: ERR_PTR(-ENOMEM);
}

static int __init xxx_module_init(void)
{
	struct S *s __free(kfree) = NULL;
	s = f();
	if (IS_ERR(s)) {
		return PTR_ERR(s);
	}
	return 0;
}

static void __exit xxx_module_exit(void)
{
}
module_init(xxx_module_init);
module_exit(xxx_module_exit);
MODULE_LICENSE("GPL");

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

[Bartosz Golaszewski]

On Fri, 15 Sept 2023 at 11:56, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> __free() got some usage and some of the usage is buggy:
>
>    832  static struct fwnode_handle *
>    833  gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
>    834                            struct fwnode_handle *parent)
>    835  {
>    838          char **line_names __free(kfree) = NULL;
>                 // returns NULL or ERR_PTR(-E)
>    848          line_names = gpio_sim_make_line_names(bank, &line_names_size);
>    849          if (IS_ERR(line_names))
>    850                  return ERR_CAST(line_names);
>

Thanks for the report, I'll send a fix.

Bart

>
> This pattern will result in calling kfree() on error value.
> And there are no compiler or sparse checking these things.
>
> This test module demonstrates the landmine:
>
> [  812.981089] ------------[ cut here ]------------
> [  812.981597] WARNING: CPU: 0 PID: 1326 at mm/slab_common.c:991 free_large_kmalloc+0x50/0x80
> [  813.013266] ---[ end trace 0000000000000000 ]---
> [  813.013800] object pointer: 0xfffffffffffffff4
>
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/cleanup.h>
>
> struct S {
>         int x;
> };
>
> static struct S* f(void)
> {
>         struct S* s = kmalloc(sizeof(struct S), GFP_KERNEL);
>         s = NULL;
>         return s ?: ERR_PTR(-ENOMEM);
> }
>
> static int __init xxx_module_init(void)
> {
>         struct S *s __free(kfree) = NULL;
>         s = f();
>         if (IS_ERR(s)) {
>                 return PTR_ERR(s);
>         }
>         return 0;
> }
>
> static void __exit xxx_module_exit(void)
> {
> }
> module_init(xxx_module_init);
> module_exit(xxx_module_exit);
> MODULE_LICENSE("GPL");

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

[Linus Torvalds]

On Fri, 15 Sept 2023 at 02:56, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> __free() got some usage and some of the usage is buggy:

Yeah, that's sad.

I think the '__free(kfree)' thing should *only* be used in the form

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

which is what our docs mention. Anything else is simply buggy.

But how do we *notice* this?

I do want to stress how I was unhappy about this conversion to begin with

    https://lore.kernel.org/lkml/CAHk-=wigZt6kVkY0HU1j_LJ5H1KzwPiYnwwk6CbqXqT=sGenjg@mail.gmail.com/

but I reacted to the wrong issue.

This stuff needs to be done *way* more carefully.

              Linus

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

[Bartosz Golaszewski]

On Fri, 15 Sept 2023 at 19:04, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, 15 Sept 2023 at 02:56, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> >
> > __free() got some usage and some of the usage is buggy:
>
> Yeah, that's sad.
>
> I think the '__free(kfree)' thing should *only* be used in the form
>
>         struct obj *p __free(kfree) = kmalloc(...);
>
> which is what our docs mention. Anything else is simply buggy.
>
> But how do we *notice* this?
>
> I do want to stress how I was unhappy about this conversion to begin with
>
>     https://lore.kernel.org/lkml/CAHk-=wigZt6kVkY0HU1j_LJ5H1KzwPiYnwwk6CbqXqT=sGenjg@mail.gmail.com/
>
> but I reacted to the wrong issue.
>
> This stuff needs to be done *way* more carefully.
>
>               Linus

This is why I started with a *testing* driver. It's got "simulator" in
the name for a reason. It doesn't deal with real HW and is mostly run
in VMs anyway. Few people even build it at all so it makes for good
testing grounds for experimental features.

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.

Bartosz

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

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 3946 bytes --]

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

[-- Attachment #2: maybe-sane.diff --]
[-- Type: text/x-patch, Size: 3156 bytes --]

 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);
 }

[-- Attachment #3: silly.diff --]
[-- Type: text/x-patch, Size: 3359 bytes --]

 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);
 }

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

[Bartosz Golaszewski]

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

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

[Bartosz Golaszewski]

On Fri, 15 Sept 2023 at 21:27, Bartosz Golaszewski
<bartosz.golaszewski@linaro.org> wrote:
>

[snip!]

>
> Understood. I'll go with a modified version of maybe-sane. I'll send a
> v2 tomorrow and make sure to Cc you.
>

[snip!]

One more question wrt the __free() coding style.

Is the following acceptable:

void foo(void)
{
    char *s __free(kfree) = NULL;

    do_stuff();
    s = kmalloc(42, GFP_KERNEL);
}

Or does it always have to be:

void foo(void)
{
    do_stuff();
    char *s __free(kfree) = kmalloc(42, GFP_KERNEL);
}

?

I guess it would be useful to get these rules into
Documentation/process/coding-style.rst at some point as we also have
an ongoing discussion about whether scoped guards should always
require curly braces[1] even for single statements.

Bartosz

[1] https://lore.kernel.org/lkml/CAMuHMdVYDSPGP48OXxi-s4GFegfzUu900ASBnRmMo=18UzmCrQ@mail.gmail.com/

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

[Linus Torvalds]

On Fri, 15 Sept 2023 at 13:04, Bartosz Golaszewski
<bartosz.golaszewski@linaro.org> wrote:
>
> One more question wrt the __free() coding style.

I don't think we really have much of a coding style yet.

We currently literally have _one_ use of that __free() thing, and it
was problematic.

Which is why I'd like to start off fairly strict, but I'm not sure we
should make it a "coding style" yet.

IOW, my current thinking is "let's always have the constructor and
destructor together", and see how it ends up going.

Not because I think it's necessarily any kind of final rule, but
because I think our whole cleanup thing is new enough that I think
we're better off being a bit inflexible, and having a syntax where a
simple "grep" ends up showing pretty much exactly what is going on wrt
the pairing.

                  Linus

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

[Peter Zijlstra]

On Fri, Sep 15, 2023 at 01:40:25PM -0700, Linus Torvalds wrote:

> Not because I think it's necessarily any kind of final rule, but
> because I think our whole cleanup thing is new enough that I think
> we're better off being a bit inflexible, and having a syntax where a
> simple "grep" ends up showing pretty much exactly what is going on wrt
> the pairing.

So in the perf-event conversion patches I do have this: 

	struct task_struct *task __free(put_task) = NULL;

	...

	if (pid != -1) {
		task = find_lively_task_by_vpid(pid);
		if (!task)
			return -ESRCH;
	}

	...

pattern. The having of task is fully optional in the code-flow.

I suppose I can try and rewrite that a little something like:

	...

	struct task_struct *task __free(put_task) =
		find_lively_task_by_vpid(pid); /* ensure pid==-1 returns NULL */

	if (!task && pid > 0)
		return -ESRCH;

	...


But a little later in that same function I then have:

	do {
		struct rw_semaphore *exec_update_lock __free(up_read) = NULL;
		if (task) {
			err = down_read_interruptible(&task->signal->exec_update_lock);
			if (err)
				return err;

			exec_update_lock = &task->signal->exec_update_lock;

			if (!perf_check_permissions(&attr, task))
				return -EACCESS;
		}

		... stuff serialized against exec *if* this is a task event ...

	} while (0);


And that might be a little harder to 'fix'.


I suppose I'm saying that when thing truly are conditional, this is a
useful pattern, but avoid where reasonably possible.

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

[Peter Zijlstra]

On Fri, Sep 15, 2023 at 11:08:51PM +0200, Peter Zijlstra wrote:

> But a little later in that same function I then have:
> 
> 	do {
> 		struct rw_semaphore *exec_update_lock __free(up_read) = NULL;
> 		if (task) {
> 			err = down_read_interruptible(&task->signal->exec_update_lock);
> 			if (err)
> 				return err;
> 
> 			exec_update_lock = &task->signal->exec_update_lock;
> 
> 			if (!perf_check_permissions(&attr, task))
> 				return -EACCESS;
> 		}
> 
> 		... stuff serialized against exec *if* this is a task event ...
> 
> 	} while (0);
> 
> 
> And that might be a little harder to 'fix'.

Hmm, perhaps I can do a class for it and write the thing like:

	do {
		CLASS(cond_exec_update_lock, exec_lock_guard)(task); /* allow task == NULL */
		if (task && !exec_lock_guard)
			return -EINTR;

		if (task && !perf_check_permissions(&attr, task))
			return -EACCESS;

		... the rest ...

	} while (0);

that might be nicer..

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

[Linus Torvalds]

On Fri, 15 Sept 2023 at 14:18, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Hmm, perhaps I can do a class for it and write the thing like:

.. crossed emails.

Yes.

Except I think a full-fledged class thing is overkill for a one-time
use, and I think you really could just write it out as plain "this is
the constructor, this is the cleanup".

Yes, yes, that is obviously what our CLASS() thing *is*, but at least
my personal mental model is that a "class" is for when you really
export it to other uses.

If you just have a one-off, you don't need the class abstraction to
export to anybody else.  You just do a one-off "alloc like this, free
like this".

Again, this I think is really just a "mental model" rather than any hard rule.

               Linus

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

[Linus Torvalds]

On Fri, 15 Sept 2023 at 14:08, Peter Zijlstra <peterz@infradead.org> wrote:
>
> So in the perf-event conversion patches I do have this:
>
>         struct task_struct *task __free(put_task) = NULL;
>
>         ...
>
>         if (pid != -1) {
>                 task = find_lively_task_by_vpid(pid);
>                 if (!task)
>                         return -ESRCH;
>         }
>
>         ...
>
> pattern. The having of task is fully optional in the code-flow.

Yeah, if you end up having conditional initialization, you can't have
the cleanup declaration in the same place, since it would be in an
inner scope and get free'd immediately.

Still, I think that's likely the exception rather than the rule.

Side note: I hope your code snippets are "something like this" rather
than the real deal.

Because code like this:

> But a little later in that same function I then have:
>
>      do {
>              struct rw_semaphore *exec_update_lock __free(up_read) = NULL;
>              if (task) {
>                      err = down_read_interruptible(&task->signal->exec_update_lock);
>
>              struct rw_semaphore *exec_update_lock __free(up_read) = NULL;

is just garbage. That's not a "freeing" function. That should be "__cleanup()".

The last thing we want is misleading naming, making people think that
you are "freeing" a lock.

Naming is hard, let's not make it worse by making it actively misleading.

And honestly, I think the above is actually a *HORIBLE* argument for
doing that "initialize to NULL, change later". I think the above is
exactly the kind of code that we ABSOLUTELY DO NOT WANT.

You should aim for a nice

        struct rw_semaphore *struct rw_semaphore *exec_update_lock
            __cleanup(release_exec_update_lock) = get_exec_update_lock(task);

and simply have proper constructors and destructors. It's going to be
much cleaner.

You can literally do something like

    static inline void release_exec_update_lock(struct rw_semaphore *sem)
    { if (!IS_ERR_OR_NULL(sem)) up_read(sem); }

    static inline void get_exec_update_lock(struct task_struct *tsk)
    {
        if (!task)
                return NULL;
        if (down_read_interruptible(&task->signal->exec_update_lock))
                return ERR_PTR(-EINTR);
        retuin &task->signal->exec_update_lock;
    }

and the code will be *much* cleaner, wouldn't you say?

Please use proper constructors and destructors when you do these kinds
of automatic cleanup things. Don't write ad-hoc garbage.

You'll thank me a year from now when the code is actually legible.

                 Linus

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

[Peter Zijlstra]

On Fri, Sep 15, 2023 at 02:22:02PM -0700, Linus Torvalds wrote:

> Naming is hard, let's not make it worse by making it actively misleading.

I actually did use the DEFINE_FREE() helper, will go fix. Because yes,
free is not the right word in this case.

> And honestly, I think the above is actually a *HORIBLE* argument for
> doing that "initialize to NULL, change later". I think the above is
> exactly the kind of code that we ABSOLUTELY DO NOT WANT.
> 
> You should aim for a nice
> 
>         struct rw_semaphore *struct rw_semaphore *exec_update_lock
>             __cleanup(release_exec_update_lock) = get_exec_update_lock(task);

Ah, that might be nicer still than the class thing I proposed in a
follow up email.

It also got me thinking about named_guard() for the myriad of
conditional locks we have.

	named_guard(try_mutex, foo_guard)(&foo->lock);
	if (foo_guard) {
		// we got the lock, do our thing
	}


or

	named_guard(interruptible_mutex, foo_guard)(&foo->lock);
	if (!foo_guard)
		return -EINTR;


Are these sane patterns?

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

[Linus Torvalds]

On Fri, 15 Sept 2023 at 14:32, Peter Zijlstra <peterz@infradead.org> wrote:
>
>
> It also got me thinking about named_guard() for the myriad of
> conditional locks we have.
>
>         named_guard(try_mutex, foo_guard)(&foo->lock);
>         if (foo_guard) {
>                 // we got the lock, do our thing
>         }

Hmm. It looks ugly to me. I really hate the "named_guard" thing. One
of the reasons I liked the guard/scoped_guard() macros was because how
it created _anonymous_ guards, and made it completely unnecessary to
make up a pointless name.

If trylock ends up being a common pattern, I think we should strive to
make it a lot easier to use.

Can we make it act like "scoped_guard()", except the lock function is
fundamentally conditional?

Call it "cond_guard()", and make the syntax otherwise be the same as
"scoped_guard()", iow, using a unique ID for the guard name.

So

        cond_guard(try_mutex)(&foo->lock) {
                .. this is the "we got the lock" region ..
        }

would I think be a much better syntax.

Could we live with that?

               Linus

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

[Linus Torvalds]

On Fri, 15 Sept 2023 at 14:50, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Call it "cond_guard()", and make the syntax otherwise be the same as
> "scoped_guard()", iow, using a unique ID for the guard name.

I *think* you don't even need a new "cond_guard()" macro.

This might be as simple as having our current scoped_guard() macro
have the conditional

  "scope.lock && !done"

instead of just the current "!done".

Then you introduce "trylock" variants that conditionally set "lock = 1".

I dunno. Maybe there's something really obvious I'm missing, but I
really think that gives us the option to literally do just

        scoped_guard(try_mutex, ..somelock..) {
                ...
        }

and all it requires is for us to declare a "try_mutex" lock guard class.

Ok, so the current DEFINE_LOCK_GUARD_x() macros clearly set .lock to 1
unconditionally, so there's a small matter of extending the lock side
to be conditional...

"SMOP".

                 Linus

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

[Peter Zijlstra]

On Fri, Sep 15, 2023 at 02:50:48PM -0700, Linus Torvalds wrote:
> On Fri, 15 Sept 2023 at 14:32, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> >
> > It also got me thinking about named_guard() for the myriad of
> > conditional locks we have.
> >
> >         named_guard(try_mutex, foo_guard)(&foo->lock);
> >         if (foo_guard) {
> >                 // we got the lock, do our thing
> >         }
> 
> Hmm. It looks ugly to me. I really hate the "named_guard" thing. One
> of the reasons I liked the guard/scoped_guard() macros was because how
> it created _anonymous_ guards, and made it completely unnecessary to
> make up a pointless name.

Yeah, the anonymous thing is very nice.

My ulterior motive for the above was perhaps extending it to allow the
lock argument to be NULL. Which would give us a handy conditional
pattern.

	struct rw_semaphore *exec_update_lock = task ?  &task->exec_update_lock : NULL;
	named_guard(rwsem_read_interruptible, exec_lock_guard)(exec_update_lock);
	if (task && !exec_lock_guard)
		return -EINTR;

And yes, that is definitely not pretty, but it does provide a fairly
wide array of options.

> If trylock ends up being a common pattern, I think we should strive to
> make it a lot easier to use.
> 
> Can we make it act like "scoped_guard()", except the lock function is
> fundamentally conditional?
> 
> Call it "cond_guard()", and make the syntax otherwise be the same as
> "scoped_guard()", iow, using a unique ID for the guard name.
> 
> So
> 
>         cond_guard(try_mutex)(&foo->lock) {
>                 .. this is the "we got the lock" region ..
>         }
> 
> would I think be a much better syntax.
> 
> Could we live with that?

For the trypical use-case that is definitely the more appealing syntax.

Something like:

  #define cond_guard(_name, args...) \
	  for (CLASS(_name, scope)(args), *done = NULL; \
	       !done && scope; done = (void *)1)
	     
works for the simple cases, but something like: try_spinlock_irqsave
would be a bit of a challenge because that would end up with one of
those structs that is not a pointer and they don't cast to a boolean :/

I think I can make it work, I'll go have a play, but perhaps not now,
it's past midnight ;-)

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

[Peter Zijlstra]

On Sat, Sep 16, 2023 at 12:13:32AM +0200, Peter Zijlstra wrote:

> I think I can make it work, I'll go have a play, but perhaps not now,
> it's past midnight ;-)

So I have been playing about with this a bit and it keeps falling short
of 'nice' :/

What I ended up with is the below. The simple scoped_guard () extension
is useful, albeit slightly awkward (I'll reply with another patch making
use of it). But the basic problem is that it will have to have the form:

	scoped_guard (mutex_intr, &task->signal->cred_guard_mutex) {
		...
		return 0;
	}
	return -EINTR;

That is; the guard body must terminate such that after the body is the
'fail' case. This is not always convenient.

Additionally, this did not help with my case where I need to hold the
lock conditionally (when I have a task) but need to execute the body
unconditionally. Then the above form just doesn't work.

For that, I ended up with:

	cond_guard(name, stmt, args...)

Which can be used like:


	do {
		cond_guard(rwsem_down_intr, if (task) return -EINTR,
			   task ? task->signal->exec_update_lock : NULL);

		... do stuff that is conditionally guarded by @exec_update_lock ...

	} while (0);


I'll continue poking for a bit,.. see if there's something better.


---
Subject: cleanup: Add conditional guard support
From: Peter Zijlstra <peterz@infradead.org>
Date: Sun Sep 17 13:22:17 CEST 2023

Adds:

 - DEFINE_GUARD_COND() / DEFINE_LOCK_GUARD_1_COND() to extend existing
   guards with conditional lock primitives, eg. mutex_trylock(),
   mutex_lock_interruptible().

   nb. both primitives allow NULL 'locks', which cause the lock to
       fail (obviously).

 - extends scoped_guard() to not take the body when the the
   conditional guard 'fails'. eg.

     scoped_guard (mutex_intr, &task->signal_cred_guard_mutex) {
	...
     }

   will only execute the body when the mutex is held.

 - provides cond_guard(name, stmt, args...); which extends guard()
   to execute @stmt when the lock fails.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
Index: linux-2.6/include/linux/cleanup.h
===================================================================
--- linux-2.6.orig/include/linux/cleanup.h
+++ linux-2.6/include/linux/cleanup.h
@@ -136,14 +136,36 @@ static inline class_##_name##_t class_##
  */
 
 #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
-	DEFINE_CLASS(_name, _type, _unlock, ({ _lock; _T; }), _type _T)
+	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \
+	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
+	{ return *_T; }
+
+#define DEFINE_GUARD_COND(_name, _ext, _condlock) \
+	EXTEND_CLASS(_name, _ext, \
+		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \
+		     class_##_name##_t _T) \
+	static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
+	{ return class_##_name##_lock_ptr(_T); }
+
+#define _guard(_name, var)						\
+	class_##_name##_t var __cleanup(class_##_name##_destructor) =	\
+		class_##_name##_constructor
 
 #define guard(_name) \
-	CLASS(_name, __UNIQUE_ID(guard))
+	_guard(_name, __UNIQUE_ID(guard))
+
+#define __guard_ptr(_name) class_##_name##_lock_ptr
 
 #define scoped_guard(_name, args...)					\
 	for (CLASS(_name, scope)(args),					\
-	     *done = NULL; !done; done = (void *)1)
+	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
+
+#define _cond_guard(_name, _var, _stmt, args...) \
+	_guard(_name, _var)(args); \
+	if (!__guard_ptr(_name)(&_var)) _stmt
+
+#define cond_guard(_name, _stmt, args...) \
+	_cond_guard(_name, __UNIQUE_ID(guard), _stmt, args)
 
 /*
  * Additional helper macros for generating lock guards with types, either for
@@ -173,6 +195,11 @@ typedef struct {							\
 static inline void class_##_name##_destructor(class_##_name##_t *_T)	\
 {									\
 	if (_T->lock) { _unlock; }					\
+}									\
+									\
+static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T)	\
+{									\
+	return _T->lock;						\
 }
 
 
@@ -201,4 +228,14 @@ __DEFINE_LOCK_GUARD_1(_name, _type, _loc
 __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__)		\
 __DEFINE_LOCK_GUARD_0(_name, _lock)
 
+#define DEFINE_LOCK_GUARD_1_COND(_name, _ext, _condlock)		\
+	EXTEND_CLASS(_name, _ext,					\
+		     ({ class_##_name##_t _t = { .lock = l }, *_T = &_t;\
+		        if (_T->lock && !(_condlock)) _T->lock = NULL;	\
+			_t; }),						\
+		     typeof_member(class_##_name##_t, lock) l)		\
+	static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
+	{ return class_##_name##_lock_ptr(_T); }
+
+
 #endif /* __LINUX_GUARDS_H */
Index: linux-2.6/include/linux/mutex.h
===================================================================
--- linux-2.6.orig/include/linux/mutex.h
+++ linux-2.6/include/linux/mutex.h
@@ -221,6 +221,7 @@ extern void mutex_unlock(struct mutex *l
 extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
 
 DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
-DEFINE_FREE(mutex, struct mutex *, if (_T) mutex_unlock(_T))
+DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
+DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
 
 #endif /* __LINUX_MUTEX_H */
Index: linux-2.6/include/linux/spinlock.h
===================================================================
--- linux-2.6.orig/include/linux/spinlock.h
+++ linux-2.6/include/linux/spinlock.h
@@ -507,6 +507,8 @@ DEFINE_LOCK_GUARD_1(raw_spinlock, raw_sp
 		    raw_spin_lock(_T->lock),
 		    raw_spin_unlock(_T->lock))
 
+DEFINE_LOCK_GUARD_1_COND(raw_spinlock, _try, raw_spin_trylock(_T->lock))
+
 DEFINE_LOCK_GUARD_1(raw_spinlock_nested, raw_spinlock_t,
 		    raw_spin_lock_nested(_T->lock, SINGLE_DEPTH_NESTING),
 		    raw_spin_unlock(_T->lock))
@@ -515,23 +517,36 @@ DEFINE_LOCK_GUARD_1(raw_spinlock_irq, ra
 		    raw_spin_lock_irq(_T->lock),
 		    raw_spin_unlock_irq(_T->lock))
 
+DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irq, _try, raw_spin_trylock_irq(_T->lock))
+
 DEFINE_LOCK_GUARD_1(raw_spinlock_irqsave, raw_spinlock_t,
 		    raw_spin_lock_irqsave(_T->lock, _T->flags),
 		    raw_spin_unlock_irqrestore(_T->lock, _T->flags),
 		    unsigned long flags)
 
+DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irqsave, _try,
+			 raw_spin_trylock_irqsave(_T->lock, _T->flags))
+
 DEFINE_LOCK_GUARD_1(spinlock, spinlock_t,
 		    spin_lock(_T->lock),
 		    spin_unlock(_T->lock))
 
+DEFINE_LOCK_GUARD_1_COND(spinlock, _try, spin_trylock(_T->lock))
+
 DEFINE_LOCK_GUARD_1(spinlock_irq, spinlock_t,
 		    spin_lock_irq(_T->lock),
 		    spin_unlock_irq(_T->lock))
 
+DEFINE_LOCK_GUARD_1_COND(spinlock_irq, _try,
+			 spin_trylock_irq(_T->lock))
+
 DEFINE_LOCK_GUARD_1(spinlock_irqsave, spinlock_t,
 		    spin_lock_irqsave(_T->lock, _T->flags),
 		    spin_unlock_irqrestore(_T->lock, _T->flags),
 		    unsigned long flags)
 
+DEFINE_LOCK_GUARD_1_COND(spinlock_irqsave, _try,
+			 spin_trylock_irqsave(_T->lock, _T->flags))
+
 #undef __LINUX_INSIDE_SPINLOCK_H
 #endif /* __LINUX_SPINLOCK_H */
Index: linux-2.6/include/linux/rwsem.h
===================================================================
--- linux-2.6.orig/include/linux/rwsem.h
+++ linux-2.6/include/linux/rwsem.h
@@ -203,11 +203,11 @@ extern void up_read(struct rw_semaphore
 extern void up_write(struct rw_semaphore *sem);
 
 DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T))
-DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
-
-DEFINE_FREE(up_read, struct rw_semaphore *, if (_T) up_read(_T))
-DEFINE_FREE(up_write, struct rw_semaphore *, if (_T) up_write(_T))
+DEFINE_GUARD_COND(rwsem_read, _try, down_read_trylock(_T))
+DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T) == 0)
 
+DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
+DEFINE_GUARD_COND(rwsem_write, _try, down_write_trylock(_T))
 
 /*
  * downgrade write lock to read lock

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

[Peter Zijlstra]

On Tue, Sep 19, 2023 at 02:57:52PM +0200, Peter Zijlstra wrote:
> On Sat, Sep 16, 2023 at 12:13:32AM +0200, Peter Zijlstra wrote:
> 
> > I think I can make it work, I'll go have a play, but perhaps not now,
> > it's past midnight ;-)
> 
> So I have been playing about with this a bit and it keeps falling short
> of 'nice' :/
> 
> What I ended up with is the below. The simple scoped_guard () extension
> is useful, albeit slightly awkward (I'll reply with another patch making
> use of it). But the basic problem is that it will have to have the form:

---
Index: linux-2.6/include/linux/sched/task.h
===================================================================
--- linux-2.6.orig/include/linux/sched/task.h
+++ linux-2.6/include/linux/sched/task.h
@@ -226,4 +226,6 @@ static inline void task_unlock(struct ta
 	spin_unlock(&p->alloc_lock);
 }
 
+DEFINE_GUARD(task_lock, struct task_struct *, task_lock(_T), task_unlock(_T))
+
 #endif /* _LINUX_SCHED_TASK_H */
Index: linux-2.6/include/linux/spinlock.h
===================================================================
--- linux-2.6.orig/include/linux/spinlock.h
+++ linux-2.6/include/linux/spinlock.h
@@ -548,5 +548,31 @@ DEFINE_LOCK_GUARD_1(spinlock_irqsave, sp
 DEFINE_LOCK_GUARD_1_COND(spinlock_irqsave, _try,
 			 spin_trylock_irqsave(_T->lock, _T->flags))
 
+DEFINE_LOCK_GUARD_1(read_lock, rwlock_t,
+		    read_lock(_T->lock),
+		    read_unlock(_T->lock))
+
+DEFINE_LOCK_GUARD_1(read_lock_irq, rwlock_t,
+		    read_lock_irq(_T->lock),
+		    read_unlock_irq(_T->lock))
+
+DEFINE_LOCK_GUARD_1(read_lock_irqsave, rwlock_t,
+		    read_lock_irqsave(_T->lock, _T->flags),
+		    read_unlock_irqrestore(_T->lock, _T->flags),
+		    unsigned long flags)
+
+DEFINE_LOCK_GUARD_1(write_lock, rwlock_t,
+		    write_lock(_T->lock),
+		    write_unlock(_T->lock))
+
+DEFINE_LOCK_GUARD_1(write_lock_irq, rwlock_t,
+		    write_lock_irq(_T->lock),
+		    write_unlock_irq(_T->lock))
+
+DEFINE_LOCK_GUARD_1(write_lock_irqsave, rwlock_t,
+		    write_lock_irqsave(_T->lock, _T->flags),
+		    write_unlock_irqrestore(_T->lock, _T->flags),
+		    unsigned long flags)
+
 #undef __LINUX_INSIDE_SPINLOCK_H
 #endif /* __LINUX_SPINLOCK_H */
Index: linux-2.6/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/kernel/ptrace.c
+++ linux-2.6/kernel/ptrace.c
@@ -386,6 +386,34 @@ static int check_ptrace_options(unsigned
 	return 0;
 }
 
+static inline void ptrace_set_stopped(struct task_struct *task)
+{
+	guard(spinlock)(&task->sighand->siglock);
+
+	/*
+	 * If the task is already STOPPED, set JOBCTL_TRAP_STOP and
+	 * TRAPPING, and kick it so that it transits to TRACED.  TRAPPING
+	 * will be cleared if the child completes the transition or any
+	 * event which clears the group stop states happens.  We'll wait
+	 * for the transition to complete before returning from this
+	 * function.
+	 *
+	 * This hides STOPPED -> RUNNING -> TRACED transition from the
+	 * attaching thread but a different thread in the same group can
+	 * still observe the transient RUNNING state.  IOW, if another
+	 * thread's WNOHANG wait(2) on the stopped tracee races against
+	 * ATTACH, the wait(2) may fail due to the transient RUNNING.
+	 *
+	 * The following task_is_stopped() test is safe as both transitions
+	 * in and out of STOPPED are protected by siglock.
+	 */
+	if (task_is_stopped(task) &&
+	    task_set_jobctl_pending(task, JOBCTL_TRAP_STOP | JOBCTL_TRAPPING)) {
+		task->jobctl &= ~JOBCTL_STOPPED;
+		signal_wake_up_state(task, __TASK_STOPPED);
+	}
+}
+
 static int ptrace_attach(struct task_struct *task, long request,
 			 unsigned long addr,
 			 unsigned long flags)
@@ -393,17 +421,17 @@ static int ptrace_attach(struct task_str
 	bool seize = (request == PTRACE_SEIZE);
 	int retval;
 
-	retval = -EIO;
 	if (seize) {
 		if (addr != 0)
-			goto out;
+			return -EIO;
 		/*
 		 * This duplicates the check in check_ptrace_options() because
 		 * ptrace_attach() and ptrace_setoptions() have historically
 		 * used different error codes for unknown ptrace options.
 		 */
 		if (flags & ~(unsigned long)PTRACE_O_MASK)
-			goto out;
+			return -EIO;
+
 		retval = check_ptrace_options(flags);
 		if (retval)
 			return retval;
@@ -414,88 +442,58 @@ static int ptrace_attach(struct task_str
 
 	audit_ptrace(task);
 
-	retval = -EPERM;
 	if (unlikely(task->flags & PF_KTHREAD))
-		goto out;
+		return -EPERM;
 	if (same_thread_group(task, current))
-		goto out;
+		return -EPERM;
 
 	/*
 	 * Protect exec's credential calculations against our interference;
 	 * SUID, SGID and LSM creds get determined differently
 	 * under ptrace.
 	 */
-	retval = -ERESTARTNOINTR;
-	if (mutex_lock_interruptible(&task->signal->cred_guard_mutex))
-		goto out;
-
-	task_lock(task);
-	retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
-	task_unlock(task);
-	if (retval)
-		goto unlock_creds;
-
-	write_lock_irq(&tasklist_lock);
-	retval = -EPERM;
-	if (unlikely(task->exit_state))
-		goto unlock_tasklist;
-	if (task->ptrace)
-		goto unlock_tasklist;
-
-	task->ptrace = flags;
-
-	ptrace_link(task, current);
-
-	/* SEIZE doesn't trap tracee on attach */
-	if (!seize)
-		send_sig_info(SIGSTOP, SEND_SIG_PRIV, task);
+	scoped_guard (mutex_intr, &task->signal->cred_guard_mutex) {
 
-	spin_lock(&task->sighand->siglock);
+		scoped_guard (task_lock, task) {
+			retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
+			if (retval)
+				return retval;
+		}
 
-	/*
-	 * If the task is already STOPPED, set JOBCTL_TRAP_STOP and
-	 * TRAPPING, and kick it so that it transits to TRACED.  TRAPPING
-	 * will be cleared if the child completes the transition or any
-	 * event which clears the group stop states happens.  We'll wait
-	 * for the transition to complete before returning from this
-	 * function.
-	 *
-	 * This hides STOPPED -> RUNNING -> TRACED transition from the
-	 * attaching thread but a different thread in the same group can
-	 * still observe the transient RUNNING state.  IOW, if another
-	 * thread's WNOHANG wait(2) on the stopped tracee races against
-	 * ATTACH, the wait(2) may fail due to the transient RUNNING.
-	 *
-	 * The following task_is_stopped() test is safe as both transitions
-	 * in and out of STOPPED are protected by siglock.
-	 */
-	if (task_is_stopped(task) &&
-	    task_set_jobctl_pending(task, JOBCTL_TRAP_STOP | JOBCTL_TRAPPING)) {
-		task->jobctl &= ~JOBCTL_STOPPED;
-		signal_wake_up_state(task, __TASK_STOPPED);
-	}
+		scoped_guard (write_lock, &tasklist_lock) {
+			if (unlikely(task->exit_state))
+				return -EPERM;
+			if (task->ptrace)
+				return -EPERM;
 
-	spin_unlock(&task->sighand->siglock);
+			task->ptrace = flags;
 
-	retval = 0;
-unlock_tasklist:
-	write_unlock_irq(&tasklist_lock);
-unlock_creds:
-	mutex_unlock(&task->signal->cred_guard_mutex);
-out:
-	if (!retval) {
-		/*
-		 * We do not bother to change retval or clear JOBCTL_TRAPPING
-		 * if wait_on_bit() was interrupted by SIGKILL. The tracer will
-		 * not return to user-mode, it will exit and clear this bit in
-		 * __ptrace_unlink() if it wasn't already cleared by the tracee;
-		 * and until then nobody can ptrace this task.
-		 */
-		wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT, TASK_KILLABLE);
-		proc_ptrace_connector(task, PTRACE_ATTACH);
+			ptrace_link(task, current);
+
+			/* SEIZE doesn't trap tracee on attach */
+			if (!seize)
+				send_sig_info(SIGSTOP, SEND_SIG_PRIV, task);
+
+			ptrace_set_stopped(task);
+
+		}
+
+		goto success;
 	}
+	return -ERESTARTNOINTR;
 
-	return retval;
+success:
+	/*
+	 * We do not bother to change retval or clear JOBCTL_TRAPPING
+	 * if wait_on_bit() was interrupted by SIGKILL. The tracer will
+	 * not return to user-mode, it will exit and clear this bit in
+	 * __ptrace_unlink() if it wasn't already cleared by the tracee;
+	 * and until then nobody can ptrace this task.
+	 */
+	wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT, TASK_KILLABLE);
+	proc_ptrace_connector(task, PTRACE_ATTACH);
+
+	return 0;
 }
 
 /**

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

[Peter Zijlstra]

On Tue, Sep 19, 2023 at 02:59:54PM +0200, Peter Zijlstra wrote:


> +	scoped_guard (mutex_intr, &task->signal->cred_guard_mutex) {
>  
> +		scoped_guard (task_lock, task) {
> +			retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
> +			if (retval)
> +				return retval;
> +		}
>  
> +		scoped_guard (write_lock, &tasklist_lock) {
> +			if (unlikely(task->exit_state))
> +				return -EPERM;
> +			if (task->ptrace)
> +				return -EPERM;
>  
> +			task->ptrace = flags;
>  
> +			ptrace_link(task, current);
> +
> +			/* SEIZE doesn't trap tracee on attach */
> +			if (!seize)
> +				send_sig_info(SIGSTOP, SEND_SIG_PRIV, task);
> +
> +			ptrace_set_stopped(task);
> +
> +		}
> +
> +		goto success;
>  	}
> +	return -ERESTARTNOINTR;
>  
> +success:
> +	/*
> +	 * We do not bother to change retval or clear JOBCTL_TRAPPING
> +	 * if wait_on_bit() was interrupted by SIGKILL. The tracer will
> +	 * not return to user-mode, it will exit and clear this bit in
> +	 * __ptrace_unlink() if it wasn't already cleared by the tracee;
> +	 * and until then nobody can ptrace this task.
> +	 */
> +	wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT, TASK_KILLABLE);
> +	proc_ptrace_connector(task, PTRACE_ATTACH);
> +
> +	return 0;

This isn't exactly nice..

I tried something like:

	scoped_cond_guard (mutex_intr, return -EINTR, &task->signal->cred_guard_mutex) {
		...
	}

Which I can make work, but then I also tried to capture my other case:

	scoped_cond_guard (rwsem_down_intr, if (task) return -EINTR,
			   task ? &task->signal->exec_guard_mutex : NULL)  {

		...
	}

But I can't get that to work because of that extra if, the not case
doesn't fall through and do the body.

Anyway, I'll poke more..

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

[Peter Zijlstra]

On Tue, Sep 19, 2023 at 03:10:38PM +0200, Peter Zijlstra wrote:

> This isn't exactly nice..
> 
> I tried something like:
> 
> 	scoped_cond_guard (mutex_intr, return -EINTR, &task->signal->cred_guard_mutex) {
> 		...
> 	}
> 
> Which I can make work, but then I also tried to capture my other case:
> 
> 	scoped_cond_guard (rwsem_down_intr, if (task) return -EINTR,
> 			   task ? &task->signal->exec_guard_mutex : NULL)  {
> 
> 		...
> 	}
> 
> But I can't get that to work because of that extra if, the not case
> doesn't fall through and do the body.
> 
> Anyway, I'll poke more..


#define scoped_cond_guard(_name, _label, args...)		\
	for (CLASS(_name, scope)(args),				\
	     *done = NULL; !done; done = (void *)1)		\
		if (!__guard_ptr(_name)(&scope)) goto _label;	\
		else

Allows one to write:

	scoped_cond_guard (rwsem_down_intr, no_lock,
 			   task ? &task->signal->exec_guard_mutex : NULL)  {

		if (0) {
no_lock:
			if (task)
				return -EINTR;
		}

		... block that holds exec_guard_mutex if task ...
	}

Still not exactly pretty, but perhaps better than before...

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

[David Laight]

If this stuff is so hard to get right for non-trivial cases
perhaps it should all be ripped out?

The trivial cases are pretty easy to eve-ball check and
static analysis tools do a reasonable job.

Maybe I'm 'old-school' but I'd much rather see explicit
unlock() and free() than have 'some compiler magic' do
it for you.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)