[PATCH 0/3] libtracefs: Updates to trace_marker functions

[PATCH 0/3] libtracefs: Updates to trace_marker functions

[Steven Rostedt]

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

[
  This is on top of:
  https://lore.kernel.org/linux-trace-devel/20210409042739.3179257-1-tz.stoyanov@gmail.com/
]

This is some updates to the tracefs_print*() and tracefs_binary*()
functions. The first patch does some clean ups, the second adds a helper
function to find the right pthread mutex to use, and the last patch adds
a pthread mutex around the modifications of the file descriptors used
for writing to the file.

Note, the lock only protects against the modification of the file
descriptor (opening and closing it). It does not protect against writing
to it (a write updates the file the file descriptor points to, but the
value of the file descriptor - the number - is left unchanged).

It is now safe to use tracefs_v?print() and tracefs_binary_write() along
with tracefs_print_init() and tracefs_binary_init(), where it will
open the file descriptor if it is not open yet, and return it if it is
already opened.

The writes are not safe to use against tracefs_print_close() or
tracefs_binary_close(), and that is up to the application to protect.


Steven Rostedt (VMware) (3):
  libtracefs: Do some cleanups to the trace_marker.c code
  libtracefs: Add helper function trace_get_lock()
  libtracefs: Add lock around modifying the trace_marker file descriptor

 include/tracefs-local.h |  5 +++++
 src/tracefs-marker.c    | 44 +++++++++++++++++++++++++++++++----------
 src/tracefs-tools.c     |  2 +-
 3 files changed, 40 insertions(+), 11 deletions(-)

-- 
2.29.2

[PATCH 1/3] libtracefs: Do some cleanups to the trace_marker.c code

[Steven Rostedt]

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

Just some clean ups.

- Make the if statement simpler in marker_init()
- Remove unnecessary "+ 1" to string length when writing to the
  trace_marker file.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 src/tracefs-marker.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/tracefs-marker.c b/src/tracefs-marker.c
index 0dc72f6..924ceea 100644
--- a/src/tracefs-marker.c
+++ b/src/tracefs-marker.c
@@ -24,14 +24,13 @@ static inline int *get_marker_fd(struct tracefs_instance *instance, bool raw)
 
 static int marker_init(struct tracefs_instance *instance, bool raw)
 {
+	const char *file = raw ? "trace_marker_raw" : "trace_marker";
 	int *fd = get_marker_fd(instance, raw);
 
 	if (*fd >= 0)
 		return 0;
-	if (raw)
-		*fd = tracefs_instance_file_open(instance, "trace_marker_raw", O_WRONLY | O_CLOEXEC);
-	else
-		*fd = tracefs_instance_file_open(instance, "trace_marker", O_WRONLY | O_CLOEXEC);
+
+	*fd = tracefs_instance_file_open(instance, file, O_WRONLY | O_CLOEXEC);
 
 	return *fd < 0 ? -1 : 0;
 }
