perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

There is no need to free NULL pointers.

index f5a3afc..c358877 100644
@@ -135,12 +135,12 @@ static int __init pt_pmu_hw_init(void)
 	size = sizeof(struct attribute *) * (ARRAY_SIZE(pt_caps) + 1);
 	attrs = kzalloc(size, GFP_KERNEL);
 	if (!attrs)
-		goto err_attrs;
+		return -ENOMEM;
 
 	size = sizeof(struct dev_ext_attribute) * (ARRAY_SIZE(pt_caps) + 1);
 	de_attrs = kzalloc(size, GFP_KERNEL);
 	if (!de_attrs)
-		goto err_de_attrs;
+		goto err_attrs;
 
 	for (i = 0; i < ARRAY_SIZE(pt_caps); i++) {
 		de_attrs[i].attr.attr.name = pt_caps[i].name;

@@ -155,8 +155,6 @@ static int __init pt_pmu_hw_init(void)
 	pt_cap_group.attrs = attrs;
 	return 0;
 
-err_de_attrs:
-	kfree(de_attrs);
 err_attrs:
 	kfree(attrs);
 

There was one earlier already.

Also I have come to despise "err" labels. They do one of three things:

1) Nothing. These are a pointless interruption for people reading the code from top to bottom because you wonder, "How is goto err different from return -ENOMEM?". You scroll down to the bottom and find that they are the same. Now you have lost your place and your train of thought.

2) One thing. In this case the label is poorly named, better to say "goto unlock".

3) Everything. This is a leading source of error handling bugs. It looks like this.

err:

	kfree(foo);
	return ret;

Later we will change it so that foo is allocated with kmemdup_user() instead of kmalloc() so now kfree() will oops trying to free the error pointer. I see so many one err bugs it's not even funny.

regards,
dan carpenter

4) unwind complex state; see for example copy_process(). You do not want to endlessly duplicate the increasing cleanup for every exit.

That's just poor engineering in the first place, if you change the alloc site you had better also look for all the free sites. Error labels have nothing to do with that, alloc/free can be otherwise separated and hard to correlate, in fact error labels can be some of the easier places to find.

Well, that's a side effect of us not actually testing many of our error paths :/

Nice catch!

FWIW,

I *like* using gotos for unwinding. It's specifically the err:, out:, and bail: type labels I object to. The error handling in copy_process() is pretty clean but the fork_out label is kind of useless. Here is how it's used.

kernel/fork.c
1285 if (clone_flags & CLONE_SIGHAND) {
1286 if ((clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) ||
1287 (task_active_pid_ns(current) ! 1288 current->nsproxy->pid_ns_for_children))

  1289                          return ERR_PTR(-EINVAL);
                                ^^^^^^^^^^^^^^^^^^^^^^^

This direct return is clear.

  1290          }
  1291  
  1292          retval = security_task_create(clone_flags);
  1293          if (retval)
  1294                  goto fork_out;
                        ^^^^^^^^^^^^^

This does the same thing but it's more ambigous. You have to scroll down 350 lines to find out what it does.

A more typical example is:

err:

	mega_free_function(foo);
	return ret;

But inside mega_free_function() function it dereferences foo->bar which is still NULL. The problem with one err bugs is that you're merging a lot of error paths together and then separating them out using if statements and it's easy to make a mistake. If you unwind like:

err_free_bar:

	kfree(foo->bar);
err_free_foo:
	kfree(foo);
	return ret;

That is less error prone.

regards,
dan carpenter

Agreed, and I'd suggest you don't use poorly named and poorly constructed labels.

That's how I name and structure unwind labels as well, and my suggestion is to use something similar here in this code too, in arch/x86/kernel/cpu/perf_event_intel_pt.c.

Agreed?

Thanks,

Ingo

I don't understand what you want me to do here. I think you are saying I should do this:

err_attrs:

	kfree(attrs);
err:
	return ret;

That's not the style that the rest of this file uses. Every function uses direct returns where possible except pt_event_add() and that function seems buggy.

