[PATCH] Allow arbitrary tracefs mount points

[PATCH] Allow arbitrary tracefs mount points

[Nick Alcock]

So as of kernel 6.3 (upstream commit 2455f0e124d317dd08d337a75), the
canonical tracefs location has moved to /sys/kernel/tracing.  Unfortunately
this requires an explicit mount, and a great many installations have not
done this and perhaps never will.  So if the debugfs is mounted on
/sys/kernel/debug, it automatically makes /sys/kernel/debug/tracing appear
as it used to, as another tracefs mount. And of course there's nothing
special about the "canonical location": it's up to the admin, who might
choose to remount the thing anywhere at all.

To make this even more fun, it's quite possible to end up with the tracefs
on /sys/kernel/debug/tracing, but an empty directory at /sys/kernel/tracing
(I got that during testing with no effort at all).

All this means that the existing DTrace hardwiring for tracefs/eventsfs
locations isn't good enough. Instead, hunt for a suitable tracefs mount with
getmntent(), and add a function to return an arbitrary path under that
directory (mimicking the things we used to do with EVENTSFS defines and the
like).  Return a pointer to static (per-dtrace_hdl) storage so that we don't
need to clog up all the callers with dynamic allocation: only one path is
ever needed at any one time anyway.

Tested with both in-practice-observed locations of debugfs.

Bug: https://github.com/oracle/dtrace-utils/issues/111
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
 include/tracefs.h                             | 14 -----
 libdtrace/dt_error.c                          |  3 +-
 libdtrace/dt_impl.h                           |  4 ++
 libdtrace/dt_open.c                           |  2 +
 libdtrace/dt_prov_dtrace.c                    | 31 ++++++-----
 libdtrace/dt_prov_fbt.c                       | 40 ++++++++------
 libdtrace/dt_prov_rawtp.c                     | 10 ++--
 libdtrace/dt_prov_sdt.c                       | 23 +++++---
 libdtrace/dt_prov_syscall.c                   | 18 ++++---
 libdtrace/dt_prov_uprobe.c                    | 52 +++++++++++--------
 libdtrace/dt_provider.h                       |  1 -
 libdtrace/dt_subr.c                           | 52 +++++++++++++++++++
 test/unittest/funcs/tst.rw_.x                 |  7 ++-
 test/unittest/providers/tst.dtrace_cleanup.sh |  9 +++-
 test/utils/clean_probes.sh                    |  6 ++-
 15 files changed, 187 insertions(+), 85 deletions(-)
 delete mode 100644 include/tracefs.h

diff --git a/include/tracefs.h b/include/tracefs.h
deleted file mode 100644
index d671f51adefc..000000000000
--- a/include/tracefs.h
+++ /dev/null
@@ -1,14 +0,0 @@
-/*
- * Oracle Linux DTrace; simple uprobe helper functions
- * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
- * Licensed under the Universal Permissive License v 1.0 as shown at
- * http://oss.oracle.com/licenses/upl.
- */
-
-#ifndef	_TRACEFS_H
-#define	_TRACEFS_H
-
-#define TRACEFS		"/sys/kernel/debug/tracing/"
-#define EVENTSFS	TRACEFS "events/"
-
-#endif /* _TRACEFS_H */
diff --git a/libdtrace/dt_error.c b/libdtrace/dt_error.c
index 213f0d9e1385..f6b41bf72b9f 100644
--- a/libdtrace/dt_error.c
+++ b/libdtrace/dt_error.c
@@ -98,7 +98,8 @@ static const struct {
 	{ EDT_READMAXSTACK, "Cannot read kernel param perf_event_max_stack" },
 	{ EDT_TRACEMEM, "Missing or corrupt tracemem() record" },
 	{ EDT_PCAP, "Missing or corrupt pcap() record" },
-	{ EDT_PRINT, "Missing or corrupt print() record" }
+	{ EDT_PRINT, "Missing or corrupt print() record" },
+	{ EDT_TRACEFS_MNT, "Cannot find tracefs mount point" }
 };
 
 static const int _dt_nerr = sizeof(_dt_errlist) / sizeof(_dt_errlist[0]);
diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
index 68fb8ec53c06..9d246cef2e7f 100644
--- a/libdtrace/dt_impl.h
+++ b/libdtrace/dt_impl.h
@@ -354,6 +354,8 @@ struct dtrace_hdl {
 	char *dt_module_path;	/* pathname of kernel module root */
 	dt_version_t dt_kernver;/* kernel version, used in the libpath */
 	char *dt_dofstash_path;	/* Path to the DOF stash.  */
+	char *dt_tracefs_path;	/* Path to tracefs.  */
+	char *dt_tracefs_file;	/* See tracefs_file().  */
 	uid_t dt_useruid;	/* lowest non-system uid: set via -xuseruid */
 	char *dt_sysslice;	/* the systemd system slice: set via -xsysslice */
 	uint_t dt_lazyload;	/* boolean:  set via -xlazyload */
@@ -643,6 +645,7 @@ enum {
 	EDT_TRACEMEM,		/* missing or corrupt tracemem() record */
 	EDT_PCAP,		/* missing or corrupt pcap() record */
 	EDT_PRINT,		/* missing or corrupt print() record */
+	EDT_TRACEFS_MNT,	/* cannot find tracefs mount point */
 };
 
 /*
@@ -713,6 +716,7 @@ extern void dt_conf_init(dtrace_hdl_t *);
 
 extern int dt_gmatch(const char *, const char *);
 extern char *dt_basename(char *);
+extern const char *tracefs_file(dtrace_hdl_t *, const char *);
 
 extern ulong_t dt_popc(ulong_t);
 extern ulong_t dt_popcb(const ulong_t *, ulong_t);
diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
index e1972aa821e7..e74b11244a35 100644
--- a/libdtrace/dt_open.c
+++ b/libdtrace/dt_open.c
@@ -1317,6 +1317,8 @@ dtrace_close(dtrace_hdl_t *dtp)
 	free(dtp->dt_cpp_argv);
 	free(dtp->dt_cpp_path);
 	free(dtp->dt_ld_path);
+	free(dtp->dt_tracefs_path);
+	free(dtp->dt_tracefs_file);
 	free(dtp->dt_sysslice);
 	free(dtp->dt_dofstash_path);
 
diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
index 34b5d8e2467f..221a46bd50b8 100644
--- a/libdtrace/dt_prov_dtrace.c
+++ b/libdtrace/dt_prov_dtrace.c
@@ -23,8 +23,6 @@ static const char		funname[] = "";
 
 #define PROBE_FUNC_SUFFIX	"_probe"
 
-#define UPROBE_EVENTS		TRACEFS "uprobe_events"
-
 static const dtrace_pattr_t	pattr = {
 { DTRACE_STABILITY_STABLE, DTRACE_STABILITY_STABLE, DTRACE_CLASS_COMMON },
 { DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_UNKNOWN },
@@ -229,11 +227,12 @@ out:
 static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
 {
 	if (!dt_tp_probe_has_info(prp)) {
-		char	*spec;
-		char	*fn;
-		FILE	*f;
-		size_t	len;
-		int	fd, rc = -1;
+		char		*spec;
+		const char	*uprobe_events;
+		char		*fn;
+		FILE		*f;
+		size_t		len;
+		int		fd = -1, rc = -1;
 
 		/* get a uprobe specification for this probe */
 		spec = uprobe_spec(getpid(), prp->desc->prb);
@@ -241,7 +240,9 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
 			return -ENOENT;
 
 		/* add a uprobe */
-		fd = open(UPROBE_EVENTS, O_WRONLY | O_APPEND);
+		if ((uprobe_events = tracefs_file(dtp, "uprobe_events")) != NULL)
+			fd = open(uprobe_events, O_WRONLY | O_APPEND);
+
 		if (fd != -1) {
 			rc = dprintf(fd, "p:" GROUP_FMT "/%s %s\n",
 				     GROUP_DATA, prp->desc->prb, spec);
@@ -252,14 +253,18 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
 			return -ENOENT;
 
 		/* open format file */
+
+		if ((uprobe_events = tracefs_file(dtp, "events/")) == NULL)
+			return -ENOENT;
+
 		len = snprintf(NULL, 0, "%s" GROUP_FMT "/%s/format",
-			       EVENTSFS, GROUP_DATA, prp->desc->prb) + 1;
+			       uprobe_events, GROUP_DATA, prp->desc->prb) + 1;
 		fn = dt_alloc(dtp, len);
 		if (fn == NULL)
 			return -ENOENT;
 
 		snprintf(fn, len, "%s" GROUP_FMT "/%s/format",
-			 EVENTSFS, GROUP_DATA, prp->desc->prb);
+			 uprobe_events, GROUP_DATA, prp->desc->prb);
 		f = fopen(fn, "r");
 		dt_free(dtp, fn);
 		if (f == NULL)
