* [PATCH] perf data: Fix double free in perf_session__delete @ 2022-02-18 11:30 Alexey Bayduraev 2022-02-18 12:39 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 3+ messages in thread From: Alexey Bayduraev @ 2022-02-18 11:30 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Adrian Hunter, Alexander Antonov, Alexei Budankov When perf_data__create_dir fails, it calls close_dir, but perf_session__delete also calls close_dir and since dir.version and dir.nr was initialized by perf_data__create_dir, a double free occurs. This patch moves the initialization of dir.version and dir.nr after successful initialization of dir.files, that prevents double freeing. This behavior is already implemented in perf_data__open_dir. The patch also adds a missing error message in case data directory creation fails. Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> --- tools/perf/builtin-record.c | 4 +++- tools/perf/util/data.c | 7 +++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index 0bc6529814b2..0306d5911de2 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -1186,8 +1186,10 @@ static int record__mmap_evlist(struct record *rec, if (record__threads_enabled(rec)) { ret = perf_data__create_dir(&rec->data, evlist->core.nr_mmaps); - if (ret) + if (ret) { + pr_err("Failed to create data directory: %s\n", strerror(errno)); return ret; + } for (i = 0; i < evlist->core.nr_mmaps; i++) { if (evlist->mmap) evlist->mmap[i].file = &rec->data.dir.files[i]; diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c index f5d260b1df4d..15a4547d608e 100644 --- a/tools/perf/util/data.c +++ b/tools/perf/util/data.c @@ -44,10 +44,6 @@ int perf_data__create_dir(struct perf_data *data, int nr) if (!files) return -ENOMEM; - data->dir.version = PERF_DIR_VERSION; - data->dir.files = files; - data->dir.nr = nr; - for (i = 0; i < nr; i++) { struct perf_data_file *file = &files[i]; @@ -62,6 +58,9 @@ int perf_data__create_dir(struct perf_data *data, int nr) file->fd = ret; } + data->dir.version = PERF_DIR_VERSION; + data->dir.files = files; + data->dir.nr = nr; return 0; out_err: -- 2.19.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] perf data: Fix double free in perf_session__delete 2022-02-18 11:30 [PATCH] perf data: Fix double free in perf_session__delete Alexey Bayduraev @ 2022-02-18 12:39 ` Arnaldo Carvalho de Melo 2022-02-18 12:45 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 3+ messages in thread From: Arnaldo Carvalho de Melo @ 2022-02-18 12:39 UTC (permalink / raw) To: Alexey Bayduraev Cc: Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Adrian Hunter, Alexander Antonov, Alexei Budankov Em Fri, Feb 18, 2022 at 02:30:08PM +0300, Alexey Bayduraev escreveu: > When perf_data__create_dir fails, it calls close_dir, but > perf_session__delete also calls close_dir and since dir.version and > dir.nr was initialized by perf_data__create_dir, a double free occurs. > This patch moves the initialization of dir.version and dir.nr after > successful initialization of dir.files, that prevents double freeing. > This behavior is already implemented in perf_data__open_dir. The patch > also adds a missing error message in case data directory creation fails. Is this for perf/urgent or for perf/core? Probably perf/core as that record__threads_enabled(rec) call hints. Please state for which branch the patch should be applied, something like; [PATCH next] perf data: Fix double free in perf_session__delete() Or: [PATCH urgent] perf data: Fix double free in perf_session__delete() - Arnaldo > Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> > --- > tools/perf/builtin-record.c | 4 +++- > tools/perf/util/data.c | 7 +++---- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > index 0bc6529814b2..0306d5911de2 100644 > --- a/tools/perf/builtin-record.c > +++ b/tools/perf/builtin-record.c > @@ -1186,8 +1186,10 @@ static int record__mmap_evlist(struct record *rec, > > if (record__threads_enabled(rec)) { > ret = perf_data__create_dir(&rec->data, evlist->core.nr_mmaps); > - if (ret) > + if (ret) { > + pr_err("Failed to create data directory: %s\n", strerror(errno)); > return ret; > + } > for (i = 0; i < evlist->core.nr_mmaps; i++) { > if (evlist->mmap) > evlist->mmap[i].file = &rec->data.dir.files[i]; > diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c > index f5d260b1df4d..15a4547d608e 100644 > --- a/tools/perf/util/data.c > +++ b/tools/perf/util/data.c > @@ -44,10 +44,6 @@ int perf_data__create_dir(struct perf_data *data, int nr) > if (!files) > return -ENOMEM; > > - data->dir.version = PERF_DIR_VERSION; > - data->dir.files = files; > - data->dir.nr = nr; > - > for (i = 0; i < nr; i++) { > struct perf_data_file *file = &files[i]; > > @@ -62,6 +58,9 @@ int perf_data__create_dir(struct perf_data *data, int nr) > file->fd = ret; > } > > + data->dir.version = PERF_DIR_VERSION; > + data->dir.files = files; > + data->dir.nr = nr; > return 0; > > out_err: > -- > 2.19.0 -- - Arnaldo ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] perf data: Fix double free in perf_session__delete 2022-02-18 12:39 ` Arnaldo Carvalho de Melo @ 2022-02-18 12:45 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 3+ messages in thread From: Arnaldo Carvalho de Melo @ 2022-02-18 12:45 UTC (permalink / raw) To: Alexey Bayduraev Cc: Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Adrian Hunter, Alexander Antonov, Alexei Budankov Em Fri, Feb 18, 2022 at 09:39:13AM -0300, Arnaldo Carvalho de Melo escreveu: > Em Fri, Feb 18, 2022 at 02:30:08PM +0300, Alexey Bayduraev escreveu: > > When perf_data__create_dir fails, it calls close_dir, but > > perf_session__delete also calls close_dir and since dir.version and > > dir.nr was initialized by perf_data__create_dir, a double free occurs. > > This patch moves the initialization of dir.version and dir.nr after > > successful initialization of dir.files, that prevents double freeing. > > This behavior is already implemented in perf_data__open_dir. The patch > > also adds a missing error message in case data directory creation fails. > > Is this for perf/urgent or for perf/core? Probably perf/core as that > record__threads_enabled(rec) call hints. > > Please state for which branch the patch should be applied, something > like; > > [PATCH next] perf data: Fix double free in perf_session__delete() > > Or: > > [PATCH urgent] perf data: Fix double free in perf_session__delete() So, please break this into two patches, one adding the pr_err(), because it touches things that are not in perf/urgent, the threaded work, and another fixing the double free at perf_session__delete(), because that one should go in perf/urgent, as this problem predates the threaded record patch series: Fixes: 145520631130bd64 ("perf data: Add perf_data__(create_dir|close_dir) functions") Please add the above fixes for the patch fixing the double free. Always think twice when writing 'this patch also does this extra thing', usually this is an indication the patch should be split :-) Thanks! - Arnaldo > > Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> > > --- > > tools/perf/builtin-record.c | 4 +++- > > tools/perf/util/data.c | 7 +++---- > > 2 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > > index 0bc6529814b2..0306d5911de2 100644 > > --- a/tools/perf/builtin-record.c > > +++ b/tools/perf/builtin-record.c > > @@ -1186,8 +1186,10 @@ static int record__mmap_evlist(struct record *rec, > > > > if (record__threads_enabled(rec)) { > > ret = perf_data__create_dir(&rec->data, evlist->core.nr_mmaps); > > - if (ret) > > + if (ret) { > > + pr_err("Failed to create data directory: %s\n", strerror(errno)); > > return ret; > > + } > > for (i = 0; i < evlist->core.nr_mmaps; i++) { > > if (evlist->mmap) > > evlist->mmap[i].file = &rec->data.dir.files[i]; > > diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c > > index f5d260b1df4d..15a4547d608e 100644 > > --- a/tools/perf/util/data.c > > +++ b/tools/perf/util/data.c > > @@ -44,10 +44,6 @@ int perf_data__create_dir(struct perf_data *data, int nr) > > if (!files) > > return -ENOMEM; > > > > - data->dir.version = PERF_DIR_VERSION; > > - data->dir.files = files; > > - data->dir.nr = nr; > > - > > for (i = 0; i < nr; i++) { > > struct perf_data_file *file = &files[i]; > > > > @@ -62,6 +58,9 @@ int perf_data__create_dir(struct perf_data *data, int nr) > > file->fd = ret; > > } > > > > + data->dir.version = PERF_DIR_VERSION; > > + data->dir.files = files; > > + data->dir.nr = nr; > > return 0; > > > > out_err: > > -- > > 2.19.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-18 12:45 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-02-18 11:30 [PATCH] perf data: Fix double free in perf_session__delete Alexey Bayduraev 2022-02-18 12:39 ` Arnaldo Carvalho de Melo 2022-02-18 12:45 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox