[PATCH] perf header: Fix lock/unlock imbalances

[PATCH] perf header: Fix lock/unlock imbalances

[Gustavo A. R. Silva]

Fix lock/unlock imbalances by refactoring the code a bit and adding
calls to up_write() before return.

Addresses-Coverity-ID: 1444315 ("Missing unlock")
Addresses-Coverity-ID: 1444316 ("Missing unlock")
Fixes: a70a1123174a ("perf bpf: Save BTF information as headers to perf.data")
Fixes: 606f972b1361 ("perf bpf: Save bpf_prog_info information as headers to perf.data")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 tools/perf/util/header.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index b9e693825873..06028e0ee06a 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
 		perf_env__insert_bpf_prog_info(env, info_node);
 	}
 
+	up_write(&env->bpf_progs.lock);
 	return 0;
 out:
 	free(info_linear);
@@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
 static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
 {
 	struct perf_env *env = &ff->ph->env;
+	struct btf_node *node;
 	u32 count, i;
+	int err = -1;
 
 	if (ff->ph->needs_swap) {
 		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
@@ -2636,31 +2639,33 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
 	down_write(&env->bpf_progs.lock);
 
 	for (i = 0; i < count; ++i) {
-		struct btf_node *node;
 		u32 id, data_size;
 
+		node = NULL;
 		if (do_read_u32(ff, &id))
-			return -1;
+			goto out;
 		if (do_read_u32(ff, &data_size))
-			return -1;
+			goto out;
 
 		node = malloc(sizeof(struct btf_node) + data_size);
 		if (!node)
-			return -1;
+			goto out;
 
 		node->id = id;
 		node->data_size = data_size;
 
-		if (__do_read(ff, node->data, data_size)) {
-			free(node);
-			return -1;
-		}
+		if (__do_read(ff, node->data, data_size))
+			goto out;
 
 		perf_env__insert_btf(env, node);
 	}
 
 	up_write(&env->bpf_progs.lock);
 	return 0;
+out:
+	up_write(&env->bpf_progs.lock);
+	free(node);
+	return err;
 }
 
 struct feature_ops {
-- 
2.21.0

Re: [PATCH] perf header: Fix lock/unlock imbalances

[Song Liu]

> On Apr 8, 2019, at 10:33 AM, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote:
> 
> Fix lock/unlock imbalances by refactoring the code a bit and adding
> calls to up_write() before return.
> 
> Addresses-Coverity-ID: 1444315 ("Missing unlock")
> Addresses-Coverity-ID: 1444316 ("Missing unlock")
> Fixes: a70a1123174a ("perf bpf: Save BTF information as headers to perf.data")
> Fixes: 606f972b1361 ("perf bpf: Save bpf_prog_info information as headers to perf.data")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>

Acked-by: Song Liu <songliubraving@fb.com>

Thanks for the fix!

> ---
> tools/perf/util/header.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index b9e693825873..06028e0ee06a 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
> 		perf_env__insert_bpf_prog_info(env, info_node);
> 	}
> 
> +	up_write(&env->bpf_progs.lock);
> 	return 0;
> out:
> 	free(info_linear);
> @@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
> static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
> {
> 	struct perf_env *env = &ff->ph->env;
> +	struct btf_node *node;
> 	u32 count, i;
> +	int err = -1;
> 
> 	if (ff->ph->needs_swap) {
> 		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
> @@ -2636,31 +2639,33 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
> 	down_write(&env->bpf_progs.lock);
> 
> 	for (i = 0; i < count; ++i) {
> -		struct btf_node *node;
> 		u32 id, data_size;
> 
> +		node = NULL;
> 		if (do_read_u32(ff, &id))
> -			return -1;
> +			goto out;
> 		if (do_read_u32(ff, &data_size))
> -			return -1;
> +			goto out;
> 
> 		node = malloc(sizeof(struct btf_node) + data_size);
> 		if (!node)
> -			return -1;
> +			goto out;
> 
> 		node->id = id;
> 		node->data_size = data_size;
> 
> -		if (__do_read(ff, node->data, data_size)) {
> -			free(node);
> -			return -1;
> -		}
> +		if (__do_read(ff, node->data, data_size))
> +			goto out;
> 
> 		perf_env__insert_btf(env, node);
> 	}
> 
> 	up_write(&env->bpf_progs.lock);
> 	return 0;
> +out:
> +	up_write(&env->bpf_progs.lock);
> +	free(node);
> +	return err;
> }
> 
> struct feature_ops {
> -- 
> 2.21.0
> 

Re: [PATCH] perf header: Fix lock/unlock imbalances

[Gustavo A. R. Silva]

On 4/8/19 1:22 PM, Song Liu wrote:
> 
> 
>> On Apr 8, 2019, at 10:33 AM, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote:
>>
>> Fix lock/unlock imbalances by refactoring the code a bit and adding
>> calls to up_write() before return.
>>
>> Addresses-Coverity-ID: 1444315 ("Missing unlock")
>> Addresses-Coverity-ID: 1444316 ("Missing unlock")
>> Fixes: a70a1123174a ("perf bpf: Save BTF information as headers to perf.data")
>> Fixes: 606f972b1361 ("perf bpf: Save bpf_prog_info information as headers to perf.data")
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> 
> Acked-by: Song Liu <songliubraving@fb.com>
> 
> Thanks for the fix!
> 

Glad to help. :)

Thanks
--
Gustavo

>> ---
>> tools/perf/util/header.c | 21 +++++++++++++--------
>> 1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index b9e693825873..06028e0ee06a 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
>> 		perf_env__insert_bpf_prog_info(env, info_node);
>> 	}
>>
>> +	up_write(&env->bpf_progs.lock);
>> 	return 0;
>> out:
>> 	free(info_linear);
>> @@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
>> static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
>> {
>> 	struct perf_env *env = &ff->ph->env;
>> +	struct btf_node *node;
>> 	u32 count, i;
>> +	int err = -1;
>>
>> 	if (ff->ph->needs_swap) {
>> 		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
>> @@ -2636,31 +2639,33 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
>> 	down_write(&env->bpf_progs.lock);
>>
>> 	for (i = 0; i < count; ++i) {
>> -		struct btf_node *node;
>> 		u32 id, data_size;
>>
>> +		node = NULL;
>> 		if (do_read_u32(ff, &id))
>> -			return -1;
>> +			goto out;
>> 		if (do_read_u32(ff, &data_size))
>> -			return -1;
>> +			goto out;
>>
>> 		node = malloc(sizeof(struct btf_node) + data_size);
>> 		if (!node)
>> -			return -1;
>> +			goto out;
>>
>> 		node->id = id;
>> 		node->data_size = data_size;
>>
>> -		if (__do_read(ff, node->data, data_size)) {
>> -			free(node);
>> -			return -1;
>> -		}
>> +		if (__do_read(ff, node->data, data_size))
>> +			goto out;
>>
>> 		perf_env__insert_btf(env, node);
>> 	}
>>
>> 	up_write(&env->bpf_progs.lock);
>> 	return 0;
>> +out:
>> +	up_write(&env->bpf_progs.lock);
>> +	free(node);
>> +	return err;
>> }
>>
>> struct feature_ops {
>> -- 
>> 2.21.0
>>
> 

Re: [PATCH] perf header: Fix lock/unlock imbalances

[Arnaldo Carvalho de Melo]

Em Mon, Apr 08, 2019 at 01:26:09PM -0500, Gustavo A. R. Silva escreveu:
> 
> 
> On 4/8/19 1:22 PM, Song Liu wrote:
> > 
> > 
> >> On Apr 8, 2019, at 10:33 AM, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote:
> >>
> >> Fix lock/unlock imbalances by refactoring the code a bit and adding
> >> calls to up_write() before return.
> >>
> >> Addresses-Coverity-ID: 1444315 ("Missing unlock")
> >> Addresses-Coverity-ID: 1444316 ("Missing unlock")
> >> Fixes: a70a1123174a ("perf bpf: Save BTF information as headers to perf.data")
> >> Fixes: 606f972b1361 ("perf bpf: Save bpf_prog_info information as headers to perf.data")
> >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > 
> > Acked-by: Song Liu <songliubraving@fb.com>
> > 
> > Thanks for the fix!
> > 
> 
> Glad to help. :)

Super cool, using the same idiom as the kernel and living in the kernel
sources has its advantages 8-)

But see below, 

> >> +++ b/tools/perf/util/header.c
> >> @@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
> >> 		perf_env__insert_bpf_prog_info(env, info_node);
> >> 	}
> >>
> >> +	up_write(&env->bpf_progs.lock);
> >> 	return 0;
> >> out:
> >> 	free(info_linear);
> >> @@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
> >> static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
> >> {
> >> 	struct perf_env *env = &ff->ph->env;
> >> +	struct btf_node *node;
> >> 	u32 count, i;
> >> +	int err = -1;

Why are you using this 'err' variable? It is only set here and at the
end, i.e. one write, one read. We could as well have that out: block
return -1 straight away.

Else we could do, see below

> >>
> >> 	if (ff->ph->needs_swap) {
> >> 		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
> >> @@ -2636,31 +2639,33 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
> >> 	down_write(&env->bpf_progs.lock);
> >>
> >> 	for (i = 0; i < count; ++i) {
> >> -		struct btf_node *node;
> >> 		u32 id, data_size;
> >>
> >> +		node = NULL;
> >> 		if (do_read_u32(ff, &id))
> >> -			return -1;
> >> +			goto out;
> >> 		if (do_read_u32(ff, &data_size))
> >> -			return -1;
> >> +			goto out;
> >>
> >> 		node = malloc(sizeof(struct btf_node) + data_size);
> >> 		if (!node)
> >> -			return -1;
> >> +			goto out;
> >>
> >> 		node->id = id;
> >> 		node->data_size = data_size;
> >>
> >> -		if (__do_read(ff, node->data, data_size)) {
> >> -			free(node);
> >> -			return -1;
> >> -		}
> >> +		if (__do_read(ff, node->data, data_size))
> >> +			goto out;
> >>
> >> 		perf_env__insert_btf(env, node);
> >> 	}

      err = 0;

> >>

out:

> >> 	up_write(&env->bpf_progs.lock);

      return err;

And delete the rest.

but I see, you used the same pattern in the first #ifdef HAVE_LIBBPF_SUPPORT
block :-)

Anyway, since we're fixing up that other case, we might as well
streamline this, please check the patch below.

> >> 	return 0;

> >> +out:
> >> +	up_write(&env->bpf_progs.lock);
> >> +	free(node);
> >> +	return err;

So, that is what I'm applying, please holler if I introduced some
problem:

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index b9e693825873..2d2af2ac2b1e 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
 		perf_env__insert_bpf_prog_info(env, info_node);
 	}
 