@@ -95,7 +94,7 @@ int tracefs_vprintf(struct tracefs_instance *instance, const char *fmt, va_list
 	ret = vasprintf(&str, fmt, ap);
 	if (ret < 0)
 		return ret;
-	ret = marker_write(instance, false, str, strlen(str) + 1);
+	ret = marker_write(instance, false, str, strlen(str));
 	free(str);
 
 	return ret;
-- 
2.29.2

[PATCH 2/3] libtracefs: Add helper function trace_get_lock()

[Steven Rostedt]

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

Since more APIs will need to get the instance pthread mutex or the
toplevel pthread mutex depending on if the lock needed to change an
instance or the top level state, create trace_get_lock() to not need to
reimplement the conditional for every instance it is used.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/tracefs-local.h | 5 +++++
 src/tracefs-tools.c     | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/tracefs-local.h b/include/tracefs-local.h
index ea441b5..1c06a40 100644
--- a/include/tracefs-local.h
+++ b/include/tracefs-local.h
@@ -27,6 +27,11 @@ struct tracefs_instance {
 
 extern pthread_mutex_t toplevel_lock;
 
+static inline pthread_mutex_t *trace_get_lock(struct tracefs_instance *instance)
+{
+	return instance ? &instance->lock : &toplevel_lock;
+}
+
 /* Can be overridden */
 void warning(const char *fmt, ...);
 
diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
index 037875f..6bab2b0 100644
--- a/src/tracefs-tools.c
+++ b/src/tracefs-tools.c
@@ -756,7 +756,7 @@ static int update_filter(const char *filter_path, int *fd,
 	bool reset = flags & TRACEFS_FL_RESET;
 	bool cont = flags & TRACEFS_FL_CONTINUE;
 	bool future = flags & TRACEFS_FL_FUTURE;
-	pthread_mutex_t *lock = instance ? &instance->lock : &toplevel_lock;
+	pthread_mutex_t *lock = trace_get_lock(instance);
 	int open_flags;
 	int ret = 1;
 
-- 
2.29.2

[PATCH 3/3] libtracefs: Add lock around modifying the trace_marker file descriptor

[Steven Rostedt]

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

Add a pthread mutex to protect the integrity of the file descriptor saved
for writing to the trace_marker and trace_marker_raw files. This lock is
only to protect the modification of the file descriptor and does not
protect against one thread closing the descriptor and another thread
writing to it.

It is only used to make sure that the file descriptor gets opened if it is
not already opened when doing a write, to protect opening the same file
more than once, and to protect against closing the same file descriptor
more than once.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 src/tracefs-marker.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/src/tracefs-marker.c b/src/tracefs-marker.c
index 924ceea..61a07ab 100644
--- a/src/tracefs-marker.c
+++ b/src/tracefs-marker.c
@@ -7,6 +7,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <stdio.h>
+#include <pthread.h>
 
 #include "tracefs.h"
 #include "tracefs-local.h"
@@ -25,24 +26,43 @@ static inline int *get_marker_fd(struct tracefs_instance *instance, bool raw)
 static int marker_init(struct tracefs_instance *instance, bool raw)
 {
 	const char *file = raw ? "trace_marker_raw" : "trace_marker";
+	pthread_mutex_t *lock = trace_get_lock(instance);
 	int *fd = get_marker_fd(instance, raw);
+	int ret;
 
 	if (*fd >= 0)
 		return 0;
 
-	*fd = tracefs_instance_file_open(instance, file, O_WRONLY | O_CLOEXEC);
+	/*
+	 * The mutex is only to hold the integrity of the file descriptor
+	 * to prevent opening it more than once, or closing the same
+	 * file descriptor more than once. It does not protect against
+	 * one thread closing the file descriptor and another thread
+	 * writing to it. That is up to the application to prevent
+	 * from happening.
+	 */
+	pthread_mutex_lock(lock);
+	/* The file could have been opened since we taken the lock */
+	if (*fd < 0)
+		*fd = tracefs_instance_file_open(instance, file, O_WRONLY | O_CLOEXEC);
+
+	ret = *fd < 0 ? -1 : 0;
+	pthread_mutex_unlock(lock);
 
-	return *fd < 0 ? -1 : 0;
+	return ret;
 }
 
 static void marker_close(struct tracefs_instance *instance, bool raw)
 {
+	pthread_mutex_t *lock = trace_get_lock(instance);
 	int *fd = get_marker_fd(instance, raw);
 
-	if (*fd < 0)
-		return;
-	close(*fd);
-	*fd = -1;
+	pthread_mutex_lock(lock);
+	if (*fd >= 0) {
+		close(*fd);
+		*fd = -1;
+	}
+	pthread_mutex_unlock(lock);
 }
 
 static int marker_write(struct tracefs_instance *instance, bool raw, void *data, int len)
@@ -50,6 +70,11 @@ static int marker_write(struct tracefs_instance *instance, bool raw, void *data,
 	int *fd = get_marker_fd(instance, raw);
 	int ret;
 
+	/*
+	 * The lock does not need to be taken for writes. As a write
+	 * does not modify the file descriptor. It's up to the application
+	 * to prevent it from being closed if another thread is doing a write.
+	 */
 	if (!data || len < 1)
 		return -1;
 	if (*fd < 0) {
-- 
2.29.2