* [PATCH core] perf data: Adding error message if perf_data__create_dir fails @ 2022-02-18 15:23 Alexey Bayduraev 2022-02-18 15:23 ` [PATCH urgent] perf data: Fix double free in perf_session__delete Alexey Bayduraev 2022-02-20 22:43 ` [PATCH core] perf data: Adding error message if perf_data__create_dir fails Jiri Olsa 0 siblings, 2 replies; 8+ messages in thread From: Alexey Bayduraev @ 2022-02-18 15:23 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Andi Kleen, Adrian Hunter, Alexander Antonov, Alexei Budankov There is no notification about data directory creation failure. Add it. Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> --- tools/perf/builtin-record.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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]; -- 2.19.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH urgent] perf data: Fix double free in perf_session__delete 2022-02-18 15:23 [PATCH core] perf data: Adding error message if perf_data__create_dir fails Alexey Bayduraev @ 2022-02-18 15:23 ` Alexey Bayduraev 2022-02-20 22:46 ` Jiri Olsa 2022-02-20 22:43 ` [PATCH core] perf data: Adding error message if perf_data__create_dir fails Jiri Olsa 1 sibling, 1 reply; 8+ messages in thread From: Alexey Bayduraev @ 2022-02-18 15:23 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Andi Kleen, 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. Fixes: 145520631130bd64 ("perf data: Add perf_data__(create_dir|close_dir) functions") Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> --- tools/perf/util/data.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) 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] 8+ messages in thread
* Re: [PATCH urgent] perf data: Fix double free in perf_session__delete 2022-02-18 15:23 ` [PATCH urgent] perf data: Fix double free in perf_session__delete Alexey Bayduraev @ 2022-02-20 22:46 ` Jiri Olsa 0 siblings, 0 replies; 8+ messages in thread From: Jiri Olsa @ 2022-02-20 22:46 UTC (permalink / raw) To: Alexey Bayduraev Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Andi Kleen, Adrian Hunter, Alexander Antonov, Alexei Budankov On Fri, Feb 18, 2022 at 06:23:41PM +0300, Alexey Bayduraev wrote: > 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. > > Fixes: 145520631130bd64 ("perf data: Add perf_data__(create_dir|close_dir) functions") > Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> Acked-by: Jiri Olsa <jolsa@kernel.org> thanks, jirka > --- > tools/perf/util/data.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > 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] 8+ messages in thread
* Re: [PATCH core] perf data: Adding error message if perf_data__create_dir fails 2022-02-18 15:23 [PATCH core] perf data: Adding error message if perf_data__create_dir fails Alexey Bayduraev 2022-02-18 15:23 ` [PATCH urgent] perf data: Fix double free in perf_session__delete Alexey Bayduraev @ 2022-02-20 22:43 ` Jiri Olsa 2022-02-21 13:24 ` Bayduraev, Alexey V 1 sibling, 1 reply; 8+ messages in thread From: Jiri Olsa @ 2022-02-20 22:43 UTC (permalink / raw) To: Alexey Bayduraev Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Andi Kleen, Adrian Hunter, Alexander Antonov, Alexei Budankov On Fri, Feb 18, 2022 at 06:23:40PM +0300, Alexey Bayduraev wrote: > There is no notification about data directory creation failure. Add it. > > Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> > --- > tools/perf/builtin-record.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > 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)); errno will be misleading in here, because perf_data__create_dir calls other syscalls on error path jirka > return ret; > + } > for (i = 0; i < evlist->core.nr_mmaps; i++) { > if (evlist->mmap) > evlist->mmap[i].file = &rec->data.dir.files[i]; > -- > 2.19.0 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH core] perf data: Adding error message if perf_data__create_dir fails 2022-02-20 22:43 ` [PATCH core] perf data: Adding error message if perf_data__create_dir fails Jiri Olsa @ 2022-02-21 13:24 ` Bayduraev, Alexey V 2022-02-21 18:24 ` Jiri Olsa 0 siblings, 1 reply; 8+ messages in thread From: Bayduraev, Alexey V @ 2022-02-21 13:24 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Andi Kleen, Adrian Hunter, Alexander Antonov, Alexei Budankov On 21.02.2022 1:43, Jiri Olsa wrote: > On Fri, Feb 18, 2022 at 06:23:40PM +0300, Alexey Bayduraev wrote: >> There is no notification about data directory creation failure. Add it. >> >> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> >> --- >> tools/perf/builtin-record.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> 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)); > > errno will be misleading in here, because perf_data__create_dir > calls other syscalls on error path Mostly I want to output something like: Failed to create data dir: Too many open files This will trigger the user to increase the open files limit. Would it be better to place such message to perf_data__create_dir after open() syscall? Regards, Alexey > > jirka > >> return ret; >> + } >> for (i = 0; i < evlist->core.nr_mmaps; i++) { >> if (evlist->mmap) >> evlist->mmap[i].file = &rec->data.dir.files[i]; >> -- >> 2.19.0 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH core] perf data: Adding error message if perf_data__create_dir fails 2022-02-21 13:24 ` Bayduraev, Alexey V @ 2022-02-21 18:24 ` Jiri Olsa 2022-02-21 18:45 ` Bayduraev, Alexey V 0 siblings, 1 reply; 8+ messages in thread From: Jiri Olsa @ 2022-02-21 18:24 UTC (permalink / raw) To: Bayduraev, Alexey V Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Andi Kleen, Adrian Hunter, Alexander Antonov, Alexei Budankov On Mon, Feb 21, 2022 at 04:24:28PM +0300, Bayduraev, Alexey V wrote: > On 21.02.2022 1:43, Jiri Olsa wrote: > > On Fri, Feb 18, 2022 at 06:23:40PM +0300, Alexey Bayduraev wrote: > >> There is no notification about data directory creation failure. Add it. > >> > >> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> > >> --- > >> tools/perf/builtin-record.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> 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)); > > > > errno will be misleading in here, because perf_data__create_dir > > calls other syscalls on error path > > Mostly I want to output something like: > > Failed to create data dir: Too many open files > > This will trigger the user to increase the open files limit. > Would it be better to place such message to perf_data__create_dir after > open() syscall? how about something like below (with your change) jirka --- diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c index 15a4547d608e..d3382098d6f9 100644 --- a/tools/perf/util/data.c +++ b/tools/perf/util/data.c @@ -52,8 +52,10 @@ int perf_data__create_dir(struct perf_data *data, int nr) goto out_err; ret = open(file->path, O_RDWR|O_CREAT|O_TRUNC, S_IRUSR|S_IWUSR); - if (ret < 0) + if (ret < 0) { + ret = -errno; goto out_err; + } file->fd = ret; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH core] perf data: Adding error message if perf_data__create_dir fails 2022-02-21 18:24 ` Jiri Olsa @ 2022-02-21 18:45 ` Bayduraev, Alexey V 2022-02-21 19:16 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 8+ messages in thread From: Bayduraev, Alexey V @ 2022-02-21 18:45 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Andi Kleen, Adrian Hunter, Alexander Antonov, Alexei Budankov On 21.02.2022 21:24, Jiri Olsa wrote: > On Mon, Feb 21, 2022 at 04:24:28PM +0300, Bayduraev, Alexey V wrote: >> On 21.02.2022 1:43, Jiri Olsa wrote: >>> On Fri, Feb 18, 2022 at 06:23:40PM +0300, Alexey Bayduraev wrote: >>>> There is no notification about data directory creation failure. Add it. >>>> >>>> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> >>>> --- >>>> tools/perf/builtin-record.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> 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)); >>> >>> errno will be misleading in here, because perf_data__create_dir >>> calls other syscalls on error path >> >> Mostly I want to output something like: >> >> Failed to create data dir: Too many open files >> >> This will trigger the user to increase the open files limit. >> Would it be better to place such message to perf_data__create_dir after >> open() syscall? > > how about something like below (with your change) Looks better, I will apply this to my patch. Thanks, Alexey > > jirka > > > --- > diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c > index 15a4547d608e..d3382098d6f9 100644 > --- a/tools/perf/util/data.c > +++ b/tools/perf/util/data.c > @@ -52,8 +52,10 @@ int perf_data__create_dir(struct perf_data *data, int nr) > goto out_err; > > ret = open(file->path, O_RDWR|O_CREAT|O_TRUNC, S_IRUSR|S_IWUSR); > - if (ret < 0) > + if (ret < 0) { > + ret = -errno; > goto out_err; > + } > > file->fd = ret; > } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH core] perf data: Adding error message if perf_data__create_dir fails 2022-02-21 18:45 ` Bayduraev, Alexey V @ 2022-02-21 19:16 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 8+ messages in thread From: Arnaldo Carvalho de Melo @ 2022-02-21 19:16 UTC (permalink / raw) To: Bayduraev, Alexey V, Jiri Olsa Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar, linux-kernel, Andi Kleen, Adrian Hunter, Alexander Antonov, Alexei Budankov On February 21, 2022 3:45:55 PM GMT-03:00, "Bayduraev, Alexey V" <alexey.v.bayduraev@linux.intel.com> wrote: > >On 21.02.2022 21:24, Jiri Olsa wrote: >> On Mon, Feb 21, 2022 at 04:24:28PM +0300, Bayduraev, Alexey V wrote: >>> On 21.02.2022 1:43, Jiri Olsa wrote: >>>> On Fri, Feb 18, 2022 at 06:23:40PM +0300, Alexey Bayduraev wrote: >>>>> There is no notification about data directory creation failure. Add it. >>>>> >>>>> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> >>>>> --- >>>>> tools/perf/builtin-record.c | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> 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)); >>>> >>>> errno will be misleading in here, because perf_data__create_dir >>>> calls other syscalls on error path >>> >>> Mostly I want to output something like: >>> >>> Failed to create data dir: Too many open files >>> >>> This will trigger the user to increase the open files limit. >>> Would it be better to place such message to perf_data__create_dir after >>> open() syscall? >> >> how about something like below (with your change) > >Looks better, I will apply this to my patch. Thanks a lot, I'll check when you post it. Thanks for splitting that original patch, it was the correct thing to do in the first place, and now one of the patches got reviews I even missed :-) - Arnaldo > >Thanks, >Alexey > >> >> jirka >> >> >> --- >> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c >> index 15a4547d608e..d3382098d6f9 100644 >> --- a/tools/perf/util/data.c >> +++ b/tools/perf/util/data.c >> @@ -52,8 +52,10 @@ int perf_data__create_dir(struct perf_data *data, int nr) >> goto out_err; >> >> ret = open(file->path, O_RDWR|O_CREAT|O_TRUNC, S_IRUSR|S_IWUSR); >> - if (ret < 0) >> + if (ret < 0) { >> + ret = -errno; >> goto out_err; >> + } >> >> file->fd = ret; >> } ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-02-21 19:18 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-02-18 15:23 [PATCH core] perf data: Adding error message if perf_data__create_dir fails Alexey Bayduraev 2022-02-18 15:23 ` [PATCH urgent] perf data: Fix double free in perf_session__delete Alexey Bayduraev 2022-02-20 22:46 ` Jiri Olsa 2022-02-20 22:43 ` [PATCH core] perf data: Adding error message if perf_data__create_dir fails Jiri Olsa 2022-02-21 13:24 ` Bayduraev, Alexey V 2022-02-21 18:24 ` Jiri Olsa 2022-02-21 18:45 ` Bayduraev, Alexey V 2022-02-21 19:16 ` Arnaldo Carvalho de Melo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.