+	up_write(&env->bpf_progs.lock);
 	return 0;
 out:
 	free(info_linear);
@@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
 static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
 {
 	struct perf_env *env = &ff->ph->env;
+	struct btf_node *node = NULL;
 	u32 count, i;
+	int err = -1;
 
 	if (ff->ph->needs_swap) {
 		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
@@ -2636,31 +2639,32 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
 	down_write(&env->bpf_progs.lock);
 
 	for (i = 0; i < count; ++i) {
-		struct btf_node *node;
 		u32 id, data_size;
 
 		if (do_read_u32(ff, &id))
-			return -1;
+			goto out;
 		if (do_read_u32(ff, &data_size))
-			return -1;
+			goto out;
 
 		node = malloc(sizeof(struct btf_node) + data_size);
 		if (!node)
-			return -1;
+			goto out;
 
 		node->id = id;
 		node->data_size = data_size;
 
-		if (__do_read(ff, node->data, data_size)) {
-			free(node);
-			return -1;
-		}
+		if (__do_read(ff, node->data, data_size))
+			goto out;
 
 		perf_env__insert_btf(env, node);
+		node = NULL;
 	}
 
+	err = 0;
+out:
 	up_write(&env->bpf_progs.lock);
-	return 0;
+	free(node);
+	return err;
 }
 
 struct feature_ops {

Re: [PATCH] perf header: Fix lock/unlock imbalances

[Gustavo A. R. Silva]

On 4/8/19 2:35 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Apr 08, 2019 at 01:26:09PM -0500, Gustavo A. R. Silva escreveu:
>>
>>
>> On 4/8/19 1:22 PM, Song Liu wrote:
>>>
>>>
>>>> On Apr 8, 2019, at 10:33 AM, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote:
>>>>
>>>> Fix lock/unlock imbalances by refactoring the code a bit and adding
>>>> calls to up_write() before return.
>>>>
>>>> Addresses-Coverity-ID: 1444315 ("Missing unlock")
>>>> Addresses-Coverity-ID: 1444316 ("Missing unlock")
>>>> Fixes: a70a1123174a ("perf bpf: Save BTF information as headers to perf.data")
>>>> Fixes: 606f972b1361 ("perf bpf: Save bpf_prog_info information as headers to perf.data")
>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>>
>>> Acked-by: Song Liu <songliubraving@fb.com>
>>>
>>> Thanks for the fix!
>>>
>>
>> Glad to help. :)
> 
> Super cool, using the same idiom as the kernel and living in the kernel
> sources has its advantages 8-)
> 

