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 <gust...@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 <gust...@embeddedor.com>
> >
> > Acked-by: Song Liu <songliubrav...@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 {