@@ -289,14 +294,16 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
  */
 static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
 {
-	int		fd;
+	int		fd = -1;
+	const char	*uprobe_events;
 
 	if (!dt_tp_probe_has_info(prp))
 		return;
 
 	dt_tp_probe_detach(dtp, prp);
 
-	fd = open(UPROBE_EVENTS, O_WRONLY | O_APPEND);
+	if ((uprobe_events = tracefs_file(dtp, "uprobe_events")) != NULL)
+		fd = open(uprobe_events, O_WRONLY | O_APPEND);
 	if (fd == -1)
 		return;
 
diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
index 21f63ddffc73..7d81b0f3e87d 100644
--- a/libdtrace/dt_prov_fbt.c
+++ b/libdtrace/dt_prov_fbt.c
@@ -7,7 +7,7 @@
  * The Function Boundary Tracing (FBT) provider for DTrace.
  *
  * FBT probes are exposed by the kernel as kprobes.  They are listed in the
- * TRACEFS/available_filter_functions file.  Some kprobes are associated with
+ * tracefs available_filter_functions file.  Some kprobes are associated with
  * a specific kernel module, while most are in the core kernel.
  *
  * Mapping from event name to DTrace probe name:
@@ -43,8 +43,8 @@
 static const char		prvname[] = "fbt";
 static const char		modname[] = "vmlinux";
 
-#define KPROBE_EVENTS		TRACEFS "kprobe_events"
-#define PROBE_LIST		TRACEFS "available_filter_functions"
+#define KPROBE_EVENTS		"kprobe_events"
+#define PROBE_LIST		"available_filter_functions"
 
 #define FBT_GROUP_FMT		GROUP_FMT "_%s"
 #define FBT_GROUP_DATA		GROUP_DATA, prp->desc->prb
@@ -58,14 +58,15 @@ static const dtrace_pattr_t	pattr = {
 };
 
 /*
- * Scan the PROBE_LIST file and add entry and return probes for every function
+ * Scan the available_filter_functions file and add entry and return probes for every function
  * that is listed.
  */
 static int populate(dtrace_hdl_t *dtp)
 {
 	dt_provider_t		*prv;
 	dt_provimpl_t		*impl;
-	FILE			*f;
+	const char		*probe_list;
+	FILE			*f = NULL;
 	char			*buf = NULL;
 	char			*p;
 	const char		*mod = modname;
@@ -79,7 +80,8 @@ static int populate(dtrace_hdl_t *dtp)
 	if (prv == NULL)
 		return -1;			/* errno already set */
 
-	f = fopen(PROBE_LIST, "r");
+	if ((probe_list = tracefs_file(dtp, PROBE_LIST)) != NULL)
+		f = fopen(probe_list, "r");
 	if (f == NULL)
 		return 0;
 
@@ -363,16 +365,18 @@ static int kprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
 static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
 {
 	if (!dt_tp_probe_has_info(prp)) {
-		char	*fn;
-		FILE	*f;
-		size_t	len;
-		int	fd, rc = -1;
+		const char	*kprobe_events;
+		char		*fn;
+		FILE		*f;
+		size_t		len;
+		int		fd = -1, rc = -1;
 
 		/*
 		 * Register the kprobe with the tracing subsystem.  This will
 		 * create a tracepoint event.
 		 */
-		fd = open(KPROBE_EVENTS, O_WRONLY | O_APPEND);
+		if ((kprobe_events = tracefs_file(dtp, KPROBE_EVENTS)) != NULL)
+			fd = open(kprobe_events, O_WRONLY | O_APPEND);
 		if (fd == -1)
 			return -ENOENT;
 
@@ -384,13 +388,17 @@ static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
 			return -ENOENT;
 
 		/* create format file name */
+
+		if ((kprobe_events = tracefs_file(dtp, "events/")) == NULL)
+			return -ENOENT;
+
 		len = snprintf(NULL, 0, "%s" FBT_GROUP_FMT "/%s/format",
-			       EVENTSFS, FBT_GROUP_DATA, prp->desc->fun) + 1;
+			       kprobe_events, FBT_GROUP_DATA, prp->desc->fun) + 1;
 		fn = dt_alloc(dtp, len);
 		if (fn == NULL)
 			return -ENOENT;
 
-		snprintf(fn, len, "%s" FBT_GROUP_FMT "/%s/format", EVENTSFS,
+		snprintf(fn, len, "%s" FBT_GROUP_FMT "/%s/format", kprobe_events,
 			 FBT_GROUP_DATA, prp->desc->fun);
 
 		/* open format file */
@@ -424,14 +432,16 @@ static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
  */
 static void kprobe_detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
 {
-	int		fd;
+	const char	*kprobe_events;
+	int		fd = -1;
 
 	if (!dt_tp_probe_has_info(prp))
 		return;
 
 	dt_tp_probe_detach(dtp, prp);
 
-	fd = open(KPROBE_EVENTS, O_WRONLY | O_APPEND);
+	if ((kprobe_events = tracefs_file(dtp, KPROBE_EVENTS)) != NULL)
+		fd = open(kprobe_events, O_WRONLY | O_APPEND);
 	if (fd == -1)
 		return;
 
diff --git a/libdtrace/dt_prov_rawtp.c b/libdtrace/dt_prov_rawtp.c
index 778a6f9cde90..6e495afef092 100644
--- a/libdtrace/dt_prov_rawtp.c
+++ b/libdtrace/dt_prov_rawtp.c
@@ -8,7 +8,7 @@
  *
  * Raw tracepoints are exposed by the kernel tracing system to allow access to
  * untranslated arguments to their associated tracepoint events.  Each
- * tracepoint event listed in the TRACEFS/available_events file can be traced
+ * tracepoint event listed in the tracefs available_events file can be traced
  * as a raw tracepoint using the BPF program type BPF_PROG_TYPE_RAW_TRACEPOINT.
  *
  * Mapping from event name to DTrace probe name:
@@ -38,7 +38,7 @@
 static const char		prvname[] = "rawtp";
 static const char		modname[] = "vmlinux";
 
-#define PROBE_LIST		TRACEFS "available_events"
+#define PROBE_LIST		"available_events"
 
 #define KPROBES			"kprobes"
 #define SYSCALLS		"syscalls"
@@ -64,7 +64,8 @@ static const dtrace_pattr_t	pattr = {
 static int populate(dtrace_hdl_t *dtp)
 {
 	dt_provider_t	*prv;
-	FILE		*f;
+	const char	*probe_list;
+	FILE		*f = NULL;
 	char		*buf = NULL;
 	char		*p;
 	size_t		n;
@@ -73,7 +74,8 @@ static int populate(dtrace_hdl_t *dtp)
 	if (prv == NULL)
 		return -1;			/* errno already set */
 
-	f = fopen(PROBE_LIST, "r");
+	if ((probe_list = tracefs_file(dtp, PROBE_LIST)) != NULL)
+		f = fopen(probe_list, "r");
 	if (f == NULL)
 		return 0;
 
diff --git a/libdtrace/dt_prov_sdt.c b/libdtrace/dt_prov_sdt.c
index 675e0458ca4c..717fd62f0c3a 100644
--- a/libdtrace/dt_prov_sdt.c
+++ b/libdtrace/dt_prov_sdt.c
@@ -7,7 +7,7 @@
  * The Statically Defined Tracepoint (SDT) provider for DTrace.
  *
  * SDT probes are exposed by the kernel as tracepoint events.  They are listed
- * in the TRACEFS/available_events file.
+ * in the tracefs available_events file.
  *
  * Mapping from event name to DTrace probe name:
  *
@@ -36,7 +36,7 @@
 static const char		prvname[] = "sdt";
 static const char		modname[] = "vmlinux";
 
-#define PROBE_LIST		TRACEFS "available_events"
+#define PROBE_LIST		"available_events"
 
 #define KPROBES			"kprobes"
 #define SYSCALLS		"syscalls"
@@ -62,7 +62,8 @@ static const dtrace_pattr_t	pattr = {
 static int populate(dtrace_hdl_t *dtp)
 {
 	dt_provider_t	*prv;
-	FILE		*f;
+	const char	*probe_list;
+	FILE		*f = NULL;
 	char		*buf = NULL;
 	char		*p;
 	size_t		n;
@@ -71,7 +72,8 @@ static int populate(dtrace_hdl_t *dtp)
 	if (prv == NULL)
 		return -1;			/* errno already set */
 
-	f = fopen(PROBE_LIST, "r");
+	if ((probe_list = tracefs_file(dtp, PROBE_LIST)) != NULL)
+		f = fopen(probe_list, "r");
 	if (f == NULL)
 		return 0;
 
@@ -192,12 +194,16 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
 static int probe_info_tracefs(dtrace_hdl_t *dtp, const dt_probe_t *prp,
 			      int *argcp, dt_argdesc_t **argvp)
 {
+	const char			*eventsfs;
 	FILE				*f;
 	char				*fn;
 	int				rc;
 	const dtrace_probedesc_t	*pdp = prp->desc;
 
-	if (asprintf(&fn, EVENTSFS "%s/%s/format", pdp->mod, pdp->prb) == -1)
+	if ((eventsfs = tracefs_file(dtp, "events")) == NULL)
+		return -ENOENT;
+
+	if (asprintf(&fn, "%s/%s/%s/format", eventsfs, pdp->mod, pdp->prb) == -1)
 		return dt_set_errno(dtp, EDT_NOMEM);
 
 	f = fopen(fn, "r");
@@ -215,6 +221,7 @@ static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
 		      int *argcp, dt_argdesc_t **argvp)
 {
 #ifdef HAVE_LIBCTF
+	const char		*eventsfs;
 	int			rc, i;
 	char			*str;
 	ctf_dict_t		*ctfp;
@@ -227,7 +234,11 @@ static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
 	uint32_t		id;
 
 	/* Retrieve the event id. */
-	if (asprintf(&str, EVENTSFS "%s/%s/id", prp->desc->mod, prp->desc->prb) == -1)
+
+	if ((eventsfs = tracefs_file(dtp, "events")) == NULL)
+		return -1;			/* errno is set for us. */
+
+	if (asprintf(&str, "%s/%s/%s/id", eventsfs, prp->desc->mod, prp->desc->prb) == -1)
 		return dt_set_errno(dtp, EDT_NOMEM);
 
 	f = fopen(str, "r");
diff --git a/libdtrace/dt_prov_syscall.c b/libdtrace/dt_prov_syscall.c
index 20843c6f538e..869e8e130c42 100644
--- a/libdtrace/dt_prov_syscall.c
+++ b/libdtrace/dt_prov_syscall.c
@@ -38,7 +38,7 @@
 static const char		prvname[] = "syscall";
 static const char		modname[] = "vmlinux";
 
-#define SYSCALLSFS		EVENTSFS "syscalls/"
+#define SYSCALLSFS		"events/syscalls/"
 
 /*
  * We need to skip over an extra field: __syscall_nr.
@@ -61,7 +61,7 @@ struct syscall_data {
 
 #define SCD_ARG(n)	offsetof(struct syscall_data, arg[n])
 
-#define PROBE_LIST	TRACEFS "available_events"
+#define PROBE_LIST	"available_events"
 
 #define PROV_PREFIX	"syscalls:"
 #define ENTRY_PREFIX	"sys_enter_"
@@ -71,7 +71,8 @@ struct syscall_data {
 static int populate(dtrace_hdl_t *dtp)
 {
 	dt_provider_t	*prv;
-	FILE		*f;
+	const char	*probe_list;
+	FILE		*f = NULL;
 	char		*buf = NULL;
 	size_t		n;
 
@@ -79,7 +80,8 @@ static int populate(dtrace_hdl_t *dtp)
 	if (prv == NULL)
 		return -1;			/* errno already set */
 
-	f = fopen(PROBE_LIST, "r");
+	if ((probe_list = tracefs_file(dtp, PROBE_LIST)) != NULL)
+		f = fopen(probe_list, "r");
 	if (f == NULL)
 		return 0;
 
@@ -196,14 +198,18 @@ static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
 		      int *argcp, dt_argdesc_t **argvp)
 {
 	FILE		*f;
-	char		fn[256];
+	const char	*syscallsfs;
+	char		fn[PATH_MAX];
 	int		rc;
 
 	/*
 	 * We know that the probe name is either "entry" or "return", so we can
 	 * just check the first character.
 	 */
-	strcpy(fn, SYSCALLSFS);
+	if ((syscallsfs = tracefs_file(dtp, SYSCALLSFS)) == NULL)
+		return -ENOENT;
+
+	strcpy(fn, syscallsfs);
 	if (prp->desc->prb[0] == 'e')
 		strcat(fn, "sys_enter_");
 	else
diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
index 205014617586..f8752eaa3843 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -1116,13 +1116,14 @@ static char *uprobe_name(dev_t dev, ino_t ino, uint64_t addr, int flags)
  * uprobe may be a uretprobe.  Return the probe's name as
  * a new dynamically-allocated string, or NULL on error.
  */
-static char *uprobe_create(dev_t dev, ino_t ino, const char *mapping_fn,
-			   uint64_t addr, int flags)
+static char *uprobe_create(dtrace_hdl_t *dtp, dev_t dev, ino_t ino,
+			   const char *mapping_fn, uint64_t addr, int flags)
 {
-	int	fd = -1;
-	int	rc = -1;
-	char	*name;
-	char	*spec;
+	const char	*uprobe_events;
+	int		fd = -1;
+	int		rc = -1;
+	char		*name;
+	char		*spec;
 
 	if (asprintf(&spec, "%s:0x%lx", mapping_fn, addr) < 0)
 		return NULL;
@@ -1132,7 +1133,8 @@ static char *uprobe_create(dev_t dev, ino_t ino, const char *mapping_fn,
 		goto out;
 
 	/* Add the uprobe. */
-	fd = open(TRACEFS "uprobe_events", O_WRONLY | O_APPEND);
+	if ((uprobe_events = tracefs_file(dtp, "uprobe_events")) != NULL)
+		fd = open(uprobe_events, O_WRONLY | O_APPEND);
 	if (fd == -1)
 		goto out;
 
@@ -1153,8 +1155,8 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *uprp, int bpf_fd)
 {
 	dt_uprobe_t	*upp = uprp->prv_data;
 	tp_probe_t	*tpp = upp->tp;
-	FILE		*f;
-	char		*fn;
+	const char	*eventsfs;
+	FILE		*f = NULL;
 	char		*prb = NULL;
 	int		rc = -1;
 
@@ -1163,7 +1165,7 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *uprp, int bpf_fd)
 
 	assert(upp->fn != NULL);
 
-	prb = uprobe_create(upp->dev, upp->inum, upp->fn, upp->off,
+	prb = uprobe_create(dtp, upp->dev, upp->inum, upp->fn, upp->off,
 			    upp->flags);
 
 	/*
@@ -1177,12 +1179,16 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *uprp, int bpf_fd)
 				  upp->flags);
 
 	/* open format file */
-	rc = asprintf(&fn, "%s%s/format", EVENTSFS, prb);
-	free(prb);
-	if (rc < 0)
-		return -ENOENT;
-	f = fopen(fn, "r");
-	free(fn);
+	if ((eventsfs = tracefs_file(dtp, "events")) != NULL) {
+		char	*fn;
+
+		rc = asprintf(&fn, "%s/%s/format", eventsfs, prb);
+		free(prb);
+		if (rc < 0)
+			return -ENOENT;
+		f = fopen(fn, "r");
+		free(fn);
+	}
 	if (f == NULL)
 		return -ENOENT;
 
@@ -1251,17 +1257,19 @@ done:
  * Destroy a uprobe for a given device and address.
  */
 static int
-uprobe_delete(dev_t dev, ino_t ino, uint64_t addr, int flags)
+uprobe_delete(dtrace_hdl_t *dtp, dev_t dev, ino_t ino, uint64_t addr, int flags)
 {
-	int	fd = -1;
-	int	rc = -1;
-	char	*name;
+	const char	*uprobe_events;
+	int		fd = -1;
+	int		rc = -1;
+	char		*name;
 
 	name = uprobe_name(dev, ino, addr, flags);
 	if (!name)
 		goto out;
 
-	fd = open(TRACEFS "uprobe_events", O_WRONLY | O_APPEND);
+	if ((uprobe_events = tracefs_file(dtp, "uprobe_events")) != NULL)
+		fd = open(uprobe_events, O_WRONLY | O_APPEND);
 	if (fd == -1)
 		goto out;
 
@@ -1297,7 +1305,7 @@ static void detach(dtrace_hdl_t *dtp, const dt_probe_t *uprp)
 
 	dt_tp_detach(dtp, tpp);
 
-	uprobe_delete(upp->dev, upp->inum, upp->off, upp->flags);
+	uprobe_delete(dtp, upp->dev, upp->inum, upp->off, upp->flags);
 }
 
 /*
diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
index 8f143dceaed7..4598a380b950 100644
--- a/libdtrace/dt_provider.h
+++ b/libdtrace/dt_provider.h
@@ -12,7 +12,6 @@
 #include <dt_impl.h>
 #include <dt_ident.h>
 #include <dt_list.h>
-#include <tracefs.h>
 
 #ifdef	__cplusplus
 extern "C" {
diff --git a/libdtrace/dt_subr.c b/libdtrace/dt_subr.c
index d5dca164861e..6ec03b503a66 100644
--- a/libdtrace/dt_subr.c
+++ b/libdtrace/dt_subr.c
@@ -20,6 +20,7 @@
 #include <assert.h>
 #include <limits.h>
 #include <sys/ioctl.h>
+#include <mntent.h>
 #include <port.h>
 
 #include <dt_impl.h>
@@ -998,3 +999,54 @@ uint32_t dt_gen_hval(const char *p, uint32_t hval, size_t len)
 
 	return hval;
 }
+
+/*
+ * Find the tracefs and store it away in dtp.
+ */
+static int
+find_tracefs_path(dtrace_hdl_t *dtp)
+{
+	FILE *mounts;
+	struct mntent *mnt;
+
+	if ((mounts = setmntent("/proc/mounts", "r")) == NULL) {
+		dt_dprintf("Cannot open /proc/mounts: %s\n", strerror(errno));
+		return dt_set_errno(dtp, EDT_TRACEFS_MNT);
+	}
+
+	while ((mnt = getmntent(mounts)) != NULL) {
+		if (strcmp(mnt->mnt_type, "tracefs") == 0) {
+			dtp->dt_tracefs_path = strdup(mnt->mnt_dir);
+			break;
+		}
+	}
+	endmntent(mounts);
+
+	if (!dtp->dt_tracefs_path)
+		return dt_set_errno(dtp, EDT_TRACEFS_MNT);
+
+	dt_dprintf("Found tracefs at %s\n", dtp->dt_tracefs_path);
+
+	return 0;
+}
+
+/*
+ * Return the path to a tracefs file.  (Statically allocated,
+ * discarded on reuse.)
+ */
+const char *
+tracefs_file(dtrace_hdl_t *dtp, const char *fn)
+{
+	free(dtp->dt_tracefs_file);
+
+	if (!dtp->dt_tracefs_path)
+		if (find_tracefs_path(dtp) < 0)
+			return NULL;		/* errno is set for us. */
+
+	if (asprintf(&dtp->dt_tracefs_file, "%s/%s", dtp->dt_tracefs_path, fn) < 0) {
+		dtp->dt_tracefs_file = NULL;
+		dt_set_errno(dtp, EDT_NOMEM);
+		return NULL;
+	}
+	return dtp->dt_tracefs_file;
+}
diff --git a/test/unittest/funcs/tst.rw_.x b/test/unittest/funcs/tst.rw_.x
index 29c581116154..7e178778d900 100755
--- a/test/unittest/funcs/tst.rw_.x
+++ b/test/unittest/funcs/tst.rw_.x
@@ -1,6 +1,11 @@
 #!/bin/sh
 
-FUNCS=/sys/kernel/debug/tracing/available_filter_functions
+TRACEFS=/sys/kernel/tracing
+if [[ ! -e $TRACEFS/available_filter_functions ]]; then
+	TRACEFS=/sys/kernel/debug/tracing
+fi
+
+FUNCS=${TRACEFS}/available_filter_functions
 
 if ! grep -qw  _raw_read_lock $FUNCS; then
         echo no _raw_read_lock FBT probe due to kernel config
diff --git a/test/unittest/providers/tst.dtrace_cleanup.sh b/test/unittest/providers/tst.dtrace_cleanup.sh
index 4ac59ccb4315..08b47a057783 100755
--- a/test/unittest/providers/tst.dtrace_cleanup.sh
+++ b/test/unittest/providers/tst.dtrace_cleanup.sh
@@ -1,7 +1,7 @@
 #!/bin/bash
 #
 # Oracle Linux DTrace.
-# Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
+# Copyright (c) 2020, 2024, Oracle and/or its affiliates. All rights reserved.
 # Licensed under the Universal Permissive License v 1.0 as shown at
 # http://oss.oracle.com/licenses/upl.
 
@@ -14,7 +14,12 @@
 ##
 
 dtrace=$1
-UPROBE_EVENTS=/sys/kernel/debug/tracing/uprobe_events
+TRACEFS=/sys/kernel/tracing
+if [[ ! -e $TRACEFS/uprobe_events ]]; then
+	TRACEFS=/sys/kernel/debug/tracing
+fi
+
+UPROBE_EVENTS=${TRACEFS}/uprobe_events
 
 out=/tmp/output.$$
 $dtrace $dt_flags -n BEGIN,END &>> $out &
diff --git a/test/utils/clean_probes.sh b/test/utils/clean_probes.sh
index 8292b3096424..16e98b29b275 100755
--- a/test/utils/clean_probes.sh
+++ b/test/utils/clean_probes.sh
@@ -1,6 +1,10 @@
 #!/usr/bin/bash
 
-TRACEFS=/sys/kernel/debug/tracing
+TRACEFS=/sys/kernel/tracing
+if [[ ! -e $TRACEFS/available_events ]]; then
+	TRACEFS=/sys/kernel/debug/tracing
+fi
+
 EVENTS=${TRACEFS}/available_events
 KPROBES=${TRACEFS}/kprobe_events
 UPROBES=${TRACEFS}/uprobe_events
-- 
2.46.0.278.g36e3a12567

Re: [PATCH] Allow arbitrary tracefs mount points

[Eugene Loh]

The whole thing with dtp->dt_tracefs_file is...  interesting. We store a 
temporary string in dtp-> just (if I understand correctly) to simplify 
its free.  It's kind of a cool trick but also kind of ugly.  We'll see 
what Kris says.

In dt_prov_dtrace.c attach():

*)  We should convert "len=snprintf(NULL, 0, ...); snprintf(fn, len, 
...)" into asprintf() for the format file name.  I realize that in some 
sense that is "beyond the scope of this patch," but it seems like we've 
used a couple of patches recently an an opportunity to fix this sort of 
thing.

*)  Using the variable uprobe_events first for "uprobe_events" and later 
for "events" seems confusing.  At the very least, how about naming 
uprobe_events to be something more generic.

Those two comments also apply to dt_prov_fbt.c kprobe_attach().

And...

On 11/6/24 11:46, Nick Alcock wrote:
> So as of kernel 6.3 (upstream commit 2455f0e124d317dd08d337a75), the
> canonical tracefs location has moved to /sys/kernel/tracing.  Unfortunately
> this requires an explicit mount, and a great many installations have not
> done this and perhaps never will.  So if the debugfs is mounted on
> /sys/kernel/debug, it automatically makes /sys/kernel/debug/tracing appear
> as it used to, as another tracefs mount. And of course there's nothing
> special about the "canonical location": it's up to the admin, who might
> choose to remount the thing anywhere at all.

Anywhere at all?  But the scripts in test/ only look in two places.

Speaking of which, could runtest.sh look for TRACEFS and then make that 
available to any test script that needs it?

Re: [DTrace-devel] [PATCH] Allow arbitrary tracefs mount points

[Kris Van Hees]

On Wed, Nov 06, 2024 at 04:46:56PM +0000, Nick Alcock via DTrace-devel wrote:
> So as of kernel 6.3 (upstream commit 2455f0e124d317dd08d337a75), the
> canonical tracefs location has moved to /sys/kernel/tracing.  Unfortunately
> this requires an explicit mount, and a great many installations have not
> done this and perhaps never will.  So if the debugfs is mounted on
> /sys/kernel/debug, it automatically makes /sys/kernel/debug/tracing appear
> as it used to, as another tracefs mount. And of course there's nothing
> special about the "canonical location": it's up to the admin, who might
> choose to remount the thing anywhere at all.
> 
> To make this even more fun, it's quite possible to end up with the tracefs
> on /sys/kernel/debug/tracing, but an empty directory at /sys/kernel/tracing
> (I got that during testing with no effort at all).
> 
> All this means that the existing DTrace hardwiring for tracefs/eventsfs
> locations isn't good enough. Instead, hunt for a suitable tracefs mount with
> getmntent(), and add a function to return an arbitrary path under that
> directory (mimicking the things we used to do with EVENTSFS defines and the
> like).  Return a pointer to static (per-dtrace_hdl) storage so that we don't
> need to clog up all the callers with dynamic allocation: only one path is
> ever needed at any one time anyway.

More on this below...

> Tested with both in-practice-observed locations of debugfs.

Since you mention that tracefs can be mounted anywhere, there definitely should
be a test that exercises this functionality when tracefs is *not* mounted in
any of the typical locations.

And I bet you meant to write 'tracefs' here and not 'debugfs'?

> Bug: https://github.com/oracle/dtrace-utils/issues/111
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> ---
>  include/tracefs.h                             | 14 -----
>  libdtrace/dt_error.c                          |  3 +-
>  libdtrace/dt_impl.h                           |  4 ++
>  libdtrace/dt_open.c                           |  2 +
>  libdtrace/dt_prov_dtrace.c                    | 31 ++++++-----
>  libdtrace/dt_prov_fbt.c                       | 40 ++++++++------
>  libdtrace/dt_prov_rawtp.c                     | 10 ++--
>  libdtrace/dt_prov_sdt.c                       | 23 +++++---
>  libdtrace/dt_prov_syscall.c                   | 18 ++++---
>  libdtrace/dt_prov_uprobe.c                    | 52 +++++++++++--------
>  libdtrace/dt_provider.h                       |  1 -
>  libdtrace/dt_subr.c                           | 52 +++++++++++++++++++
>  test/unittest/funcs/tst.rw_.x                 |  7 ++-
>  test/unittest/providers/tst.dtrace_cleanup.sh |  9 +++-
>  test/utils/clean_probes.sh                    |  6 ++-
>  15 files changed, 187 insertions(+), 85 deletions(-)
>  delete mode 100644 include/tracefs.h
> 
> diff --git a/include/tracefs.h b/include/tracefs.h
> deleted file mode 100644
> index d671f51adefc..000000000000
> --- a/include/tracefs.h
> +++ /dev/null
> @@ -1,14 +0,0 @@
> -/*
> - * Oracle Linux DTrace; simple uprobe helper functions
> - * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> - * Licensed under the Universal Permissive License v 1.0 as shown at
> - * http://oss.oracle.com/licenses/upl.
> - */
> -
> -#ifndef	_TRACEFS_H
> -#define	_TRACEFS_H
> -
> -#define TRACEFS		"/sys/kernel/debug/tracing/"
> -#define EVENTSFS	TRACEFS "events/"
> -
> -#endif /* _TRACEFS_H */
> diff --git a/libdtrace/dt_error.c b/libdtrace/dt_error.c
> index 213f0d9e1385..f6b41bf72b9f 100644
> --- a/libdtrace/dt_error.c
> +++ b/libdtrace/dt_error.c
> @@ -98,7 +98,8 @@ static const struct {
>  	{ EDT_READMAXSTACK, "Cannot read kernel param perf_event_max_stack" },
>  	{ EDT_TRACEMEM, "Missing or corrupt tracemem() record" },
>  	{ EDT_PCAP, "Missing or corrupt pcap() record" },
> -	{ EDT_PRINT, "Missing or corrupt print() record" }
> +	{ EDT_PRINT, "Missing or corrupt print() record" },
> +	{ EDT_TRACEFS_MNT, "Cannot find tracefs mount point" }

I think you could make the identifier a little shorter (EDT_TARCEFS would do),
and maybe the message can be "Cannot find tracefs"?  Especially since the code
that throws this error goes through all the mountpoints, so the issue is not
that the mount point cannot be found but rather that tracefs is not mounted.

>  };
>  
>  static const int _dt_nerr = sizeof(_dt_errlist) / sizeof(_dt_errlist[0]);
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 68fb8ec53c06..9d246cef2e7f 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -354,6 +354,8 @@ struct dtrace_hdl {
>  	char *dt_module_path;	/* pathname of kernel module root */
>  	dt_version_t dt_kernver;/* kernel version, used in the libpath */
>  	char *dt_dofstash_path;	/* Path to the DOF stash.  */
> +	char *dt_tracefs_path;	/* Path to tracefs.  */
> +	char *dt_tracefs_file;	/* See tracefs_file().  */

I don't think you need this _file member (see below).

>  	uid_t dt_useruid;	/* lowest non-system uid: set via -xuseruid */
>  	char *dt_sysslice;	/* the systemd system slice: set via -xsysslice */
>  	uint_t dt_lazyload;	/* boolean:  set via -xlazyload */
> @@ -643,6 +645,7 @@ enum {
>  	EDT_TRACEMEM,		/* missing or corrupt tracemem() record */
>  	EDT_PCAP,		/* missing or corrupt pcap() record */
>  	EDT_PRINT,		/* missing or corrupt print() record */
> +	EDT_TRACEFS_MNT,	/* cannot find tracefs mount point */

EDT_TRACEFS, cannot find tracefs

>  };
>  
>  /*
> @@ -713,6 +716,7 @@ extern void dt_conf_init(dtrace_hdl_t *);
>  
>  extern int dt_gmatch(const char *, const char *);
>  extern char *dt_basename(char *);
> +extern const char *tracefs_file(dtrace_hdl_t *, const char *);

As mentioned below, I think it would make sense to change this to be:

	extern char *dt_tracefs_fn(dtrace_hdl_t *, const char *, ...);

so it can operate as a *printf() style function.

As mentioned below, I think adding a function here would improve things:

	extern int dt_tracefs_open(dtrace_hdl_t *, const char *, mode_t);

This will open the given file within tracefs and return the fd. 

>  extern ulong_t dt_popc(ulong_t);
>  extern ulong_t dt_popcb(const ulong_t *, ulong_t);
> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index e1972aa821e7..e74b11244a35 100644
> --- a/libdtrace/dt_open.c
> +++ b/libdtrace/dt_open.c
> @@ -1317,6 +1317,8 @@ dtrace_close(dtrace_hdl_t *dtp)
>  	free(dtp->dt_cpp_argv);
>  	free(dtp->dt_cpp_path);
>  	free(dtp->dt_ld_path);
> +	free(dtp->dt_tracefs_path);
> +	free(dtp->dt_tracefs_file);

Don't need _file.

>  	free(dtp->dt_sysslice);
>  	free(dtp->dt_dofstash_path);
>  
> diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
> index 34b5d8e2467f..221a46bd50b8 100644
> --- a/libdtrace/dt_prov_dtrace.c
> +++ b/libdtrace/dt_prov_dtrace.c
> @@ -23,8 +23,6 @@ static const char		funname[] = "";
>  
>  #define PROBE_FUNC_SUFFIX	"_probe"
>  
> -#define UPROBE_EVENTS		TRACEFS "uprobe_events"
> -
>  static const dtrace_pattr_t	pattr = {
>  { DTRACE_STABILITY_STABLE, DTRACE_STABILITY_STABLE, DTRACE_CLASS_COMMON },
>  { DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_UNKNOWN },
> @@ -229,11 +227,12 @@ out:
>  static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>  {
>  	if (!dt_tp_probe_has_info(prp)) {
> -		char	*spec;
> -		char	*fn;
> -		FILE	*f;
> -		size_t	len;
> -		int	fd, rc = -1;
> +		char		*spec;
> +		const char	*uprobe_events;
> +		char		*fn;
> +		FILE		*f;
> +		size_t		len;
> +		int		fd = -1, rc = -1;
>  
>  		/* get a uprobe specification for this probe */
>  		spec = uprobe_spec(getpid(), prp->desc->prb);
> @@ -241,7 +240,9 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>  			return -ENOENT;
>  
>  		/* add a uprobe */
> -		fd = open(UPROBE_EVENTS, O_WRONLY | O_APPEND);
> +		if ((uprobe_events = tracefs_file(dtp, "uprobe_events")) != NULL)
> +			fd = open(uprobe_events, O_WRONLY | O_APPEND);

		fd = dt_tracefs_open(dtp, "uprobe_events", O_WRONLY | O_APPEND);

> +
>  		if (fd != -1) {
>  			rc = dprintf(fd, "p:" GROUP_FMT "/%s %s\n",
>  				     GROUP_DATA, prp->desc->prb, spec);
> @@ -252,14 +253,18 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>  			return -ENOENT;
>  
>  		/* open format file */

Replace from here...

> +
> +		if ((uprobe_events = tracefs_file(dtp, "events/")) == NULL)
> +			return -ENOENT;
> +
>  		len = snprintf(NULL, 0, "%s" GROUP_FMT "/%s/format",
> -			       EVENTSFS, GROUP_DATA, prp->desc->prb) + 1;
> +			       uprobe_events, GROUP_DATA, prp->desc->prb) + 1;
>  		fn = dt_alloc(dtp, len);
>  		if (fn == NULL)
>  			return -ENOENT;
>  
>  		snprintf(fn, len, "%s" GROUP_FMT "/%s/format",
> -			 EVENTSFS, GROUP_DATA, prp->desc->prb);
> +			 uprobe_events, GROUP_DATA, prp->desc->prb);

... to here with:

		fn = dt_tracefs_fn(dtp, "events/" GROUP_FMT "/%s/format",
				   GROUP_DATA, prp->desc->prb);
		if (fn == NULL)
			return -ENOENT;

And apply the same changes to the other providers.

> 
>  		f = fopen(fn, "r");
>  		dt_free(dtp, fn);
>  		if (f == NULL)
> @@ -289,14 +294,16 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>   */
>  static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
>  {
> -	int		fd;
> +	int		fd = -1;
> +	const char	*uprobe_events;
>  
>  	if (!dt_tp_probe_has_info(prp))
>  		return;
>  
>  	dt_tp_probe_detach(dtp, prp);
>  
> -	fd = open(UPROBE_EVENTS, O_WRONLY | O_APPEND);
> +	if ((uprobe_events = tracefs_file(dtp, "uprobe_events")) != NULL)
> +		fd = open(uprobe_events, O_WRONLY | O_APPEND);
>  	if (fd == -1)
>  		return;
>  
> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> index 21f63ddffc73..7d81b0f3e87d 100644
> --- a/libdtrace/dt_prov_fbt.c
> +++ b/libdtrace/dt_prov_fbt.c
> @@ -7,7 +7,7 @@
>   * The Function Boundary Tracing (FBT) provider for DTrace.
>   *
>   * FBT probes are exposed by the kernel as kprobes.  They are listed in the
> - * TRACEFS/available_filter_functions file.  Some kprobes are associated with
> + * tracefs available_filter_functions file.  Some kprobes are associated with

I would keep these comments as is because TRACEFS representing a static define
or being a symbol to represent that this component is not pre-defined is quite
equivalent.  CHanging it to "tracefs *" doesn't really make a difference.

Same in the other providers.

>   * a specific kernel module, while most are in the core kernel.
>   *
>   * Mapping from event name to DTrace probe name:
> @@ -43,8 +43,8 @@
>  static const char		prvname[] = "fbt";
>  static const char		modname[] = "vmlinux";
>  
> -#define KPROBE_EVENTS		TRACEFS "kprobe_events"
> -#define PROBE_LIST		TRACEFS "available_filter_functions"
> +#define KPROBE_EVENTS		"kprobe_events"
> +#define PROBE_LIST		"available_filter_functions"
>  
>  #define FBT_GROUP_FMT		GROUP_FMT "_%s"
>  #define FBT_GROUP_DATA		GROUP_DATA, prp->desc->prb
> @@ -58,14 +58,15 @@ static const dtrace_pattr_t	pattr = {
>  };
>  
>  /*
> - * Scan the PROBE_LIST file and add entry and return probes for every function
> + * Scan the available_filter_functions file and add entry and return probes for every function
>   * that is listed.
>   */
>  static int populate(dtrace_hdl_t *dtp)
>  {
>  	dt_provider_t		*prv;
>  	dt_provimpl_t		*impl;
> -	FILE			*f;
> +	const char		*probe_list;
> +	FILE			*f = NULL;
>  	char			*buf = NULL;
>  	char			*p;
>  	const char		*mod = modname;
> @@ -79,7 +80,8 @@ static int populate(dtrace_hdl_t *dtp)
>  	if (prv == NULL)
>  		return -1;			/* errno already set */
>  
> -	f = fopen(PROBE_LIST, "r");
> +	if ((probe_list = tracefs_file(dtp, PROBE_LIST)) != NULL)
> +		f = fopen(probe_list, "r");

You can use dt_tracefs_open() and fdopen() to get a stream.

>  	if (f == NULL)
>  		return 0;
>  
> @@ -363,16 +365,18 @@ static int kprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>  static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>  {
>  	if (!dt_tp_probe_has_info(prp)) {
> -		char	*fn;
> -		FILE	*f;
> -		size_t	len;
> -		int	fd, rc = -1;
> +		const char	*kprobe_events;
> +		char		*fn;
> +		FILE		*f;
> +		size_t		len;
> +		int		fd = -1, rc = -1;
>  
>  		/*
>  		 * Register the kprobe with the tracing subsystem.  This will
>  		 * create a tracepoint event.
>  		 */
> -		fd = open(KPROBE_EVENTS, O_WRONLY | O_APPEND);
> +		if ((kprobe_events = tracefs_file(dtp, KPROBE_EVENTS)) != NULL)
> +			fd = open(kprobe_events, O_WRONLY | O_APPEND);
>  		if (fd == -1)
>  			return -ENOENT;
>  
> @@ -384,13 +388,17 @@ static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>  			return -ENOENT;
>  
>  		/* create format file name */
> +
> +		if ((kprobe_events = tracefs_file(dtp, "events/")) == NULL)
> +			return -ENOENT;
> +
>  		len = snprintf(NULL, 0, "%s" FBT_GROUP_FMT "/%s/format",
> -			       EVENTSFS, FBT_GROUP_DATA, prp->desc->fun) + 1;
> +			       kprobe_events, FBT_GROUP_DATA, prp->desc->fun) + 1;
>  		fn = dt_alloc(dtp, len);
>  		if (fn == NULL)
>  			return -ENOENT;
>  
> -		snprintf(fn, len, "%s" FBT_GROUP_FMT "/%s/format", EVENTSFS,
> +		snprintf(fn, len, "%s" FBT_GROUP_FMT "/%s/format", kprobe_events,
>  			 FBT_GROUP_DATA, prp->desc->fun);
>  
>  		/* open format file */
> @@ -424,14 +432,16 @@ static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>   */
>  static void kprobe_detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
>  {
> -	int		fd;
> +	const char	*kprobe_events;
> +	int		fd = -1;
>  
>  	if (!dt_tp_probe_has_info(prp))
>  		return;
>  
>  	dt_tp_probe_detach(dtp, prp);
>  
> -	fd = open(KPROBE_EVENTS, O_WRONLY | O_APPEND);
> +	if ((kprobe_events = tracefs_file(dtp, KPROBE_EVENTS)) != NULL)
> +		fd = open(kprobe_events, O_WRONLY | O_APPEND);
>  	if (fd == -1)
>  		return;
>  
> diff --git a/libdtrace/dt_prov_rawtp.c b/libdtrace/dt_prov_rawtp.c
> index 778a6f9cde90..6e495afef092 100644
> --- a/libdtrace/dt_prov_rawtp.c
> +++ b/libdtrace/dt_prov_rawtp.c
> @@ -8,7 +8,7 @@
>   *
>   * Raw tracepoints are exposed by the kernel tracing system to allow access to
>   * untranslated arguments to their associated tracepoint events.  Each
> - * tracepoint event listed in the TRACEFS/available_events file can be traced
> + * tracepoint event listed in the tracefs available_events file can be traced
>   * as a raw tracepoint using the BPF program type BPF_PROG_TYPE_RAW_TRACEPOINT.
>   *
>   * Mapping from event name to DTrace probe name:
> @@ -38,7 +38,7 @@
>  static const char		prvname[] = "rawtp";
>  static const char		modname[] = "vmlinux";
>  
> -#define PROBE_LIST		TRACEFS "available_events"
> +#define PROBE_LIST		"available_events"
>  
>  #define KPROBES			"kprobes"
>  #define SYSCALLS		"syscalls"
> @@ -64,7 +64,8 @@ static const dtrace_pattr_t	pattr = {
>  static int populate(dtrace_hdl_t *dtp)
>  {
>  	dt_provider_t	*prv;
> -	FILE		*f;
> +	const char	*probe_list;
> +	FILE		*f = NULL;
>  	char		*buf = NULL;
>  	char		*p;
>  	size_t		n;
> @@ -73,7 +74,8 @@ static int populate(dtrace_hdl_t *dtp)
>  	if (prv == NULL)
>  		return -1;			/* errno already set */
>  
> -	f = fopen(PROBE_LIST, "r");
> +	if ((probe_list = tracefs_file(dtp, PROBE_LIST)) != NULL)
> +		f = fopen(probe_list, "r");
>  	if (f == NULL)
>  		return 0;
>  
> diff --git a/libdtrace/dt_prov_sdt.c b/libdtrace/dt_prov_sdt.c
> index 675e0458ca4c..717fd62f0c3a 100644
> --- a/libdtrace/dt_prov_sdt.c
> +++ b/libdtrace/dt_prov_sdt.c
> @@ -7,7 +7,7 @@
>   * The Statically Defined Tracepoint (SDT) provider for DTrace.
>   *
>   * SDT probes are exposed by the kernel as tracepoint events.  They are listed
> - * in the TRACEFS/available_events file.
> + * in the tracefs available_events file.
>   *
>   * Mapping from event name to DTrace probe name:
>   *
> @@ -36,7 +36,7 @@
>  static const char		prvname[] = "sdt";
>  static const char		modname[] = "vmlinux";
>  
> -#define PROBE_LIST		TRACEFS "available_events"
> +#define PROBE_LIST		"available_events"
>  
>  #define KPROBES			"kprobes"
>  #define SYSCALLS		"syscalls"
> @@ -62,7 +62,8 @@ static const dtrace_pattr_t	pattr = {
>  static int populate(dtrace_hdl_t *dtp)
>  {
>  	dt_provider_t	*prv;
> -	FILE		*f;
> +	const char	*probe_list;
> +	FILE		*f = NULL;
>  	char		*buf = NULL;
>  	char		*p;
>  	size_t		n;
> @@ -71,7 +72,8 @@ static int populate(dtrace_hdl_t *dtp)
>  	if (prv == NULL)
>  		return -1;			/* errno already set */
>  
> -	f = fopen(PROBE_LIST, "r");
> +	if ((probe_list = tracefs_file(dtp, PROBE_LIST)) != NULL)
> +		f = fopen(probe_list, "r");
>  	if (f == NULL)
>  		return 0;
>  
> @@ -192,12 +194,16 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>  static int probe_info_tracefs(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>  			      int *argcp, dt_argdesc_t **argvp)
>  {
> +	const char			*eventsfs;
>  	FILE				*f;
>  	char				*fn;
>  	int				rc;
>  	const dtrace_probedesc_t	*pdp = prp->desc;
>  
> -	if (asprintf(&fn, EVENTSFS "%s/%s/format", pdp->mod, pdp->prb) == -1)
> +	if ((eventsfs = tracefs_file(dtp, "events")) == NULL)
> +		return -ENOENT;
> +
> +	if (asprintf(&fn, "%s/%s/%s/format", eventsfs, pdp->mod, pdp->prb) == -1)
>  		return dt_set_errno(dtp, EDT_NOMEM);
>  
>  	f = fopen(fn, "r");
> @@ -215,6 +221,7 @@ static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>  		      int *argcp, dt_argdesc_t **argvp)
>  {
>  #ifdef HAVE_LIBCTF
> +	const char		*eventsfs;
>  	int			rc, i;
>  	char			*str;
>  	ctf_dict_t		*ctfp;
> @@ -227,7 +234,11 @@ static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>  	uint32_t		id;
>  
>  	/* Retrieve the event id. */
> -	if (asprintf(&str, EVENTSFS "%s/%s/id", prp->desc->mod, prp->desc->prb) == -1)
> +
> +	if ((eventsfs = tracefs_file(dtp, "events")) == NULL)
> +		return -1;			/* errno is set for us. */
> +
> +	if (asprintf(&str, "%s/%s/%s/id", eventsfs, prp->desc->mod, prp->desc->prb) == -1)
>  		return dt_set_errno(dtp, EDT_NOMEM);
>  
>  	f = fopen(str, "r");
> diff --git a/libdtrace/dt_prov_syscall.c b/libdtrace/dt_prov_syscall.c
> index 20843c6f538e..869e8e130c42 100644
> --- a/libdtrace/dt_prov_syscall.c
> +++ b/libdtrace/dt_prov_syscall.c
> @@ -38,7 +38,7 @@
>  static const char		prvname[] = "syscall";
>  static const char		modname[] = "vmlinux";
>  
> -#define SYSCALLSFS		EVENTSFS "syscalls/"
> +#define SYSCALLSFS		"events/syscalls/"
>  
>  /*
>   * We need to skip over an extra field: __syscall_nr.
> @@ -61,7 +61,7 @@ struct syscall_data {
>  
>  #define SCD_ARG(n)	offsetof(struct syscall_data, arg[n])
>  
> -#define PROBE_LIST	TRACEFS "available_events"
> +#define PROBE_LIST	"available_events"
>  
>  #define PROV_PREFIX	"syscalls:"
>  #define ENTRY_PREFIX	"sys_enter_"
> @@ -71,7 +71,8 @@ struct syscall_data {
>  static int populate(dtrace_hdl_t *dtp)
>  {
>  	dt_provider_t	*prv;
> -	FILE		*f;
> +	const char	*probe_list;
> +	FILE		*f = NULL;
>  	char		*buf = NULL;
>  	size_t		n;
>  
> @@ -79,7 +80,8 @@ static int populate(dtrace_hdl_t *dtp)
>  	if (prv == NULL)
>  		return -1;			/* errno already set */
>  
> -	f = fopen(PROBE_LIST, "r");
> +	if ((probe_list = tracefs_file(dtp, PROBE_LIST)) != NULL)
> +		f = fopen(probe_list, "r");
>  	if (f == NULL)
>  		return 0;
>  
> @@ -196,14 +198,18 @@ static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>  		      int *argcp, dt_argdesc_t **argvp)
>  {
>  	FILE		*f;
> -	char		fn[256];
> +	const char	*syscallsfs;
> +	char		fn[PATH_MAX];
>  	int		rc;
>  
>  	/*
>  	 * We know that the probe name is either "entry" or "return", so we can
>  	 * just check the first character.
>  	 */
> -	strcpy(fn, SYSCALLSFS);
> +	if ((syscallsfs = tracefs_file(dtp, SYSCALLSFS)) == NULL)
> +		return -ENOENT;
> +
> +	strcpy(fn, syscallsfs);
>  	if (prp->desc->prb[0] == 'e')
>  		strcat(fn, "sys_enter_");

This and the following code in that function could benefit from simply using
dt_tracefs_fn() and doing a free on the returned string when done.

>  	else
> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index 205014617586..f8752eaa3843 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -1116,13 +1116,14 @@ static char *uprobe_name(dev_t dev, ino_t ino, uint64_t addr, int flags)
>   * uprobe may be a uretprobe.  Return the probe's name as
>   * a new dynamically-allocated string, or NULL on error.
>   */
> -static char *uprobe_create(dev_t dev, ino_t ino, const char *mapping_fn,
> -			   uint64_t addr, int flags)
> +static char *uprobe_create(dtrace_hdl_t *dtp, dev_t dev, ino_t ino,
> +			   const char *mapping_fn, uint64_t addr, int flags)
>  {
> -	int	fd = -1;
> -	int	rc = -1;
> -	char	*name;
> -	char	*spec;
> +	const char	*uprobe_events;
> +	int		fd = -1;
> +	int		rc = -1;
> +	char		*name;
> +	char		*spec;
>  
>  	if (asprintf(&spec, "%s:0x%lx", mapping_fn, addr) < 0)
>  		return NULL;
> @@ -1132,7 +1133,8 @@ static char *uprobe_create(dev_t dev, ino_t ino, const char *mapping_fn,
>  		goto out;
>  
>  	/* Add the uprobe. */
> -	fd = open(TRACEFS "uprobe_events", O_WRONLY | O_APPEND);
> +	if ((uprobe_events = tracefs_file(dtp, "uprobe_events")) != NULL)
> +		fd = open(uprobe_events, O_WRONLY | O_APPEND);
>  	if (fd == -1)
>  		goto out;
>  
> @@ -1153,8 +1155,8 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *uprp, int bpf_fd)
>  {
>  	dt_uprobe_t	*upp = uprp->prv_data;
>  	tp_probe_t	*tpp = upp->tp;
> -	FILE		*f;
> -	char		*fn;
> +	const char	*eventsfs;
> +	FILE		*f = NULL;
>  	char		*prb = NULL;
>  	int		rc = -1;
>  
> @@ -1163,7 +1165,7 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *uprp, int bpf_fd)
>  
>  	assert(upp->fn != NULL);
>  
> -	prb = uprobe_create(upp->dev, upp->inum, upp->fn, upp->off,
> +	prb = uprobe_create(dtp, upp->dev, upp->inum, upp->fn, upp->off,
>  			    upp->flags);
>  
>  	/*
> @@ -1177,12 +1179,16 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *uprp, int bpf_fd)
>  				  upp->flags);
>  
>  	/* open format file */
> -	rc = asprintf(&fn, "%s%s/format", EVENTSFS, prb);
> -	free(prb);
> -	if (rc < 0)
> -		return -ENOENT;
> -	f = fopen(fn, "r");
> -	free(fn);
> +	if ((eventsfs = tracefs_file(dtp, "events")) != NULL) {
> +		char	*fn;
> +
> +		rc = asprintf(&fn, "%s/%s/format", eventsfs, prb);
> +		free(prb);
> +		if (rc < 0)
> +			return -ENOENT;
> +		f = fopen(fn, "r");
> +		free(fn);
> +	}
>  	if (f == NULL)
>  		return -ENOENT;
>  
> @@ -1251,17 +1257,19 @@ done:
>   * Destroy a uprobe for a given device and address.
>   */
>  static int
> -uprobe_delete(dev_t dev, ino_t ino, uint64_t addr, int flags)
> +uprobe_delete(dtrace_hdl_t *dtp, dev_t dev, ino_t ino, uint64_t addr, int flags)
>  {
> -	int	fd = -1;
> -	int	rc = -1;
> -	char	*name;
> +	const char	*uprobe_events;
> +	int		fd = -1;
> +	int		rc = -1;
> +	char		*name;
>  
>  	name = uprobe_name(dev, ino, addr, flags);
>  	if (!name)
>  		goto out;
>  
> -	fd = open(TRACEFS "uprobe_events", O_WRONLY | O_APPEND);
> +	if ((uprobe_events = tracefs_file(dtp, "uprobe_events")) != NULL)
> +		fd = open(uprobe_events, O_WRONLY | O_APPEND);
>  	if (fd == -1)
>  		goto out;
>  
> @@ -1297,7 +1305,7 @@ static void detach(dtrace_hdl_t *dtp, const dt_probe_t *uprp)
>  
>  	dt_tp_detach(dtp, tpp);
>  
> -	uprobe_delete(upp->dev, upp->inum, upp->off, upp->flags);
> +	uprobe_delete(dtp, upp->dev, upp->inum, upp->off, upp->flags);
>  }
>  
>  /*
> diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
> index 8f143dceaed7..4598a380b950 100644
> --- a/libdtrace/dt_provider.h
> +++ b/libdtrace/dt_provider.h
> @@ -12,7 +12,6 @@
>  #include <dt_impl.h>
>  #include <dt_ident.h>
>  #include <dt_list.h>
> -#include <tracefs.h>
>  
>  #ifdef	__cplusplus
>  extern "C" {
> diff --git a/libdtrace/dt_subr.c b/libdtrace/dt_subr.c
> index d5dca164861e..6ec03b503a66 100644
> --- a/libdtrace/dt_subr.c
> +++ b/libdtrace/dt_subr.c
> @@ -20,6 +20,7 @@
>  #include <assert.h>
>  #include <limits.h>
>  #include <sys/ioctl.h>
> +#include <mntent.h>
>  #include <port.h>
>  
>  #include <dt_impl.h>
> @@ -998,3 +999,54 @@ uint32_t dt_gen_hval(const char *p, uint32_t hval, size_t len)
>  
>  	return hval;
>  }
> +
> +/*
> + * Find the tracefs and store it away in dtp.
> + */
> +static int
> +find_tracefs_path(dtrace_hdl_t *dtp)
> +{
> +	FILE *mounts;
> +	struct mntent *mnt;
> +
> +	if ((mounts = setmntent("/proc/mounts", "r")) == NULL) {
> +		dt_dprintf("Cannot open /proc/mounts: %s\n", strerror(errno));
> +		return dt_set_errno(dtp, EDT_TRACEFS_MNT);
> +	}
> +
> +	while ((mnt = getmntent(mounts)) != NULL) {
> +		if (strcmp(mnt->mnt_type, "tracefs") == 0) {
> +			dtp->dt_tracefs_path = strdup(mnt->mnt_dir);
> +			break;
> +		}
> +	}
> +	endmntent(mounts);
> +
> +	if (!dtp->dt_tracefs_path)
> +		return dt_set_errno(dtp, EDT_TRACEFS_MNT);
> +
> +	dt_dprintf("Found tracefs at %s\n", dtp->dt_tracefs_path);
> +
> +	return 0;
> +}
> +
> +/*
> + * Return the path to a tracefs file.  (Statically allocated,
> + * discarded on reuse.)
> + */
> +const char *
> +tracefs_file(dtrace_hdl_t *dtp, const char *fn)

char *
dt_tracefs_fn(dtrace_hdl_t *dtp, const char *fmt, ...)

> +{
> +	free(dtp->dt_tracefs_file);
> +
> +	if (!dtp->dt_tracefs_path)
> +		if (find_tracefs_path(dtp) < 0)
> +			return NULL;		/* errno is set for us. */
> +
> +	if (asprintf(&dtp->dt_tracefs_file, "%s/%s", dtp->dt_tracefs_path, fn) < 0) {
> +		dtp->dt_tracefs_file = NULL;
> +		dt_set_errno(dtp, EDT_NOMEM);
> +		return NULL;
> +	}
> +	return dtp->dt_tracefs_file;
> +}

Make this a function that takes a format string and optional arguments, and
then uses vasprintf() to create the resulting string.  Callers can free the
string (hardly an issue).

Add a function dt_tracefs_open(dtrace_hdl_t *dtp, const char *fn, mode_t mode)
to open the given file under tracefs, returning the fd.  Returns -1 (after
settinng errno) if the open fails for some reason.

> diff --git a/test/unittest/funcs/tst.rw_.x b/test/unittest/funcs/tst.rw_.x
> index 29c581116154..7e178778d900 100755
> --- a/test/unittest/funcs/tst.rw_.x
> +++ b/test/unittest/funcs/tst.rw_.x
> @@ -1,6 +1,11 @@
>  #!/bin/sh
>  
> -FUNCS=/sys/kernel/debug/tracing/available_filter_functions
> +TRACEFS=/sys/kernel/tracing
> +if [[ ! -e $TRACEFS/available_filter_functions ]]; then
> +	TRACEFS=/sys/kernel/debug/tracing
> +fi
> +
> +FUNCS=${TRACEFS}/available_filter_functions

How about having runtest.sh determine the location (not just checking two
possible places - look at the mount table), and export it as TRACEFS?  That
way tests (incl. future tests) can depend on that.

>  
>  if ! grep -qw  _raw_read_lock $FUNCS; then
>          echo no _raw_read_lock FBT probe due to kernel config
> diff --git a/test/unittest/providers/tst.dtrace_cleanup.sh b/test/unittest/providers/tst.dtrace_cleanup.sh
> index 4ac59ccb4315..08b47a057783 100755
> --- a/test/unittest/providers/tst.dtrace_cleanup.sh
> +++ b/test/unittest/providers/tst.dtrace_cleanup.sh
> @@ -1,7 +1,7 @@
>  #!/bin/bash
>  #
>  # Oracle Linux DTrace.
> -# Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
> +# Copyright (c) 2020, 2024, Oracle and/or its affiliates. All rights reserved.
>  # Licensed under the Universal Permissive License v 1.0 as shown at
>  # http://oss.oracle.com/licenses/upl.
>  
> @@ -14,7 +14,12 @@
>  ##
>  
>  dtrace=$1
> -UPROBE_EVENTS=/sys/kernel/debug/tracing/uprobe_events
> +TRACEFS=/sys/kernel/tracing
> +if [[ ! -e $TRACEFS/uprobe_events ]]; then
> +	TRACEFS=/sys/kernel/debug/tracing
> +fi
> +
> +UPROBE_EVENTS=${TRACEFS}/uprobe_events

Same as above... if runtest.sh does the work and exports it, this can use it.

>  
>  out=/tmp/output.$$
>  $dtrace $dt_flags -n BEGIN,END &>> $out &
> diff --git a/test/utils/clean_probes.sh b/test/utils/clean_probes.sh
> index 8292b3096424..16e98b29b275 100755
> --- a/test/utils/clean_probes.sh
> +++ b/test/utils/clean_probes.sh
> @@ -1,6 +1,10 @@
>  #!/usr/bin/bash
>  
> -TRACEFS=/sys/kernel/debug/tracing
> +TRACEFS=/sys/kernel/tracing
> +if [[ ! -e $TRACEFS/available_events ]]; then
> +	TRACEFS=/sys/kernel/debug/tracing
> +fi

Same.

> +
>  EVENTS=${TRACEFS}/available_events
>  KPROBES=${TRACEFS}/kprobe_events
>  UPROBES=${TRACEFS}/uprobe_events
> -- 
> 2.46.0.278.g36e3a12567
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel

Re: [PATCH] Allow arbitrary tracefs mount points

[Nick Alcock]

On 7 Nov 2024, Eugene Loh stated:

> The whole thing with dtp->dt_tracefs_file is...  interesting. We store a temporary string in dtp-> just (if I understand correctly)
> to simplify its free.  It's kind of a cool trick but also kind of ugly.  We'll see what Kris says.

Yep! To massively reduce the amount of noise this patch produces. (This
is what torpedoed every previous attempt of mine to do this.)

I stole the idea from libctf, which does something similar (and did even
in the Solaris days) in ctf_lookup_by_name()...

It works until you think you can call tracefs_file() twice in the same
function call, but honestly if that happens the flames will be immediate
and you'll spot it in no time (and so will the valgrind runs we
sometimes do).

> In dt_prov_dtrace.c attach():
>
> *)  We should convert "len=snprintf(NULL, 0, ...); snprintf(fn, len, ...)" into asprintf() for the format file name.  I realize that
>  in some sense that is "beyond the scope of this patch," but it seems like we've used a couple of patches recently an an opportunity
>  to fix this sort of thing.

Yeah, that niggled at me but I left it explicitly because I thought
people might think it was a bridge too far. Will change.

There was one other place with a [256] static array which I flipped to a
PATH_MAX in lieu of doing something more dynamic.

> *)  Using the variable uprobe_events first for "uprobe_events" and later for "events" seems confusing.  At the very least, how about
>  naming uprobe_events to be something more generic.

Agreed. Renamed to "tmp_path" which at least makes it clear that all
that's really consistent about this variable is that it's a path and
it's temporary.

> On 11/6/24 11:46, Nick Alcock wrote:
>> So as of kernel 6.3 (upstream commit 2455f0e124d317dd08d337a75), the
>> canonical tracefs location has moved to /sys/kernel/tracing.  Unfortunately
>> this requires an explicit mount, and a great many installations have not
>> done this and perhaps never will.  So if the debugfs is mounted on
>> /sys/kernel/debug, it automatically makes /sys/kernel/debug/tracing appear
>> as it used to, as another tracefs mount. And of course there's nothing
>> special about the "canonical location": it's up to the admin, who might
>> choose to remount the thing anywhere at all.
>
> Anywhere at all?  But the scripts in test/ only look in two places.

Yeah, I didn't fix the tests the same way.

> Speaking of which, could runtest.sh look for TRACEFS and then make that available to any test script that needs it?

... in /proc/mounts? I suppose so! I'll have a go.

-- 
NULL && (void)

Re: [DTrace-devel] [PATCH] Allow arbitrary tracefs mount points

[Nick Alcock]

On 7 Nov 2024, Kris Van Hees uttered the following:

> On Wed, Nov 06, 2024 at 04:46:56PM +0000, Nick Alcock via DTrace-devel wrote:
>> Tested with both in-practice-observed locations of debugfs.
>
> Since you mention that tracefs can be mounted anywhere, there definitely should
> be a test that exercises this functionality when tracefs is *not* mounted in
> any of the typical locations.

That's hard to do without disrupting later tests, really. You can't
mount --move tracefs and if you unmount it remounting it in the same way
it was mounted is... interesting.

I'm locally testing with unusual mount points, and it's easy to tweak a
VM so it always has a mount in the "other usual place".

> And I bet you meant to write 'tracefs' here and not 'debugfs'?

Ugh yes!

>> -	{ EDT_PRINT, "Missing or corrupt print() record" }
>> +	{ EDT_PRINT, "Missing or corrupt print() record" },
>> +	{ EDT_TRACEFS_MNT, "Cannot find tracefs mount point" }
>
> I think you could make the identifier a little shorter (EDT_TARCEFS would do),
> and maybe the message can be "Cannot find tracefs"?  Especially since the code
> that throws this error goes through all the mountpoints, so the issue is not
> that the mount point cannot be found but rather that tracefs is not mounted.

It's that we can't find the mount point. It might well still be mounted
in ways we can't find (e.g. in some other namespace, in a disconnected
subtree etc). Unlikely, I suppose...

The MNT was to indicate that this is specifically about the user needing
to *mount* tracefs, rather than some other thing, but I guess I can
shorten it. I can't think of what that other thing might be anyway.

>>  };
>>  
>>  static const int _dt_nerr = sizeof(_dt_errlist) / sizeof(_dt_errlist[0]);
>> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
>> index 68fb8ec53c06..9d246cef2e7f 100644
>> --- a/libdtrace/dt_impl.h
>> +++ b/libdtrace/dt_impl.h
>> @@ -354,6 +354,8 @@ struct dtrace_hdl {
>>  	char *dt_module_path;	/* pathname of kernel module root */
>>  	dt_version_t dt_kernver;/* kernel version, used in the libpath */
>>  	char *dt_dofstash_path;	/* Path to the DOF stash.  */
>> +	char *dt_tracefs_path;	/* Path to tracefs.  */
>> +	char *dt_tracefs_file;	/* See tracefs_file().  */
>
> I don't think you need this _file member (see below).

Yeah, if you want to have to free() it over and over again. I avoided
doing that because I thought it was unbearably noisy.

But the functions you suggest eliminate any need to do that!

>>  	uid_t dt_useruid;	/* lowest non-system uid: set via -xuseruid */
>>  	char *dt_sysslice;	/* the systemd system slice: set via -xsysslice */
>>  	uint_t dt_lazyload;	/* boolean:  set via -xlazyload */
>> @@ -643,6 +645,7 @@ enum {
>>  	EDT_TRACEMEM,		/* missing or corrupt tracemem() record */
>>  	EDT_PCAP,		/* missing or corrupt pcap() record */
>>  	EDT_PRINT,		/* missing or corrupt print() record */
>> +	EDT_TRACEFS_MNT,	/* cannot find tracefs mount point */
>
> EDT_TRACEFS, cannot find tracefs

ACK.

>>  };
>>  
>>  /*
>> @@ -713,6 +716,7 @@ extern void dt_conf_init(dtrace_hdl_t *);
>>  
>>  extern int dt_gmatch(const char *, const char *);
>>  extern char *dt_basename(char *);
>> +extern const char *tracefs_file(dtrace_hdl_t *, const char *);
>
> As mentioned below, I think it would make sense to change this to be:
>
> 	extern char *dt_tracefs_fn(dtrace_hdl_t *, const char *, ...);
>
> so it can operate as a *printf() style function.

Oh that's very nice! Why didn't I think of that etc.

This is why I love reviews :)

> As mentioned below, I think adding a function here would improve things:
>
> 	extern int dt_tracefs_open(dtrace_hdl_t *, const char *, mode_t);
>
> This will open the given file within tracefs and return the fd. 

Naah... go the whole hog:

extern int dt_tracefs_open(dtrace_hdl_t *, const char *, int flags, ...);

i.e. make it a printf-style function too.  With that, we don't even need
dt_tracefs_fn(), since literally nothing calls it.

(you do need the flags, and the mode is best defaulted I think, given
that no existing callers actually need it and it would uglify every call
and there is a reasonable default (0666). I agree that we must
unconditionally *pass* it in dt_tracefs_open() though, since we don't
know there if the callers passed flags that need it or not.)

>>  		snprintf(fn, len, "%s" GROUP_FMT "/%s/format",
>> -			 EVENTSFS, GROUP_DATA, prp->desc->prb);
>> +			 uprobe_events, GROUP_DATA, prp->desc->prb);
>
> ... to here with:
>
> 		fn = dt_tracefs_fn(dtp, "events/" GROUP_FMT "/%s/format",
> 				   GROUP_DATA, prp->desc->prb);
> 		if (fn == NULL)
> 			return -ENOENT;
>
> And apply the same changes to the other providers.

Ack! except this one can actually use dt_tracefd_open since it's used
to open a file... the same is true of all of them :) goodbye,
dt_tracefs_fn! Oh this is such a nice change, thank you!

>> --- a/libdtrace/dt_prov_fbt.c
>> +++ b/libdtrace/dt_prov_fbt.c
>> @@ -7,7 +7,7 @@
>>   * The Function Boundary Tracing (FBT) provider for DTrace.
>>   *
>>   * FBT probes are exposed by the kernel as kprobes.  They are listed in the
>> - * TRACEFS/available_filter_functions file.  Some kprobes are associated with
>> + * tracefs available_filter_functions file.  Some kprobes are associated with
>
> I would keep these comments as is because TRACEFS representing a static define
> or being a symbol to represent that this component is not pre-defined is quite
> equivalent.  CHanging it to "tracefs *" doesn't really make a difference.

I thought it was a reference to the #define in particular, which is gone.

Dropped.

>>  	FILE		*f;
>> -	char		fn[256];
>> +	const char	*syscallsfs;
>> +	char		fn[PATH_MAX];
>>  	int		rc;
>>  
>>  	/*
>>  	 * We know that the probe name is either "entry" or "return", so we can
>>  	 * just check the first character.
>>  	 */
>> -	strcpy(fn, SYSCALLSFS);
>> +	if ((syscallsfs = tracefs_file(dtp, SYSCALLSFS)) == NULL)
>> +		return -ENOENT;
>> +
>> +	strcpy(fn, syscallsfs);
>>  	if (prp->desc->prb[0] == 'e')
>>  		strcat(fn, "sys_enter_");
>
> This and the following code in that function could benefit from simply using
> dt_tracefs_fn() and doing a free on the returned string when done.

Absolutely! Actually we don't even need that, it's just doing an
open with it, we can just use dt_tracefs_open().

The result is wildly simpler and easier to read, despite using a ternary
conditional:

	fd = dt_tracefs_open(dtp, SYSCALLSFS "/sys_%s_%s/format", O_RDONLY,
			     (prp->desc->prb[0] == 'e') ? "enter" : "exit",
			     prp->desc->fun);

(caveat: only compile-tested so far, I'm sending this while make check
runs).

(I'm wondering if the general pattern of "open using this format string,
call dt_tp_probe_info() on it" should be encapsulated itself. A future
improvement perhaps.)

>> +{
>> +	free(dtp->dt_tracefs_file);
>> +
>> +	if (!dtp->dt_tracefs_path)
>> +		if (find_tracefs_path(dtp) < 0)
>> +			return NULL;		/* errno is set for us. */
>> +
>> +	if (asprintf(&dtp->dt_tracefs_file, "%s/%s", dtp->dt_tracefs_path, fn) < 0) {
>> +		dtp->dt_tracefs_file = NULL;
>> +		dt_set_errno(dtp, EDT_NOMEM);
>> +		return NULL;
>> +	}
>> +	return dtp->dt_tracefs_file;
>> +}
>
> Make this a function that takes a format string and optional arguments, and
> then uses vasprintf() to create the resulting string.  Callers can free the
> string (hardly an issue).
> 
> Add a function dt_tracefs_open(dtrace_hdl_t *dtp, const char *fn, mode_t mode)
> to open the given file under tracefs, returning the fd.  Returns -1 (after
> settinng errno) if the open fails for some reason.

I ruled this out because I didn't think it would be useful. This is
clearly ridiculous, it gets used multiple times and as you show it is
ever so much better than what I have now. Adjusted (with tiny change
noted above, they're both varargs now).

Another tiny improvement: we only accept tracefs paths that do not
contain percent characters, since they never should and if they do we
can't just slam them on the front of a format string without
modification like this.

>> diff --git a/test/unittest/funcs/tst.rw_.x b/test/unittest/funcs/tst.rw_.x
>> index 29c581116154..7e178778d900 100755
>> --- a/test/unittest/funcs/tst.rw_.x
>> +++ b/test/unittest/funcs/tst.rw_.x
>> @@ -1,6 +1,11 @@
>>  #!/bin/sh
>>  
>> -FUNCS=/sys/kernel/debug/tracing/available_filter_functions
>> +TRACEFS=/sys/kernel/tracing
>> +if [[ ! -e $TRACEFS/available_filter_functions ]]; then
>> +	TRACEFS=/sys/kernel/debug/tracing
>> +fi
>> +
>> +FUNCS=${TRACEFS}/available_filter_functions
>
> How about having runtest.sh determine the location (not just checking two
> possible places - look at the mount table), and export it as TRACEFS?  That
> way tests (incl. future tests) can depend on that.

Done! (exported as $tracefs because stuff runtest.sh exports is always
lowercase currently.)

It turns out to be literally a one-liner, i.e. shorter than the code it
replaces that does a worse job in the individual tests!

-- 
NULL && (void)