linux-kernel - Re: [PATCH 06/14] perf data: Add perf_data__(create_dir|free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190205134234.GH4794@krava>
Date:   Tue, 5 Feb 2019 14:42:34 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Alexey Budankov <alexey.budankov@...ux.intel.com>
Cc:     Jiri Olsa <jolsa@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 06/14] perf data: Add perf_data__(create_dir|free_dir)
 functions

On Tue, Feb 05, 2019 at 02:52:58PM +0300, Alexey Budankov wrote:
> 
> On 03.02.2019 18:30, Jiri Olsa wrote:
> > Adding perf_data__create_dir to create nr files inside
> > struct perf_data path directory:
> >   int perf_data__create_dir(struct perf_data *data, int nr);
> > 
> > and function to free that data:
> >   void perf_data__free_dir(struct perf_data *data);
> > 
> > Link: http://lkml.kernel.org/n/tip-kl4s1f13cg6wycrg367p85qm@git.kernel.org
> > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > ---
> >  tools/perf/util/data.c | 47 ++++++++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/data.h |  8 +++++++
> >  2 files changed, 55 insertions(+)
> > 
> > diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> > index 0a3051cc0ea0..ff1d9e5bd68d 100644
> > --- a/tools/perf/util/data.c
> > +++ b/tools/perf/util/data.c
> > @@ -7,11 +7,58 @@
> >  #include <fcntl.h>
> >  #include <unistd.h>
> >  #include <string.h>
> > +#include <asm/bug.h>
> >  
> >  #include "data.h"
> >  #include "util.h"
> >  #include "debug.h"
> >  
> > +static void free_dir(struct perf_data_file *files, int nr)
> > +{
> > +	while (--nr >= 1) {
> > +		close(files[nr].fd);
> > +		free(files[nr].path);
> > +	}
> > +	free(files);
> 
> It implements closing of created files and frees corresponding memory.
> However it doesn't delete the files what looks like the proper rollback
> from perf_data__create_dir() in case of some open() failure.

we don't di rollback event for the single file:

	[jolsa@...va perf]$ ./perf record -b ls
	Error:
	You may not have permission to collect stats.

	Consider tweaking /proc/sys/kernel/perf_event_paranoid,
	which controls use of the performance events system by
	unprivileged users (without CAP_SYS_ADMIN).

	The current value is 2:

	  -1: Allow use of (almost) all events by all users
	      Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
	>= 0: Disallow ftrace function tracepoint by users without CAP_SYS_ADMIN
	      Disallow raw tracepoint access by users without CAP_SYS_ADMIN
	>= 1: Disallow CPU event access by users without CAP_SYS_ADMIN
	>= 2: Disallow kernel profiling by users without CAP_SYS_ADMIN

	To make this setting permanent, edit /etc/sysctl.conf too, e.g.:

		kernel.perf_event_paranoid = -1

	[jolsa@...va perf]$ ll perf.data
	-rw-------. 1 jolsa jolsa 0 Feb  5 14:40 perf.data

there're multiple points where we could fail during the processing
and fail record command with unfinished perf.data, but yes, I think
we should address that and have some cleanup, I'll check

thanks,
jirka