:P

> But see below, 
> 
>>>> +++ b/tools/perf/util/header.c
>>>> @@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
>>>> 		perf_env__insert_bpf_prog_info(env, info_node);
>>>> 	}
>>>>
>>>> +	up_write(&env->bpf_progs.lock);
>>>> 	return 0;
>>>> out:
>>>> 	free(info_linear);
>>>> @@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
>>>> static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
>>>> {
>>>> 	struct perf_env *env = &ff->ph->env;
>>>> +	struct btf_node *node;
>>>> 	u32 count, i;
>>>> +	int err = -1;
> 
> Why are you using this 'err' variable? It is only set here and at the
> end, i.e. one write, one read. We could as well have that out: block
> return -1 straight away.
> 
> Else we could do, see below
> 
>>>>
>>>> 	if (ff->ph->needs_swap) {
>>>> 		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
>>>> @@ -2636,31 +2639,33 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
>>>> 	down_write(&env->bpf_progs.lock);
>>>>
>>>> 	for (i = 0; i < count; ++i) {
>>>> -		struct btf_node *node;
>>>> 		u32 id, data_size;
>>>>
>>>> +		node = NULL;
>>>> 		if (do_read_u32(ff, &id))
>>>> -			return -1;
>>>> +			goto out;
>>>> 		if (do_read_u32(ff, &data_size))
>>>> -			return -1;
>>>> +			goto out;
>>>>
>>>> 		node = malloc(sizeof(struct btf_node) + data_size);
>>>> 		if (!node)
>>>> -			return -1;
>>>> +			goto out;
>>>>
>>>> 		node->id = id;
>>>> 		node->data_size = data_size;
>>>>
>>>> -		if (__do_read(ff, node->data, data_size)) {
>>>> -			free(node);
>>>> -			return -1;
>>>> -		}
>>>> +		if (__do_read(ff, node->data, data_size))
>>>> +			goto out;
>>>>
>>>> 		perf_env__insert_btf(env, node);
>>>> 	}
> 
>       err = 0;
> 
>>>>
> 
> out:
> 
>>>> 	up_write(&env->bpf_progs.lock);
> 
>       return err;
> 
> And delete the rest.
> 
> but I see, you used the same pattern in the first #ifdef HAVE_LIBBPF_SUPPORT
> block :-)
> 
> Anyway, since we're fixing up that other case, we might as well
> streamline this, please check the patch below.
> 

Yeah. This is exactly how I would have coded this from the beginning. But, as you
correctly pointed out, I'm using the same pattern as in HAVE_LIBBPF_SUPPORT. :)

Just a comment below...

>>>> 	return 0;
> 
>>>> +out:
>>>> +	up_write(&env->bpf_progs.lock);
>>>> +	free(node);
>>>> +	return err;
> 
> So, that is what I'm applying, please holler if I introduced some
> problem:
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index b9e693825873..2d2af2ac2b1e 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
>  		perf_env__insert_bpf_prog_info(env, info_node);
>  	}
>  
> +	up_write(&env->bpf_progs.lock);
>  	return 0;
>  out:
>  	free(info_linear);
> @@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
>  static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
>  {
>  	struct perf_env *env = &ff->ph->env;
> +	struct btf_node *node = NULL;
>  	u32 count, i;
> +	int err = -1;
>  
>  	if (ff->ph->needs_swap) {
>  		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
> @@ -2636,31 +2639,32 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
>  	down_write(&env->bpf_progs.lock);
>  
>  	for (i = 0; i < count; ++i) {
> -		struct btf_node *node;
>  		u32 id, data_size;
>  
>  		if (do_read_u32(ff, &id))
> -			return -1;
> +			goto out;
>  		if (do_read_u32(ff, &data_size))
> -			return -1;
> +			goto out;
>  
>  		node = malloc(sizeof(struct btf_node) + data_size);
>  		if (!node)
> -			return -1;
> +			goto out;
>  
>  		node->id = id;
>  		node->data_size = data_size;
>  
> -		if (__do_read(ff, node->data, data_size)) {
> -			free(node);
> -			return -1;
> -		}
> +		if (__do_read(ff, node->data, data_size))
> +			goto out;
>  
>  		perf_env__insert_btf(env, node);
> +		node = NULL;

If we move this assignment to the beginning of the for loop, as in
the original patch, we avoid the same assignment while declaring
node at the beginning of the function.

For the rest, the code looks good to me.

Thanks for your comments. :)
--
Gustavo

>  	}
>  
> +	err = 0;
> +out:
>  	up_write(&env->bpf_progs.lock);
> -	return 0;
> +	free(node);
> +	return err;
>  }
>  
>  struct feature_ops {
> 

Re: [PATCH] perf header: Fix lock/unlock imbalances

[Arnaldo Carvalho de Melo]

Em Mon, Apr 08, 2019 at 02:52:52PM -0500, Gustavo A. R. Silva escreveu:
> 
> 
> On 4/8/19 2:35 PM, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Apr 08, 2019 at 01:26:09PM -0500, Gustavo A. R. Silva escreveu:
> >>
> >>
> >> On 4/8/19 1:22 PM, Song Liu wrote:
> >>>
> >>>
> >>>> On Apr 8, 2019, at 10:33 AM, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote:
> >>>>
> >>>> Fix lock/unlock imbalances by refactoring the code a bit and adding
> >>>> calls to up_write() before return.
> >>>>
> >>>> Addresses-Coverity-ID: 1444315 ("Missing unlock")
> >>>> Addresses-Coverity-ID: 1444316 ("Missing unlock")
> >>>> Fixes: a70a1123174a ("perf bpf: Save BTF information as headers to perf.data")
> >>>> Fixes: 606f972b1361 ("perf bpf: Save bpf_prog_info information as headers to perf.data")
> >>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> >>>
> >>> Acked-by: Song Liu <songliubraving@fb.com>
> >>>
> >>> Thanks for the fix!
> >>>
> >>
> >> Glad to help. :)
> > 
> > Super cool, using the same idiom as the kernel and living in the kernel
> > sources has its advantages 8-)
> > 
> 
> :P
> 
> > But see below, 
> > 
> >>>> +++ b/tools/perf/util/header.c
> >>>> @@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
> >>>> 		perf_env__insert_bpf_prog_info(env, info_node);
> >>>> 	}
> >>>>
> >>>> +	up_write(&env->bpf_progs.lock);
> >>>> 	return 0;
> >>>> out:
> >>>> 	free(info_linear);
> >>>> @@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
> >>>> static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
> >>>> {
> >>>> 	struct perf_env *env = &ff->ph->env;
> >>>> +	struct btf_node *node;
> >>>> 	u32 count, i;
> >>>> +	int err = -1;
> > 
> > Why are you using this 'err' variable? It is only set here and at the
> > end, i.e. one write, one read. We could as well have that out: block
> > return -1 straight away.
> > 
> > Else we could do, see below
> > 
> >>>>
> >>>> 	if (ff->ph->needs_swap) {
> >>>> 		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
> >>>> @@ -2636,31 +2639,33 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
> >>>> 	down_write(&env->bpf_progs.lock);
> >>>>
> >>>> 	for (i = 0; i < count; ++i) {
> >>>> -		struct btf_node *node;
> >>>> 		u32 id, data_size;
> >>>>
> >>>> +		node = NULL;
> >>>> 		if (do_read_u32(ff, &id))
> >>>> -			return -1;
> >>>> +			goto out;
> >>>> 		if (do_read_u32(ff, &data_size))
> >>>> -			return -1;
> >>>> +			goto out;
> >>>>
> >>>> 		node = malloc(sizeof(struct btf_node) + data_size);
> >>>> 		if (!node)
> >>>> -			return -1;
> >>>> +			goto out;
> >>>>
> >>>> 		node->id = id;
> >>>> 		node->data_size = data_size;
> >>>>
> >>>> -		if (__do_read(ff, node->data, data_size)) {
> >>>> -			free(node);
> >>>> -			return -1;
> >>>> -		}
> >>>> +		if (__do_read(ff, node->data, data_size))
> >>>> +			goto out;
> >>>>
> >>>> 		perf_env__insert_btf(env, node);
> >>>> 	}
> > 
> >       err = 0;
> > 
> >>>>
> > 
> > out:
> > 
> >>>> 	up_write(&env->bpf_progs.lock);
> > 
> >       return err;
> > 
> > And delete the rest.
> > 
> > but I see, you used the same pattern in the first #ifdef HAVE_LIBBPF_SUPPORT
> > block :-)
> > 
> > Anyway, since we're fixing up that other case, we might as well
> > streamline this, please check the patch below.
> > 
> 
> Yeah. This is exactly how I would have coded this from the beginning. But, as you
> correctly pointed out, I'm using the same pattern as in HAVE_LIBBPF_SUPPORT. :)
> 
> Just a comment below...
> 
> >>>> 	return 0;
> > 
> >>>> +out:
> >>>> +	up_write(&env->bpf_progs.lock);
> >>>> +	free(node);
> >>>> +	return err;
> > 
> > So, that is what I'm applying, please holler if I introduced some
> > problem:
> > 
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index b9e693825873..2d2af2ac2b1e 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
> >  		perf_env__insert_bpf_prog_info(env, info_node);
> >  	}
> >  
> > +	up_write(&env->bpf_progs.lock);
> >  	return 0;
> >  out:
> >  	free(info_linear);
> > @@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
> >  static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
> >  {
> >  	struct perf_env *env = &ff->ph->env;
> > +	struct btf_node *node = NULL;
> >  	u32 count, i;
> > +	int err = -1;
> >  
> >  	if (ff->ph->needs_swap) {
> >  		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
> > @@ -2636,31 +2639,32 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
> >  	down_write(&env->bpf_progs.lock);
> >  
> >  	for (i = 0; i < count; ++i) {
> > -		struct btf_node *node;
> >  		u32 id, data_size;
> >  
> >  		if (do_read_u32(ff, &id))
> > -			return -1;
> > +			goto out;
> >  		if (do_read_u32(ff, &data_size))
> > -			return -1;
> > +			goto out;
> >  
> >  		node = malloc(sizeof(struct btf_node) + data_size);
> >  		if (!node)
> > -			return -1;
> > +			goto out;
> >  
> >  		node->id = id;
> >  		node->data_size = data_size;
> >  
> > -		if (__do_read(ff, node->data, data_size)) {
> > -			free(node);
> > -			return -1;
> > -		}
> > +		if (__do_read(ff, node->data, data_size))
> > +			goto out;
> >  
> >  		perf_env__insert_btf(env, node);
> > +		node = NULL;
> 
> If we move this assignment to the beginning of the for loop, as in
> the original patch, we avoid the same assignment while declaring
> node at the beginning of the function.

No, we don't, since the common exit path frees node, we better not free
the last node in the success case, that is why I moved it to the end,
i.e. after we're done with it, nullify it, so that the last btf_node
isn't freed in the now uncoditionall free(node); call :-)

Right?

- Arnaldo
 
> For the rest, the code looks good to me.
> 
> Thanks for your comments. :)
> --
> Gustavo
> 
> >  	}
> >  
> > +	err = 0;
> > +out:
> >  	up_write(&env->bpf_progs.lock);
> > -	return 0;
> > +	free(node);
> > +	return err;
> >  }
> >  
> >  struct feature_ops {
> > 

-- 

- Arnaldo

Re: [PATCH] perf header: Fix lock/unlock imbalances

[Gustavo A. R. Silva]

On 4/8/19 3:00 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Apr 08, 2019 at 02:52:52PM -0500, Gustavo A. R. Silva escreveu:
>>
>>
>> On 4/8/19 2:35 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Mon, Apr 08, 2019 at 01:26:09PM -0500, Gustavo A. R. Silva escreveu:
>>>>
>>>>
>>>> On 4/8/19 1:22 PM, Song Liu wrote:
>>>>>
>>>>>
>>>>>> On Apr 8, 2019, at 10:33 AM, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote:
>>>>>>
>>>>>> Fix lock/unlock imbalances by refactoring the code a bit and adding
>>>>>> calls to up_write() before return.
>>>>>>
>>>>>> Addresses-Coverity-ID: 1444315 ("Missing unlock")
>>>>>> Addresses-Coverity-ID: 1444316 ("Missing unlock")
>>>>>> Fixes: a70a1123174a ("perf bpf: Save BTF information as headers to perf.data")
>>>>>> Fixes: 606f972b1361 ("perf bpf: Save bpf_prog_info information as headers to perf.data")
>>>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>>>>
>>>>> Acked-by: Song Liu <songliubraving@fb.com>
>>>>>
>>>>> Thanks for the fix!
>>>>>
>>>>
>>>> Glad to help. :)
>>>
>>> Super cool, using the same idiom as the kernel and living in the kernel
>>> sources has its advantages 8-)
>>>
>>
>> :P
>>
>>> But see below, 
>>>
>>>>>> +++ b/tools/perf/util/header.c
>>>>>> @@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
>>>>>> 		perf_env__insert_bpf_prog_info(env, info_node);
>>>>>> 	}
>>>>>>
>>>>>> +	up_write(&env->bpf_progs.lock);
>>>>>> 	return 0;
>>>>>> out:
>>>>>> 	free(info_linear);
>>>>>> @@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
>>>>>> static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
>>>>>> {
>>>>>> 	struct perf_env *env = &ff->ph->env;
>>>>>> +	struct btf_node *node;
>>>>>> 	u32 count, i;
>>>>>> +	int err = -1;
>>>
>>> Why are you using this 'err' variable? It is only set here and at the
>>> end, i.e. one write, one read. We could as well have that out: block
>>> return -1 straight away.
>>>
>>> Else we could do, see below
>>>
>>>>>>
>>>>>> 	if (ff->ph->needs_swap) {
>>>>>> 		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
>>>>>> @@ -2636,31 +2639,33 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
>>>>>> 	down_write(&env->bpf_progs.lock);
>>>>>>
>>>>>> 	for (i = 0; i < count; ++i) {
>>>>>> -		struct btf_node *node;
>>>>>> 		u32 id, data_size;
>>>>>>
>>>>>> +		node = NULL;
>>>>>> 		if (do_read_u32(ff, &id))
>>>>>> -			return -1;
>>>>>> +			goto out;
>>>>>> 		if (do_read_u32(ff, &data_size))
>>>>>> -			return -1;
>>>>>> +			goto out;
>>>>>>
>>>>>> 		node = malloc(sizeof(struct btf_node) + data_size);
>>>>>> 		if (!node)
>>>>>> -			return -1;
>>>>>> +			goto out;
>>>>>>
>>>>>> 		node->id = id;
>>>>>> 		node->data_size = data_size;
>>>>>>
>>>>>> -		if (__do_read(ff, node->data, data_size)) {
>>>>>> -			free(node);
>>>>>> -			return -1;
>>>>>> -		}
>>>>>> +		if (__do_read(ff, node->data, data_size))
>>>>>> +			goto out;
>>>>>>
>>>>>> 		perf_env__insert_btf(env, node);
>>>>>> 	}
>>>
>>>       err = 0;
>>>
>>>>>>
>>>
>>> out:
>>>
>>>>>> 	up_write(&env->bpf_progs.lock);
>>>
>>>       return err;
>>>
>>> And delete the rest.
>>>
>>> but I see, you used the same pattern in the first #ifdef HAVE_LIBBPF_SUPPORT
>>> block :-)
>>>
>>> Anyway, since we're fixing up that other case, we might as well
>>> streamline this, please check the patch below.
>>>
>>
>> Yeah. This is exactly how I would have coded this from the beginning. But, as you
>> correctly pointed out, I'm using the same pattern as in HAVE_LIBBPF_SUPPORT. :)
>>
>> Just a comment below...
>>
>>>>>> 	return 0;
>>>
>>>>>> +out:
>>>>>> +	up_write(&env->bpf_progs.lock);
>>>>>> +	free(node);
>>>>>> +	return err;
>>>
>>> So, that is what I'm applying, please holler if I introduced some
>>> problem:
>>>
>>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>>> index b9e693825873..2d2af2ac2b1e 100644
>>> --- a/tools/perf/util/header.c
>>> +++ b/tools/perf/util/header.c
>>> @@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
>>>  		perf_env__insert_bpf_prog_info(env, info_node);
>>>  	}
>>>  
>>> +	up_write(&env->bpf_progs.lock);
>>>  	return 0;
>>>  out:
>>>  	free(info_linear);
>>> @@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
>>>  static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
>>>  {
>>>  	struct perf_env *env = &ff->ph->env;
>>> +	struct btf_node *node = NULL;
>>>  	u32 count, i;
>>> +	int err = -1;
>>>  
>>>  	if (ff->ph->needs_swap) {
>>>  		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
>>> @@ -2636,31 +2639,32 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
>>>  	down_write(&env->bpf_progs.lock);
>>>  
>>>  	for (i = 0; i < count; ++i) {
>>> -		struct btf_node *node;
>>>  		u32 id, data_size;
>>>  
>>>  		if (do_read_u32(ff, &id))
>>> -			return -1;
>>> +			goto out;
>>>  		if (do_read_u32(ff, &data_size))
>>> -			return -1;
>>> +			goto out;
>>>  
>>>  		node = malloc(sizeof(struct btf_node) + data_size);
>>>  		if (!node)
>>> -			return -1;
>>> +			goto out;
>>>  
>>>  		node->id = id;
>>>  		node->data_size = data_size;
>>>  
>>> -		if (__do_read(ff, node->data, data_size)) {
>>> -			free(node);
>>> -			return -1;
>>> -		}
>>> +		if (__do_read(ff, node->data, data_size))
>>> +			goto out;
>>>  
>>>  		perf_env__insert_btf(env, node);
>>> +		node = NULL;
>>
>> If we move this assignment to the beginning of the for loop, as in
>> the original patch, we avoid the same assignment while declaring
>> node at the beginning of the function.
> 
> No, we don't, since the common exit path frees node, we better not free
> the last node in the success case, that is why I moved it to the end,
> i.e. after we're done with it, nullify it, so that the last btf_node
> isn't freed in the now uncoditionall free(node); call :-)
> 

Yep. You're right.

So, everything is fine now. :)

Thanks
--
Gustavo

[tip:perf/urgent] perf header: Fix lock/unlock imbalances when processing BPF/BTF info

[tip-bot for Gustavo A. R. Silva]

Commit-ID:  f0f702655b09cfe07bcbc1027be67c3fd2a7387b
Gitweb:     https://git.kernel.org/tip/f0f702655b09cfe07bcbc1027be67c3fd2a7387b
Author:     Gustavo A. R. Silva <gustavo@embeddedor.com>
AuthorDate: Mon, 8 Apr 2019 12:33:55 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 8 Apr 2019 16:58:07 -0300

perf header: Fix lock/unlock imbalances when processing BPF/BTF info

Fix lock/unlock imbalances by refactoring the code a bit and adding
calls to up_write() before return.

Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Acked-by: Song Liu <songliubraving@fb.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Addresses-Coverity-ID: 1444315 ("Missing unlock")
Addresses-Coverity-ID: 1444316 ("Missing unlock")
Fixes: a70a1123174a ("perf bpf: Save BTF information as headers to perf.data")
Fixes: 606f972b1361 ("perf bpf: Save bpf_prog_info information as headers to perf.data")
Link: http://lkml.kernel.org/r/20190408173355.GA10501@embeddedor
[ Simplified the exit path to have just one up_write() + return ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index b9e693825873..2d2af2ac2b1e 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
 		perf_env__insert_bpf_prog_info(env, info_node);
 	}
 
+	up_write(&env->bpf_progs.lock);
 	return 0;
 out:
 	free(info_linear);
@@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
 static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
 {
 	struct perf_env *env = &ff->ph->env;
+	struct btf_node *node = NULL;
 	u32 count, i;
+	int err = -1;
 
 	if (ff->ph->needs_swap) {
 		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
@@ -2636,31 +2639,32 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
 	down_write(&env->bpf_progs.lock);
 
 	for (i = 0; i < count; ++i) {
-		struct btf_node *node;
 		u32 id, data_size;
 
 		if (do_read_u32(ff, &id))
-			return -1;
+			goto out;
 		if (do_read_u32(ff, &data_size))
-			return -1;
+			goto out;
 
 		node = malloc(sizeof(struct btf_node) + data_size);
 		if (!node)
-			return -1;
+			goto out;
 
 		node->id = id;
 		node->data_size = data_size;
 
-		if (__do_read(ff, node->data, data_size)) {
-			free(node);
-			return -1;
-		}
+		if (__do_read(ff, node->data, data_size))
+			goto out;
 
 		perf_env__insert_btf(env, node);
+		node = NULL;
 	}
 
+	err = 0;
+out:
 	up_write(&env->bpf_progs.lock);
-	return 0;
+	free(node);
+	return err;
 }
 
 struct feature_ops {

[tip:perf/urgent] perf header: Fix lock/unlock imbalances when processing BPF/BTF info

[tip-bot for Gustavo A. R. Silva]

Commit-ID:  14c9b31a925a9f5c647523a12e2b575b97c0aa47
Gitweb:     https://git.kernel.org/tip/14c9b31a925a9f5c647523a12e2b575b97c0aa47
Author:     Gustavo A. R. Silva <gustavo@embeddedor.com>
AuthorDate: Mon, 8 Apr 2019 12:33:55 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 16 Apr 2019 11:26:43 -0300

perf header: Fix lock/unlock imbalances when processing BPF/BTF info

Fix lock/unlock imbalances by refactoring the code a bit and adding
calls to up_write() before return.

Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Acked-by: Song Liu <songliubraving@fb.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Addresses-Coverity-ID: 1444315 ("Missing unlock")
Addresses-Coverity-ID: 1444316 ("Missing unlock")
Fixes: a70a1123174a ("perf bpf: Save BTF information as headers to perf.data")
Fixes: 606f972b1361 ("perf bpf: Save bpf_prog_info information as headers to perf.data")
Link: http://lkml.kernel.org/r/20190408173355.GA10501@embeddedor
[ Simplified the exit path to have just one up_write() + return ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index b9e693825873..2d2af2ac2b1e 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2606,6 +2606,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
 		perf_env__insert_bpf_prog_info(env, info_node);
 	}
 
+	up_write(&env->bpf_progs.lock);
 	return 0;
 out:
 	free(info_linear);
@@ -2623,7 +2624,9 @@ static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data _
 static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
 {
 	struct perf_env *env = &ff->ph->env;
+	struct btf_node *node = NULL;
 	u32 count, i;
+	int err = -1;
 
 	if (ff->ph->needs_swap) {
 		pr_warning("interpreting btf from systems with endianity is not yet supported\n");
@@ -2636,31 +2639,32 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
 	down_write(&env->bpf_progs.lock);
 
 	for (i = 0; i < count; ++i) {
-		struct btf_node *node;
 		u32 id, data_size;
 
 		if (do_read_u32(ff, &id))
-			return -1;
+			goto out;
 		if (do_read_u32(ff, &data_size))
-			return -1;
+			goto out;
 
 		node = malloc(sizeof(struct btf_node) + data_size);
 		if (!node)
-			return -1;
+			goto out;
 
 		node->id = id;
 		node->data_size = data_size;
 
-		if (__do_read(ff, node->data, data_size)) {
-			free(node);
-			return -1;
-		}
+		if (__do_read(ff, node->data, data_size))
+			goto out;
 
 		perf_env__insert_btf(env, node);
+		node = NULL;
 	}
 
+	err = 0;
+out:
 	up_write(&env->bpf_progs.lock);
-	return 0;
+	free(node);
+	return err;
 }
 
 struct feature_ops {