Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] trace-cmd: Fix the memory mangement of trace-cmd agent.
@ 2021-03-31 23:06 Steven Rostedt
  2021-03-31 23:06 ` [PATCH 1/3] trace-cmd agent: Clean up tsync_agent_thread() and tsync_host_thread() Steven Rostedt
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Steven Rostedt @ 2021-03-31 23:06 UTC (permalink / raw)
  To: linux-trace-devel

When testing out the time sync logic, the agent crashed due to
various memory management issues.

Steven Rostedt (VMware) (3):
      trace-cmd agent: Clean up tsync_agent_thread() and tsync_host_thread()
      trace-cmd: Have tracecmd_tsync_free() free even with no context
      trace-cmd: Do not free tsync from thread

----
 lib/trace-cmd/trace-timesync.c | 49 +++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

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

* [PATCH 1/3] trace-cmd agent: Clean up tsync_agent_thread() and tsync_host_thread()
  2021-03-31 23:06 [PATCH 0/3] trace-cmd: Fix the memory mangement of trace-cmd agent Steven Rostedt
@ 2021-03-31 23:06 ` Steven Rostedt
  2021-03-31 23:06 ` [PATCH 2/3] trace-cmd: Have tracecmd_tsync_free() free even with no context Steven Rostedt
  2021-03-31 23:06 ` [PATCH 3/3] trace-cmd: Do not free tsync from thread Steven Rostedt
  2 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2021-03-31 23:06 UTC (permalink / raw)
  To: linux-trace-devel

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

