[PATCH 1/2] trace-cmd: Enable kptr_restrict

[PATCH 1/2] trace-cmd: Enable kptr_restrict

[vincent.donnefort]

From: Vincent Donnefort <vincent.donnefort@arm.com>

kptr_restrict might prevent trace-cmd from accessing /proc/kallsyms,
leading to a trace without the kernel function names resolved.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 41932ee..3c4f306 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -674,6 +674,39 @@ static int read_event_files(struct tracecmd_output *handle,
 	return ret;
 }
 
+static void set_proc_kptr_restrict(int reset)
+{
+	char *path = "/proc/sys/kernel/kptr_restrict";
+	static char saved = 'X';
+	int fd, ret = -1;
+	char buf;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		goto err;
+
+	if (reset) {
+		buf = saved;
+	} else {
+		if (read(fd, &buf, 1) < 0)
+			goto err;
+		saved = buf;
+		buf = '0';
+	}
+	close(fd);
+
+	fd = open(path, O_WRONLY);
+	if (fd < 0)
+		goto err;
+	if (write(fd, &buf, 1) > 0)
+		ret = 0;
+err:
+	if (fd > 0)
+		close(fd);
+	if (ret)
+		warning("can't set kptr_restrict");
+}
+
 static int read_proc_kallsyms(struct tracecmd_output *handle,
 			      const char *kallsyms)
 {
@@ -698,12 +731,16 @@ static int read_proc_kallsyms(struct tracecmd_output *handle,
 	endian4 = convert_endian_4(handle, size);
 	if (do_write_check(handle, &endian4, 4))
 		return -1;
+
+	set_proc_kptr_restrict(0);
 	check_size = copy_file(handle, path);
 	if (size != check_size) {
 		errno = EINVAL;
 		warning("error in size of file '%s'", path);
+		set_proc_kptr_restrict(1);
 		return -1;
 	}
+	set_proc_kptr_restrict(1);
 
 	return 0;
 }
-- 
2.7.4

[PATCH 2/2] trace-cmd: Add an option to set saved_cmdlines_size

[vincent.donnefort]

From: Vincent Donnefort <vincent.donnefort@arm.com>

The tracing file saved_cmdlines_size allows setting the number of entries
that saved_cmdlines will contain. The latter is then dumped into the
trace.dat file to map PIDs with comm. The default value is 128.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 7260d27..a4e10a4 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -229,6 +229,7 @@ struct common_record_context {
 	int topt;
 	int do_child;
 	int run_command;
+	int saved_cmdlines_size;
 };
 
 static void add_reset_file(const char *file, const char *val, int prio)
@@ -1810,6 +1811,39 @@ static void set_options(void)
 	}
 }
 