arch/x86/kernel/cpu/perf_event_intel_pt.c
1000
1001 if (mode & PERF_EF_START) {
1002 pt_event_start(event, 0);
1003 if (hwc->state = PERF_HES_STOPPED) {
1004 pt_event_del(event, 0);

  1005                          ret = -EBUSY;
                                ^^^^^^^^^^^^

We set "ret" here but then return zero.

  1006                  }
  1007          } else {
  1008                  hwc->state = PERF_HES_STOPPED;
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Shouldn't we set "ret" here?

  1009          }
  1010  
  1011          ret = 0;
  1012  out:
  1013  
  1014          if (ret)
  1015                  hwc->state = PERF_HES_STOPPED;
  1016  
  1017          return ret;
  1018  }

regards,
dan carpenter

Yeah, that looks cleaner.

That might have slipped through review, but it's not a reason to continue the bad practice.

Thanks,

Ingo

I used to use out: and err: labels but for the past few years my job has been to look at error handling every day. I now believe very strongly that do-nothing gotos are harmful. It's going to be my contribution to computer engineering to persuade people to hate exit labels.

https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

1) "goto err;" is annoying to read because the name is meaningless.

2) Exit labels bad associations because code which uses err: or out:
labels is often buggy as we have seen in pt_event_add().

3) They don't do anything for return in the middle bugs. They only discourage people from returning at the start of the function where direct returns are fine.

In theory, one place where they might be useful is if we added locking to a function later on. First of all, when we add locking no one ever changes the out: label to out_unlock:. It never happens.

Also missing unlocks on error paths is sort of a sloppy/newbie mistake. I have looked through the git log and newbies are just as likely to get locking wrong when there is an exit label as when there is not.

https://plus.google.com/106378716002406849458/posts/DfuAkt8szf2

4) Exit labels introduce a whole new class of "forgot to set the error code" bugs which don't happen with direct returns.

regards,
dan carpenter

So I ended up cleaning up that function myself:

static int __init pt_pmu_hw_init(void)

{
	struct dev_ext_attribute *de_attrs;
	struct attribute **attrs;
	size_t size;
	int ret;
	long i;

	attrs = NULL;
	ret = -ENODEV;
	if (!test_cpu_cap(&boot_cpu_data, X86_FEATURE_INTEL_PT))
		goto fail;

	for (i = 0; i < PT_CPUID_LEAVES; i++) {
		cpuid_count(20, i,
			    &pt_pmu.caps[CR_EAX + i*4],
			    &pt_pmu.caps[CR_EBX + i*4],
			    &pt_pmu.caps[CR_ECX + i*4],
			    &pt_pmu.caps[CR_EDX + i*4]);
	}

	ret = -ENOMEM;
	size = sizeof(struct attribute *) * (ARRAY_SIZE(pt_caps)+1);
	attrs = kzalloc(size, GFP_KERNEL);
	if (!attrs)
		goto fail;

	size = sizeof(struct dev_ext_attribute) * (ARRAY_SIZE(pt_caps)+1);
	de_attrs = kzalloc(size, GFP_KERNEL);
	if (!de_attrs)
		goto fail;

	for (i = 0; i < ARRAY_SIZE(pt_caps); i++) {
		struct dev_ext_attribute *de_attr = de_attrs + i;

		de_attr->attr.attr.name = pt_caps[i].name;

		sysfs_attr_init(&de_attrs->attr.attr);

		de_attr->attr.attr.mode		= S_IRUGO;
		de_attr->attr.show		= pt_cap_show;
		de_attr->var			= (void *)i;

		attrs[i] = &de_attr->attr.attr;
	}

	pt_cap_group.attrs = attrs;

	return 0;

fail:

	kfree(attrs);

	return ret;
}

Note the fundamentally robust failure handling logic: all places can do a 'goto fail' and it will just clean itself up. Since this is init code, the kfree(NULL) is a non-issue.

