Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] trace-cmd: Fix a crash on "trace-cmd reset" command
@ 2019-10-18 14:57 Tzvetomir Stoyanov (VMware)
  2019-10-18 14:57 ` [PATCH 2/2] trace-cmd: Check if ftrace file exists, before writing in it Tzvetomir Stoyanov (VMware)
  0 siblings, 1 reply; 4+ messages in thread
From: Tzvetomir Stoyanov (VMware) @ 2019-10-18 14:57 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel

A leftover in reset_cpu_mask() function causes a segmentation
fault of "trace-cmd reset".

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 tracecmd/trace-record.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index cfe67f9..81aca1f 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -4113,8 +4113,6 @@ static void reset_cpu_mask(void)
 
 	for_all_instances(instance)
 		write_instance_file(instance, "tracing_cpumask", buf, "cpumask");
-
-	free(buf);
 }
 
 static void reset_event_pid(void)
-- 
2.21.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] trace-cmd: Check if ftrace file exists, before writing in it.
  2019-10-18 14:57 [PATCH 1/2] trace-cmd: Fix a crash on "trace-cmd reset" command Tzvetomir Stoyanov (VMware)
@ 2019-10-18 14:57 ` Tzvetomir Stoyanov (VMware)
  2019-10-18 15:18   ` Steven Rostedt
  2019-10-18 15:20   ` Valentin Schneider
  0 siblings, 2 replies; 4+ messages in thread
From: Tzvetomir Stoyanov (VMware) @ 2019-10-18 14:57 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel

Depending of the kernel configuration, some ftrace files are optional
and may not exist. This should not confuse trace-cmd, as it is a valid
case. One such case is tracing_max_latency file, when CONFIG_TRACER_MAX_TRACE
or CONFIG_HWLAT_TRACER are not set.
A check is added in write_instance_file() to ensure the file exist before trying
to write in it.

https://bugzilla.kernel.org/show_bug.cgi?id=205241

Reported-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 tracecmd/trace-record.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 81aca1f..c65731f 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -813,11 +813,14 @@ static int
 write_instance_file(struct buffer_instance *instance,
 		    const char *file, const char *str, const char *type)
 {
+	struct stat st;
 	char *path;
 	int ret;
 
 	path = get_instance_file(instance, file);
-	ret = write_file(path, str, type);
+	ret = stat(path, &st);
+	if (ret == 0)
+		ret = write_file(path, str, type);
 	tracecmd_put_tracing_file(path);
 
 	return ret;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] trace-cmd: Check if ftrace file exists, before writing in it.
  2019-10-18 14:57 ` [PATCH 2/2] trace-cmd: Check if ftrace file exists, before writing in it Tzvetomir Stoyanov (VMware)
@ 2019-10-18 15:18   ` Steven Rostedt
  2019-10-18 15:20   ` Valentin Schneider
  1 sibling, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2019-10-18 15:18 UTC (permalink / raw)
  To: Tzvetomir Stoyanov (VMware); +Cc: linux-trace-devel

On Fri, 18 Oct 2019 17:57:59 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> ---
>  tracecmd/trace-record.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 81aca1f..c65731f 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -813,11 +813,14 @@ static int
>  write_instance_file(struct buffer_instance *instance,
>  		    const char *file, const char *str, const char *type)
>  {
> +	struct stat st;
>  	char *path;
>  	int ret;
>  
>  	path = get_instance_file(instance, file);
> -	ret = write_file(path, str, type);
> +	ret = stat(path, &st);

This is fine for now, but perhaps in the future we should check if it
is writable by the user. Hmm, we could move that check to the
write_file() itself. But that will require changing other locations
that expect write_file() to die. Which in the long run, we want to
remove that assumption.

Thanks for the patch, I just applied it.

-- Steve

> +	if (ret == 0)
> +		ret = write_file(path, str, type);
>  	tracecmd_put_tracing_file(path);
>  
>  	return ret;


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] trace-cmd: Check if ftrace file exists, before writing in it.
  2019-10-18 14:57 ` [PATCH 2/2] trace-cmd: Check if ftrace file exists, before writing in it Tzvetomir Stoyanov (VMware)
  2019-10-18 15:18   ` Steven Rostedt
@ 2019-10-18 15:20   ` Valentin Schneider
  1 sibling, 0 replies; 4+ messages in thread
From: Valentin Schneider @ 2019-10-18 15:20 UTC (permalink / raw)
  To: Tzvetomir Stoyanov (VMware), rostedt; +Cc: linux-trace-devel

Hi Tzvetomir,

On 18/10/2019 15:57, Tzvetomir Stoyanov (VMware) wrote:
> Depending of the kernel configuration, some ftrace files are optional
> and may not exist. This should not confuse trace-cmd, as it is a valid
> case. One such case is tracing_max_latency file, when CONFIG_TRACER_MAX_TRACE
> or CONFIG_HWLAT_TRACER are not set.
> A check is added in write_instance_file() to ensure the file exist before trying
> to write in it.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=205241
> 
> Reported-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>

Ran through my test just fine, so:

Tested-by: Valentin Schneider <valentin.schneider@arm.com>

Thanks for looking at this!

> ---
>  tracecmd/trace-record.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 81aca1f..c65731f 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -813,11 +813,14 @@ static int
>  write_instance_file(struct buffer_instance *instance,
>  		    const char *file, const char *str, const char *type)
>  {
> +	struct stat st;
>  	char *path;
>  	int ret;
>  
>  	path = get_instance_file(instance, file);
> -	ret = write_file(path, str, type);
> +	ret = stat(path, &st);
> +	if (ret == 0)
> +		ret = write_file(path, str, type);
>  	tracecmd_put_tracing_file(path);
>  
>  	return ret;
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-18 14:57 [PATCH 1/2] trace-cmd: Fix a crash on "trace-cmd reset" command Tzvetomir Stoyanov (VMware)
2019-10-18 14:57 ` [PATCH 2/2] trace-cmd: Check if ftrace file exists, before writing in it Tzvetomir Stoyanov (VMware)
2019-10-18 15:18   ` Steven Rostedt
2019-10-18 15:20   ` Valentin Schneider

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org
	public-inbox-index linux-trace-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git