+static void set_saved_cmdlines_size(struct common_record_context *ctx)
+{
+	char *path, *str;
+	int fd, len, ret;
+
+	if (!ctx->saved_cmdlines_size)
+		return;
+
+	path = tracecmd_get_tracing_file("saved_cmdlines_size");
+	if (!path)
+		goto err;
+
+	reset_save_file(path, RESET_DEFAULT_PRIO);
+
+	fd = open(path, O_WRONLY);
+	tracecmd_put_tracing_file(path);
+	if (fd < 0)
+		goto err;
+
+	len = asprintf(&str, "%d", ctx->saved_cmdlines_size);
+	if (len < 0)
+		die("%s couldn't allocate memory", __func__);
+
+	if (write(fd, str, len) > 0)
+		ret = 0;
+
+	close(fd);
+	free(str);
+err:
+	if (ret)
+		warning("Couldn't set saved_cmdlines_size");
+}
+
 static int trace_check_file_exists(struct buffer_instance *instance, char *file)
 {
 	struct stat st;
@@ -5480,7 +5514,7 @@ static void parse_record_options(int argc,
 		if (IS_EXTRACT(ctx))
 			opts = "+haf:Fp:co:O:sr:g:l:n:P:N:tb:B:ksiT";
 		else
-			opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:ksSiTm:M:H:q";
+			opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:kK:sSiTm:M:H:qK";
 		c = getopt_long (argc-1, argv+1, opts, long_options, &option_index);
 		if (c == -1)
 			break;
@@ -5741,6 +5775,9 @@ static void parse_record_options(int argc,
 		case 'k':
 			keep = 1;
 			break;
+		case 'K':
+			ctx->saved_cmdlines_size = atoi(optarg);
+			break;
 		case 'i':
 			ignore_event_not_found = 1;
 			break;
@@ -5990,6 +6027,7 @@ static void record_trace(int argc, char **argv,
 			enable_events(instance);
 	}
 
+	set_saved_cmdlines_size(ctx);
 	set_buffer_size();
 	update_plugins(type);
 	set_options();
diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
index b5788f7..ddfb480 100644
--- a/tracecmd/trace-usage.c
+++ b/tracecmd/trace-usage.c
@@ -47,6 +47,7 @@ static struct usage_help usage_help[] = {
 		"          -b change kernel buffersize (in kilobytes per CPU)\n"
 		"          -B create sub buffer and following events will be enabled here\n"
 		"          -k do not reset the buffers after tracing.\n"
+		"          -K change kernel saved_cmdlines_size\n"
 		"          -i do not fail if an event is not found\n"
 		"          -q print no output to the screen\n"
 		"          --quiet print no output to the screen\n"
-- 
2.7.4

Re: [PATCH 1/2] trace-cmd: Enable kptr_restrict

[Steven Rostedt]

Hi Vincent!


On Thu,  7 Nov 2019 10:51:05 +0000
vincent.donnefort@arm.com wrote:

> From: Vincent Donnefort <vincent.donnefort@arm.com>
> 
> kptr_restrict might prevent trace-cmd from accessing /proc/kallsyms,
> leading to a trace without the kernel function names resolved.
> 
> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> 
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 41932ee..3c4f306 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -674,6 +674,39 @@ static int read_event_files(struct tracecmd_output *handle,
>  	return ret;
>  }
>  
> +static void set_proc_kptr_restrict(int reset)
> +{
> +	char *path = "/proc/sys/kernel/kptr_restrict";

I believe this is a relatively new file. We should do a stat to see if
it exists, and if it does not, simply fail silently. I don't think we
want to give a warning if the file doesn't exist because the kernel
doesn't have it.

> +	static char saved = 'X';
> +	int fd, ret = -1;
> +	char buf;
> +
> +	fd = open(path, O_RDONLY);
> +	if (fd < 0)
> +		goto err;
> +
> +	if (reset) {
> +		buf = saved;
> +	} else {
> +		if (read(fd, &buf, 1) < 0)
> +			goto err;
> +		saved = buf;
> +		buf = '0';
> +	}
> +	close(fd);
> +

Perhaps if reset is true and buf == 'X', we should simply exit, as it
would appear that we never put anything into buf. And probably should
make the 'X' a macro:

#define KPTR_UNINITIALIZED	'X'

	static char saved = KPTR_UNINITIALIZED;

[..]

	if (reset && buf == KPTR_UNINITIALIZED)
		return;


-- Steve

> +	fd = open(path, O_WRONLY);
> +	if (fd < 0)
> +		goto err;
> +	if (write(fd, &buf, 1) > 0)
> +		ret = 0;
> +err:
> +	if (fd > 0)
> +		close(fd);
> +	if (ret)
> +		warning("can't set kptr_restrict");
> +}
> +
>  static int read_proc_kallsyms(struct tracecmd_output *handle,
>  			      const char *kallsyms)
>  {
> @@ -698,12 +731,16 @@ static int read_proc_kallsyms(struct tracecmd_output *handle,
>  	endian4 = convert_endian_4(handle, size);
>  	if (do_write_check(handle, &endian4, 4))
>  		return -1;
> +
> +	set_proc_kptr_restrict(0);
>  	check_size = copy_file(handle, path);
>  	if (size != check_size) {
>  		errno = EINVAL;
>  		warning("error in size of file '%s'", path);
> +		set_proc_kptr_restrict(1);
>  		return -1;
>  	}
> +	set_proc_kptr_restrict(1);
>  
>  	return 0;
>  }

Re: [PATCH 2/2] trace-cmd: Add an option to set saved_cmdlines_size

[Steven Rostedt]

On Thu,  7 Nov 2019 10:51:06 +0000
vincent.donnefort@arm.com wrote:

> From: Vincent Donnefort <vincent.donnefort@arm.com>
> 
> The tracing file saved_cmdlines_size allows setting the number of entries
> that saved_cmdlines will contain. The latter is then dumped into the
> trace.dat file to map PIDs with comm. The default value is 128.
> 
> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 7260d27..a4e10a4 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -229,6 +229,7 @@ struct common_record_context {
>  	int topt;
>  	int do_child;
>  	int run_command;
> +	int saved_cmdlines_size;
>  };
>  
>  static void add_reset_file(const char *file, const char *val, int prio)
> @@ -1810,6 +1811,39 @@ static void set_options(void)
>  	}
>  }
>  
> +static void set_saved_cmdlines_size(struct common_record_context *ctx)
> +{
> +	char *path, *str;
> +	int fd, len, ret;
> +
> +	if (!ctx->saved_cmdlines_size)
> +		return;
> +
> +	path = tracecmd_get_tracing_file("saved_cmdlines_size");
> +	if (!path)
> +		goto err;
> +
> +	reset_save_file(path, RESET_DEFAULT_PRIO);
> +
> +	fd = open(path, O_WRONLY);
> +	tracecmd_put_tracing_file(path);
> +	if (fd < 0)
> +		goto err;
> +
> +	len = asprintf(&str, "%d", ctx->saved_cmdlines_size);
> +	if (len < 0)
> +		die("%s couldn't allocate memory", __func__);
> +
> +	if (write(fd, str, len) > 0)
> +		ret = 0;
> +
> +	close(fd);
> +	free(str);
> +err:
> +	if (ret)
> +		warning("Couldn't set saved_cmdlines_size");
> +}
> +
>  static int trace_check_file_exists(struct buffer_instance *instance, char *file)
>  {
>  	struct stat st;
> @@ -5480,7 +5514,7 @@ static void parse_record_options(int argc,
>  		if (IS_EXTRACT(ctx))
>  			opts = "+haf:Fp:co:O:sr:g:l:n:P:N:tb:B:ksiT";
>  		else
> -			opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:ksSiTm:M:H:q";
> +			opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:kK:sSiTm:M:H:qK";
>  		c = getopt_long (argc-1, argv+1, opts, long_options, &option_index);
>  		if (c == -1)
>  			break;
> @@ -5741,6 +5775,9 @@ static void parse_record_options(int argc,
>  		case 'k':
>  			keep = 1;
>  			break;
> +		case 'K':
> +			ctx->saved_cmdlines_size = atoi(optarg);
> +			break;

Perhaps we should make this a long variable --cmdline-size(?), and not
yet give it a short option. 

>  		case 'i':
>  			ignore_event_not_found = 1;
>  			break;
> @@ -5990,6 +6027,7 @@ static void record_trace(int argc, char **argv,
>  			enable_events(instance);
>  	}
>  
> +	set_saved_cmdlines_size(ctx);
>  	set_buffer_size();
>  	update_plugins(type);
>  	set_options();
> diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
> index b5788f7..ddfb480 100644
> --- a/tracecmd/trace-usage.c
> +++ b/tracecmd/trace-usage.c
> @@ -47,6 +47,7 @@ static struct usage_help usage_help[] = {
>  		"          -b change kernel buffersize (in kilobytes per CPU)\n"
>  		"          -B create sub buffer and following events will be enabled here\n"
>  		"          -k do not reset the buffers after tracing.\n"
> +		"          -K change kernel saved_cmdlines_size\n"

We would also want to update the man pages too.

Thanks!

-- Steve

>  		"          -i do not fail if an event is not found\n"
>  		"          -q print no output to the screen\n"
>  		"          --quiet print no output to the screen\n"

Re: [PATCH 2/2] trace-cmd: Add an option to set saved_cmdlines_size

[Vincent Donnefort]

On Mon, Nov 11, 2019 at 05:52:03PM -0500, Steven Rostedt wrote:
> On Thu,  7 Nov 2019 10:51:06 +0000
> vincent.donnefort@arm.com wrote:
>
> > From: Vincent Donnefort <vincent.donnefort@arm.com>
> >
> > The tracing file saved_cmdlines_size allows setting the number of entries
> > that saved_cmdlines will contain. The latter is then dumped into the
> > trace.dat file to map PIDs with comm. The default value is 128.
> >
> > Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> >
> > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> > index 7260d27..a4e10a4 100644
> > --- a/tracecmd/trace-record.c
> > +++ b/tracecmd/trace-record.c
> > @@ -229,6 +229,7 @@ struct common_record_context {
> >     int topt;
> >     int do_child;
> >     int run_command;
> > +   int saved_cmdlines_size;
> >  };
> >
> >  static void add_reset_file(const char *file, const char *val, int prio)
> > @@ -1810,6 +1811,39 @@ static void set_options(void)
> >     }
> >  }
> >
> > +static void set_saved_cmdlines_size(struct common_record_context *ctx)
> > +{
> > +   char *path, *str;
> > +   int fd, len, ret;
> > +
> > +   if (!ctx->saved_cmdlines_size)
> > +           return;
> > +
> > +   path = tracecmd_get_tracing_file("saved_cmdlines_size");
> > +   if (!path)
> > +           goto err;
> > +
> > +   reset_save_file(path, RESET_DEFAULT_PRIO);
> > +
> > +   fd = open(path, O_WRONLY);
> > +   tracecmd_put_tracing_file(path);
> > +   if (fd < 0)
> > +           goto err;
> > +
> > +   len = asprintf(&str, "%d", ctx->saved_cmdlines_size);
> > +   if (len < 0)
> > +           die("%s couldn't allocate memory", __func__);
> > +
> > +   if (write(fd, str, len) > 0)
> > +           ret = 0;
> > +
> > +   close(fd);
> > +   free(str);
> > +err:
> > +   if (ret)
> > +           warning("Couldn't set saved_cmdlines_size");
> > +}
> > +
> >  static int trace_check_file_exists(struct buffer_instance *instance, char *file)
> >  {
> >     struct stat st;
> > @@ -5480,7 +5514,7 @@ static void parse_record_options(int argc,
> >             if (IS_EXTRACT(ctx))
> >                     opts = "+haf:Fp:co:O:sr:g:l:n:P:N:tb:B:ksiT";
> >             else
> > -                   opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:ksSiTm:M:H:q";
> > +                   opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:kK:sSiTm:M:H:qK";
> >             c = getopt_long (argc-1, argv+1, opts, long_options, &option_index);
> >             if (c == -1)
> >                     break;
> > @@ -5741,6 +5775,9 @@ static void parse_record_options(int argc,
> >             case 'k':
> >                     keep = 1;
> >                     break;
> > +           case 'K':
> > +                   ctx->saved_cmdlines_size = atoi(optarg);
> > +                   break;
>
> Perhaps we should make this a long variable --cmdline-size(?), and not
> yet give it a short option.

Hi Steve,

Thank you for your review. I also thought that we could avoid having a new
option and try to write /proc/sys/kernel/threads-max into saved_cmdlines_size,
before recording the trace. Do you think that would be a more suitable
solution?

--
Vincent

>
> >             case 'i':
> >                     ignore_event_not_found = 1;
> >                     break;
> > @@ -5990,6 +6027,7 @@ static void record_trace(int argc, char **argv,
> >                     enable_events(instance);
> >     }
> >
> > +   set_saved_cmdlines_size(ctx);
> >     set_buffer_size();
> >     update_plugins(type);
> >     set_options();
> > diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
> > index b5788f7..ddfb480 100644
> > --- a/tracecmd/trace-usage.c
> > +++ b/tracecmd/trace-usage.c
> > @@ -47,6 +47,7 @@ static struct usage_help usage_help[] = {
> >             "          -b change kernel buffersize (in kilobytes per CPU)\n"
> >             "          -B create sub buffer and following events will be enabled here\n"
> >             "          -k do not reset the buffers after tracing.\n"
> > +           "          -K change kernel saved_cmdlines_size\n"
>
> We would also want to update the man pages too.
>
> Thanks!
>
> -- Steve
>
> >             "          -i do not fail if an event is not found\n"
> >             "          -q print no output to the screen\n"
> >             "          --quiet print no output to the screen\n"
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v2 1/2] trace-cmd: Enable kptr_restrict

[vincent.donnefort]

From: Vincent Donnefort <vincent.donnefort@arm.com>

kptr_restrict might prevent trace-cmd from accessing /proc/kallsyms,
leading to a trace without the kernel function names resolved.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 41932ee..be4d3f5 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -674,6 +674,46 @@ static int read_event_files(struct tracecmd_output *handle,
 	return ret;
 }
 
+#define KPTR_UNINITIALIZED 'X'
+
+static void set_proc_kptr_restrict(int reset)
+{
+	char *path = "/proc/sys/kernel/kptr_restrict";
+	static char saved = KPTR_UNINITIALIZED;
+	int fd, ret = -1;
+	struct stat st;
+	char buf;
+
+	if ((reset && saved == KPTR_UNINITIALIZED) ||
+	    (stat(path, &st) < 0))
+		return;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		goto err;
+
+	if (reset) {
+		buf = saved;
+	} else {
+		if (read(fd, &buf, 1) < 0)
+			goto err;
+		saved = buf;
+		buf = '0';
+	}
+	close(fd);
+
+	fd = open(path, O_WRONLY);
+	if (fd < 0)
+		goto err;
+	if (write(fd, &buf, 1) > 0)
+		ret = 0;
+err:
+	if (fd > 0)
+		close(fd);
+	if (ret)
+		warning("can't set kptr_restrict");
+}
+
 static int read_proc_kallsyms(struct tracecmd_output *handle,
 			      const char *kallsyms)
 {
@@ -698,12 +738,16 @@ static int read_proc_kallsyms(struct tracecmd_output *handle,
 	endian4 = convert_endian_4(handle, size);
 	if (do_write_check(handle, &endian4, 4))
 		return -1;
+
+	set_proc_kptr_restrict(0);
 	check_size = copy_file(handle, path);
 	if (size != check_size) {
 		errno = EINVAL;
 		warning("error in size of file '%s'", path);
+		set_proc_kptr_restrict(1);
 		return -1;
 	}
+	set_proc_kptr_restrict(1);
 
 	return 0;
 }
-- 
2.7.4

[PATCH v2 2/2] trace-cmd: Add an option to set saved_cmdlines_size

[vincent.donnefort]

From: Vincent Donnefort <vincent.donnefort@arm.com>

The tracing file saved_cmdlines_size allows setting the number of entries
that saved_cmdlines will contain. The latter is then dumped into the
trace.dat file to map PIDs with comm. The default value is 128.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>

diff --git a/Documentation/trace-cmd-record.1.txt b/Documentation/trace-cmd-record.1.txt
index bb18e88..0d75e43 100644
--- a/Documentation/trace-cmd-record.1.txt
+++ b/Documentation/trace-cmd-record.1.txt
@@ -280,6 +280,12 @@ OPTIONS
     A value of one will only show where userspace enters the kernel but not any
     functions called in the kernel. The default is zero, which means no limit.
 
+*--cmdlines-size* 'size'::
+    Set the number of entries the kernel tracing file "saved_cmdlines" can
+    contain. This file is a circular buffer which stores the mapping between
+    cmdlines and PIDs. If full, it leads to unresolved cmdlines ("<...>") within
+    the trace. The kernel default value is 128.
+
 *--module* 'module'::
     Filter a module's name in function tracing. It is equivalent to adding
     ':mod:module' after all other functions being filtered. If no other function
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 7260d27..82556b0 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -229,6 +229,7 @@ struct common_record_context {
 	int topt;
 	int do_child;
 	int run_command;
+	int saved_cmdlines_size;
 };
 
 static void add_reset_file(const char *file, const char *val, int prio)
@@ -1810,6 +1811,39 @@ static void set_options(void)
 	}
 }
 
+static void set_saved_cmdlines_size(struct common_record_context *ctx)
+{
+	char *path, *str;
+	int fd, len, ret;
+
+	if (!ctx->saved_cmdlines_size)
+		return;
+
+	path = tracecmd_get_tracing_file("saved_cmdlines_size");
+	if (!path)
+		goto err;
+
+	reset_save_file(path, RESET_DEFAULT_PRIO);
+
+	fd = open(path, O_WRONLY);
+	tracecmd_put_tracing_file(path);
+	if (fd < 0)
+		goto err;
+
+	len = asprintf(&str, "%d", ctx->saved_cmdlines_size);
+	if (len < 0)
+		die("%s couldn't allocate memory", __func__);
+
+	if (write(fd, str, len) > 0)
+		ret = 0;
+
+	close(fd);
+	free(str);
+err:
+	if (ret)
+		warning("Couldn't set saved_cmdlines_size");
+}
+
 static int trace_check_file_exists(struct buffer_instance *instance, char *file)
 {
 	struct stat st;
@@ -5190,6 +5224,7 @@ enum {
 	OPT_date		= 255,
 	OPT_module		= 256,
 	OPT_nofifos		= 257,
+	OPT_cmdlines_size	= 258,
 };
 
 void trace_stop(int argc, char **argv)
@@ -5467,6 +5502,7 @@ static void parse_record_options(int argc,
 			{"by-comm", no_argument, NULL, OPT_bycomm},
 			{"ts-offset", required_argument, NULL, OPT_tsoffset},
 			{"max-graph-depth", required_argument, NULL, OPT_max_graph_depth},
+			{"cmdlines-size", required_argument, NULL, OPT_cmdlines_size},
 			{"no-filter", no_argument, NULL, OPT_no_filter},
 			{"debug", no_argument, NULL, OPT_debug},
 			{"quiet", no_argument, NULL, OPT_quiet},
@@ -5480,7 +5516,7 @@ static void parse_record_options(int argc,
 		if (IS_EXTRACT(ctx))
 			opts = "+haf:Fp:co:O:sr:g:l:n:P:N:tb:B:ksiT";
 		else
-			opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:ksSiTm:M:H:q";
+			opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:kK:sSiTm:M:H:qK";
 		c = getopt_long (argc-1, argv+1, opts, long_options, &option_index);
 		if (c == -1)
 			break;
@@ -5795,6 +5831,9 @@ static void parse_record_options(int argc,
 			if (!ctx->instance->max_graph_depth)
 				die("Could not allocate option");
 			break;
+		case OPT_cmdlines_size:
+			ctx->saved_cmdlines_size = atoi(optarg);
+			break;
 		case OPT_no_filter:
 			no_filter = true;
 			break;
@@ -5990,6 +6029,7 @@ static void record_trace(int argc, char **argv,
 			enable_events(instance);
 	}
 
+	set_saved_cmdlines_size(ctx);
 	set_buffer_size();
 	update_plugins(type);
 	set_options();
diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
index b5788f7..0080522 100644
--- a/tracecmd/trace-usage.c
+++ b/tracecmd/trace-usage.c
@@ -56,6 +56,7 @@ static struct usage_help usage_help[] = {
 		"          --func-stack perform a stack trace for function tracer\n"
 		"             (use with caution)\n"
 		"          --max-graph-depth limit function_graph depth\n"
+		"          --cmdlines_size change kernel saved_cmdlines_size\n"
 		"          --no-filter include trace-cmd threads in the trace\n"
 		"          --proc-map save the traced processes address map into the trace.dat file\n"
 		"          --user execute the specified [command ...] as given user\n"
-- 
2.7.4

Re: [PATCH 2/2] trace-cmd: Add an option to set saved_cmdlines_size

[Steven Rostedt]

On Tue, 12 Nov 2019 12:03:15 +0000
Vincent Donnefort <vincent.donnefort@arm.com> wrote:

> Hi Steve,
> 
> Thank you for your review. I also thought that we could avoid having a new
> option and try to write /proc/sys/kernel/threads-max into saved_cmdlines_size,
> before recording the trace. Do you think that would be a more suitable
> solution?

That's an interesting approach. I still think that having a parameter
would be good as well, but then use the threads-max approach if one is
not supplied.

-- Steve

Re: [PATCH 2/2] trace-cmd: Add an option to set saved_cmdlines_size

[Vincent Donnefort]

On Tue, Nov 12, 2019 at 09:40:37AM -0500, Steven Rostedt wrote:
> On Tue, 12 Nov 2019 12:03:15 +0000
> Vincent Donnefort <vincent.donnefort@arm.com> wrote:
>
> > Hi Steve,
> >
> > Thank you for your review. I also thought that we could avoid having a new
> > option and try to write /proc/sys/kernel/threads-max into saved_cmdlines_size,
> > before recording the trace. Do you think that would be a more suitable
> > solution?
>
> That's an interesting approach. I still think that having a parameter
> would be good as well, but then use the threads-max approach if one is
> not supplied.
>
> -- Steve

This was not a good idea in the end. We can't rely on the sysctl entries
threads-max or pid_max, as the pid/comm map is static and the maximum is
defined by PID_MAX_DEFAULT. I do not see any way of getting this value from
user-space.

We could though, as an alternative, use a hard-coded value of 32768 for
--cmdlines-size. Without being perfect, this should still be true for most of
the systems. PID_MAX_DEFAULT depends on CONFIG_BASE_SMALL and hasn't changed
since Linux started using Git.

--
Vincent
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 2/2] trace-cmd: Add an option to set saved_cmdlines_size

[Steven Rostedt]

On Tue, 12 Nov 2019 19:01:02 +0000
Vincent Donnefort <vincent.donnefort@arm.com> wrote:

> This was not a good idea in the end. We can't rely on the sysctl entries
> threads-max or pid_max, as the pid/comm map is static and the maximum is
> defined by PID_MAX_DEFAULT. I do not see any way of getting this value from
> user-space.
> 
> We could though, as an alternative, use a hard-coded value of 32768 for
> --cmdlines-size. Without being perfect, this should still be true for most of
> the systems. PID_MAX_DEFAULT depends on CONFIG_BASE_SMALL and hasn't changed
> since Linux started using Git.

But that does take up a bit of memory in the kernel, which isn't
probably needed. What we could do, is periodically record the contents
of the file, and update the map at the end of the record.

Perhaps I should even make it so that we can just get notifications
when it does get updated. But that would require a kernel change.

-- Steve

[PATCH v2 1/2] trace-cmd: Enable kptr_restrict

[vincent.donnefort]

From: Vincent Donnefort <vincent.donnefort@arm.com>

kptr_restrict might prevent trace-cmd from accessing /proc/kallsyms,
leading to a trace without the kernel function names resolved.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>

diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 41932ee..be4d3f5 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -674,6 +674,46 @@ static int read_event_files(struct tracecmd_output *handle,
 	return ret;
 }
 
+#define KPTR_UNINITIALIZED 'X'
+
+static void set_proc_kptr_restrict(int reset)
+{
+	char *path = "/proc/sys/kernel/kptr_restrict";
+	static char saved = KPTR_UNINITIALIZED;
+	int fd, ret = -1;
+	struct stat st;
+	char buf;
+
+	if ((reset && saved == KPTR_UNINITIALIZED) ||
+	    (stat(path, &st) < 0))
+		return;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		goto err;
+
+	if (reset) {
+		buf = saved;
+	} else {
+		if (read(fd, &buf, 1) < 0)
+			goto err;
+		saved = buf;
+		buf = '0';
+	}
+	close(fd);
+
+	fd = open(path, O_WRONLY);
+	if (fd < 0)
+		goto err;
+	if (write(fd, &buf, 1) > 0)
+		ret = 0;
+err:
+	if (fd > 0)
+		close(fd);
+	if (ret)
+		warning("can't set kptr_restrict");
+}
+
 static int read_proc_kallsyms(struct tracecmd_output *handle,
 			      const char *kallsyms)
 {
@@ -698,12 +738,16 @@ static int read_proc_kallsyms(struct tracecmd_output *handle,
 	endian4 = convert_endian_4(handle, size);
 	if (do_write_check(handle, &endian4, 4))
 		return -1;
+
+	set_proc_kptr_restrict(0);
 	check_size = copy_file(handle, path);
 	if (size != check_size) {
 		errno = EINVAL;
 		warning("error in size of file '%s'", path);
+		set_proc_kptr_restrict(1);
 		return -1;
 	}
+	set_proc_kptr_restrict(1);
 
 	return 0;
 }
-- 
2.7.4

[PATCH v2 2/2] trace-cmd: Add an option to set saved_cmdlines_size

[vincent.donnefort]

From: Vincent Donnefort <vincent.donnefort@arm.com>

The tracing file saved_cmdlines_size allows setting the number of entries
that saved_cmdlines will contain. The latter is then dumped into the
trace.dat file to map PIDs with comm. The default value is 128.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>

diff --git a/Documentation/trace-cmd-record.1.txt b/Documentation/trace-cmd-record.1.txt
index bb18e88..0d75e43 100644
--- a/Documentation/trace-cmd-record.1.txt
+++ b/Documentation/trace-cmd-record.1.txt
@@ -280,6 +280,12 @@ OPTIONS
     A value of one will only show where userspace enters the kernel but not any
     functions called in the kernel. The default is zero, which means no limit.
 
+*--cmdlines-size* 'size'::
+    Set the number of entries the kernel tracing file "saved_cmdlines" can
+    contain. This file is a circular buffer which stores the mapping between
+    cmdlines and PIDs. If full, it leads to unresolved cmdlines ("<...>") within
+    the trace. The kernel default value is 128.
+
 *--module* 'module'::
     Filter a module's name in function tracing. It is equivalent to adding
     ':mod:module' after all other functions being filtered. If no other function
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 7260d27..8fbfa35 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -229,6 +229,7 @@ struct common_record_context {
 	int topt;
 	int do_child;
 	int run_command;
+	int saved_cmdlines_size;
 };
 
 static void add_reset_file(const char *file, const char *val, int prio)
@@ -1810,6 +1811,39 @@ static void set_options(void)
 	}
 }
 
+static void set_saved_cmdlines_size(struct common_record_context *ctx)
+{
+	int fd, len, ret = -1;
+	char *path, *str;
+
+	if (!ctx->saved_cmdlines_size)
+		return;
+
+	path = tracecmd_get_tracing_file("saved_cmdlines_size");
+	if (!path)
+		goto err;
+
+	reset_save_file(path, RESET_DEFAULT_PRIO);
+
+	fd = open(path, O_WRONLY);
+	tracecmd_put_tracing_file(path);
+	if (fd < 0)
+		goto err;
+
+	len = asprintf(&str, "%d", ctx->saved_cmdlines_size);
+	if (len < 0)
+		die("%s couldn't allocate memory", __func__);
+
+	if (write(fd, str, len) > 0)
+		ret = 0;
+
+	close(fd);
+	free(str);
+err:
+	if (ret)
+		warning("Couldn't set saved_cmdlines_size");
+}
+
 static int trace_check_file_exists(struct buffer_instance *instance, char *file)
 {
 	struct stat st;
@@ -5190,6 +5224,7 @@ enum {
 	OPT_date		= 255,
 	OPT_module		= 256,
 	OPT_nofifos		= 257,
+	OPT_cmdlines_size	= 258,
 };
 
 void trace_stop(int argc, char **argv)
@@ -5467,6 +5502,7 @@ static void parse_record_options(int argc,
 			{"by-comm", no_argument, NULL, OPT_bycomm},
 			{"ts-offset", required_argument, NULL, OPT_tsoffset},
 			{"max-graph-depth", required_argument, NULL, OPT_max_graph_depth},
+			{"cmdlines-size", required_argument, NULL, OPT_cmdlines_size},
 			{"no-filter", no_argument, NULL, OPT_no_filter},
 			{"debug", no_argument, NULL, OPT_debug},
 			{"quiet", no_argument, NULL, OPT_quiet},
@@ -5795,6 +5831,9 @@ static void parse_record_options(int argc,
 			if (!ctx->instance->max_graph_depth)
 				die("Could not allocate option");
 			break;
+		case OPT_cmdlines_size:
+			ctx->saved_cmdlines_size = atoi(optarg);
+			break;
 		case OPT_no_filter:
 			no_filter = true;
 			break;
@@ -5990,6 +6029,7 @@ static void record_trace(int argc, char **argv,
 			enable_events(instance);
 	}
 
+	set_saved_cmdlines_size(ctx);
 	set_buffer_size();
 	update_plugins(type);
 	set_options();
diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
index b5788f7..05ec021 100644
--- a/tracecmd/trace-usage.c
+++ b/tracecmd/trace-usage.c
@@ -56,6 +56,7 @@ static struct usage_help usage_help[] = {
 		"          --func-stack perform a stack trace for function tracer\n"
 		"             (use with caution)\n"
 		"          --max-graph-depth limit function_graph depth\n"
+		"          --cmdlines-size change kernel saved_cmdlines_size\n"
 		"          --no-filter include trace-cmd threads in the trace\n"
 		"          --proc-map save the traced processes address map into the trace.dat file\n"
 		"          --user execute the specified [command ...] as given user\n"
-- 
2.7.4

Re: [PATCH 2/2] trace-cmd: Add an option to set saved_cmdlines_size

[Vincent Donnefort]

On Tue, Nov 12, 2019 at 02:09:20PM -0500, Steven Rostedt wrote:
> On Tue, 12 Nov 2019 19:01:02 +0000
> Vincent Donnefort <vincent.donnefort@arm.com> wrote:
> 
> > This was not a good idea in the end. We can't rely on the sysctl entries
> > threads-max or pid_max, as the pid/comm map is static and the maximum is
> > defined by PID_MAX_DEFAULT. I do not see any way of getting this value from
> > user-space.
> > 
> > We could though, as an alternative, use a hard-coded value of 32768 for
> > --cmdlines-size. Without being perfect, this should still be true for most of
> > the systems. PID_MAX_DEFAULT depends on CONFIG_BASE_SMALL and hasn't changed
> > since Linux started using Git.
> 
> But that does take up a bit of memory in the kernel, which isn't
> probably needed. What we could do, is periodically record the contents
> of the file, and update the map at the end of the record.
> 
> Perhaps I should even make it so that we can just get notifications
> when it does get updated. But that would require a kernel change.
> 
> -- Steve

Hi Steve,

I then pushed an updated version covering your previous comments, small issues
and introducing the --cmdlines-size option without any default value.

Aside, apology for the disclaimer notice into the previous emails. This should
hopefully be gone by now.

-- 
Vincent

Re: [PATCH 2/2] trace-cmd: Add an option to set saved_cmdlines_size

[Steven Rostedt]

On Wed, 13 Nov 2019 12:27:35 +0000
Vincent Donnefort <vincent.donnefort@arm.com> wrote:

> Hi Steve,
> 
> I then pushed an updated version covering your previous comments, small issues
> and introducing the --cmdlines-size option without any default value.

I actually had applied both your patches and pushed them up. I reverted
the second one to apply this updated version.

> 
> Aside, apology for the disclaimer notice into the previous emails. This should
> hopefully be gone by now.

No problem.

Thanks!

-- Steve