Should this be extended in the future, the 'fail' path can be extended indefinitely to clean up other resources as well.

Let me react to your list of concerns:

Agreed, 'goto fail' is more meaningful.

Not if the cleanup path is robustly constructed. Multiple labels can also be used:

- if performance is of concern
- if we need to unroll layers of taken locks

Of course they do: if the standard pattern is to use goto labels then a stray 'return' sticks out like a sore thumb.

I disagree - I always notice middle returns during review.

I frequently do that in code I maintain, but if it's missing anywhere I'll take patches gladly - control flow readability, especially as it relates to exception/error handling, is one of my pet peeves.

The main point isn't to discourage newbies from making newbie mistakes.

The point is to make code more naturally readable/maintainable and extensible in the long run.

Well, that's not really a concern as long as you use robust techniques:

- initialize the return code before any attempt that might fail.
- update the return code accordingly

The worst that can happen in that case is to return say -ENODEV instead of -ENOMEM which isn't a serious bug in any case, and it typically sticks out during review as well.

Plus what you have not considered, direct returns may have some code generation costs as well.

For example branch predictions: forward labels are more likely to result in default-untaken forward branches - while direct returns are more likely to result in a RET instruction which is suboptimal.

( Which is obviously not a concern in init code - but it's of
relevence in hotter codepaths. )

Thanks,

Ingo

A cascade of labels helps the reader understand what goes with what, in what order, because functions are not called when they are not needed.

julia

I certainly agree for complex init/teardown sequences that are non-trivial to clean up.

For the specific case of kfree() in init paths it's also a valid model to just auto-cleanup everything in the failure path, to simplify things. (which I did here.)

Thanks,

Ingo

Indeed.

No, or we'll end up returning -EBUSY where we should return zero for snapshot counters. It can be done above the quoted if statement.

How does the following look to you?

From 726515f8bbef2ca02c495695b9451533d1bc6207 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <[email protected]>
Date: Wed, 15 Apr 2015 12:56:52 +0300
Subject: [PATCH] perf/x86/intel/pt: cleanup error paths in pt_event_add()

pt_event_add() ends up returning 0 instead of -EBUSY in case of failure to start the newly added event. This is a result of complex handling of its return code.

This patch makes the return code handling of pt_event_add() more obvious and fixes the mentioned bug.

---
 arch/x86/kernel/cpu/perf_event_intel_pt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

index b58ba99cf8..b61a337856 100644
@@ -1110,15 +1110,17 @@ static int pt_event_add(struct perf_event *event, int mode)
 	struct pt_buffer *buf;
 	struct pt *pt = this_cpu_ptr(&pt_ctx);
 	struct hw_perf_event *hwc = &event->hw;
-	int ret = -EBUSY;
+	int ret = 0;
 
-	if (pt->handle.event)
-		goto out;
+	if (pt->handle.event) {
+		ret = -EBUSY;
+		goto out_stop;
+	}
 
 	buf = perf_aux_output_begin(&pt->handle, event);
 	if (!buf) {
 		ret = -EINVAL;
-		goto out;
+		goto out_stop;
 	}
 
 	pt_buffer_reset_offsets(buf, pt->handle.head);

@@ -1126,7 +1128,7 @@ static int pt_event_add(struct perf_event *event, int mode)
 		ret = pt_buffer_reset_markers(buf, &pt->handle);
 		if (ret) {
 			perf_aux_output_end(&pt->handle, 0, true);
-			goto out;
+			goto out_stop;
 		}
 	}
 

@@ -1140,9 +1142,7 @@ static int pt_event_add(struct perf_event *event, int mode)
 		hwc->state = PERF_HES_STOPPED;
 	}
 
-	ret = 0;
-out:
-
+out_stop:
 	if (ret)
 		hwc->state = PERF_HES_STOPPED;
 

2.1.4

Are you sure you want to know?? :P

	return 0;

	hwc->state = PERF_HES_STOPPED;

	return ret;

Why would I ask if I didn't?

Thanks,
--
Alex