There's no reason to assign tsync to NULL nor add a typecast to data, as
void pointers naturally can be converted to any type.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 lib/trace-cmd/trace-timesync.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/trace-cmd/trace-timesync.c b/lib/trace-cmd/trace-timesync.c
index ad73f08f7070..4ed283eb1fb8 100644
--- a/lib/trace-cmd/trace-timesync.c
+++ b/lib/trace-cmd/trace-timesync.c
@@ -697,9 +697,8 @@ static int tsync_with_guest(struct tracecmd_time_sync *tsync)
 
 static void *tsync_host_thread(void *data)
 {
-	struct tracecmd_time_sync *tsync = NULL;
+	struct tracecmd_time_sync *tsync = data;
 
-	tsync = (struct tracecmd_time_sync *)data;
 	tsync_with_guest(tsync);
 	tracecmd_msg_handle_close(tsync->msg_handle);
 	tsync->msg_handle = NULL;
@@ -865,11 +864,9 @@ int tracecmd_tsync_with_guest_stop(struct tracecmd_time_sync *tsync)
 
 static void *tsync_agent_thread(void *data)
 {
-	struct tracecmd_time_sync *tsync = NULL;
+	struct tracecmd_time_sync *tsync = data;
 	int sd;
 
-	tsync = (struct tracecmd_time_sync *)data;
-
 	while (true) {
 		sd = accept(tsync->msg_handle->fd, NULL, NULL);
 		if (sd < 0) {
-- 
2.30.1



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

* [PATCH 2/3] trace-cmd: Have tracecmd_tsync_free() free even with no context
  2021-03-31 23:06 [PATCH 0/3] trace-cmd: Fix the memory mangement of trace-cmd agent Steven Rostedt
  2021-03-31 23:06 ` [PATCH 1/3] trace-cmd agent: Clean up tsync_agent_thread() and tsync_host_thread() Steven Rostedt
@ 2021-03-31 23:06 ` Steven Rostedt
  2021-03-31 23:06 ` [PATCH 3/3] trace-cmd: Do not free tsync from thread Steven Rostedt
  2 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2021-03-31 23:06 UTC (permalink / raw)
  To: linux-trace-devel

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

tracecmd_tsync_free() incorrectly exits the function if the tsync has no
context. But it may still need to free up the resources in that case. Do
not leak memory if the context failed to be created.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 lib/trace-cmd/trace-timesync.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/lib/trace-cmd/trace-timesync.c b/lib/trace-cmd/trace-timesync.c
index 4ed283eb1fb8..24984fb17dab 100644
--- a/lib/trace-cmd/trace-timesync.c
+++ b/lib/trace-cmd/trace-timesync.c
@@ -513,25 +513,28 @@ void tracecmd_tsync_free(struct tracecmd_time_sync *tsync)
 	struct clock_sync_context *tsync_context;
 	struct tsync_proto *proto;
 
-	if (!tsync || !tsync->context)
+	if (!tsync)
 		return;
+
 	tsync_context = (struct clock_sync_context *)tsync->context;
 
 	proto = tsync_proto_find(tsync->proto_name);
 	if (proto && proto->clock_sync_free)
 		proto->clock_sync_free(tsync);
 
-	clock_synch_delete_instance(tsync_context->instance);
-	tsync_context->instance = NULL;
-
-	free(tsync_context->sync_ts);
-	free(tsync_context->sync_offsets);
-	free(tsync_context->sync_scalings);
-	tsync_context->sync_ts = NULL;
-	tsync_context->sync_offsets = NULL;
-	tsync_context->sync_scalings = NULL;
-	tsync_context->sync_count = 0;
-	tsync_context->sync_size = 0;
+	if (tsync_context) {
+		clock_synch_delete_instance(tsync_context->instance);
+		tsync_context->instance = NULL;
+
+		free(tsync_context->sync_ts);
+		free(tsync_context->sync_offsets);
+		free(tsync_context->sync_scalings);
+		tsync_context->sync_ts = NULL;
+		tsync_context->sync_offsets = NULL;
+		tsync_context->sync_scalings = NULL;
+		tsync_context->sync_count = 0;
+		tsync_context->sync_size = 0;
+	}
 	pthread_mutex_destroy(&tsync->lock);
 	pthread_cond_destroy(&tsync->cond);
 	pthread_barrier_destroy(&tsync->first_sync);
-- 
2.30.1



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

* [PATCH 3/3] trace-cmd: Do not free tsync from thread
  2021-03-31 23:06 [PATCH 0/3] trace-cmd: Fix the memory mangement of trace-cmd agent Steven Rostedt
  2021-03-31 23:06 ` [PATCH 1/3] trace-cmd agent: Clean up tsync_agent_thread() and tsync_host_thread() Steven Rostedt
  2021-03-31 23:06 ` [PATCH 2/3] trace-cmd: Have tracecmd_tsync_free() free even with no context Steven Rostedt
@ 2021-03-31 23:06 ` Steven Rostedt
  2 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2021-03-31 23:06 UTC (permalink / raw)
  To: linux-trace-devel

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The freeing of an object should happen in the same context as where it is
created. The tsync object is created in the main thread and passed to the
guest and host handling threads. It should be closed and freed by the thread
that created it, especially since that thread accesses the tsync object
after creating the thread. Having the created thread free it would cause all
sorts of dangerous race conditions.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 lib/trace-cmd/trace-timesync.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/trace-cmd/trace-timesync.c b/lib/trace-cmd/trace-timesync.c
index 24984fb17dab..ba877f701c85 100644
--- a/lib/trace-cmd/trace-timesync.c
+++ b/lib/trace-cmd/trace-timesync.c
@@ -535,6 +535,10 @@ void tracecmd_tsync_free(struct tracecmd_time_sync *tsync)
 		tsync_context->sync_count = 0;
 		tsync_context->sync_size = 0;
 	}
+
+	if (tsync->msg_handle)
+		tracecmd_msg_handle_close(tsync->msg_handle);
+
 	pthread_mutex_destroy(&tsync->lock);
 	pthread_cond_destroy(&tsync->cond);
 	pthread_barrier_destroy(&tsync->first_sync);
@@ -703,9 +707,6 @@ static void *tsync_host_thread(void *data)
 	struct tracecmd_time_sync *tsync = data;
 
 	tsync_with_guest(tsync);
-	tracecmd_msg_handle_close(tsync->msg_handle);
-	tsync->msg_handle = NULL;
-
 	pthread_exit(0);
 }
 
@@ -868,6 +869,7 @@ int tracecmd_tsync_with_guest_stop(struct tracecmd_time_sync *tsync)
 static void *tsync_agent_thread(void *data)
 {
 	struct tracecmd_time_sync *tsync = data;
+	long ret = 0;
 	int sd;
 
 	while (true) {
@@ -875,6 +877,7 @@ static void *tsync_agent_thread(void *data)
 		if (sd < 0) {
 			if (errno == EINTR)
 				continue;
+			ret = -1;
 			goto out;
 		}
 		break;
@@ -885,11 +888,7 @@ static void *tsync_agent_thread(void *data)
 	tsync_with_host(tsync);
 
 out:
-	tracecmd_msg_handle_close(tsync->msg_handle);
-	tracecmd_tsync_free(tsync);
-	close(sd);
-
-	pthread_exit(0);
+	pthread_exit((void *)ret);
 }
 
 /**
-- 
2.30.1



^ 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 --
2021-03-31 23:06 [PATCH 0/3] trace-cmd: Fix the memory mangement of trace-cmd agent Steven Rostedt
2021-03-31 23:06 ` [PATCH 1/3] trace-cmd agent: Clean up tsync_agent_thread() and tsync_host_thread() Steven Rostedt
2021-03-31 23:06 ` [PATCH 2/3] trace-cmd: Have tracecmd_tsync_free() free even with no context Steven Rostedt
2021-03-31 23:06 ` [PATCH 3/3] trace-cmd: Do not free tsync from thread Steven Rostedt

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