* [PATCH 1/6] error: add missing EDT_PRINT entry
2024-10-18 19:58 [PATCH 0/6] usdt typed args, translators and arg remapping Nick Alcock
@ 2024-10-18 19:58 ` Nick Alcock
2024-10-21 9:10 ` Alan Maguire
2024-10-22 14:40 ` Kris Van Hees
2024-10-18 19:58 ` [PATCH 2/6] usdt: get arg types and xlations into DTrace from the DOF Nick Alcock
` (4 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: Nick Alcock @ 2024-10-18 19:58 UTC (permalink / raw)
To: dtrace, dtrace-devel
The lack of this made it impossible to print out the last error
in the list (which is EDT_PRINT itself).
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
libdtrace/dt_error.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libdtrace/dt_error.c b/libdtrace/dt_error.c
index 82e114a5b292..213f0d9e1385 100644
--- a/libdtrace/dt_error.c
+++ b/libdtrace/dt_error.c
@@ -97,7 +97,8 @@ static const struct {
{ EDT_OBJIO, "Cannot read object file or modules.dep" },
{ 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_PCAP, "Missing or corrupt pcap() record" },
+ { EDT_PRINT, "Missing or corrupt print() record" }
};
static const int _dt_nerr = sizeof(_dt_errlist) / sizeof(_dt_errlist[0]);
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 1/6] error: add missing EDT_PRINT entry
2024-10-18 19:58 ` [PATCH 1/6] error: add missing EDT_PRINT entry Nick Alcock
@ 2024-10-21 9:10 ` Alan Maguire
2024-10-22 14:40 ` Kris Van Hees
1 sibling, 0 replies; 23+ messages in thread
From: Alan Maguire @ 2024-10-21 9:10 UTC (permalink / raw)
To: Nick Alcock, dtrace, dtrace-devel
On 18/10/2024 20:58, Nick Alcock wrote:
> The lack of this made it impossible to print out the last error
> in the list (which is EDT_PRINT itself).
>
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
Thanks for the fix!
> ---
> libdtrace/dt_error.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libdtrace/dt_error.c b/libdtrace/dt_error.c
> index 82e114a5b292..213f0d9e1385 100644
> --- a/libdtrace/dt_error.c
> +++ b/libdtrace/dt_error.c
> @@ -97,7 +97,8 @@ static const struct {
> { EDT_OBJIO, "Cannot read object file or modules.dep" },
> { 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_PCAP, "Missing or corrupt pcap() record" },
> + { EDT_PRINT, "Missing or corrupt print() record" }
> };
>
> static const int _dt_nerr = sizeof(_dt_errlist) / sizeof(_dt_errlist[0]);
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/6] error: add missing EDT_PRINT entry
2024-10-18 19:58 ` [PATCH 1/6] error: add missing EDT_PRINT entry Nick Alcock
2024-10-21 9:10 ` Alan Maguire
@ 2024-10-22 14:40 ` Kris Van Hees
1 sibling, 0 replies; 23+ messages in thread
From: Kris Van Hees @ 2024-10-22 14:40 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
On Fri, Oct 18, 2024 at 08:58:03PM +0100, Nick Alcock wrote:
> The lack of this made it impossible to print out the last error
> in the list (which is EDT_PRINT itself).
>
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
> libdtrace/dt_error.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libdtrace/dt_error.c b/libdtrace/dt_error.c
> index 82e114a5b292..213f0d9e1385 100644
> --- a/libdtrace/dt_error.c
> +++ b/libdtrace/dt_error.c
> @@ -97,7 +97,8 @@ static const struct {
> { EDT_OBJIO, "Cannot read object file or modules.dep" },
> { 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_PCAP, "Missing or corrupt pcap() record" },
> + { EDT_PRINT, "Missing or corrupt print() record" }
> };
>
> static const int _dt_nerr = sizeof(_dt_errlist) / sizeof(_dt_errlist[0]);
> --
> 2.46.0.278.g36e3a12567
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/6] usdt: get arg types and xlations into DTrace from the DOF
2024-10-18 19:58 [PATCH 0/6] usdt typed args, translators and arg remapping Nick Alcock
2024-10-18 19:58 ` [PATCH 1/6] error: add missing EDT_PRINT entry Nick Alcock
@ 2024-10-18 19:58 ` Nick Alcock
2024-10-22 17:03 ` Kris Van Hees
2024-10-18 19:58 ` [PATCH 3/6] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c Nick Alcock
` (3 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Nick Alcock @ 2024-10-18 19:58 UTC (permalink / raw)
To: dtrace, dtrace-devel
This change propagates native and xlated arg types and remapping info all
the way from the DOF, through the parser and dtprobed and the DOF stash,
into DTrace, where they end up in the pid_probespec_t for USDT probes.
We don't do anything with them once they get there in this commit: no
user-visible impacts expected.
We bump the DOF_PARSED_VERSION since we add three new types of record to the
dof_parser_t, all optional, covering native and xlated args and arg
remapping tables; to make life easier for consumers we emit them in a
defined order (documennted in dof_parser.h), and add a flags word to the
DIT_PROBE record that precedes them indicating which will be present. This
means we retain the property that you can always tell which records within a
provider are coming next purely from records you already have: there's no
need to make decisions once the records turn up. The DOF stash hardly
changes: all that happens is that the parsed data written to each per-probe
file gains some extra types of record (it can have DIT_NATIVE_ARGS,
DIT_XLAT_ARGS and DIT_REMAP_ARGS entries following the DIT_PROBE record
now).
As usual with DOF_PARSED_VERSION bumps, DTraces that cross this change will
refuse to read probes added by dtprobeds that are on the other side of it.
(Restarting dtprobed will reparse all the probes in the stash to match the
new layout, and newly-started DTraces will pick that up -- so this only
affects running DTraces picking up new probes, which DTrace cannot do yet:
so no visible effects are expected.)
The code is a bit repetitive, with nargs, xargs and remapping all handled
separately even though the code is very similar. In future maybe we could
unify them (but my attempts led to code that was much harder to read than
the original).
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
dtprobed/dof_stash.c | 14 +++-
dtprobed/dtprobed.c | 12 ++-
include/dtrace/pid.h | 9 +++
libcommon/dof_parser.c | 167 ++++++++++++++++++++++++++++++++---------
libcommon/dof_parser.h | 80 ++++++++++++++++++--
libdtrace/dt_pid.c | 71 ++++++++++++++++++
6 files changed, 302 insertions(+), 51 deletions(-)
diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
index 0e639076f655..d9731844066c 100644
--- a/dtprobed/dof_stash.c
+++ b/dtprobed/dof_stash.c
@@ -59,8 +59,9 @@
* startup if the dof_parsed_t structure changed. The file consists of a
* uint64_t version number (DOF_PARSED_VERSION), then a stream of
* dof_parsed_t records, the first of type DIT_PROVIDER, the second
- * DIT_PROBE, then as many struct dof_parsed_t's of type DIT_TRACEPOINT as
- * the DIT_PROBE record specifies.
+ * DIT_PROBE, then optionally some DIT_*ARGS records if the probe's flags
+ * word calls for them, then as many struct dof_parsed_t's of type
+ * DIT_TRACEPOINT as the DIT_PROBE record specifies.
*
* /run/dtrace/probes/: Per-probe info, written by dtprobed, read by DTrace.
*
@@ -640,9 +641,14 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
break;
}
- /* Tracepoint: add to already-open file. */
+ /* Args info or tracepoint: add to already-open file. */
+ case DIT_NATIVE_ARGS:
+ case DIT_XLAT_ARGS:
+ case DIT_REMAP_ARGS:
case DIT_TRACEPOINT:
- assert(state == DIT_PROBE || state == DIT_TRACEPOINT);
+ assert(state == DIT_PROBE || state == DIT_NATIVE_ARGS ||
+ state == DIT_XLAT_ARGS || state == DIT_REMAP_ARGS ||
+ state == DIT_TRACEPOINT);
state = accump->parsed->type;
if (write_chunk(parsed_fd, accump->parsed, accump->parsed->size) < 0)
diff --git a/dtprobed/dtprobed.c b/dtprobed/dtprobed.c
index fdcdee14f851..ecf27216ff80 100644
--- a/dtprobed/dtprobed.c
+++ b/dtprobed/dtprobed.c
@@ -795,16 +795,20 @@ process_dof(pid_t pid, int out, int in, dev_t dev, ino_t inum, dev_t exec_dev,
if (dof_stash_push_parsed(&accum, probe) < 0)
goto oom;
- for (j = 0; j < probe->probe.ntp; j++) {
+ j = 0;
+ do {
dof_parsed_t *tp = dof_read(pid, in);
- errmsg = "no tracepoints in a probe, or parse state corrupt";
- if (!tp || tp->type != DIT_TRACEPOINT)
+ errmsg = "no tracepoints in a probe, or parse state corrupt";
+ if (!tp || tp->type == DIT_PROVIDER || tp->type == DIT_PROBE)
goto err;
if (dof_stash_push_parsed(&accum, tp) < 0)
goto oom;
- }
+
+ if (tp->type == DIT_TRACEPOINT)
+ j++;
+ } while (j < probe->probe.ntp);
}
if (!reparsing)
diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
index 0129674adf0c..d9a5a40484a5 100644
--- a/include/dtrace/pid.h
+++ b/include/dtrace/pid.h
@@ -35,6 +35,15 @@ typedef struct pid_probespec {
ino_t pps_inum; /* object inode */
char *pps_fn; /* object full filename */
uint64_t pps_off; /* probe offset (in object) */
+ int pps_nargc; /* number of native args */
+ int pps_xargc; /* number of xlated args */
+ int pps_remapargc; /* number of remapped args */
+ char *pps_nargv; /* array of native args */
+ size_t pps_nargvlen; /* (high estimate of) length of array */
+ char *pps_xargv; /* array of xlated args */
+ size_t pps_xargvlen; /* (high estimate of) length of array */
+ int8_t *pps_remapargs; /* remapped arg indexes */
+ size_t pps_remapargslen; /* (high estimate of) length of array */
/*
* Fields below this point do not apply to underlying probes.
diff --git a/libcommon/dof_parser.c b/libcommon/dof_parser.c
index d6631a1afdcb..4a45ac41a64d 100644
--- a/libcommon/dof_parser.c
+++ b/libcommon/dof_parser.c
@@ -879,13 +879,31 @@ validate_probe(int out, dtrace_helper_probedesc_t *dhpb)
return 0;
}
+static size_t
+strings_len(const char *strtab, size_t count)
+{
+ size_t len = 0;
+
+ for (; count > 0; count--) {
+ size_t this_len = strlen(strtab) + 1;
+
+ len += this_len;
+ strtab += this_len;
+ }
+ return len;
+}
+
static void
emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
{
int i;
- dof_parsed_t *probe_msg;
- size_t probe_msg_size;
+ dof_parsed_t *msg;
+ size_t msg_size;
+ size_t nargs_size;
+ size_t xargs_size;
+ size_t remap_size = 0;
char *ptr;
+ int remap_args = 0;
dt_dbg_dof(" Creating probe %s:%s:%s:%s\n", dhpb->dthpb_prov,
dhpb->dthpb_mod, dhpb->dthpb_func, dhpb->dthpb_name);
@@ -893,37 +911,124 @@ emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
if (validate_probe(out, dhpb) != 0)
return;
- probe_msg_size = offsetof(dof_parsed_t, probe.name) +
- strlen(dhpb->dthpb_mod) + 1 + strlen(dhpb->dthpb_func) + 1 +
- strlen(dhpb->dthpb_name) + 1;
-
- probe_msg = malloc(probe_msg_size);
- if (!probe_msg) {
- dof_error(out, ENOMEM, "Out of memory allocating probe");
- return;
+ /*
+ * Figure out if the arguments are shuffled around (in which case we
+ * need to emit the argument remapping table).
+ */
+ for (i = 0; i < dhpb->dthpb_xargc; i++) {
+ if (dhpb->dthpb_args[i] != i) {
+ remap_args = 1;
+ break;
+ }
}
- memset(probe_msg, 0, probe_msg_size);
+ /*
+ * Compute the size of all the optional elements first, to fill out the
+ * flags.
+ */
- probe_msg->size = probe_msg_size;
- probe_msg->type = DIT_PROBE;
- probe_msg->probe.ntp = dhpb->dthpb_noffs + dhpb->dthpb_nenoffs;
- ptr = stpcpy(probe_msg->probe.name, dhpb->dthpb_mod);
+ msg_size = offsetof(dof_parsed_t, probe.name) +
+ strlen(dhpb->dthpb_mod) + 1 +
+ strlen(dhpb->dthpb_func) + 1 +
+ strlen(dhpb->dthpb_name) + 1;
+
+ nargs_size = strings_len(dhpb->dthpb_ntypes, dhpb->dthpb_nargc);
+ xargs_size = strings_len(dhpb->dthpb_xtypes, dhpb->dthpb_xargc);
+ if (remap_args)
+ remap_size = dhpb->dthpb_xargc * sizeof(uint8_t);
+
+ msg = malloc(msg_size);
+ if (!msg)
+ goto oom;
+
+ memset(msg, 0, msg_size);
+
+ msg->size = msg_size;
+ msg->type = DIT_PROBE;
+ msg->probe.ntp = dhpb->dthpb_noffs + dhpb->dthpb_nenoffs;
+
+ if (nargs_size)
+ msg->probe.flags |= DIT_HAS_NATIVE_ARGS;
+ if (xargs_size)
+ msg->probe.flags |= DIT_HAS_XLAT_ARGS;
+ if (remap_size)
+ msg->probe.flags |= DIT_HAS_REMAP_ARGS;
+
+ ptr = stpcpy(msg->probe.name, dhpb->dthpb_mod);
ptr++;
ptr = stpcpy(ptr, dhpb->dthpb_func);
ptr++;
strcpy(ptr, dhpb->dthpb_name);
- dof_parser_write_one(out, probe_msg, probe_msg_size);
+ dof_parser_write_one(out, msg, msg_size);
- free(probe_msg);
-
- /* XXX TODO translated args
- pp->ftp_nargs = dhpb->dthpb_xargc;
- pp->ftp_xtypes = dhpb->dthpb_xtypes;
- pp->ftp_ntypes = dhpb->dthpb_ntypes;
- */
+ free(msg);
/*
+ * Emit info on all native and translated args in turn.
+ *
+ * FIXME: this code is a bit repetitious.
+ *
+ * First native args (if any).
+ */
+
+ if (nargs_size > 0) {
+ msg_size = offsetof(dof_parsed_t, nargs.args) + nargs_size;
+
+ msg = malloc(msg_size);
+ if (!msg)
+ goto oom;
+
+ memset(msg, 0, msg_size);
+
+ msg->size = msg_size;
+ msg->type = DIT_NATIVE_ARGS;
+ msg->nargs.nargc = dhpb->dthpb_nargc;
+ memcpy(msg->nargs.args, dhpb->dthpb_ntypes, nargs_size);
+ dof_parser_write_one(out, msg, msg_size);
+
+ free(msg);
+ }
+
+ /* Then translated args. */
+
+ if (xargs_size > 0) {
+ msg_size = offsetof(dof_parsed_t, xargs.args) + xargs_size;
+
+ msg = malloc(msg_size);
+ if (!msg)
+ goto oom;
+
+ memset(msg, 0, msg_size);
+
+ msg->size = msg_size;
+ msg->type = DIT_XLAT_ARGS;
+ msg->xargs.nargc = dhpb->dthpb_xargc;
+ memcpy(msg->xargs.args, dhpb->dthpb_xtypes, xargs_size);
+ dof_parser_write_one(out, msg, msg_size);
+
+ free(msg);
+ }
+
+ /* Then the remapping table. */
+
+ if (remap_size > 0) {
+ msg_size = offsetof(dof_parsed_t, argremap.remapargs) + remap_size;
+
+ msg = malloc(msg_size);
+ if (!msg)
+ goto oom;
+
+ memset(msg, 0, msg_size);
+
+ msg->size = msg_size;
+ msg->type = DIT_REMAP_ARGS;
+ msg->argremap.nremapargs = dhpb->dthpb_xargc;
+ memcpy(msg->argremap.remapargs, dhpb->dthpb_args, remap_size);
+ dof_parser_write_one(out, msg, msg_size);
+ free(msg);
+ }
+
+ /*
* Emit info on each tracepoint in turn.
*/
for (i = 0; i < dhpb->dthpb_noffs; i++)
@@ -938,18 +1043,10 @@ emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
for (i = 0; i < dhpb->dthpb_nenoffs; i++)
emit_tp(out, dhpb->dthpb_base, dhpb->dthpb_enoffs[i], 1);
- /*
- * XXX later:
- * If the arguments are shuffled around we set the argument remapping
- * table. Later, when the probe fires, we only remap the arguments
- * if the table is non-NULL.
- *
- for (i = 0; i < dhpb->dthpb_xargc; i++) {
- if (dhpb->dthpb_args[i] != i) {
- pp->ftp_argmap = dhpb->dthpb_args;
- break;
- }
- } */
+ return;
+ oom:
+ dof_error(out, ENOMEM, "Out of memory allocating %zi bytes for probe",
+ msg_size);
}
static void
diff --git a/libcommon/dof_parser.h b/libcommon/dof_parser.h
index d3a6836f15fd..bfcac2c52de4 100644
--- a/libcommon/dof_parser.h
+++ b/libcommon/dof_parser.h
@@ -15,10 +15,19 @@
#include <dtrace/helpers.h>
/*
- * Result of DOF probe parsing. We receive a provider info structure, followed
- * by N probe info structures each of which is followed by possibly many
- * tracepoint info structures, all tagged. Things not yet used for probe
- * creation (like args, translated types, etc) are not returned.
+ * Result of DOF probe parsing. The order of elements in the parsed stream
+ * is:
+ *
+ * DIT_PROVIDER (at least 1, which contains...)
+ * DIT_PROBE (at least 1, each of which has...)
+ * DIT_NATIVE_ARGS (1, optional)
+ * DIT_XLAT_ARGS (1, optional)
+ * DIT_REMAP_ARGS (1, optional)
+ * DIT_TRACEPOINT (any number >= 1)
+ *
+ * The dof_parsed.provider.flags word indicates the presence of the
+ * various optional args records in the following stream (you can rely on
+ * them if it simplifies things, but you don't have to).
*
* On error, a DIT_ERR structure is returned with an error message.
*/
@@ -27,9 +36,19 @@ typedef enum dof_parsed_info {
DIT_PROVIDER = 0,
DIT_PROBE = 1,
DIT_TRACEPOINT = 2,
- DIT_ERR = 3
+ DIT_ERR = 3,
+ DIT_NATIVE_ARGS = 4,
+ DIT_XLAT_ARGS = 5,
+ DIT_REMAP_ARGS = 6,
} dof_parsed_info_t;
+/*
+ * Record-presence flags for dof_parsed.provider.flags.
+ */
+#define DIT_HAS_NATIVE_ARGS 0x1
+#define DIT_HAS_XLAT_ARGS 0x2
+#define DIT_HAS_REMAP_ARGS 0x4
+
/*
* Bump this whenever dof_parsed changes.
*
@@ -37,7 +56,7 @@ typedef enum dof_parsed_info {
* start which is the version of the dof_parseds within it. The data flowing
* over the stream from the seccomped parser has no such prefix.
*/
-#define DOF_PARSED_VERSION 1
+#define DOF_PARSED_VERSION 2
typedef struct dof_parsed {
/*
@@ -59,17 +78,62 @@ typedef struct dof_parsed {
*/
char name[1];
} provider;
- struct dpi_probe_info {
+
+ struct dpi_probe_info {
/*
* Number of tracepoints that follow.
*/
size_t ntp;
+ /*
+ * Flags indicating the presence of DIT_*_ARGS.
+ */
+ int flags;
+
/*
* Probe module, function, and name (\0-separated).
*/
char name[1];
- } probe;
+ } probe;
+
+ /* V2+ only. */
+ struct dpi_probe_native_args {
+ /*
+ * Number of native args.
+ */
+ uint8_t nargc;
+ /*
+ * Array of native args.
+ */
+ char args[1];
+ } nargs;
+
+ /* V2+ only. */
+ struct dpi_probe_xlat_args {
+ /*
+ * Number of translated args.
+ */
+ uint8_t nargc;
+ /*
+ * Array of translated args.
+ */
+ char args[1];
+ } xargs;
+
+ /*
+ * V2+ only. If provided, the given args have been shuffled.
+ */
+ struct dpi_probe_remap_args {
+ /*
+ * Number of remapped arguments.
+ */
+ uint8_t nremapargs;
+
+ /*
+ * Mapping from native arg index to xlated arg index.
+ */
+ uint8_t remapargs[1];
+ } argremap;
struct dpi_tracepoint_info {
/*
diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
index 62060b5921b6..356b671c8385 100644
--- a/libdtrace/dt_pid.c
+++ b/libdtrace/dt_pid.c
@@ -866,6 +866,10 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
uint64_t *dof_version;
char *prv, *mod, *fun, *prb;
dof_parsed_t *provider, *probe;
+ int nargc = 0, xargc = 0, remapargc = 0;
+ ssize_t nargvlen = 0, xargvlen = 0, remapargslen = 0;
+ char *nargv = NULL, *xargv = NULL;
+ int8_t *remapargs = NULL;
/*
* Regular files only: in particular, skip . and ..,
@@ -917,6 +921,55 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
p += probe->size;
seen_size += probe->size;
+ /*
+ * Assume the order given in dof_parser.h, for simplicity.
+ */
+ if (probe->probe.flags & DIT_HAS_NATIVE_ARGS) {
+ dof_parsed_t *args = (dof_parsed_t *) p;
+
+ if (!validate_dof_record(path, args, DIT_NATIVE_ARGS,
+ dof_buf_size, seen_size))
+ goto parse_err;
+
+ nargc = args->nargs.nargc;
+ nargv = args->nargs.args;
+ nargvlen = args->size - offsetof(dof_parsed_t, nargs.args);
+ assert(nargvlen >= 0);
+
+ p += args->size;
+ seen_size += args->size;
+ }
+ if (probe->probe.flags & DIT_HAS_XLAT_ARGS) {
+ dof_parsed_t *args = (dof_parsed_t *) p;
+
+ if (!validate_dof_record(path, args, DIT_XLAT_ARGS,
+ dof_buf_size, seen_size))
+ goto parse_err;
+
+ xargc = args->xargs.nargc;
+ xargv = args->xargs.args;
+ xargvlen = args->size - offsetof(dof_parsed_t, xargs.args);
+ assert(xargvlen >= 0);
+
+ p += args->size;
+ seen_size += args->size;
+ }
+ if (probe->probe.flags & DIT_HAS_REMAP_ARGS) {
+ dof_parsed_t *args = (dof_parsed_t *) p;
+
+ if (!validate_dof_record(path, args, DIT_REMAP_ARGS,
+ dof_buf_size, seen_size))
+ goto parse_err;
+
+ remapargc = args->argremap.nremapargs;
+ remapargs = args->argremap.remapargs;
+ remapargslen = args->size - offsetof(dof_parsed_t, argremap.remapargs);
+ assert(remapargslen >= 0);
+
+ p += args->size;
+ seen_size += args->size;
+ }
+
/*
* Now the parsed DOF for this probe's tracepoints.
*/
@@ -967,6 +1020,24 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
psp.pps_off = tp->tracepoint.addr - pmp->pr_file->first_segment->pr_vaddr;
psp.pps_nameoff = 0;
+ if (nargv) {
+ psp.pps_nargc = nargc;
+ psp.pps_nargvlen = nargvlen;
+ psp.pps_nargv = nargv;
+ }
+
+ if (xargv) {
+ psp.pps_xargc = xargc;
+ psp.pps_xargvlen = xargvlen;
+ psp.pps_xargv = xargv;
+ }
+
+ if (remapargs) {
+ psp.pps_remapargc = remapargc;
+ psp.pps_remapargslen = remapargslen;
+ psp.pps_remapargs = remapargs;
+ }
+
dt_dprintf("providing %s:%s:%s:%s for pid %d\n", psp.pps_prv,
psp.pps_mod, psp.pps_fun, psp.pps_prb, psp.pps_pid);
if (pvp->impl->provide_probe(dtp, &psp) < 0) {
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 2/6] usdt: get arg types and xlations into DTrace from the DOF
2024-10-18 19:58 ` [PATCH 2/6] usdt: get arg types and xlations into DTrace from the DOF Nick Alcock
@ 2024-10-22 17:03 ` Kris Van Hees
0 siblings, 0 replies; 23+ messages in thread
From: Kris Van Hees @ 2024-10-22 17:03 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
On Fri, Oct 18, 2024 at 08:58:04PM +0100, Nick Alcock wrote:
> This change propagates native and xlated arg types and remapping info all
> the way from the DOF, through the parser and dtprobed and the DOF stash,
> into DTrace, where they end up in the pid_probespec_t for USDT probes.
> We don't do anything with them once they get there in this commit: no
> user-visible impacts expected.
General comment for the entire series... the DTrace code consistently uses
'mapping arguments' rather than 'remapping'. I think it would be wise for us
to be consistent with that. That affects commit messages, comments, code,
and variable naming, but it really helps if we are consistent with the
existing code base. (When I see 'remapping' I start wondering why we are
mapping twice...)
> We bump the DOF_PARSED_VERSION since we add three new types of record to the
> dof_parser_t, all optional, covering native and xlated args and arg
> remapping tables; to make life easier for consumers we emit them in a
> defined order (documennted in dof_parser.h), and add a flags word to the
documented
> DIT_PROBE record that precedes them indicating which will be present. This
> means we retain the property that you can always tell which records within a
> provider are coming next purely from records you already have: there's no
> need to make decisions once the records turn up. The DOF stash hardly
> changes: all that happens is that the parsed data written to each per-probe
> file gains some extra types of record (it can have DIT_NATIVE_ARGS,
> DIT_XLAT_ARGS and DIT_REMAP_ARGS entries following the DIT_PROBE record
> now).
The generated DOF will always contains a list of native arg types, a list
of translated arg types, and a mapping. So, it is an all or nothing deal,
if the probe has arguments. Also, since your probe record already records
the number of tracepoints for the probe, it would be more consistent that you
als include an entry in the probe record for the number of probe arguments
and similarly, number of translated arguments (no need to record number of
arguument mapping values because that always equals the number of translated
argumwnts).
So, adding those two counts to the probe record (similar to number of
tracepoints) is consistent with the code you wrote before, and removes the need
for the flags member. The argument type and mapping data then no longer needs
to have its count at the start of their record, because you fold the flags
and counts into the probe record. That is also consistent with how DOF itself
stores this.
> As usual with DOF_PARSED_VERSION bumps, DTraces that cross this change will
> refuse to read probes added by dtprobeds that are on the other side of it.
> (Restarting dtprobed will reparse all the probes in the stash to match the
> new layout, and newly-started DTraces will pick that up -- so this only
> affects running DTraces picking up new probes, which DTrace cannot do yet:
> so no visible effects are expected.)
>
> The code is a bit repetitive, with nargs, xargs and remapping all handled
> separately even though the code is very similar. In future maybe we could
> unify them (but my attempts led to code that was much harder to read than
> the original).
>
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> ---
> dtprobed/dof_stash.c | 14 +++-
> dtprobed/dtprobed.c | 12 ++-
> include/dtrace/pid.h | 9 +++
> libcommon/dof_parser.c | 167 ++++++++++++++++++++++++++++++++---------
> libcommon/dof_parser.h | 80 ++++++++++++++++++--
> libdtrace/dt_pid.c | 71 ++++++++++++++++++
> 6 files changed, 302 insertions(+), 51 deletions(-)
>
> diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
> index 0e639076f655..d9731844066c 100644
> --- a/dtprobed/dof_stash.c
> +++ b/dtprobed/dof_stash.c
> @@ -59,8 +59,9 @@
> * startup if the dof_parsed_t structure changed. The file consists of a
> * uint64_t version number (DOF_PARSED_VERSION), then a stream of
> * dof_parsed_t records, the first of type DIT_PROVIDER, the second
> - * DIT_PROBE, then as many struct dof_parsed_t's of type DIT_TRACEPOINT as
> - * the DIT_PROBE record specifies.
> + * DIT_PROBE, then optionally some DIT_*ARGS records if the probe's flags
> + * word calls for them, then as many struct dof_parsed_t's of type
> + * DIT_TRACEPOINT as the DIT_PROBE record specifies.
Rewrite based on storing nargc and xargc in DIT_PROBE.
> /*
> * /run/dtrace/probes/: Per-probe info, written by dtprobed, read by DTrace.
> *
> @@ -640,9 +641,14 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
> break;
> }
>
> - /* Tracepoint: add to already-open file. */
> + /* Args info or tracepoint: add to already-open file. */
> + case DIT_NATIVE_ARGS:
> + case DIT_XLAT_ARGS:
> + case DIT_REMAP_ARGS:
DIT_MAP_ARGS
> case DIT_TRACEPOINT:
> - assert(state == DIT_PROBE || state == DIT_TRACEPOINT);
> + assert(state == DIT_PROBE || state == DIT_NATIVE_ARGS ||
> + state == DIT_XLAT_ARGS || state == DIT_REMAP_ARGS ||
DIT_MAP_ARGS
> + state == DIT_TRACEPOINT);
> state = accump->parsed->type;
>
> if (write_chunk(parsed_fd, accump->parsed, accump->parsed->size) < 0)
> diff --git a/dtprobed/dtprobed.c b/dtprobed/dtprobed.c
> index fdcdee14f851..ecf27216ff80 100644
> --- a/dtprobed/dtprobed.c
> +++ b/dtprobed/dtprobed.c
> @@ -795,16 +795,20 @@ process_dof(pid_t pid, int out, int in, dev_t dev, ino_t inum, dev_t exec_dev,
> if (dof_stash_push_parsed(&accum, probe) < 0)
> goto oom;
>
> - for (j = 0; j < probe->probe.ntp; j++) {
> + j = 0;
> + do {
> dof_parsed_t *tp = dof_read(pid, in);
>
> - errmsg = "no tracepoints in a probe, or parse state corrupt";
> - if (!tp || tp->type != DIT_TRACEPOINT)
> + errmsg = "no tracepoints in a probe, or parse state corrupt";
Indent using spaces rather than tabs.
> + if (!tp || tp->type == DIT_PROVIDER || tp->type == DIT_PROBE)
> goto err;
>
> if (dof_stash_push_parsed(&accum, tp) < 0)
> goto oom;
> - }
> +
> + if (tp->type == DIT_TRACEPOINT)
Indent using spaces rather than tabs.
> + j++;
> + } while (j < probe->probe.ntp);
> }
>
> if (!reparsing)
> diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
> index 0129674adf0c..d9a5a40484a5 100644
> --- a/include/dtrace/pid.h
> +++ b/include/dtrace/pid.h
> @@ -35,6 +35,15 @@ typedef struct pid_probespec {
> ino_t pps_inum; /* object inode */
> char *pps_fn; /* object full filename */
> uint64_t pps_off; /* probe offset (in object) */
> + int pps_nargc; /* number of native args */
> + int pps_xargc; /* number of xlated args */
> + int pps_remapargc; /* number of remapped args */
Always the same as pps_xargc so not needed.
> + char *pps_nargv; /* array of native args */
> + size_t pps_nargvlen; /* (high estimate of) length of array */
> + char *pps_xargv; /* array of xlated args */
> + size_t pps_xargvlen; /* (high estimate of) length of array */
> + int8_t *pps_remapargs; /* remapped arg indexes */
pps_argmap (which is a more consistent name than e.g. pps_mapargs)
> + size_t pps_remapargslen; /* (high estimate of) length of array */
Not needed (always pps_xargc * sizeof(uint8_t)).
>
> /*
> * Fields below this point do not apply to underlying probes.
> diff --git a/libcommon/dof_parser.c b/libcommon/dof_parser.c
> index d6631a1afdcb..4a45ac41a64d 100644
> --- a/libcommon/dof_parser.c
> +++ b/libcommon/dof_parser.c
> @@ -879,13 +879,31 @@ validate_probe(int out, dtrace_helper_probedesc_t *dhpb)
> return 0;
> }
>
> +static size_t
> +strings_len(const char *strtab, size_t count)
> +{
> + size_t len = 0;
> +
> + for (; count > 0; count--) {
> + size_t this_len = strlen(strtab) + 1;
> +
> + len += this_len;
Indent using spaces rather than tabs.
> + strtab += this_len;
> + }
> + return len;
> +}
> +
> static void
> emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
> {
> int i;
> - dof_parsed_t *probe_msg;
> - size_t probe_msg_size;
> + dof_parsed_t *msg;
> + size_t msg_size;
> + size_t nargs_size;
> + size_t xargs_size;
> + size_t remap_size = 0;
map_size
> char *ptr;
> + int remap_args = 0;
Not needed, because baswd on your code you could simply use the fact that
map_size will be 0 if there is no need to map.
>
> dt_dbg_dof(" Creating probe %s:%s:%s:%s\n", dhpb->dthpb_prov,
> dhpb->dthpb_mod, dhpb->dthpb_func, dhpb->dthpb_name);
> @@ -893,37 +911,124 @@ emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
> if (validate_probe(out, dhpb) != 0)
> return;
>
> - probe_msg_size = offsetof(dof_parsed_t, probe.name) +
> - strlen(dhpb->dthpb_mod) + 1 + strlen(dhpb->dthpb_func) + 1 +
> - strlen(dhpb->dthpb_name) + 1;
> -
> - probe_msg = malloc(probe_msg_size);
> - if (!probe_msg) {
> - dof_error(out, ENOMEM, "Out of memory allocating probe");
> - return;
> + /*
> + * Figure out if the arguments are shuffled around (in which case we
> + * need to emit the argument remapping table).
> + */
> + for (i = 0; i < dhpb->dthpb_xargc; i++) {
> + if (dhpb->dthpb_args[i] != i) {
> + remap_args = 1;
> + break;
> + }
> }
The existing implementation expects mapping data regardless of whether real
argument mapping takes place or not. So, it is best to always emit mapping
data, even if it is simply i -> i.
>
> - memset(probe_msg, 0, probe_msg_size);
> + /*
> + * Compute the size of all the optional elements first, to fill out the
> + * flags.
> + */
>
> - probe_msg->size = probe_msg_size;
> - probe_msg->type = DIT_PROBE;
> - probe_msg->probe.ntp = dhpb->dthpb_noffs + dhpb->dthpb_nenoffs;
> - ptr = stpcpy(probe_msg->probe.name, dhpb->dthpb_mod);
> + msg_size = offsetof(dof_parsed_t, probe.name) +
> + strlen(dhpb->dthpb_mod) + 1 +
> + strlen(dhpb->dthpb_func) + 1 +
> + strlen(dhpb->dthpb_name) + 1;
> +
> + nargs_size = strings_len(dhpb->dthpb_ntypes, dhpb->dthpb_nargc);
Indent using spaces rather than tabs.
> + xargs_size = strings_len(dhpb->dthpb_xtypes, dhpb->dthpb_xargc);
I would move the assignment of nargs_size and xargs_size firther down, into
the conditionals that handle storing their data. If you give them an initial
0 value, then the remainder of this function will be handle things fine as far
as I can see.
> + if (remap_args)
> + remap_size = dhpb->dthpb_xargc * sizeof(uint8_t);
No need for the conditional. Always emit mapping data.
> +
> + msg = malloc(msg_size);
> + if (!msg)
> + goto oom;
> +
> + memset(msg, 0, msg_size);
> +
> + msg->size = msg_size;
> + msg->type = DIT_PROBE;
> + msg->probe.ntp = dhpb->dthpb_noffs + dhpb->dthpb_nenoffs;
> +
> + if (nargs_size)
> + msg->probe.flags |= DIT_HAS_NATIVE_ARGS;
> + if (xargs_size)
> + msg->probe.flags |= DIT_HAS_XLAT_ARGS;
> + if (remap_size)
> + msg->probe.flags |= DIT_HAS_REMAP_ARGS;
Not needed - just store the nargc and xargc in the probe record.
> +
> + ptr = stpcpy(msg->probe.name, dhpb->dthpb_mod);
> ptr++;
> ptr = stpcpy(ptr, dhpb->dthpb_func);
> ptr++;
> strcpy(ptr, dhpb->dthpb_name);
> - dof_parser_write_one(out, probe_msg, probe_msg_size);
> + dof_parser_write_one(out, msg, msg_size);
Indent using spaces rather than tabs.
>
> - free(probe_msg);
> -
> - /* XXX TODO translated args
> - pp->ftp_nargs = dhpb->dthpb_xargc;
> - pp->ftp_xtypes = dhpb->dthpb_xtypes;
> - pp->ftp_ntypes = dhpb->dthpb_ntypes;
> - */
> + free(msg);
>
> /*
> + * Emit info on all native and translated args in turn.
> + *
> + * FIXME: this code is a bit repetitious.
> + *
> + * First native args (if any).
> + */
> +
> + if (nargs_size > 0) {
Make the conditional on dhpb->dthpb_ntypes instead, and the calculation of
nargs_size would be within the contional.
> + msg_size = offsetof(dof_parsed_t, nargs.args) + nargs_size;
> +
> + msg = malloc(msg_size);
> + if (!msg)
> + goto oom;
> +
> + memset(msg, 0, msg_size);
> +
> + msg->size = msg_size;
> + msg->type = DIT_NATIVE_ARGS;
> + msg->nargs.nargc = dhpb->dthpb_nargc;
Not needed.
> + memcpy(msg->nargs.args, dhpb->dthpb_ntypes, nargs_size);
> + dof_parser_write_one(out, msg, msg_size);
> +
> + free(msg);
> + }
> +
> + /* Then translated args. */
You could put this conditional inside the previous one in the sense that you
cannot have translated arguments if there are no native arguments (but the
opposite *is* possible, by the way).
> +
> + if (xargs_size > 0) {
Indent using spaces rather than tabs.
Make the conditional on dhpb->dthpb_xtypes instead, and the calculation of
xargs_size would be within the contional.
> + msg_size = offsetof(dof_parsed_t, xargs.args) + xargs_size;
> +
> + msg = malloc(msg_size);
> + if (!msg)
> + goto oom;
> +
> + memset(msg, 0, msg_size);
> +
> + msg->size = msg_size;
> + msg->type = DIT_XLAT_ARGS;
> + msg->xargs.nargc = dhpb->dthpb_xargc;
Not needed. Besides, an nargc member of xargs seems odd anyway :)
> + memcpy(msg->xargs.args, dhpb->dthpb_xtypes, xargs_size);
> + dof_parser_write_one(out, msg, msg_size);
> +
> + free(msg);
> + }
> +
> + /* Then the remapping table. */
> +
> + if (remap_size > 0) {
Move this into the handling of xargs because translated arg data and mapping
always go together. There is always a mapping entry for every translated
type.
> + msg_size = offsetof(dof_parsed_t, argremap.remapargs) + remap_size;
argmap.map perhaps?
map_size
> +
> + msg = malloc(msg_size);
> + if (!msg)
> + goto oom;
> +
> + memset(msg, 0, msg_size);
> +
> + msg->size = msg_size;
> + msg->type = DIT_REMAP_ARGS;
> + msg->argremap.nremapargs = dhpb->dthpb_xargc;
Not needed.
> + memcpy(msg->argremap.remapargs, dhpb->dthpb_args, remap_size);
argmap.map perhaps?
map_size
> + dof_parser_write_one(out, msg, msg_size);
> + free(msg);
> + }
> +
> + /*
Indent using spaces rather than tabs.
> * Emit info on each tracepoint in turn.
> */
> for (i = 0; i < dhpb->dthpb_noffs; i++)
> @@ -938,18 +1043,10 @@ emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
> for (i = 0; i < dhpb->dthpb_nenoffs; i++)
> emit_tp(out, dhpb->dthpb_base, dhpb->dthpb_enoffs[i], 1);
>
> - /*
> - * XXX later:
> - * If the arguments are shuffled around we set the argument remapping
> - * table. Later, when the probe fires, we only remap the arguments
> - * if the table is non-NULL.
> - *
> - for (i = 0; i < dhpb->dthpb_xargc; i++) {
> - if (dhpb->dthpb_args[i] != i) {
> - pp->ftp_argmap = dhpb->dthpb_args;
> - break;
> - }
> - } */
> + return;
Indent using spaces rather than tabs.
> + oom:
> + dof_error(out, ENOMEM, "Out of memory allocating %zi bytes for probe",
> + msg_size);
> }
>
> static void
> diff --git a/libcommon/dof_parser.h b/libcommon/dof_parser.h
> index d3a6836f15fd..bfcac2c52de4 100644
> --- a/libcommon/dof_parser.h
> +++ b/libcommon/dof_parser.h
> @@ -15,10 +15,19 @@
> #include <dtrace/helpers.h>
>
> /*
> - * Result of DOF probe parsing. We receive a provider info structure, followed
> - * by N probe info structures each of which is followed by possibly many
> - * tracepoint info structures, all tagged. Things not yet used for probe
> - * creation (like args, translated types, etc) are not returned.
> + * Result of DOF probe parsing. The order of elements in the parsed stream
> + * is:
> + *
> + * DIT_PROVIDER (at least 1, which contains...)
> + * DIT_PROBE (at least 1, each of which has...)
> + * DIT_NATIVE_ARGS (1, optional)
> + * DIT_XLAT_ARGS (1, optional)
A more accurate naming might be DIT_NATIVE_ARGTYPES and DIT_XLAT_ARGTYPES?
Since this is all the data that is contains in those elements?
> + * DIT_REMAP_ARGS (1, optional)
> + * DIT_TRACEPOINT (any number >= 1)
> + *
> + * The dof_parsed.provider.flags word indicates the presence of the
> + * various optional args records in the following stream (you can rely on
> + * them if it simplifies things, but you don't have to).
Not needed if you store the number of nargs and xargs, which is certainly
more consistent with e.g. how you handle tracepoints. And avoids duplicating
data (since you were only using the flags to indicate whether there are any
native args or xlat types, and that is as easily determined by nargc > 0 and
xargc > 0).
> *
> * On error, a DIT_ERR structure is returned with an error message.
> */
> @@ -27,9 +36,19 @@ typedef enum dof_parsed_info {
> DIT_PROVIDER = 0,
> DIT_PROBE = 1,
> DIT_TRACEPOINT = 2,
> - DIT_ERR = 3
> + DIT_ERR = 3,
> + DIT_NATIVE_ARGS = 4,
> + DIT_XLAT_ARGS = 5,
> + DIT_REMAP_ARGS = 6,
> } dof_parsed_info_t;
>
> +/*
> + * Record-presence flags for dof_parsed.provider.flags.
> + */
> +#define DIT_HAS_NATIVE_ARGS 0x1
> +#define DIT_HAS_XLAT_ARGS 0x2
> +#define DIT_HAS_REMAP_ARGS 0x4
Not needed.
> +
> /*
> * Bump this whenever dof_parsed changes.
> *
> @@ -37,7 +56,7 @@ typedef enum dof_parsed_info {
> * start which is the version of the dof_parseds within it. The data flowing
> * over the stream from the seccomped parser has no such prefix.
> */
> -#define DOF_PARSED_VERSION 1
> +#define DOF_PARSED_VERSION 2
>
> typedef struct dof_parsed {
> /*
> @@ -59,17 +78,62 @@ typedef struct dof_parsed {
> */
> char name[1];
> } provider;
> - struct dpi_probe_info {
> +
> + struct dpi_probe_info {
Indent using spaces rather than tabs.
> /*
> * Number of tracepoints that follow.
> */
> size_t ntp;
>
> + /*
> + * Flags indicating the presence of DIT_*_ARGS.
> + */
> + int flags;
> +
Add int nargc and int xargc instead?
> /*
> * Probe module, function, and name (\0-separated).
> */
> char name[1];
> - } probe;
> + } probe;
Indent using spaces rather than tabs.
> +
> + /* V2+ only. */
> + struct dpi_probe_native_args {
> + /*
> + * Number of native args.
> + */
> + uint8_t nargc;
Move to probe record.
> + /*
> + * Array of native args.
> + */
> + char args[1];
> + } nargs;
> +
> + /* V2+ only. */
> + struct dpi_probe_xlat_args {
> + /*
> + * Number of translated args.
> + */
> + uint8_t nargc;
Move to probe record.
> + /*
> + * Array of translated args.
> + */
> + char args[1];
> + } xargs;
> +
> + /*
> + * V2+ only. If provided, the given args have been shuffled.
> + */
> + struct dpi_probe_remap_args {
map instead of remap
> + /*
> + * Number of remapped arguments.
> + */
> + uint8_t nremapargs;
Not needed - same as xargc in probe record.
> +
> + /*
> + * Mapping from native arg index to xlated arg index.
> + */
> + uint8_t remapargs[1];
argmap seems more appropriate as a name?
> + } argremap;
>
> struct dpi_tracepoint_info {
> /*
> diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> index 62060b5921b6..356b671c8385 100644
> --- a/libdtrace/dt_pid.c
> +++ b/libdtrace/dt_pid.c
> @@ -866,6 +866,10 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> uint64_t *dof_version;
> char *prv, *mod, *fun, *prb;
> dof_parsed_t *provider, *probe;
> + int nargc = 0, xargc = 0, remapargc = 0;
remapargc not needed.
> + ssize_t nargvlen = 0, xargvlen = 0, remapargslen = 0;
remapargslen not needed.
> + char *nargv = NULL, *xargv = NULL;
> + int8_t *remapargs = NULL;
int8_t *argmap = NUL;
>
> /*
> * Regular files only: in particular, skip . and ..,
> @@ -917,6 +921,55 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> p += probe->size;
> seen_size += probe->size;
>
> + /*
> + * Assume the order given in dof_parser.h, for simplicity.
> + */
> + if (probe->probe.flags & DIT_HAS_NATIVE_ARGS) {
Indent using spaces rather than tabs.
Use probe->probe.nargc instead.
> + dof_parsed_t *args = (dof_parsed_t *) p;
> +
> + if (!validate_dof_record(path, args, DIT_NATIVE_ARGS,
Indent using spaces rather than tabs.
> + dof_buf_size, seen_size))
> + goto parse_err;
> +
> + nargc = args->nargs.nargc;
> + nargv = args->nargs.args;
> + nargvlen = args->size - offsetof(dof_parsed_t, nargs.args);
> + assert(nargvlen >= 0);
> +
> + p += args->size;
> + seen_size += args->size;
> + }
> + if (probe->probe.flags & DIT_HAS_XLAT_ARGS) {
You can move this into the previous conditional, and make this condition on
probe->probe.xargc > 0.
> + dof_parsed_t *args = (dof_parsed_t *) p;
> +
> + if (!validate_dof_record(path, args, DIT_XLAT_ARGS,
Indent using spaces rather than tabs.
> + dof_buf_size, seen_size))
> + goto parse_err;
> +
> + xargc = args->xargs.nargc;
> + xargv = args->xargs.args;
> + xargvlen = args->size - offsetof(dof_parsed_t, xargs.args);
> + assert(xargvlen >= 0);
> +
> + p += args->size;
Indent using spaces rather than tabs.
> + seen_size += args->size;
> + }
> + if (probe->probe.flags & DIT_HAS_REMAP_ARGS) {
> + dof_parsed_t *args = (dof_parsed_t *) p;
> +
> + if (!validate_dof_record(path, args, DIT_REMAP_ARGS,
Indent using spaces rather than tabs.
> + dof_buf_size, seen_size))
> + goto parse_err;
> +
> + remapargc = args->argremap.nremapargs;
Not needed. Same as xargc.
> + remapargs = args->argremap.remapargs;
> + remapargslen = args->size - offsetof(dof_parsed_t, argremap.remapargs);
Not needed. We know that argmap is an array of xargc unsigned bytes.
> + assert(remapargslen >= 0);
> +
> + p += args->size;
Indent using spaces rather than tabs.
> + seen_size += args->size;
> + }
> +
> /*
> * Now the parsed DOF for this probe's tracepoints.
> */
> @@ -967,6 +1020,24 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> psp.pps_off = tp->tracepoint.addr - pmp->pr_file->first_segment->pr_vaddr;
> psp.pps_nameoff = 0;
>
> + if (nargv) {
> + psp.pps_nargc = nargc;
> + psp.pps_nargvlen = nargvlen;
> + psp.pps_nargv = nargv;
> + }
> +
> + if (xargv) {
Indent using spaces rather than tabs.
> + psp.pps_xargc = xargc;
> + psp.pps_xargvlen = xargvlen;
> + psp.pps_xargv = xargv;
> + }
> +
> + if (remapargs) {
Indent using spaces rather than tabs.
> + psp.pps_remapargc = remapargc;
> + psp.pps_remapargslen = remapargslen;
Previous 2 lines not needed.
> + psp.pps_remapargs = remapargs;
argmap
> + }
> +
> dt_dprintf("providing %s:%s:%s:%s for pid %d\n", psp.pps_prv,
> psp.pps_mod, psp.pps_fun, psp.pps_prb, psp.pps_pid);
> if (pvp->impl->provide_probe(dtp, &psp) < 0) {
> --
> 2.46.0.278.g36e3a12567
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/6] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c
2024-10-18 19:58 [PATCH 0/6] usdt typed args, translators and arg remapping Nick Alcock
2024-10-18 19:58 ` [PATCH 1/6] error: add missing EDT_PRINT entry Nick Alcock
2024-10-18 19:58 ` [PATCH 2/6] usdt: get arg types and xlations into DTrace from the DOF Nick Alcock
@ 2024-10-18 19:58 ` Nick Alcock
2024-10-22 17:12 ` Kris Van Hees
2024-10-18 19:58 ` [PATCH 4/6] cg: add argument remapping in the trampoline Nick Alcock
` (2 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Nick Alcock @ 2024-10-18 19:58 UTC (permalink / raw)
To: dtrace, dtrace-devel
We already validate that they don't exist in dof_parser.c, so no such probes
can ever get here. Handling this impossible, untestable case has just got
harder, because we'd have to skip DIT_{NATIVE,XLAT,REMAP}_ARGS records too.
Just stop considering it.
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
dtprobed/dof_stash.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
index d9731844066c..cc2284aadbc0 100644
--- a/dtprobed/dof_stash.c
+++ b/dtprobed/dof_stash.c
@@ -585,12 +585,6 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
if (err != 0)
goto err_provider;
- /*
- * Skip probes with zero tracepoints entirely.
- */
- if (accump->parsed->probe.ntp == 0)
- break;
-
mod = accump->parsed->probe.name;
assert(accump->parsed->size > (mod - (char *) accump->parsed));
fun = mod + strlen(mod) + 1;
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 3/6] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c
2024-10-18 19:58 ` [PATCH 3/6] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c Nick Alcock
@ 2024-10-22 17:12 ` Kris Van Hees
2024-10-24 14:12 ` Nick Alcock
0 siblings, 1 reply; 23+ messages in thread
From: Kris Van Hees @ 2024-10-22 17:12 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
On Fri, Oct 18, 2024 at 08:58:05PM +0100, Nick Alcock wrote:
> We already validate that they don't exist in dof_parser.c, so no such probes
> can ever get here. Handling this impossible, untestable case has just got
> harder, because we'd have to skip DIT_{NATIVE,XLAT,REMAP}_ARGS records too.
> Just stop considering it.
>
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
> dtprobed/dof_stash.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
> index d9731844066c..cc2284aadbc0 100644
> --- a/dtprobed/dof_stash.c
> +++ b/dtprobed/dof_stash.c
> @@ -585,12 +585,6 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
> if (err != 0)
> goto err_provider;
>
> - /*
> - * Skip probes with zero tracepoints entirely.
> - */
> - if (accump->parsed->probe.ntp == 0)
> - break;
> -
> mod = accump->parsed->probe.name;
> assert(accump->parsed->size > (mod - (char *) accump->parsed));
> fun = mod + strlen(mod) + 1;
> --
> 2.46.0.278.g36e3a12567
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 3/6] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c
2024-10-22 17:12 ` Kris Van Hees
@ 2024-10-24 14:12 ` Nick Alcock
0 siblings, 0 replies; 23+ messages in thread
From: Nick Alcock @ 2024-10-24 14:12 UTC (permalink / raw)
To: Kris Van Hees; +Cc: dtrace, dtrace-devel
On 22 Oct 2024, Kris Van Hees verbalised:
> On Fri, Oct 18, 2024 at 08:58:05PM +0100, Nick Alcock wrote:
>> We already validate that they don't exist in dof_parser.c, so no such probes
>> can ever get here. Handling this impossible, untestable case has just got
>> harder, because we'd have to skip DIT_{NATIVE,XLAT,REMAP}_ARGS records too.
>> Just stop considering it.
>>
>> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
>
> Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
Ack (adjusted comment to match the previous review comments).
--
NULL && (void)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/6] cg: add argument remapping in the trampoline
2024-10-18 19:58 [PATCH 0/6] usdt typed args, translators and arg remapping Nick Alcock
` (2 preceding siblings ...)
2024-10-18 19:58 ` [PATCH 3/6] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c Nick Alcock
@ 2024-10-18 19:58 ` Nick Alcock
2024-10-22 17:43 ` Kris Van Hees
2024-10-18 19:58 ` [PATCH 5/6] usdt: typed args and arg remapping Nick Alcock
2024-10-18 19:58 ` [PATCH 6/6] usdt: fix create_underlying error path Nick Alcock
5 siblings, 1 reply; 23+ messages in thread
From: Nick Alcock @ 2024-10-18 19:58 UTC (permalink / raw)
To: dtrace, dtrace-devel
Before now, argument remapping was restricted to the args[] array, and was
implemented in dt_cg_array_op in the same place where we decide whether
to automatically copyin() userspace strings.
But existing DTrace testcases suggest that remapping is also applied to argN
for USDT, and it would definitely be more *convenient* if it were (so that
argN and arg[N] always contain the same content, and only their types
differ). Add a new function dt_cg_tramp_remap_args(), which can be
called from trampolines to apply remappings (destructively, but it keeps a
copy of the old args in the save area so you can unapply them if you need
to). No existing providers do any remapping, so this is not yet called, but
it's about to be.
The remapping info is still propagated up out of per-provider state so that
the parser can use it to tell what types the remapped arguments are and
to apply array-op remapping: users of dt_cg_tramp_remap_args() should set
the probe remapping to 1:1 and shuffle the native types correspondingly,
to match what dt_cg_tramp_remap_args() has done (in effect, calling it has
moved the native types around).
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
libdtrace/dt_cg.c | 35 ++++++++++++++++++++++++++++++++++-
libdtrace/dt_cg.h | 1 +
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index 39c27ab0ec0b..9709667d97fb 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -831,6 +831,34 @@ dt_cg_tramp_restore_args(dt_pcb_t *pcb)
}
}
+/*
+ * Remap arguments according to the array of remappings given. The original
+ * arguments are overwritten (but saved).
+ *
+ * The caller must ensure that %r7 contains the value set by the
+ * dt_cg_tramp_prologue*() functions.
+ */
+void
+dt_cg_tramp_remap_args(dt_pcb_t *pcb, uint8_t *remappings, size_t nremappings)
+{
+ dt_irlist_t *dlp = &pcb->pcb_ir;
+ int i;
+
+ /*
+ * Work from saved args so we don't need to worry about overwriting regs
+ * we're about to remap.
+ */
+ dt_cg_tramp_save_args(pcb);
+
+ for (i = 0; i < nremappings; i++) {
+ if (remappings[i] != i) {
+ dt_dprintf("Remap: loading from %i, storing to %i\n", remappings[i], i);
+ emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(remappings[i])));
+ emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
+ }
+ }
+}
+
typedef struct {
dt_irlist_t *dlp;
dt_activity_t act;
@@ -5060,7 +5088,12 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
* array index according to the static argument mapping (if any),
* unless the argument reference is provided by a dynamic translator.
* If we're using a dynamic translator for args[], then just set dn_reg
- * to an invalid reg and return: DIF_OP_XLARG will fetch the arg later.
+ *
+ * If dt_cg_tramp_remap_args is in use, you should apply the remapping
+ * at the probe_info stage, since the effective "native" arg positions
+ * will be changed.
+ *
+ * If this is a userland variable, note that we need to copy it in.
*/
if (idp->di_id == DIF_VAR_ARGS) {
if ((idp->di_kind == DT_IDENT_XLPTR ||
diff --git a/libdtrace/dt_cg.h b/libdtrace/dt_cg.h
index 0a7c7ba6a8c5..4c6d0f5dac6a 100644
--- a/libdtrace/dt_cg.h
+++ b/libdtrace/dt_cg.h
@@ -34,6 +34,7 @@ extern void dt_cg_tramp_get_var(dt_pcb_t *pcb, const char *name, int isstore,
extern void dt_cg_tramp_del_var(dt_pcb_t *pcb, const char *name);
extern void dt_cg_tramp_save_args(dt_pcb_t *pcb);
extern void dt_cg_tramp_restore_args(dt_pcb_t *pcb);
+extern void dt_cg_tramp_remap_args(dt_pcb_t *pcb, uint8_t *remappings, size_t nremappings);
extern void dt_cg_tramp_call_clauses(dt_pcb_t *pcb, const dt_probe_t *prp,
dt_activity_t act);
extern void dt_cg_tramp_return(dt_pcb_t *pcb);
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 4/6] cg: add argument remapping in the trampoline
2024-10-18 19:58 ` [PATCH 4/6] cg: add argument remapping in the trampoline Nick Alcock
@ 2024-10-22 17:43 ` Kris Van Hees
2024-10-25 15:56 ` Nick Alcock
0 siblings, 1 reply; 23+ messages in thread
From: Kris Van Hees @ 2024-10-22 17:43 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
On Fri, Oct 18, 2024 at 08:58:06PM +0100, Nick Alcock wrote:
> Before now, argument remapping was restricted to the args[] array, and was
> implemented in dt_cg_array_op in the same place where we decide whether
> to automatically copyin() userspace strings.
remapping -> mapping (everywhere in this patch, and similarly, remap -> map)
> But existing DTrace testcases suggest that remapping is also applied to argN
> for USDT, and it would definitely be more *convenient* if it were (so that
> argN and arg[N] always contain the same content, and only their types
> differ). Add a new function dt_cg_tramp_remap_args(), which can be
> called from trampolines to apply remappings (destructively, but it keeps a
> copy of the old args in the save area so you can unapply them if you need
> to). No existing providers do any remapping, so this is not yet called, but
> it's about to be.
I think that the preceding paragraph makes some wild assumptions that are not
really in line with what the anticipated behaviour is. For one, there is only
a single existing DTrace testcase that involves argument mapping, and it is
uncertain whether that reflects a conscious design decision. It is certainly
not consistent with the SDT implementation where argN reflects native values
and args[] is to be used for translated arguments.
The documentation states:
args[]
The typed arguments, if any, to the current probe. The args[] array is
accessed using an integer index, but each element is defined to be the type
corresponding to the given probe argument. For information about any typed
arguments, use dtrace -l with the verbose option -v and check Argument Types.
int64_t arg0, ..., arg9
The first ten input arguments to a probe, represented as raw 64-bit integers.
Values are meaningful only for arguments defined for the current probe.
Now, granted, this is a bit vague and does not entirely apply to SDT probes
because there we actually have types for probe arguments anyway. But I think
a valid case can be made for argN to *always* be a raw 64-bit integer value
(which you could explicitly cast to the type it really is) and to *always* be
the raw arguments of the probe, i.e. what traditionally was passed as the
argument to the dtrace_probe() function. That would correspond to the native
arguments for SDT and USDT.
Anyway, rather than making assumptions on what would be better or implying
that there is sufficient evidence for one versus the other implementation,
you should simply document that a test in the testsuite implies that the
expectation is that argNM and args[N] are the same for USDT probes, and that
you are adding a function to do apply this mapping, for use in trampoline code.
I also disagree that the function performs the saving of arguments itself.
I think that the caller ought to take care of that, because the caller has a
much better idea about when/if to save arguments, and when/if to restore them.
> The remapping info is still propagated up out of per-provider state so that
> the parser can use it to tell what types the remapped arguments are and
> to apply array-op remapping: users of dt_cg_tramp_remap_args() should set
> the probe remapping to 1:1 and shuffle the native types correspondingly,
> to match what dt_cg_tramp_remap_args() has done (in effect, calling it has
> moved the native types around).
I would drop this paragraph. It is unnecessary and speculative at best about
what other code should or should not do.
However, I think it might be a good idea to have this function actually rewrite
the mapping data to become a pure i -> i mapping. That seems appropriate
given that it applying the mapping and thereby should ensure that the mapping
will not be applied again by any other code.
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> ---
> libdtrace/dt_cg.c | 35 ++++++++++++++++++++++++++++++++++-
> libdtrace/dt_cg.h | 1 +
> 2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 39c27ab0ec0b..9709667d97fb 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -831,6 +831,34 @@ dt_cg_tramp_restore_args(dt_pcb_t *pcb)
> }
> }
>
> +/*
> + * Remap arguments according to the array of remappings given. The original
Remap -> Map
remapping -> mapping
> + * arguments are overwritten (but saved).
Caller should save.
> + *
> + * The caller must ensure that %r7 contains the value set by the
> + * dt_cg_tramp_prologue*() functions.
> + */
> +void
> +dt_cg_tramp_remap_args(dt_pcb_t *pcb, uint8_t *remappings, size_t nremappings)
remap -> map
remappings -> mapping (or argmap)
nremappings -> mapc (or xargc)
> +{
> + dt_irlist_t *dlp = &pcb->pcb_ir;
> + int i;
> +
> + /*
> + * Work from saved args so we don't need to worry about overwriting regs
> + * we're about to remap.
> + */
> + dt_cg_tramp_save_args(pcb);
Caller should save.
> +
> + for (i = 0; i < nremappings; i++) {
> + if (remappings[i] != i) {
> + dt_dprintf("Remap: loading from %i, storing to %i\n", remappings[i], i);
I would remove this debugging statement. We don't really report debugging data
at this level anywhere else, really.
> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(remappings[i])));
> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
I would add:
argmap[i] = i;
to convert the mapping into a pure i -> i mapping, so no other code will apply
argument mapping again.
> + }
> + }
> +}
> +
> typedef struct {
> dt_irlist_t *dlp;
> dt_activity_t act;
> @@ -5060,7 +5088,12 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> * array index according to the static argument mapping (if any),
> * unless the argument reference is provided by a dynamic translator.
> * If we're using a dynamic translator for args[], then just set dn_reg
> - * to an invalid reg and return: DIF_OP_XLARG will fetch the arg later.
> + *
> + * If dt_cg_tramp_remap_args is in use, you should apply the remapping
> + * at the probe_info stage, since the effective "native" arg positions
> + * will be changed.
> + *
> + * If this is a userland variable, note that we need to copy it in.
Not needed. The original comment is sufficient.
> */
> if (idp->di_id == DIF_VAR_ARGS) {
> if ((idp->di_kind == DT_IDENT_XLPTR ||
> diff --git a/libdtrace/dt_cg.h b/libdtrace/dt_cg.h
> index 0a7c7ba6a8c5..4c6d0f5dac6a 100644
> --- a/libdtrace/dt_cg.h
> +++ b/libdtrace/dt_cg.h
> @@ -34,6 +34,7 @@ extern void dt_cg_tramp_get_var(dt_pcb_t *pcb, const char *name, int isstore,
> extern void dt_cg_tramp_del_var(dt_pcb_t *pcb, const char *name);
> extern void dt_cg_tramp_save_args(dt_pcb_t *pcb);
> extern void dt_cg_tramp_restore_args(dt_pcb_t *pcb);
> +extern void dt_cg_tramp_remap_args(dt_pcb_t *pcb, uint8_t *remappings, size_t nremappings);
dt_cg_tramp_map_args
> extern void dt_cg_tramp_call_clauses(dt_pcb_t *pcb, const dt_probe_t *prp,
> dt_activity_t act);
> extern void dt_cg_tramp_return(dt_pcb_t *pcb);
> --
> 2.46.0.278.g36e3a12567
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 4/6] cg: add argument remapping in the trampoline
2024-10-22 17:43 ` Kris Van Hees
@ 2024-10-25 15:56 ` Nick Alcock
2024-10-25 19:15 ` Kris Van Hees
0 siblings, 1 reply; 23+ messages in thread
From: Nick Alcock @ 2024-10-25 15:56 UTC (permalink / raw)
To: Kris Van Hees; +Cc: dtrace, dtrace-devel
On 22 Oct 2024, Kris Van Hees uttered the following:
> On Fri, Oct 18, 2024 at 08:58:06PM +0100, Nick Alcock wrote:
>> Before now, argument remapping was restricted to the args[] array, and was
>> implemented in dt_cg_array_op in the same place where we decide whether
>> to automatically copyin() userspace strings.
>
> remapping -> mapping (everywhere in this patch, and similarly, remap -> map)
Ack. I do hope none of the mapping code ever has to deal with BPF maps
for any reason or this will get *immensely* confusing.
>> But existing DTrace testcases suggest that remapping is also applied to argN
>> for USDT, and it would definitely be more *convenient* if it were (so that
>> argN and arg[N] always contain the same content, and only their types
>> differ). Add a new function dt_cg_tramp_remap_args(), which can be
>> called from trampolines to apply remappings (destructively, but it keeps a
>> copy of the old args in the save area so you can unapply them if you need
>> to). No existing providers do any remapping, so this is not yet called, but
>> it's about to be.
>
> I think that the preceding paragraph makes some wild assumptions that are not
> really in line with what the anticipated behaviour is. For one, there is only
> a single existing DTrace testcase that involves argument mapping, and it is
> uncertain whether that reflects a conscious design decision. It is certainly
> not consistent with the SDT implementation where argN reflects native values
> and args[] is to be used for translated arguments.
Did the prior SDT implementation do that? My admittedly fading memory
said that args[] was unremapped even there. I thought this argN
is-is-nonremapped approach was something *you* thought up for DTrace
v2 SDT... but I could be wrong.
> The documentation states:
>
> args[]
> The typed arguments, if any, to the current probe. The args[] array is
> accessed using an integer index, but each element is defined to be the type
> corresponding to the given probe argument. For information about any typed
> arguments, use dtrace -l with the verbose option -v and check Argument Types.
>
> int64_t arg0, ..., arg9
>
> The first ten input arguments to a probe, represented as raw 64-bit integers.
> Values are meaningful only for arguments defined for the current probe.
... this doesn't really say anything one way or the other.
> Now, granted, this is a bit vague and does not entirely apply to SDT probes
> because there we actually have types for probe arguments anyway. But I think
> a valid case can be made for argN to *always* be a raw 64-bit integer value
> (which you could explicitly cast to the type it really is) and to *always* be
> the raw arguments of the probe, i.e. what traditionally was passed as the
> argument to the dtrace_probe() function. That would correspond to the native
> arguments for SDT and USDT.
I think a valid case can be made that you can always get to the untyped
equivalent of a mapped arg by just casting args[N] to uintptr_t, and
thus having args[N] be unmapped as well does give you access to
something you never could before. But that hardly makes what I said in
this commit message a "wild assumption", for goodness' sake. The
allegedly conclusive documentation you cite is nothing of the kind!
(However, what sdt is doing now does seem better in general.)
> Anyway, rather than making assumptions on what would be better or implying
> that there is sufficient evidence for one versus the other implementation,
> you should simply document that a test in the testsuite implies that the
> expectation is that argNM and args[N] are the same for USDT probes, and that
> you are adding a function to do apply this mapping, for use in trampoline code.
Sure!
> I also disagree that the function performs the saving of arguments itself.
> I think that the caller ought to take care of that, because the caller has a
> much better idea about when/if to save arguments, and when/if to restore them.
Well, the reshuffling is *done* by saving arguments and then copying
them out of the saved orig_arg area into the args area, so either it
does the saving itself, or it silently malfunctions if the caller
doesn't save, or it reimplements all the saving infrastructure for no
other reason than to use a *different name* so the caller can save.
The latter two options seem, well, entirely terrible, so I went the
former route.
>> The remapping info is still propagated up out of per-provider state so that
>> the parser can use it to tell what types the remapped arguments are and
>> to apply array-op remapping: users of dt_cg_tramp_remap_args() should set
>> the probe remapping to 1:1 and shuffle the native types correspondingly,
>> to match what dt_cg_tramp_remap_args() has done (in effect, calling it has
>> moved the native types around).
>
> I would drop this paragraph. It is unnecessary and speculative at best about
> what other code should or should not do.
Hardly. It's documentation about how other code can get the arg types
right if they are using dt_cg_tramp_remap_args(). Would you prefer I
leave this entirely undescribed?! I could put it in a comment above
dt_cg_tramp_remap_args() (or rather the renamed dt_cg_tramp_map_args())
instead.
(Shifted into a comment, for now.)
> However, I think it might be a good idea to have this function actually rewrite
> the mapping data to become a pure i -> i mapping.
Can we guarantee that trampoline() is only ever invoked once, and that
it's invoked after probe_info is called? If it changes the remapping
array, any subsequent invocation will do the wrong thing. If it's
invoked before probe_info, it becomes impossible for probe_info to
adjust the xlated types appropriately.
This all seems like way too much coupling to me. At least done
this way trampoline() is idempotent and doesn't depend on being called
in the right (undocumented) order wrt probe_info.
(Or are you suggesting that dt_cg_tramp_map_args(), or its trampoline()
caller, shuffle the xargs too?! This seems even *further* from anything
a trampoline() function should do to me.)
>> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
>> ---
>> libdtrace/dt_cg.c | 35 ++++++++++++++++++++++++++++++++++-
>> libdtrace/dt_cg.h | 1 +
>> 2 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> index 39c27ab0ec0b..9709667d97fb 100644
>> --- a/libdtrace/dt_cg.c
>> +++ b/libdtrace/dt_cg.c
>> @@ -831,6 +831,34 @@ dt_cg_tramp_restore_args(dt_pcb_t *pcb)
>> }
>> }
>>
>> +/*
>> + * Remap arguments according to the array of remappings given. The original
>
> Remap -> Map
> remapping -> mapping
Done.
>> + * arguments are overwritten (but saved).
>
> Caller should save.
As far as I can see this is not implementable without massive disruptive
ugly (or at least pointlessly duplicative) changes, see above. It
doesn't seem to buy us anything, either.
>> +
>> + for (i = 0; i < nremappings; i++) {
>> + if (remappings[i] != i) {
>> + dt_dprintf("Remap: loading from %i, storing to %i\n", remappings[i], i);
>
> I would remove this debugging statement. We don't really report debugging data
> at this level anywhere else, really.
Oops, didn't mean to leave that in.
>> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(remappings[i])));
>> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
>
> I would add:
> argmap[i] = i;
> to convert the mapping into a pure i -> i mapping, so no other code will apply
> argument mapping again.
I think not: see above.
--
NULL && (void)
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 4/6] cg: add argument remapping in the trampoline
2024-10-25 15:56 ` Nick Alcock
@ 2024-10-25 19:15 ` Kris Van Hees
2024-10-28 21:11 ` Nick Alcock
2024-10-29 13:09 ` Nick Alcock
0 siblings, 2 replies; 23+ messages in thread
From: Kris Van Hees @ 2024-10-25 19:15 UTC (permalink / raw)
To: Nick Alcock; +Cc: Kris Van Hees, dtrace, dtrace-devel
On Fri, Oct 25, 2024 at 04:56:26PM +0100, Nick Alcock wrote:
> On 22 Oct 2024, Kris Van Hees uttered the following:
>
> > On Fri, Oct 18, 2024 at 08:58:06PM +0100, Nick Alcock wrote:
> >> Before now, argument remapping was restricted to the args[] array, and was
> >> implemented in dt_cg_array_op in the same place where we decide whether
> >> to automatically copyin() userspace strings.
> >
> > remapping -> mapping (everywhere in this patch, and similarly, remap -> map)
>
> Ack. I do hope none of the mapping code ever has to deal with BPF maps
> for any reason or this will get *immensely* confusing.
Not really, because we never refer to 'mapping' when talking about BPF maps :)
> >> But existing DTrace testcases suggest that remapping is also applied to argN
> >> for USDT, and it would definitely be more *convenient* if it were (so that
> >> argN and arg[N] always contain the same content, and only their types
> >> differ). Add a new function dt_cg_tramp_remap_args(), which can be
> >> called from trampolines to apply remappings (destructively, but it keeps a
> >> copy of the old args in the save area so you can unapply them if you need
> >> to). No existing providers do any remapping, so this is not yet called, but
> >> it's about to be.
> >
> > I think that the preceding paragraph makes some wild assumptions that are not
> > really in line with what the anticipated behaviour is. For one, there is only
> > a single existing DTrace testcase that involves argument mapping, and it is
> > uncertain whether that reflects a conscious design decision. It is certainly
> > not consistent with the SDT implementation where argN reflects native values
> > and args[] is to be used for translated arguments.
>
> Did the prior SDT implementation do that? My admittedly fading memory
> said that args[] was unremapped even there. I thought this argN
> is-is-nonremapped approach was something *you* thought up for DTrace
> v2 SDT... but I could be wrong.
SDT has distinct different interpretation of argN vs args[N] (raw arguments,
i.e. literal arguments passed to the probe vs documented, potentially
translated/mapped probe arguments).
> > The documentation states:
> >
> > args[]
> > The typed arguments, if any, to the current probe. The args[] array is
> > accessed using an integer index, but each element is defined to be the type
> > corresponding to the given probe argument. For information about any typed
> > arguments, use dtrace -l with the verbose option -v and check Argument Types.
> >
> > int64_t arg0, ..., arg9
> >
> > The first ten input arguments to a probe, represented as raw 64-bit integers.
> > Values are meaningful only for arguments defined for the current probe.
>
> ... this doesn't really say anything one way or the other.
It actually does... "The first ten INPUT arguments to a probe."
> > Now, granted, this is a bit vague and does not entirely apply to SDT probes
> > because there we actually have types for probe arguments anyway. But I think
> > a valid case can be made for argN to *always* be a raw 64-bit integer value
> > (which you could explicitly cast to the type it really is) and to *always* be
> > the raw arguments of the probe, i.e. what traditionally was passed as the
> > argument to the dtrace_probe() function. That would correspond to the native
> > arguments for SDT and USDT.
>
> I think a valid case can be made that you can always get to the untyped
> equivalent of a mapped arg by just casting args[N] to uintptr_t, and
> thus having args[N] be unmapped as well does give you access to
> something you never could before. But that hardly makes what I said in
> this commit message a "wild assumption", for goodness' sake. The
> allegedly conclusive documentation you cite is nothing of the kind!
> (However, what sdt is doing now does seem better in general.)
> > Anyway, rather than making assumptions on what would be better or implying
> > that there is sufficient evidence for one versus the other implementation,
> > you should simply document that a test in the testsuite implies that the
> > expectation is that argNM and args[N] are the same for USDT probes, and that
> > you are adding a function to do apply this mapping, for use in trampoline code.
>
> Sure!
>
> > I also disagree that the function performs the saving of arguments itself.
> > I think that the caller ought to take care of that, because the caller has a
> > much better idea about when/if to save arguments, and when/if to restore them.
>
> Well, the reshuffling is *done* by saving arguments and then copying
> them out of the saved orig_arg area into the args area, so either it
> does the saving itself, or it silently malfunctions if the caller
> doesn't save, or it reimplements all the saving infrastructure for no
> other reason than to use a *different name* so the caller can save.
>
> The latter two options seem, well, entirely terrible, so I went the
> former route.
The way this has been implemented already for SDT and the design behind that
is that the underlying probe saves arguments before overlying probes do
anything with them. The reshuffling is only one of the things that might
get done to arguments, and since there may be multiple overlying probes that
have different arguments, the caller needs to preserve the arguments it was
given in case any of the overlying ones changes them.
> >> The remapping info is still propagated up out of per-provider state so that
> >> the parser can use it to tell what types the remapped arguments are and
> >> to apply array-op remapping: users of dt_cg_tramp_remap_args() should set
> >> the probe remapping to 1:1 and shuffle the native types correspondingly,
> >> to match what dt_cg_tramp_remap_args() has done (in effect, calling it has
> >> moved the native types around).
> >
> > I would drop this paragraph. It is unnecessary and speculative at best about
> > what other code should or should not do.
>
> Hardly. It's documentation about how other code can get the arg types
> right if they are using dt_cg_tramp_remap_args(). Would you prefer I
> leave this entirely undescribed?! I could put it in a comment above
> dt_cg_tramp_remap_args() (or rather the renamed dt_cg_tramp_map_args())
> instead.
>
> (Shifted into a comment, for now.)
But that is not relevant to this patch, and other code shouldn't be doing this
because it is almost certainly wrong that USDT is remapping the arguments in
the trampoline because of the argN vs args[N] thng discussed above.
So, let's not add comments or commit message info that might point people to
do the wrong thing.
> > However, I think it might be a good idea to have this function actually rewrite
> > the mapping data to become a pure i -> i mapping.
>
> Can we guarantee that trampoline() is only ever invoked once, and that
> it's invoked after probe_info is called? If it changes the remapping
> array, any subsequent invocation will do the wrong thing. If it's
> invoked before probe_info, it becomes impossible for probe_info to
> adjust the xlated types appropriately.
>
> This all seems like way too much coupling to me. At least done
> this way trampoline() is idempotent and doesn't depend on being called
> in the right (undocumented) order wrt probe_info.
If the actual probe arguments (argN) are being reshuffled (which is what you
are doing by shuffling the arguments in the trampoline), then the mapping
needs to be updated to become an identity mapping i -> i). Otherwise there
could be other code at some point that tries to apply the argument mapping
and that would be really bad.
> (Or are you suggesting that dt_cg_tramp_map_args(), or its trampoline()
> caller, shuffle the xargs too?! This seems even *further* from anything
> a trampoline() function should do to me.)
Why would xargs ever be shuffled? There is nothing in DTrace that does that.
> >> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> >> ---
> >> libdtrace/dt_cg.c | 35 ++++++++++++++++++++++++++++++++++-
> >> libdtrace/dt_cg.h | 1 +
> >> 2 files changed, 35 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> >> index 39c27ab0ec0b..9709667d97fb 100644
> >> --- a/libdtrace/dt_cg.c
> >> +++ b/libdtrace/dt_cg.c
> >> @@ -831,6 +831,34 @@ dt_cg_tramp_restore_args(dt_pcb_t *pcb)
> >> }
> >> }
> >>
> >> +/*
> >> + * Remap arguments according to the array of remappings given. The original
> >
> > Remap -> Map
> > remapping -> mapping
>
> Done.
>
> >> + * arguments are overwritten (but saved).
> >
> > Caller should save.
>
> As far as I can see this is not implementable without massive disruptive
> ugly (or at least pointlessly duplicative) changes, see above. It
> doesn't seem to buy us anything, either.
How so? Just put the dt_cg_tramp_save_args() call in the trampoline right
before you call dt_cg_tramp_map_args(). How is that a massive disruptive ugly
change?
> >> +
> >> + for (i = 0; i < nremappings; i++) {
> >> + if (remappings[i] != i) {
> >> + dt_dprintf("Remap: loading from %i, storing to %i\n", remappings[i], i);
> >
> > I would remove this debugging statement. We don't really report debugging data
> > at this level anywhere else, really.
>
> Oops, didn't mean to leave that in.
>
> >> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(remappings[i])));
> >> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
> >
> > I would add:
> > argmap[i] = i;
> > to convert the mapping into a pure i -> i mapping, so no other code will apply
> > argument mapping again.
>
> I think not: see above.
I think so, above also :)
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 4/6] cg: add argument remapping in the trampoline
2024-10-25 19:15 ` Kris Van Hees
@ 2024-10-28 21:11 ` Nick Alcock
2024-10-28 21:20 ` Kris Van Hees
2024-10-29 13:09 ` Nick Alcock
1 sibling, 1 reply; 23+ messages in thread
From: Nick Alcock @ 2024-10-28 21:11 UTC (permalink / raw)
To: Kris Van Hees; +Cc: Nick Alcock, dtrace, dtrace-devel
On 25 Oct 2024, Kris Van Hees verbalised:
> On Fri, Oct 25, 2024 at 04:56:26PM +0100, Nick Alcock wrote:
>> > I also disagree that the function performs the saving of arguments itself.
>> > I think that the caller ought to take care of that, because the caller has a
>> > much better idea about when/if to save arguments, and when/if to restore them.
>>
>> Well, the reshuffling is *done* by saving arguments and then copying
>> them out of the saved orig_arg area into the args area, so either it
>> does the saving itself, or it silently malfunctions if the caller
>> doesn't save, or it reimplements all the saving infrastructure for no
>> other reason than to use a *different name* so the caller can save.
>>
>> The latter two options seem, well, entirely terrible, so I went the
>> former route.
>
> The way this has been implemented already for SDT and the design behind that
> is that the underlying probe saves arguments before overlying probes do
> anything with them. The reshuffling is only one of the things that might
> get done to arguments, and since there may be multiple overlying probes that
> have different arguments, the caller needs to preserve the arguments it was
> given in case any of the overlying ones changes them.
... that doesn't help me figure out what you want me to do. Can I save
the args and then pick them out in a reshuffled fashion, or what? I'm
trying to avoid having to write some sort of nightmarish in-place
shuffler in raw BPF here, because I'm fairly sure doing so would drive
me quite mad and take forever. Saving all the args and picking them out
in the reshuffled order is only a couple of lines. (See below.)
I still have no idea what's wrong with saving the args in
dt_cg_tramp_remap_args(): it will only cause trouble if something else
has saved the args before trampoline() is called, then modified them,
then expects to restore the originals after the trampoline(). Surely
nothing does that?
>> >> The remapping info is still propagated up out of per-provider state so that
>> >> the parser can use it to tell what types the remapped arguments are and
>> >> to apply array-op remapping: users of dt_cg_tramp_remap_args() should set
>> >> the probe remapping to 1:1 and shuffle the native types correspondingly,
>> >> to match what dt_cg_tramp_remap_args() has done (in effect, calling it has
>> >> moved the native types around).
>> >
>> > I would drop this paragraph. It is unnecessary and speculative at best about
>> > what other code should or should not do.
>>
>> Hardly. It's documentation about how other code can get the arg types
>> right if they are using dt_cg_tramp_remap_args(). Would you prefer I
>> leave this entirely undescribed?! I could put it in a comment above
>> dt_cg_tramp_remap_args() (or rather the renamed dt_cg_tramp_map_args())
>> instead.
>>
>> (Shifted into a comment, for now.)
>
> But that is not relevant to this patch, and other code shouldn't be doing this
> because it is almost certainly wrong that USDT is remapping the arguments in
> the trampoline because of the argN vs args[N] thng discussed above.
>
> So, let's not add comments or commit message info that might point people to
> do the wrong thing.
It's correct *iff* you're using the in-trampoline remapping (the comment
is above dt_cg_tramp_remap_args). Obviously if we end up removing that
function, we'll remove the comment too :)
>> Can we guarantee that trampoline() is only ever invoked once, and that
>> it's invoked after probe_info is called? If it changes the remapping
>> array, any subsequent invocation will do the wrong thing. If it's
>> invoked before probe_info, it becomes impossible for probe_info to
>> adjust the xlated types appropriately.
>>
>> This all seems like way too much coupling to me. At least done
>> this way trampoline() is idempotent and doesn't depend on being called
>> in the right (undocumented) order wrt probe_info.
>
> If the actual probe arguments (argN) are being reshuffled (which is what you
> are doing by shuffling the arguments in the trampoline), then the mapping
> needs to be updated to become an identity mapping i -> i).
Yes, the code does exactly that, in probe_info.
I don't know the relative ordering of the calls to probe_info() versus
trampoline(), but when probe_info() is called the mapping *must* be
fixed up, because it's probe_info()s caller that stashes it away -- so
it's safest to set up that revised mapping there. If I set it up in
trampoline(), I have no idea whether we'd reliably have the remapping
worked out by the time probe_info() is called or not. Yes, this means
there is an implied coupling between trampoline() calling
dt_cg_tramp_remap_args() and between probe_info() having to change the
mapping in the args -- but given that you've been suggesting similar
coupling between parts of *dt_pid* and dt_prov_uprobe, I was assuming
you'd find such a minor coupling (between, really, trampoline() and
probe_info() in dt_prov_uprobe) uncontroversial.
> Otherwise there
> could be other code at some point that tries to apply the argument mapping
> and that would be really bad.
Well, yes. Is there anything anywhere that documents the order in which
the dt_provimpl_t callbacks are called? They're invoked from all over
the place and their call order is distinctly obscure, and deciding
whether it's safe to play with DTrace-side arg mapping info from
trampoline() is core to that. (As opposed to merely *generating code*
that shuffles args. That can obviously be done at any time.)
>> (Or are you suggesting that dt_cg_tramp_map_args(), or its trampoline()
>> caller, shuffle the xargs too?! This seems even *further* from anything
>> a trampoline() function should do to me.)
>
> Why would xargs ever be shuffled? There is nothing in DTrace that does that.
Good. I thought you were suggesting it, and was rather confused.
>> >> + * arguments are overwritten (but saved).
>> >
>> > Caller should save.
>>
>> As far as I can see this is not implementable without massive disruptive
>> ugly (or at least pointlessly duplicative) changes, see above. It
>> doesn't seem to buy us anything, either.
>
> How so? Just put the dt_cg_tramp_save_args() call in the trampoline right
> before you call dt_cg_tramp_map_args(). How is that a massive disruptive ugly
> change?
Here's the code again:
/*
* Work from saved args so we don't need to worry about overwriting regs
* we're about to remap.
*/
dt_cg_tramp_save_args(pcb);
for (i = 0; i < nmappings; i++) {
if (mappings[i] != i) {
emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(mappings[i])));
emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
}
}
i.e. it's *using* the save area. It relies on the args being saved in
it. Either it does so itself, or it silently relies on callers doing it
(! ew), or you're asking me to *not use* the save area but instead
implement arg mapping in some other way (and this is what I thought you
were asking for, because having dt_cg_tramp_map_args() not save the args
but nonetheless rely on something else having saved them already is
horrible, surely you don't mean that).
At this point I have no idea if you want me to reimplement this in terms
of something else, add *another* saved arg area just for the sake of arg
mapping, implement some completely different sort of arg mapping
algorithm in raw BPF (no thank you), or what.
>> >> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(remappings[i])));
>> >> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
>> >
>> > I would add:
>> > argmap[i] = i;
>> > to convert the mapping into a pure i -> i mapping, so no other code will apply
>> > argument mapping again.
>>
>> I think not: see above.
>
> I think so, above also :)
See above: I have no idea if this is even slightly safe, because I don't
know the relative order in which trampoline and arg_info are called. I
at least know what I'm doing now works.
--
NULL && (void)
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 4/6] cg: add argument remapping in the trampoline
2024-10-28 21:11 ` Nick Alcock
@ 2024-10-28 21:20 ` Kris Van Hees
0 siblings, 0 replies; 23+ messages in thread
From: Kris Van Hees @ 2024-10-28 21:20 UTC (permalink / raw)
To: Nick Alcock; +Cc: Kris Van Hees, dtrace, dtrace-devel
Concerning the mapping in the trampoline - yes, as I said before, I believe
the caller (the trampoline) should save the reg, and then the
dt_cg_tramp_map_args() function can use those saved copies to do the mapping.
As I said... caller should save. I see no reason why dt_cg_tramp_map_args()
cannot depend on that. You can add a comment before it saying that this is
expected.
On Mon, Oct 28, 2024 at 09:11:42PM +0000, Nick Alcock wrote:
> On 25 Oct 2024, Kris Van Hees verbalised:
> > On Fri, Oct 25, 2024 at 04:56:26PM +0100, Nick Alcock wrote:
> >> > I also disagree that the function performs the saving of arguments itself.
> >> > I think that the caller ought to take care of that, because the caller has a
> >> > much better idea about when/if to save arguments, and when/if to restore them.
> >>
> >> Well, the reshuffling is *done* by saving arguments and then copying
> >> them out of the saved orig_arg area into the args area, so either it
> >> does the saving itself, or it silently malfunctions if the caller
> >> doesn't save, or it reimplements all the saving infrastructure for no
> >> other reason than to use a *different name* so the caller can save.
> >>
> >> The latter two options seem, well, entirely terrible, so I went the
> >> former route.
> >
> > The way this has been implemented already for SDT and the design behind that
> > is that the underlying probe saves arguments before overlying probes do
> > anything with them. The reshuffling is only one of the things that might
> > get done to arguments, and since there may be multiple overlying probes that
> > have different arguments, the caller needs to preserve the arguments it was
> > given in case any of the overlying ones changes them.
>
> ... that doesn't help me figure out what you want me to do. Can I save
> the args and then pick them out in a reshuffled fashion, or what? I'm
> trying to avoid having to write some sort of nightmarish in-place
> shuffler in raw BPF here, because I'm fairly sure doing so would drive
> me quite mad and take forever. Saving all the args and picking them out
> in the reshuffled order is only a couple of lines. (See below.)
>
> I still have no idea what's wrong with saving the args in
> dt_cg_tramp_remap_args(): it will only cause trouble if something else
> has saved the args before trampoline() is called, then modified them,
> then expects to restore the originals after the trampoline(). Surely
> nothing does that?
>
> >> >> The remapping info is still propagated up out of per-provider state so that
> >> >> the parser can use it to tell what types the remapped arguments are and
> >> >> to apply array-op remapping: users of dt_cg_tramp_remap_args() should set
> >> >> the probe remapping to 1:1 and shuffle the native types correspondingly,
> >> >> to match what dt_cg_tramp_remap_args() has done (in effect, calling it has
> >> >> moved the native types around).
> >> >
> >> > I would drop this paragraph. It is unnecessary and speculative at best about
> >> > what other code should or should not do.
> >>
> >> Hardly. It's documentation about how other code can get the arg types
> >> right if they are using dt_cg_tramp_remap_args(). Would you prefer I
> >> leave this entirely undescribed?! I could put it in a comment above
> >> dt_cg_tramp_remap_args() (or rather the renamed dt_cg_tramp_map_args())
> >> instead.
> >>
> >> (Shifted into a comment, for now.)
> >
> > But that is not relevant to this patch, and other code shouldn't be doing this
> > because it is almost certainly wrong that USDT is remapping the arguments in
> > the trampoline because of the argN vs args[N] thng discussed above.
> >
> > So, let's not add comments or commit message info that might point people to
> > do the wrong thing.
>
> It's correct *iff* you're using the in-trampoline remapping (the comment
> is above dt_cg_tramp_remap_args). Obviously if we end up removing that
> function, we'll remove the comment too :)
>
> >> Can we guarantee that trampoline() is only ever invoked once, and that
> >> it's invoked after probe_info is called? If it changes the remapping
> >> array, any subsequent invocation will do the wrong thing. If it's
> >> invoked before probe_info, it becomes impossible for probe_info to
> >> adjust the xlated types appropriately.
> >>
> >> This all seems like way too much coupling to me. At least done
> >> this way trampoline() is idempotent and doesn't depend on being called
> >> in the right (undocumented) order wrt probe_info.
> >
> > If the actual probe arguments (argN) are being reshuffled (which is what you
> > are doing by shuffling the arguments in the trampoline), then the mapping
> > needs to be updated to become an identity mapping i -> i).
>
> Yes, the code does exactly that, in probe_info.
>
> I don't know the relative ordering of the calls to probe_info() versus
> trampoline(), but when probe_info() is called the mapping *must* be
> fixed up, because it's probe_info()s caller that stashes it away -- so
> it's safest to set up that revised mapping there. If I set it up in
> trampoline(), I have no idea whether we'd reliably have the remapping
> worked out by the time probe_info() is called or not. Yes, this means
> there is an implied coupling between trampoline() calling
> dt_cg_tramp_remap_args() and between probe_info() having to change the
> mapping in the args -- but given that you've been suggesting similar
> coupling between parts of *dt_pid* and dt_prov_uprobe, I was assuming
> you'd find such a minor coupling (between, really, trampoline() and
> probe_info() in dt_prov_uprobe) uncontroversial.
>
> > Otherwise there
> > could be other code at some point that tries to apply the argument mapping
> > and that would be really bad.
>
> Well, yes. Is there anything anywhere that documents the order in which
> the dt_provimpl_t callbacks are called? They're invoked from all over
> the place and their call order is distinctly obscure, and deciding
> whether it's safe to play with DTrace-side arg mapping info from
> trampoline() is core to that. (As opposed to merely *generating code*
> that shuffles args. That can obviously be done at any time.)
>
> >> (Or are you suggesting that dt_cg_tramp_map_args(), or its trampoline()
> >> caller, shuffle the xargs too?! This seems even *further* from anything
> >> a trampoline() function should do to me.)
> >
> > Why would xargs ever be shuffled? There is nothing in DTrace that does that.
>
> Good. I thought you were suggesting it, and was rather confused.
>
> >> >> + * arguments are overwritten (but saved).
> >> >
> >> > Caller should save.
> >>
> >> As far as I can see this is not implementable without massive disruptive
> >> ugly (or at least pointlessly duplicative) changes, see above. It
> >> doesn't seem to buy us anything, either.
> >
> > How so? Just put the dt_cg_tramp_save_args() call in the trampoline right
> > before you call dt_cg_tramp_map_args(). How is that a massive disruptive ugly
> > change?
>
> Here's the code again:
>
> /*
> * Work from saved args so we don't need to worry about overwriting regs
> * we're about to remap.
> */
> dt_cg_tramp_save_args(pcb);
>
> for (i = 0; i < nmappings; i++) {
> if (mappings[i] != i) {
> emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(mappings[i])));
> emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
> }
> }
>
> i.e. it's *using* the save area. It relies on the args being saved in
> it. Either it does so itself, or it silently relies on callers doing it
> (! ew), or you're asking me to *not use* the save area but instead
> implement arg mapping in some other way (and this is what I thought you
> were asking for, because having dt_cg_tramp_map_args() not save the args
> but nonetheless rely on something else having saved them already is
> horrible, surely you don't mean that).
>
> At this point I have no idea if you want me to reimplement this in terms
> of something else, add *another* saved arg area just for the sake of arg
> mapping, implement some completely different sort of arg mapping
> algorithm in raw BPF (no thank you), or what.
>
> >> >> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(remappings[i])));
> >> >> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
> >> >
> >> > I would add:
> >> > argmap[i] = i;
> >> > to convert the mapping into a pure i -> i mapping, so no other code will apply
> >> > argument mapping again.
> >>
> >> I think not: see above.
> >
> > I think so, above also :)
>
> See above: I have no idea if this is even slightly safe, because I don't
> know the relative order in which trampoline and arg_info are called. I
> at least know what I'm doing now works.
>
> --
> NULL && (void)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/6] cg: add argument remapping in the trampoline
2024-10-25 19:15 ` Kris Van Hees
2024-10-28 21:11 ` Nick Alcock
@ 2024-10-29 13:09 ` Nick Alcock
2024-10-29 14:51 ` Kris Van Hees
1 sibling, 1 reply; 23+ messages in thread
From: Nick Alcock @ 2024-10-29 13:09 UTC (permalink / raw)
To: Kris Van Hees; +Cc: dtrace, dtrace-devel
On 25 Oct 2024, Kris Van Hees said:
> On Fri, Oct 25, 2024 at 04:56:26PM +0100, Nick Alcock wrote:
>> > However, I think it might be a good idea to have this function actually rewrite
>> > the mapping data to become a pure i -> i mapping.
>>
>> Can we guarantee that trampoline() is only ever invoked once, and that
>> it's invoked after probe_info is called? If it changes the remapping
>> array, any subsequent invocation will do the wrong thing. If it's
>> invoked before probe_info, it becomes impossible for probe_info to
>> adjust the xlated types appropriately.
>>
>> This all seems like way too much coupling to me. At least done
>> this way trampoline() is idempotent and doesn't depend on being called
>> in the right (undocumented) order wrt probe_info.
>
> If the actual probe arguments (argN) are being reshuffled (which is what you
> are doing by shuffling the arguments in the trampoline), then the mapping
> needs to be updated to become an identity mapping i -> i). Otherwise there
> could be other code at some point that tries to apply the argument mapping
> and that would be really bad.
OK so I tried to implement this and it all fell over as soon as I
thought about it in depth. The problem is -- and you might not have
realised this because it was different when I started and I had to move
things around before anything would work -- trampoline() and
probe_info() are functions defined on *different probes*. trampoline()
is called on the underlying probes, and emits code for them that
reshuffles the args according to the mapping info (while ignoring the
types entirely); but probe_info() is called on the *overlying* probes.
It digs down to *one* underlying probe to get the dt_uprobe_t, but it
has no idea which one (if there is more than one): it's basically
arbitrary.
So we cannot possibly have the trampoline() for the underlying probe go
changing the dt_uprobe_t's mapping unless we can be absolutely sure that
the trampoline() for all the underlying probes is called before the
probe_info() for *any* of the overlying probes. Are we really sorting
things like that? I thought this was a TODO, if that...
I tried storing the arg info in dt_argdesc_t structures in the
dt_uprobe_t too and it rapidly turned into *another* nightmare, because
now instead of one allocation for all the args at once we have one
allocation per arg, plus up to two more for its arg type strings -- but
we can't just hand the argdesc back to probe_info()s caller wholesale
because it frees it after it's done with it! So we'd actually have to
loop through them and copy the whole thing (including all the arg
strings) and then do the *same thing* in the underlying probe freeing
function, and all this complexity buys us what? The ability to save four
lines in probe_info() going through the nargv so we can index it like an
array (which we'd have to do the nargc *and* xargc in your scheme).
Sorry, this seems again to have zero benefits and a great deal of extra
complexity.
--
NULL && (void)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/6] cg: add argument remapping in the trampoline
2024-10-29 13:09 ` Nick Alcock
@ 2024-10-29 14:51 ` Kris Van Hees
0 siblings, 0 replies; 23+ messages in thread
From: Kris Van Hees @ 2024-10-29 14:51 UTC (permalink / raw)
To: Nick Alcock; +Cc: Kris Van Hees, dtrace, dtrace-devel
On Tue, Oct 29, 2024 at 01:09:08PM +0000, Nick Alcock wrote:
> On 25 Oct 2024, Kris Van Hees said:
>
> > On Fri, Oct 25, 2024 at 04:56:26PM +0100, Nick Alcock wrote:
> >> > However, I think it might be a good idea to have this function actually rewrite
> >> > the mapping data to become a pure i -> i mapping.
> >>
> >> Can we guarantee that trampoline() is only ever invoked once, and that
> >> it's invoked after probe_info is called? If it changes the remapping
> >> array, any subsequent invocation will do the wrong thing. If it's
> >> invoked before probe_info, it becomes impossible for probe_info to
> >> adjust the xlated types appropriately.
> >>
> >> This all seems like way too much coupling to me. At least done
> >> this way trampoline() is idempotent and doesn't depend on being called
> >> in the right (undocumented) order wrt probe_info.
> >
> > If the actual probe arguments (argN) are being reshuffled (which is what you
> > are doing by shuffling the arguments in the trampoline), then the mapping
> > needs to be updated to become an identity mapping i -> i). Otherwise there
> > could be other code at some point that tries to apply the argument mapping
> > and that would be really bad.
>
> OK so I tried to implement this and it all fell over as soon as I
> thought about it in depth. The problem is -- and you might not have
> realised this because it was different when I started and I had to move
> things around before anything would work -- trampoline() and
> probe_info() are functions defined on *different probes*. trampoline()
> is called on the underlying probes, and emits code for them that
> reshuffles the args according to the mapping info (while ignoring the
> types entirely); but probe_info() is called on the *overlying* probes.
> It digs down to *one* underlying probe to get the dt_uprobe_t, but it
> has no idea which one (if there is more than one): it's basically
> arbitrary.
Yes, that is correct. It is also not really important in the sense that by
their very nature the underlying probes *must* be identical in terms of the
argument data. After all, the argument data is defined at the level of the
meta-probe (the probe as defined in the provider declaration that gets
processed with dtrace -G at build tie to generate the DOF). So it is always
associated with all the tracepoints for that probe (and each tracepoint has
its own uprobe and thus also its own underlying probe).
None of this matters though in terms of how the argument data is stored. If
you can store it in raw format, you can also store it in dt_argdesc_t format.
> So we cannot possibly have the trampoline() for the underlying probe go
> changing the dt_uprobe_t's mapping unless we can be absolutely sure that
> the trampoline() for all the underlying probes is called before the
> probe_info() for *any* of the overlying probes. Are we really sorting
> things like that? I thought this was a TODO, if that...
Changing how you store the data does not change the logic you already have in
place to handling the mapping of arguments, etc. It is just a change in
how things are stored - not a change in behaviour.
> I tried storing the arg info in dt_argdesc_t structures in the
> dt_uprobe_t too and it rapidly turned into *another* nightmare, because
> now instead of one allocation for all the args at once we have one
> allocation per arg, plus up to two more for its arg type strings -- but
> we can't just hand the argdesc back to probe_info()s caller wholesale
> because it frees it after it's done with it! So we'd actually have to
> loop through them and copy the whole thing (including all the arg
> strings) and then do the *same thing* in the underlying probe freeing
> function, and all this complexity buys us what? The ability to save four
> lines in probe_info() going through the nargv so we can index it like an
> array (which we'd have to do the nargc *and* xargc in your scheme).
Really? Your current patch stores the argument data in dt_uprobe_t in
create_underlying() using 3 allocations each with a memcpy. The alternative
could use a single dt_calloc(dtp, psp->pps_xargc, sizeof(dt_argdesc_t)) and
a loop to populate the elements. It would have to do 2 strdups per argument
or you could be fancy and copy all type strings as a single memcpy() into the
first argument and store pointers into that for the rest for the other args.
As long as we have our own code to free all that, such an opitmization can
certainly be used.
The loop to find the strings in the combined memory blobs therefore gets done
here (once) instead of in probe_info (every time that gets called).
You already need to allocate an dt_argdesc_t array and populate it within
probe_info because that is what the caller expects, so the work to do that
does not change, other than that you are making a deep copy of the dt_argdesc_t
array already available from the underlying probe rather than needing to
construct one from raw data for every call to probe_info.
> Sorry, this seems again to have zero benefits and a great deal of extra
> complexity.
Pretty much the same complexity, and you gain the benefit of actually storing
the argument data in the structure format that exists for that very reason.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/6] usdt: typed args and arg remapping
2024-10-18 19:58 [PATCH 0/6] usdt typed args, translators and arg remapping Nick Alcock
` (3 preceding siblings ...)
2024-10-18 19:58 ` [PATCH 4/6] cg: add argument remapping in the trampoline Nick Alcock
@ 2024-10-18 19:58 ` Nick Alcock
2024-10-22 18:40 ` Kris Van Hees
2024-10-18 19:58 ` [PATCH 6/6] usdt: fix create_underlying error path Nick Alcock
5 siblings, 1 reply; 23+ messages in thread
From: Nick Alcock @ 2024-10-18 19:58 UTC (permalink / raw)
To: dtrace, dtrace-devel
This change propagates the arg type and mapping info we just got into the
pid_probespec_t onward into the underlying dt_uprobe_t, shuffles the native
types before handing off a 1:1 mapping to the dt_probe_t, and calls
dt_cg_tramp_remap_args() to apply the requested remappings. With this, USDT
probes should now have visible types which obey the :-notation translators
specified in their .d fils (this is now tested).
The probe info has been moved from the underlying probe to the overlying one
as part of this: underlying probes don't have any user-visible args to
get info for, and their probe_info functions are never called.
We make several assumptions for simplicity, all noted by TODOs in the
source:
- overlying probes all have the same native types and remappings, so we
don't need to look at more than one underlying probe to figure out what
they are. Given the structure of DOF it seems to me this must always be
true, and even a malicious attacker could not produce DOF which said
otherwise.
- any given underlying probe locus is either a PID or a USDT probe, never
both, so we can just remap the args at the start of the trampoline and
then never touch them again. This is obviously wrong, but the both-at
the-same-locus case is not yet implemented, and significant changes need
to be made to create_underlying() to fix this. Right now we can't easily
even tell if a given locus has both types of probe on it... fixing this
is literally a matter of shuffling the pid probes after the usdt probes
and then doing a single dt_cg_tramp_restore_args before all the pid
probes fire.
PID args are already broken before this commit (something to do with the
wildcard USDT work?) so I cannot test if this commit broke them further,
but it shouldn't have.
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
libdtrace/dt_prov_uprobe.c | 163 +++++++++++++++++-
test/triggers/usdt-tst-argmap-prov.d | 3 +-
test/triggers/usdt-tst-argmap.c | 3 +-
.../dtrace-util/tst.ListProbesArgsUSDT.r | 34 ++++
.../dtrace-util/tst.ListProbesArgsUSDT.r.p | 2 +
.../dtrace-util/tst.ListProbesArgsUSDT.sh | 83 +++++++++
test/unittest/usdt/tst.argmap-typed.d | 40 +++++
test/unittest/usdt/tst.argmap.d | 3 +-
8 files changed, 320 insertions(+), 11 deletions(-)
create mode 100644 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
create mode 100755 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
create mode 100755 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
create mode 100644 test/unittest/usdt/tst.argmap-typed.d
diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
index 481159b01c24..767d9a80b5e1 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -55,7 +55,13 @@ typedef struct dt_uprobe {
uint64_t off;
int flags;
tp_probe_t *tp;
- dt_list_t probes; /* pid/USDT probes triggered by it */
+ int nargc; /* number of native args */
+ int xargc; /* number of xlated args */
+ int remapargc; /* number of remapped args */
+ char *nargv; /* array of native args */
+ char *xargv; /* array of xlated args */
+ int8_t *remapargs; /* remapped arg indexes */
+ dt_list_t probes; /* pid/USDT probes triggered by it */
} dt_uprobe_t;
typedef struct list_probe {
@@ -112,6 +118,9 @@ static void probe_destroy_underlying(dtrace_hdl_t *dtp, void *datap)
dt_tp_destroy(dtp, tpp);
free_probe_list(dtp, dt_list_next(&upp->probes));
dt_free(dtp, upp->fn);
+ dt_free(dtp, upp->nargv);
+ dt_free(dtp, upp->xargv);
+ dt_free(dtp, upp->remapargs);
dt_free(dtp, upp);
}
@@ -138,7 +147,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
char prb[DTRACE_NAMELEN];
dtrace_probedesc_t pd;
dt_probe_t *uprp;
- dt_uprobe_t *upp;
+ dt_uprobe_t *upp = NULL;
int is_enabled = 0;
/*
@@ -201,7 +210,48 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
if (upp->tp == NULL)
goto fail;
- uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
+ /*
+ * Arg info. This really relates to the overlying probe (all
+ * underlying probes associated with a given underlying probe
+ * must have the same args), but the overlying probe doesn't
+ * exist at this point so we must put it here and pull it out
+ * when the overlying probe info is called for.
+ *
+ * TODO: is this true? can distinct underlying probes have
+ * different native args, as long as their xlated types are the
+ * same?
+ */
+
+ if (psp->pps_nargv) {
+ upp->nargc = psp->pps_nargc;
+ upp->nargv = dt_alloc(dtp, psp->pps_nargvlen);
+ if(upp->nargv == NULL)
+ goto fail;
+ memcpy(upp->nargv, psp->pps_nargv, psp->pps_nargvlen);
+ }
+
+ if (psp->pps_xargv) {
+ upp->xargc = psp->pps_xargc;
+ upp->xargv = dt_alloc(dtp, psp->pps_xargvlen);
+ if(upp->xargv == NULL)
+ goto fail;
+ memcpy(upp->xargv, psp->pps_xargv, psp->pps_xargvlen);
+ }
+
+ if (psp->pps_remapargs) {
+ upp->remapargc = psp->pps_remapargc;
+ upp->remapargs = dt_alloc(dtp, psp->pps_remapargslen);
+ if(upp->remapargs == NULL)
+ goto fail;
+ memcpy(upp->remapargs, psp->pps_remapargs, psp->pps_remapargslen);
+ }
+
+ /*
+ * TODO: validate that all underlying probes for a given
+ * overlying probe have the same args.
+ */
+
+ uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
upp);
if (uprp == NULL)
goto fail;
@@ -413,6 +463,9 @@ static void enable_usdt(dtrace_hdl_t *dtp, dt_probe_t *prp)
*
* The trampoline will first populate a dt_dctx_t struct. It will then emulate
* the firing of all dependent pid* probes and their clauses.
+ *
+ * uprobe trampolines will also arrange for argument shuffling if the remapping
+ * array calls for it.
*/
static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
{
@@ -437,6 +490,34 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
dt_cg_tramp_copy_args_from_regs(pcb,
!(upp->flags & PP_IS_FUNCALL));
+ /*
+ * Figure out if we need to remap args, and remap them if needed.
+ */
+ if (upp->remapargc) {
+ int needs_remapping = 0;
+ size_t i;
+
+ for (i = 0; i < upp->remapargc; i++) {
+ if (upp->remapargs[i] != i) {
+ needs_remapping = 1;
+ break;
+ }
+ }
+
+ /*
+ * TODO: pid probes are never remapped, so a remapped set of
+ * args for UDST probes need saving/restoring to reverse the
+ * remapping for pid probes at the same locus -- but right now
+ * pid and usdt probes hitting the same locus simply end up as
+ * whichever registered first because create_underlying cannot
+ * handle this case, so we can avoid it here too.
+ * dt_cg_tramp_remap_args already saves regs: all we need to do
+ * is restore them before any pid probe is hit.
+ */
+ if (needs_remapping)
+ dt_cg_tramp_remap_args(pcb, upp->remapargs, upp->remapargc);
+ }
+
/*
* Retrieve the PID of the process that caused the probe to fire.
*/
@@ -710,8 +791,77 @@ attach_bpf:
static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
int *argcp, dt_argdesc_t **argvp)
{
- *argcp = 0; /* no known arguments */
- *argvp = NULL;
+ size_t i;
+ list_probe_t *pup = prp->prv_data;
+ dt_uprobe_t *upp;
+ char *nptr;
+ char *xptr;
+ int argc = 0;
+ dt_argdesc_t *argv = NULL;
+ char **nargv = NULL;
+
+ /* No underlying probes? No args. */
+ if (!pup)
+ goto done;
+
+ upp = pup->probe->prv_data;
+ nptr = upp->nargv;
+ xptr = upp->xargv;
+
+ argc = upp->xargc;
+ if (argc == 0)
+ argc = upp->nargc;
+ if (argc == 0)
+ goto done;
+ if (!upp->nargv || upp->nargc == 0)
+ goto done;
+
+ /*
+ * Construct an array to allow accessing native args by index, if we
+ * need to (which we do whenever we have remapped args). We don't need
+ * to worry that any of the indexes might be too large, etc: this has
+ * been checked already by validate_provider() before parsing and
+ * writing to the DOF stash, and anyone who could inject invalid records
+ * into the DOF stash is already root.
+ *
+ * Shuffle the native args according to the mapping, then set up a 1:1
+ * mapping in its place: we do the remapping ourselves, driven directly
+ * from upp->remapargs, in the trampoline, so array ops etc should not
+ * do it.
+ */
+ if (upp->xargv || upp->remapargs) {
+ if ((nargv = dt_zalloc(dtp, upp->nargc * sizeof(char *))) == NULL)
+ return dt_set_errno(dtp, ENOMEM);
+
+ for (i = 0; i < upp->nargc; i++, nptr += strlen(nptr) + 1)
+ nargv[i] = nptr;
+ }
+
+ argv = dt_zalloc(dtp, argc * sizeof(dt_argdesc_t));
+ if (argv == NULL) {
+ dt_free(dtp, nargv);
+ return dt_set_errno(dtp, EDT_NOMEM);
+ }
+
+ for (i = 0; i < argc; i++) {
+ int remap_arg = i;
+
+ if (upp->remapargc > i)
+ remap_arg = upp->remapargs[i];
+
+ if (xptr) {
+ argv[i].native = strdup(nargv[remap_arg]);
+ argv[i].xlate = strdup(xptr);
+ }
+ argv[i].mapping = i;
+
+ /* Guaranteed safe: see libcommon/dof_parser.c:strings_len() */
+ if (xptr)
+ xptr += strlen(xptr) + 1;
+ }
+done:
+ *argcp = argc;
+ *argvp = argv;
return 0;
}
@@ -779,7 +929,6 @@ dt_provimpl_t dt_uprobe = {
.load_prog = &dt_bpf_prog_load,
.trampoline = &trampoline,
.attach = &attach,
- .probe_info = &probe_info,
.detach = &detach,
.probe_destroy = &probe_destroy_underlying,
};
@@ -794,7 +943,6 @@ dt_provimpl_t dt_uprobe_is_enabled = {
.load_prog = &dt_bpf_prog_load,
.trampoline = &trampoline_is_enabled,
.attach = &attach,
- .probe_info = &probe_info,
.detach = &detach,
.probe_destroy = &probe_destroy_underlying,
};
@@ -818,5 +966,6 @@ dt_provimpl_t dt_usdt = {
.prog_type = BPF_PROG_TYPE_UNSPEC,
.provide_probe = &provide_usdt_probe,
.enable = &enable_usdt,
+ .probe_info = &probe_info,
.probe_destroy = &probe_destroy,
};
diff --git a/test/triggers/usdt-tst-argmap-prov.d b/test/triggers/usdt-tst-argmap-prov.d
index 28134baa6fa3..7cec525c908a 100644
--- a/test/triggers/usdt-tst-argmap-prov.d
+++ b/test/triggers/usdt-tst-argmap-prov.d
@@ -1,10 +1,11 @@
/*
* Oracle Linux DTrace.
- * Copyright (c) 2006, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2006, 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.
*/
provider test_prov {
probe place(int i, int j) : (int j, int i, int i, int j);
+ probe place2(int i, char *j) : (char *j, int i, int i, char *j);
};
diff --git a/test/triggers/usdt-tst-argmap.c b/test/triggers/usdt-tst-argmap.c
index 89a0a53fc1d5..cbdcaf1f0b61 100644
--- a/test/triggers/usdt-tst-argmap.c
+++ b/test/triggers/usdt-tst-argmap.c
@@ -1,6 +1,6 @@
/*
* Oracle Linux DTrace.
- * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2006, 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.
*/
@@ -12,6 +12,7 @@ main(int argc, char **argv)
{
for (;;) {
DTRACE_PROBE2(test_prov, place, 10, 4);
+ DTRACE_PROBE(test_prov, place2, 255, "foo");
}
return 0;
diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
new file mode 100644
index 000000000000..8bcfa92ff6b0
--- /dev/null
+++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
@@ -0,0 +1,34 @@
+ ID PROVIDER MODULE FUNCTION NAME
+ XX test_provXXXX test main go
+
+ Probe Description Attributes
+ Identifier Names: Private
+ Data Semantics: Private
+ Dependency Class: Unknown
+
+ Argument Attributes
+ Identifier Names: Private
+ Data Semantics: Private
+ Dependency Class: Unknown
+
+ Argument Types
+ args[0]: char *
+ args[1]: int
+
+ ID PROVIDER MODULE FUNCTION NAME
+ XX test_provXXXX test main go
+
+ Probe Description Attributes
+ Identifier Names: Private
+ Data Semantics: Private
+ Dependency Class: Unknown
+
+ Argument Attributes
+ Identifier Names: Private
+ Data Semantics: Private
+ Dependency Class: Unknown
+
+ Argument Types
+ args[0]: char *
+ args[1]: int
+
diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
new file mode 100755
index 000000000000..c575983adf65
--- /dev/null
+++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
@@ -0,0 +1,2 @@
+#!/bin/sed -rf
+s,test_prov[0-9]*,test_provXXXX,g; s,^ *[0-9]+, XX,g;
diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
new file mode 100755
index 000000000000..ad8dc2e7c267
--- /dev/null
+++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
@@ -0,0 +1,83 @@
+#!/bin/bash
+#
+# Oracle Linux DTrace.
+# Copyright (c) 2013, 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.
+
+##
+#
+# ASSERTION:
+# Testing -lvn option with USDT probes with a valid probe name.
+#
+# SECTION: dtrace Utility/-ln Option
+#
+##
+
+if [ $# != 1 ]; then
+ echo expected one argument: '<'dtrace-path'>'
+ exit 2
+fi
+
+dtrace=$1
+CC=/usr/bin/gcc
+CFLAGS=
+
+DIRNAME="$tmpdir/list-probes-args-usdt.$$.$RANDOM"
+mkdir -p $DIRNAME
+cd $DIRNAME
+
+cat > prov.d <<EOF
+provider test_prov {
+ probe go(int a, char *b) : (char *b, int a);
+};
+EOF
+
+$dtrace -h -s prov.d
+if [ $? -ne 0 ]; then
+ echo "failed to generate header file" >& 2
+ exit 1
+fi
+
+cat > test.c <<EOF
+#include <sys/types.h>
+#include "prov.h"
+
+int
+main(int argc, char **argv)
+{
+ TEST_PROV_GO(666, "foo");
+ sleep(10);
+}
+EOF
+
+${CC} ${CFLAGS} -c test.c
+if [ $? -ne 0 ]; then
+ echo "failed to compile test.c" >& 2
+ exit 1
+fi
+$dtrace -G -s prov.d test.o
+if [ $? -ne 0 ]; then
+ echo "failed to create DOF" >& 2
+ exit 1
+fi
+${CC} ${CFLAGS} -o test test.o prov.o
+if [ $? -ne 0 ]; then
+ echo "failed to link final executable" >& 2
+ exit 1
+fi
+
+script()
+{
+ $dtrace -c ./test -lvn 'test_prov$target:::go'
+ ./test &
+ PID=$!
+ disown %+
+ $dtrace -p $PID -lvn 'test_prov$target:::go'
+ kill -9 $PID
+}
+
+script
+status=$?
+
+exit $status
diff --git a/test/unittest/usdt/tst.argmap-typed.d b/test/unittest/usdt/tst.argmap-typed.d
new file mode 100644
index 000000000000..0e0c923e1736
--- /dev/null
+++ b/test/unittest/usdt/tst.argmap-typed.d
@@ -0,0 +1,40 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2007, 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.
+ */
+
+/* @@trigger: usdt-tst-argmap */
+/* @@trigger-timing: before */
+/* @@runtest-opts: $_pid */
+
+/*
+ * ASSERTION: Verify that args[N] variables are properly typed when remapped.
+ */
+
+BEGIN
+{
+ /* Timeout after 5 seconds */
+ timeout = timestamp + 5000000000;
+}
+
+test_prov$1:::place2
+/stringof(args[0]) != "foo" || args[1] != 255 || args[2] != 255 || stringof(args[3]) != "foo"/
+{
+ printf("args are %s, %d, %d, %s; should be \"foo\", 255, 255, \"foo\"",
+ stringof(args[0]), args[1], args[2], stringof(args[3]));
+ exit(1);
+}
+
+test_prov$1:::place2
+{
+ exit(0);
+}
+
+profile:::tick-1
+/timestamp > timeout/
+{
+ trace("test timed out");
+ exit(1);
+}
diff --git a/test/unittest/usdt/tst.argmap.d b/test/unittest/usdt/tst.argmap.d
index 7896dc07e0e2..9cd3425bf8e0 100644
--- a/test/unittest/usdt/tst.argmap.d
+++ b/test/unittest/usdt/tst.argmap.d
@@ -1,11 +1,10 @@
/*
* Oracle Linux DTrace.
- * Copyright (c) 2007, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2007, 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.
*/
-/* @@xfail: dtv2 */
/* @@trigger: usdt-tst-argmap */
/* @@trigger-timing: before */
/* @@runtest-opts: $_pid */
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 5/6] usdt: typed args and arg remapping
2024-10-18 19:58 ` [PATCH 5/6] usdt: typed args and arg remapping Nick Alcock
@ 2024-10-22 18:40 ` Kris Van Hees
2024-10-28 12:52 ` Nick Alcock
0 siblings, 1 reply; 23+ messages in thread
From: Kris Van Hees @ 2024-10-22 18:40 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
Standard comments on remapping vs mapping apply :)
On Fri, Oct 18, 2024 at 08:58:07PM +0100, Nick Alcock wrote:
> This change propagates the arg type and mapping info we just got into the
> pid_probespec_t onward into the underlying dt_uprobe_t, shuffles the native
> types before handing off a 1:1 mapping to the dt_probe_t, and calls
> dt_cg_tramp_remap_args() to apply the requested remappings. With this, USDT
> probes should now have visible types which obey the :-notation translators
> specified in their .d fils (this is now tested).
>
> The probe info has been moved from the underlying probe to the overlying one
> as part of this: underlying probes don't have any user-visible args to
> get info for, and their probe_info functions are never called.
>
> We make several assumptions for simplicity, all noted by TODOs in the
> source:
>
> - overlying probes all have the same native types and remappings, so we
> don't need to look at more than one underlying probe to figure out what
> they are. Given the structure of DOF it seems to me this must always be
> true, and even a malicious attacker could not produce DOF which said
> otherwise.
You cannot place more than one USDT at a particular tracepoint (underlying
probe) so it is guaranteed that all overlying probes can only differ in terms
of their provider name. The USDT design in DTrace encapsulates the USDT probes
(and providers) as instances of a meta-provider that acts as a template for its
instances. So, all instances contain the same probes, together with all their
data (arg data, tracepoints, etc). So, this is a guarantee and need not be
mentioned as an assumption or TODO.
> - any given underlying probe locus is either a PID or a USDT probe, never
> both, so we can just remap the args at the start of the trampoline and
> then never touch them again. This is obviously wrong, but the both-at
> the-same-locus case is not yet implemented, and significant changes need
> to be made to create_underlying() to fix this. Right now we can't easily
> even tell if a given locus has both types of probe on it... fixing this
> is literally a matter of shuffling the pid probes after the usdt probes
> and then doing a single dt_cg_tramp_restore_args before all the pid
> probes fire.
I would rewrite this to point out that PID and USDT probes being at the same
locus is not implemented yet, and that this implementation assumes that either
all probes at a particular locus are PID or that they are all USDT.
I would avoid speculative statements about how this might be fixed in the
future (however near that may be) because there may still be design decisions
to be made about that and there is no need to bring it up here.
> PID args are already broken before this commit (something to do with the
> wildcard USDT work?) so I cannot test if this commit broke them further,
> but it shouldn't have.
This does not belong here but rather in a bug report.
In addition, I think it would be good to add a few more test cases to cover
edge cases:
- A probe that has native arguments, but no translated arguments.
E.g.
probe place3(int i, char *j) : ();
- A probe that has native arguments that do not get used.
E.g.
probe place4(int i, char *j) : (char *j);
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> ---
> libdtrace/dt_prov_uprobe.c | 163 +++++++++++++++++-
> test/triggers/usdt-tst-argmap-prov.d | 3 +-
> test/triggers/usdt-tst-argmap.c | 3 +-
> .../dtrace-util/tst.ListProbesArgsUSDT.r | 34 ++++
> .../dtrace-util/tst.ListProbesArgsUSDT.r.p | 2 +
> .../dtrace-util/tst.ListProbesArgsUSDT.sh | 83 +++++++++
> test/unittest/usdt/tst.argmap-typed.d | 40 +++++
> test/unittest/usdt/tst.argmap.d | 3 +-
> 8 files changed, 320 insertions(+), 11 deletions(-)
> create mode 100644 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
> create mode 100755 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
> create mode 100755 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
> create mode 100644 test/unittest/usdt/tst.argmap-typed.d
>
> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index 481159b01c24..767d9a80b5e1 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -55,7 +55,13 @@ typedef struct dt_uprobe {
> uint64_t off;
> int flags;
> tp_probe_t *tp;
> - dt_list_t probes; /* pid/USDT probes triggered by it */
> + int nargc; /* number of native args */
> + int xargc; /* number of xlated args */
> + int remapargc; /* number of remapped args */
> + char *nargv; /* array of native args */
> + char *xargv; /* array of xlated args */
> + int8_t *remapargs; /* remapped arg indexes */
Since all overlying USDT probes associated with a given underlying probe
(uprobe) have the sane argument type data (native, translated, and mapping),
and PID probes do not have arguments by definition, you can certainly store
the data here. But why in this raw format rather than a dt_argdesc_t array
(and an argc member to indicate the number of array elements - xargc).
Calls to the probe_info() callback will want an array of dt_argdesc_t elements
to be provided, and nothing else needs the rarw format. So, you might as well
store that array here rather than the raw data, and implement the probe_info()
callback for USDT as a pass-through that simply takes the data from here (from
the underlying probe), copies it (using strdup on the strings because the
caller will free them), and assigned it to *argv.
For PID probes, there is no probe_info() callback so they will report not
having arguments, and therefore the data being stored at the uprobe level is
still fine.
> + dt_list_t probes; /* pid/USDT probes triggered by it */
> } dt_uprobe_t;
>
> typedef struct list_probe {
> @@ -112,6 +118,9 @@ static void probe_destroy_underlying(dtrace_hdl_t *dtp, void *datap)
> dt_tp_destroy(dtp, tpp);
> free_probe_list(dtp, dt_list_next(&upp->probes));
> dt_free(dtp, upp->fn);
> + dt_free(dtp, upp->nargv);
> + dt_free(dtp, upp->xargv);
> + dt_free(dtp, upp->remapargs);
> dt_free(dtp, upp);
> }
>
> @@ -138,7 +147,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> char prb[DTRACE_NAMELEN];
> dtrace_probedesc_t pd;
> dt_probe_t *uprp;
> - dt_uprobe_t *upp;
> + dt_uprobe_t *upp = NULL;
> int is_enabled = 0;
>
> /*
> @@ -201,7 +210,48 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> if (upp->tp == NULL)
> goto fail;
>
> - uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
> + /*
> + * Arg info. This really relates to the overlying probe (all
> + * underlying probes associated with a given underlying probe
> + * must have the same args), but the overlying probe doesn't
> + * exist at this point so we must put it here and pull it out
> + * when the overlying probe info is called for.
> + *
> + * TODO: is this true? can distinct underlying probes have
> + * different native args, as long as their xlated types are the
> + * same?
> + */
> +
> + if (psp->pps_nargv) {
> + upp->nargc = psp->pps_nargc;
> + upp->nargv = dt_alloc(dtp, psp->pps_nargvlen);
> + if(upp->nargv == NULL)
> + goto fail;
> + memcpy(upp->nargv, psp->pps_nargv, psp->pps_nargvlen);
> + }
> +
> + if (psp->pps_xargv) {
> + upp->xargc = psp->pps_xargc;
> + upp->xargv = dt_alloc(dtp, psp->pps_xargvlen);
> + if(upp->xargv == NULL)
> + goto fail;
> + memcpy(upp->xargv, psp->pps_xargv, psp->pps_xargvlen);
> + }
> +
> + if (psp->pps_remapargs) {
> + upp->remapargc = psp->pps_remapargc;
> + upp->remapargs = dt_alloc(dtp, psp->pps_remapargslen);
> + if(upp->remapargs == NULL)
> + goto fail;
> + memcpy(upp->remapargs, psp->pps_remapargs, psp->pps_remapargslen);
> + }
> +
> + /*
> + * TODO: validate that all underlying probes for a given
> + * overlying probe have the same args.
> + */
> +
> + uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
> upp);
> if (uprp == NULL)
> goto fail;
> @@ -413,6 +463,9 @@ static void enable_usdt(dtrace_hdl_t *dtp, dt_probe_t *prp)
> *
> * The trampoline will first populate a dt_dctx_t struct. It will then emulate
> * the firing of all dependent pid* probes and their clauses.
> + *
> + * uprobe trampolines will also arrange for argument shuffling if the remapping
> + * array calls for it.
> */
> static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> {
> @@ -437,6 +490,34 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> dt_cg_tramp_copy_args_from_regs(pcb,
> !(upp->flags & PP_IS_FUNCALL));
>
> + /*
> + * Figure out if we need to remap args, and remap them if needed.
> + */
> + if (upp->remapargc) {
> + int needs_remapping = 0;
> + size_t i;
> +
> + for (i = 0; i < upp->remapargc; i++) {
> + if (upp->remapargs[i] != i) {
> + needs_remapping = 1;
> + break;
> + }
> + }
> +
> + /*
> + * TODO: pid probes are never remapped, so a remapped set of
> + * args for UDST probes need saving/restoring to reverse the
> + * remapping for pid probes at the same locus -- but right now
> + * pid and usdt probes hitting the same locus simply end up as
> + * whichever registered first because create_underlying cannot
> + * handle this case, so we can avoid it here too.
> + * dt_cg_tramp_remap_args already saves regs: all we need to do
> + * is restore them before any pid probe is hit.
> + */
> + if (needs_remapping)
> + dt_cg_tramp_remap_args(pcb, upp->remapargs, upp->remapargc);
> + }
> +
> /*
> * Retrieve the PID of the process that caused the probe to fire.
> */
> @@ -710,8 +791,77 @@ attach_bpf:
> static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> int *argcp, dt_argdesc_t **argvp)
> {
> - *argcp = 0; /* no known arguments */
> - *argvp = NULL;
> + size_t i;
> + list_probe_t *pup = prp->prv_data;
> + dt_uprobe_t *upp;
> + char *nptr;
> + char *xptr;
> + int argc = 0;
> + dt_argdesc_t *argv = NULL;
> + char **nargv = NULL;
> +
> + /* No underlying probes? No args. */
> + if (!pup)
> + goto done;
> +
> + upp = pup->probe->prv_data;
> + nptr = upp->nargv;
> + xptr = upp->xargv;
> +
> + argc = upp->xargc;
> + if (argc == 0)
> + argc = upp->nargc;
> + if (argc == 0)
> + goto done;
> + if (!upp->nargv || upp->nargc == 0)
> + goto done;
> +
> + /*
> + * Construct an array to allow accessing native args by index, if we
> + * need to (which we do whenever we have remapped args). We don't need
> + * to worry that any of the indexes might be too large, etc: this has
> + * been checked already by validate_provider() before parsing and
> + * writing to the DOF stash, and anyone who could inject invalid records
> + * into the DOF stash is already root.
> + *
> + * Shuffle the native args according to the mapping, then set up a 1:1
> + * mapping in its place: we do the remapping ourselves, driven directly
> + * from upp->remapargs, in the trampoline, so array ops etc should not
> + * do it.
> + */
> + if (upp->xargv || upp->remapargs) {
> + if ((nargv = dt_zalloc(dtp, upp->nargc * sizeof(char *))) == NULL)
> + return dt_set_errno(dtp, ENOMEM);
> +
> + for (i = 0; i < upp->nargc; i++, nptr += strlen(nptr) + 1)
> + nargv[i] = nptr;
> + }
> +
> + argv = dt_zalloc(dtp, argc * sizeof(dt_argdesc_t));
> + if (argv == NULL) {
> + dt_free(dtp, nargv);
> + return dt_set_errno(dtp, EDT_NOMEM);
> + }
> +
> + for (i = 0; i < argc; i++) {
> + int remap_arg = i;
> +
> + if (upp->remapargc > i)
> + remap_arg = upp->remapargs[i];
> +
> + if (xptr) {
> + argv[i].native = strdup(nargv[remap_arg]);
> + argv[i].xlate = strdup(xptr);
> + }
> + argv[i].mapping = i;
> +
> + /* Guaranteed safe: see libcommon/dof_parser.c:strings_len() */
> + if (xptr)
> + xptr += strlen(xptr) + 1;
> + }
> +done:
> + *argcp = argc;
> + *argvp = argv;
>
> return 0;
> }
> @@ -779,7 +929,6 @@ dt_provimpl_t dt_uprobe = {
> .load_prog = &dt_bpf_prog_load,
> .trampoline = &trampoline,
> .attach = &attach,
> - .probe_info = &probe_info,
> .detach = &detach,
> .probe_destroy = &probe_destroy_underlying,
> };
> @@ -794,7 +943,6 @@ dt_provimpl_t dt_uprobe_is_enabled = {
> .load_prog = &dt_bpf_prog_load,
> .trampoline = &trampoline_is_enabled,
> .attach = &attach,
> - .probe_info = &probe_info,
> .detach = &detach,
> .probe_destroy = &probe_destroy_underlying,
> };
> @@ -818,5 +966,6 @@ dt_provimpl_t dt_usdt = {
> .prog_type = BPF_PROG_TYPE_UNSPEC,
> .provide_probe = &provide_usdt_probe,
> .enable = &enable_usdt,
> + .probe_info = &probe_info,
> .probe_destroy = &probe_destroy,
> };
> diff --git a/test/triggers/usdt-tst-argmap-prov.d b/test/triggers/usdt-tst-argmap-prov.d
> index 28134baa6fa3..7cec525c908a 100644
> --- a/test/triggers/usdt-tst-argmap-prov.d
> +++ b/test/triggers/usdt-tst-argmap-prov.d
> @@ -1,10 +1,11 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 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.
> */
>
> provider test_prov {
> probe place(int i, int j) : (int j, int i, int i, int j);
> + probe place2(int i, char *j) : (char *j, int i, int i, char *j);
> };
> diff --git a/test/triggers/usdt-tst-argmap.c b/test/triggers/usdt-tst-argmap.c
> index 89a0a53fc1d5..cbdcaf1f0b61 100644
> --- a/test/triggers/usdt-tst-argmap.c
> +++ b/test/triggers/usdt-tst-argmap.c
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 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.
> */
> @@ -12,6 +12,7 @@ main(int argc, char **argv)
> {
> for (;;) {
> DTRACE_PROBE2(test_prov, place, 10, 4);
> + DTRACE_PROBE(test_prov, place2, 255, "foo");
> }
>
> return 0;
> diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
> new file mode 100644
> index 000000000000..8bcfa92ff6b0
> --- /dev/null
> +++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
> @@ -0,0 +1,34 @@
> + ID PROVIDER MODULE FUNCTION NAME
> + XX test_provXXXX test main go
> +
> + Probe Description Attributes
> + Identifier Names: Private
> + Data Semantics: Private
> + Dependency Class: Unknown
> +
> + Argument Attributes
> + Identifier Names: Private
> + Data Semantics: Private
> + Dependency Class: Unknown
> +
> + Argument Types
> + args[0]: char *
> + args[1]: int
> +
> + ID PROVIDER MODULE FUNCTION NAME
> + XX test_provXXXX test main go
> +
> + Probe Description Attributes
> + Identifier Names: Private
> + Data Semantics: Private
> + Dependency Class: Unknown
> +
> + Argument Attributes
> + Identifier Names: Private
> + Data Semantics: Private
> + Dependency Class: Unknown
> +
> + Argument Types
> + args[0]: char *
> + args[1]: int
> +
> diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
> new file mode 100755
> index 000000000000..c575983adf65
> --- /dev/null
> +++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
> @@ -0,0 +1,2 @@
> +#!/bin/sed -rf
> +s,test_prov[0-9]*,test_provXXXX,g; s,^ *[0-9]+, XX,g;
> diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
> new file mode 100755
> index 000000000000..ad8dc2e7c267
> --- /dev/null
> +++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
> @@ -0,0 +1,83 @@
> +#!/bin/bash
> +#
> +# Oracle Linux DTrace.
> +# Copyright (c) 2013, 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.
> +
> +##
> +#
> +# ASSERTION:
> +# Testing -lvn option with USDT probes with a valid probe name.
> +#
> +# SECTION: dtrace Utility/-ln Option
> +#
> +##
> +
> +if [ $# != 1 ]; then
> + echo expected one argument: '<'dtrace-path'>'
> + exit 2
> +fi
> +
> +dtrace=$1
> +CC=/usr/bin/gcc
> +CFLAGS=
> +
> +DIRNAME="$tmpdir/list-probes-args-usdt.$$.$RANDOM"
> +mkdir -p $DIRNAME
> +cd $DIRNAME
> +
> +cat > prov.d <<EOF
> +provider test_prov {
> + probe go(int a, char *b) : (char *b, int a);
> +};
> +EOF
> +
> +$dtrace -h -s prov.d
> +if [ $? -ne 0 ]; then
> + echo "failed to generate header file" >& 2
> + exit 1
> +fi
> +
> +cat > test.c <<EOF
> +#include <sys/types.h>
> +#include "prov.h"
> +
> +int
> +main(int argc, char **argv)
> +{
> + TEST_PROV_GO(666, "foo");
> + sleep(10);
> +}
> +EOF
> +
> +${CC} ${CFLAGS} -c test.c
> +if [ $? -ne 0 ]; then
> + echo "failed to compile test.c" >& 2
> + exit 1
> +fi
> +$dtrace -G -s prov.d test.o
> +if [ $? -ne 0 ]; then
> + echo "failed to create DOF" >& 2
> + exit 1
> +fi
> +${CC} ${CFLAGS} -o test test.o prov.o
> +if [ $? -ne 0 ]; then
> + echo "failed to link final executable" >& 2
> + exit 1
> +fi
> +
> +script()
> +{
> + $dtrace -c ./test -lvn 'test_prov$target:::go'
> + ./test &
> + PID=$!
> + disown %+
> + $dtrace -p $PID -lvn 'test_prov$target:::go'
> + kill -9 $PID
> +}
> +
> +script
> +status=$?
> +
> +exit $status
> diff --git a/test/unittest/usdt/tst.argmap-typed.d b/test/unittest/usdt/tst.argmap-typed.d
> new file mode 100644
> index 000000000000..0e0c923e1736
> --- /dev/null
> +++ b/test/unittest/usdt/tst.argmap-typed.d
> @@ -0,0 +1,40 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2007, 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.
> + */
> +
> +/* @@trigger: usdt-tst-argmap */
> +/* @@trigger-timing: before */
> +/* @@runtest-opts: $_pid */
> +
> +/*
> + * ASSERTION: Verify that args[N] variables are properly typed when remapped.
> + */
> +
> +BEGIN
> +{
> + /* Timeout after 5 seconds */
> + timeout = timestamp + 5000000000;
> +}
> +
> +test_prov$1:::place2
> +/stringof(args[0]) != "foo" || args[1] != 255 || args[2] != 255 || stringof(args[3]) != "foo"/
> +{
> + printf("args are %s, %d, %d, %s; should be \"foo\", 255, 255, \"foo\"",
> + stringof(args[0]), args[1], args[2], stringof(args[3]));
> + exit(1);
> +}
This test should also test that argN (N = 0 .. 3) has the correct value as
well. Later we may change that which would require the test to be updated,
but the current immplementation you provide is aimed at ensuring that argN and
args[N] are identical, so you should test that in this case also (because the
other test does not involve different types, which could make a difference).
> +
> +test_prov$1:::place2
> +{
> + exit(0);
> +}
> +
> +profile:::tick-1
> +/timestamp > timeout/
> +{
> + trace("test timed out");
> + exit(1);
> +}
> diff --git a/test/unittest/usdt/tst.argmap.d b/test/unittest/usdt/tst.argmap.d
> index 7896dc07e0e2..9cd3425bf8e0 100644
> --- a/test/unittest/usdt/tst.argmap.d
> +++ b/test/unittest/usdt/tst.argmap.d
> @@ -1,11 +1,10 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2007, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2007, 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.
> */
>
> -/* @@xfail: dtv2 */
> /* @@trigger: usdt-tst-argmap */
> /* @@trigger-timing: before */
> /* @@runtest-opts: $_pid */
> --
> 2.46.0.278.g36e3a12567
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 5/6] usdt: typed args and arg remapping
2024-10-22 18:40 ` Kris Van Hees
@ 2024-10-28 12:52 ` Nick Alcock
2024-10-28 16:00 ` Kris Van Hees
0 siblings, 1 reply; 23+ messages in thread
From: Nick Alcock @ 2024-10-28 12:52 UTC (permalink / raw)
To: Kris Van Hees; +Cc: Nick Alcock, dtrace, dtrace-devel
On 22 Oct 2024, Kris Van Hees told this:
> On Fri, Oct 18, 2024 at 08:58:07PM +0100, Nick Alcock wrote:
>> This change propagates the arg type and mapping info we just got into the
>> pid_probespec_t onward into the underlying dt_uprobe_t, shuffles the native
>> types before handing off a 1:1 mapping to the dt_probe_t, and calls
>> dt_cg_tramp_remap_args() to apply the requested remappings. With this, USDT
>> probes should now have visible types which obey the :-notation translators
>> specified in their .d fils (this is now tested).
>>
>> The probe info has been moved from the underlying probe to the overlying one
>> as part of this: underlying probes don't have any user-visible args to
>> get info for, and their probe_info functions are never called.
>>
>> We make several assumptions for simplicity, all noted by TODOs in the
>> source:
>>
>> - overlying probes all have the same native types and remappings, so we
>> don't need to look at more than one underlying probe to figure out what
>> they are. Given the structure of DOF it seems to me this must always be
>> true, and even a malicious attacker could not produce DOF which said
>> otherwise.
>
> You cannot place more than one USDT at a particular tracepoint (underlying
> probe) so it is guaranteed that all overlying probes can only differ in terms
> of their provider name.
I'm fairly sure I'm wrong above -- this is not validated, so malicious
DOF can perfectly well do that, simply by emitting multiple distinct
probes citing the same (or overlapping) sets of addresses! We could, of
course, add validation...
Just because DTrace is the only legitimate generator of DOF embedded
into programs, it is not true that it is the only *possible* such
program. If DTrace gets popular and attackers realise that a program
running as root is consuming DOF provided by arbitrary programs owned by
anyone, this will surely not long remain the case. Non-structural
properties of DOF not validated by dof_parser.c cannot be relied upon by
code consuming DOF contributed by USDT programs. (So this is a genuine
problem, though it might just be missing validation -- which will be
*fun* to implement in an environment with very limited malloc.)
> The USDT design in DTrace encapsulates the USDT probes
> (and providers) as instances of a meta-provider that acts as a template for its
> instances. So, all instances contain the same probes, together with all their
> data (arg data, tracepoints, etc). So, this is a guarantee and need not be
> mentioned as an assumption or TODO.
I wish :(
>> - any given underlying probe locus is either a PID or a USDT probe, never
>> both, so we can just remap the args at the start of the trampoline and
>> then never touch them again. This is obviously wrong, but the both-at
>> the-same-locus case is not yet implemented, and significant changes need
>> to be made to create_underlying() to fix this. Right now we can't easily
>> even tell if a given locus has both types of probe on it... fixing this
>> is literally a matter of shuffling the pid probes after the usdt probes
>> and then doing a single dt_cg_tramp_restore_args before all the pid
>> probes fire.
>
> I would rewrite this to point out that PID and USDT probes being at the same
> locus is not implemented yet, and that this implementation assumes that either
> all probes at a particular locus are PID or that they are all USDT.
Rephrased a bit.
> I would avoid speculative statements about how this might be fixed in the
> future (however near that may be) because there may still be design decisions
> to be made about that and there is no need to bring it up here.
Oh, I thought we'd already decided that! I'll drop the last couple of
sentences.
>> PID args are already broken before this commit (something to do with the
>> wildcard USDT work?) so I cannot test if this commit broke them further,
>> but it shouldn't have.
>
> This does not belong here but rather in a bug report.
Errr... a bug report about a not-yet-released DTrace with a the USDT
patch series half-applied? I don't think we've ever had bug reports
about non-released half-applied patch series before and I'd have assumed
that a bug report was exactly what you *wouldn't* want about something
in that sort of state of flux.
Obviously I wasn't planning to leave this here in the long term, but at
least this way if you tested the series it is obvious that a few tests
are expected to fail and some things have not yet been verified. I have
no idea where you'd expect me to say that if not in the commit message
for the change in question.
> In addition, I think it would be good to add a few more test cases to cover
> edge cases:
>
> - A probe that has native arguments, but no translated arguments.
> E.g.
> probe place3(int i, char *j) : ();
>
> - A probe that has native arguments that do not get used.
> E.g.
> probe place4(int i, char *j) : (char *j);
Good point, implemented! (The first one was fun.)
>> + int nargc; /* number of native args */
>> + int xargc; /* number of xlated args */
>> + int remapargc; /* number of remapped args */
>> + char *nargv; /* array of native args */
>> + char *xargv; /* array of xlated args */
>> + int8_t *remapargs; /* remapped arg indexes */
>
> Since all overlying USDT probes associated with a given underlying probe
> (uprobe) have the sane argument type data (native, translated, and mapping),
> and PID probes do not have arguments by definition, you can certainly store
> the data here. But why in this raw format rather than a dt_argdesc_t array
> (and an argc member to indicate the number of array elements - xargc).
I started out doing that, but it meant infiltrating knowledge about
dt_argdesc_t into dt_pid_create_usdt_probes_proc(), and it proved
outright confusion-inducing when it turned out that dt_prov_uprobe.c had
to pass down a *different mapping* with reshuffled elements, because of
the need to move native args around in the translator. So the
dt_argdesc_t approach gained us nothing and made things noticeably
harder to understand :(
> Calls to the probe_info() callback will want an array of dt_argdesc_t elements
> to be provided, and nothing else needs the rarw format. So, you might as well
> store that array here rather than the raw data, and implement the probe_info()
> callback for USDT as a pass-through that simply takes the data from here (from
> the underlying probe), copies it (using strdup on the strings because the
> caller will free them), and assigned it to *argv.
Yes, if it wasn't that the uprobe probe_info needs to reshuffle it, and
the only reason it needs to do that is the nature of the uprobe
trampoline (so knowledge about that shuffling should not be propagated
into dt_pid.c: as you note, in the future it would be nice to not need
the trampoline arg remapper at all, so we don't want to spread things
that imply its existence too far if we can avoid it).
So... maybe later once we're not shuffling native args anywhere, if
we're sure that nobody is likely to want to do that?
>> +test_prov$1:::place2
>> +/stringof(args[0]) != "foo" || args[1] != 255 || args[2] != 255 || stringof(args[3]) != "foo"/
>> +{
>> + printf("args are %s, %d, %d, %s; should be \"foo\", 255, 255, \"foo\"",
>> + stringof(args[0]), args[1], args[2], stringof(args[3]));
>> + exit(1);
>> +}
>
> This test should also test that argN (N = 0 .. 3) has the correct value as
> well. Later we may change that which would require the test to be updated,
> but the current immplementation you provide is aimed at ensuring that argN and
> args[N] are identical, so you should test that in this case also (because the
> other test does not involve different types, which could make a difference).
Good point! Hmm we may need to copyin() there too.
(Adjusted all tests in this family.)
--
NULL && (void)
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 5/6] usdt: typed args and arg remapping
2024-10-28 12:52 ` Nick Alcock
@ 2024-10-28 16:00 ` Kris Van Hees
0 siblings, 0 replies; 23+ messages in thread
From: Kris Van Hees @ 2024-10-28 16:00 UTC (permalink / raw)
To: Nick Alcock; +Cc: Kris Van Hees, dtrace, dtrace-devel
On Mon, Oct 28, 2024 at 12:52:01PM +0000, Nick Alcock wrote:
> On 22 Oct 2024, Kris Van Hees told this:
>
> > On Fri, Oct 18, 2024 at 08:58:07PM +0100, Nick Alcock wrote:
> >> This change propagates the arg type and mapping info we just got into the
> >> pid_probespec_t onward into the underlying dt_uprobe_t, shuffles the native
> >> types before handing off a 1:1 mapping to the dt_probe_t, and calls
> >> dt_cg_tramp_remap_args() to apply the requested remappings. With this, USDT
> >> probes should now have visible types which obey the :-notation translators
> >> specified in their .d fils (this is now tested).
> >>
> >> The probe info has been moved from the underlying probe to the overlying one
> >> as part of this: underlying probes don't have any user-visible args to
> >> get info for, and their probe_info functions are never called.
> >>
> >> We make several assumptions for simplicity, all noted by TODOs in the
> >> source:
> >>
> >> - overlying probes all have the same native types and remappings, so we
> >> don't need to look at more than one underlying probe to figure out what
> >> they are. Given the structure of DOF it seems to me this must always be
> >> true, and even a malicious attacker could not produce DOF which said
> >> otherwise.
> >
> > You cannot place more than one USDT at a particular tracepoint (underlying
> > probe) so it is guaranteed that all overlying probes can only differ in terms
> > of their provider name.
>
> I'm fairly sure I'm wrong above -- this is not validated, so malicious
> DOF can perfectly well do that, simply by emitting multiple distinct
> probes citing the same (or overlapping) sets of addresses! We could, of
> course, add validation...
>
> Just because DTrace is the only legitimate generator of DOF embedded
> into programs, it is not true that it is the only *possible* such
> program. If DTrace gets popular and attackers realise that a program
> running as root is consuming DOF provided by arbitrary programs owned by
> anyone, this will surely not long remain the case. Non-structural
> properties of DOF not validated by dof_parser.c cannot be relied upon by
> code consuming DOF contributed by USDT programs. (So this is a genuine
> problem, though it might just be missing validation -- which will be
> *fun* to implement in an environment with very limited malloc.)
The validation need not be at the level of dtprobed. DTrace itself can very
easily determine when a USDT probe pops up at a location (identified by the
underlying probe) that already has a USDT probe.
Malicious DOF encoding data that results in more than one USDT probe being at
a given location, possibly with different argument data, will result in
undefined (yet still safe) behaviour. But that goes for most other things
that anyone could fake - DTrace does guard against such bad data posing a
security risk though. It is not necessary for dtprobed to do all validation.
> > The USDT design in DTrace encapsulates the USDT probes
> > (and providers) as instances of a meta-provider that acts as a template for its
> > instances. So, all instances contain the same probes, together with all their
> > data (arg data, tracepoints, etc). So, this is a guarantee and need not be
> > mentioned as an assumption or TODO.
>
> I wish :(
Well, it is a guarantee for what DOF gets generated by DTrace. Sure, malicious
DOF will result in things going wrong in terms of functionality (but not safety
thankfully), but that is a given, right? For the purpose of this patch, you
can assume valid data and I think that the commit message can stick to that.
We can come up with a variety of issues that can occur when malicious (or
buggy) DOF gets processed, but I don't think it is useful to provide that
level of detail or analysis in a commit message. Especially because the
way it is phrased as assumptions it gives the impression that we dont know.
And we do.
My main objection here is really that it appears that your patch is making
assumptions for simplicity, and that is not the case - you are implementing
the correct behaviour based on knowing how the DOF is structured and generated.
If you want to mention something here for peace of mind, perhaps just say that
dtprobed does not validate that e.g. no two USDT probes list the same
tracepoint. But even that I think is unnecessary - after all, it wasn't at all
mentioned when dtprobed was first implemented, so why would it now suddenly be
an issue?
> >> - any given underlying probe locus is either a PID or a USDT probe, never
> >> both, so we can just remap the args at the start of the trampoline and
> >> then never touch them again. This is obviously wrong, but the both-at
> >> the-same-locus case is not yet implemented, and significant changes need
> >> to be made to create_underlying() to fix this. Right now we can't easily
> >> even tell if a given locus has both types of probe on it... fixing this
> >> is literally a matter of shuffling the pid probes after the usdt probes
> >> and then doing a single dt_cg_tramp_restore_args before all the pid
> >> probes fire.
> >
> > I would rewrite this to point out that PID and USDT probes being at the same
> > locus is not implemented yet, and that this implementation assumes that either
> > all probes at a particular locus are PID or that they are all USDT.
>
> Rephrased a bit.
>
> > I would avoid speculative statements about how this might be fixed in the
> > future (however near that may be) because there may still be design decisions
> > to be made about that and there is no need to bring it up here.
>
> Oh, I thought we'd already decided that! I'll drop the last couple of
> sentences.
>
> >> PID args are already broken before this commit (something to do with the
> >> wildcard USDT work?) so I cannot test if this commit broke them further,
> >> but it shouldn't have.
> >
> > This does not belong here but rather in a bug report.
>
> Errr... a bug report about a not-yet-released DTrace with a the USDT
> patch series half-applied? I don't think we've ever had bug reports
> about non-released half-applied patch series before and I'd have assumed
> that a bug report was exactly what you *wouldn't* want about something
> in that sort of state of flux.
>
> Obviously I wasn't planning to leave this here in the long term, but at
> least this way if you tested the series it is obvious that a few tests
> are expected to fail and some things have not yet been verified. I have
> no idea where you'd expect me to say that if not in the commit message
> for the change in question.
Point is that you are mentioning that there is a patch before yours that you
say breaks PID args. Since your series is based on the current development
branch, that means that said patch has already been reviewed and merged. So,
it makes totla sense to open a bug (github issue) for it so that it can be
looked into. Mentioning the breakage in a commit message is not going to get
the attention it needs to track down the issue and fix it. Also, the way it
is mentioned here does not really provide detail on *what* appears broken, i.e.
which test fails, what the results are vs expected, etc... All stuff you would
add to an issue you file.
I am also not sure what you mean with "wasn't planning to leave this here in
the long term"... as this patch is posted for rwview, I would expect that if
reviewed as OK, I can merge it as-is and so this portion of the commnit msg
would certainly be there for the long term.
> > In addition, I think it would be good to add a few more test cases to cover
> > edge cases:
> >
> > - A probe that has native arguments, but no translated arguments.
> > E.g.
> > probe place3(int i, char *j) : ();
> >
> > - A probe that has native arguments that do not get used.
> > E.g.
> > probe place4(int i, char *j) : (char *j);
>
> Good point, implemented! (The first one was fun.)
>
> >> + int nargc; /* number of native args */
> >> + int xargc; /* number of xlated args */
> >> + int remapargc; /* number of remapped args */
> >> + char *nargv; /* array of native args */
> >> + char *xargv; /* array of xlated args */
> >> + int8_t *remapargs; /* remapped arg indexes */
> >
> > Since all overlying USDT probes associated with a given underlying probe
> > (uprobe) have the sane argument type data (native, translated, and mapping),
> > and PID probes do not have arguments by definition, you can certainly store
> > the data here. But why in this raw format rather than a dt_argdesc_t array
> > (and an argc member to indicate the number of array elements - xargc).
>
> I started out doing that, but it meant infiltrating knowledge about
> dt_argdesc_t into dt_pid_create_usdt_probes_proc(), and it proved
> outright confusion-inducing when it turned out that dt_prov_uprobe.c had
> to pass down a *different mapping* with reshuffled elements, because of
> the need to move native args around in the translator. So the
> dt_argdesc_t approach gained us nothing and made things noticeably
> harder to understand :(
How would this leak dt_argdesc_t into dt_pid_create_usdt_probes_proc()? That
function can still handle the raw format. My point is that the dt_uprobe_t
you store the data in (associated with the underlying probe) can already store
it in dt_argdesc_t format rather than storing the raw data.
It does not pose an issue with the mapping of arguments in the trampoline
because wherever you switch things from being a shuffling to simply being a
i -> i mapping, that can be done as easily based on an dt_argdesc_t array as
it can be done with the raw data.
> > Calls to the probe_info() callback will want an array of dt_argdesc_t elements
> > to be provided, and nothing else needs the rarw format. So, you might as well
> > store that array here rather than the raw data, and implement the probe_info()
> > callback for USDT as a pass-through that simply takes the data from here (from
> > the underlying probe), copies it (using strdup on the strings because the
> > caller will free them), and assigned it to *argv.
>
> Yes, if it wasn't that the uprobe probe_info needs to reshuffle it, and
> the only reason it needs to do that is the nature of the uprobe
> trampoline (so knowledge about that shuffling should not be propagated
> into dt_pid.c: as you note, in the future it would be nice to not need
> the trampoline arg remapper at all, so we don't want to spread things
> that imply its existence too far if we can avoid it).
>
> So... maybe later once we're not shuffling native args anywhere, if
> we're sure that nobody is likely to want to do that?
I really do not see the problem. It is the same data, just in a different
format.
With the underlying probe keeping the arg data in a dt_argdesc_t array, the
probe_info() calls would still need to provide a copy of the array, with strdup
for the strings, and thus you can easily store i -> i mapping there also and
thus the dt_argdesc_t data on the uderlying probe still retains the correct
napping data that the trampoline will need.
And if/when we adjust things to get away from this silly argN == args[N] thing,
that altering of the mapping index in the dt_argdesc_t elements can simply
be dropped and all will be well (aside from also needing to update the
trampoline code to stop its reshuffling).
> >> +test_prov$1:::place2
> >> +/stringof(args[0]) != "foo" || args[1] != 255 || args[2] != 255 || stringof(args[3]) != "foo"/
> >> +{
> >> + printf("args are %s, %d, %d, %s; should be \"foo\", 255, 255, \"foo\"",
> >> + stringof(args[0]), args[1], args[2], stringof(args[3]));
> >> + exit(1);
> >> +}
> >
> > This test should also test that argN (N = 0 .. 3) has the correct value as
> > well. Later we may change that which would require the test to be updated,
> > but the current immplementation you provide is aimed at ensuring that argN and
> > args[N] are identical, so you should test that in this case also (because the
> > other test does not involve different types, which could make a difference).
>
> Good point! Hmm we may need to copyin() there too.
>
> (Adjusted all tests in this family.)
>
> --
> NULL && (void)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 6/6] usdt: fix create_underlying error path
2024-10-18 19:58 [PATCH 0/6] usdt typed args, translators and arg remapping Nick Alcock
` (4 preceding siblings ...)
2024-10-18 19:58 ` [PATCH 5/6] usdt: typed args and arg remapping Nick Alcock
@ 2024-10-18 19:58 ` Nick Alcock
2024-10-22 18:46 ` Kris Van Hees
5 siblings, 1 reply; 23+ messages in thread
From: Nick Alcock @ 2024-10-18 19:58 UTC (permalink / raw)
To: dtrace, dtrace-devel
On error, we were destroying the underlying probe using the wrong call,
as if it were an overlying probe.
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
libdtrace/dt_prov_uprobe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
index 767d9a80b5e1..83d6f65a5207 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -274,7 +274,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
return uprp;
fail:
- probe_destroy(dtp, upp);
+ probe_destroy_underlying(dtp, upp);
return NULL;
}
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 6/6] usdt: fix create_underlying error path
2024-10-18 19:58 ` [PATCH 6/6] usdt: fix create_underlying error path Nick Alcock
@ 2024-10-22 18:46 ` Kris Van Hees
0 siblings, 0 replies; 23+ messages in thread
From: Kris Van Hees @ 2024-10-22 18:46 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
On Fri, Oct 18, 2024 at 08:58:08PM +0100, Nick Alcock wrote:
> On error, we were destroying the underlying probe using the wrong call,
> as if it were an overlying probe.
>
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
> libdtrace/dt_prov_uprobe.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index 767d9a80b5e1..83d6f65a5207 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -274,7 +274,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> return uprp;
>
> fail:
> - probe_destroy(dtp, upp);
> + probe_destroy_underlying(dtp, upp);
> return NULL;
> }
>
> --
> 2.46.0.278.g36e3a12567
>
^ permalink raw reply [flat|nested] 23+ messages in thread