public inbox for dtrace@lists.linux.dev
 help / color / mirror / Atom feed
* [PATCH] Allow probes not to attach if they were not "needed"
@ 2025-09-04 19:02 eugene.loh
  2025-09-17 17:43 ` Kris Van Hees
  0 siblings, 1 reply; 4+ messages in thread
From: eugene.loh @ 2025-09-04 19:02 UTC (permalink / raw)
  To: dtrace, dtrace-devel

From: Eugene Loh <eugene.loh@oracle.com>

Various kernels prevent the placement of uprobes on particular
instructions -- e.g., on x86 on hlt or multi-byte nops, or on aarch64 on
autiasp.  This means, for example, that one cannot place a pid probe on
every possible instruction.  If a user explicitly places a probe on a
forbidden instruction, an error message indicates there is a problem.

On the other hand, if the user specifies a wildcard in a pid probe
description, we should skip instructions where no uprobe can be placed.

The problem is that we do not find out an instruction is problematic
until we try to attach, which is long after probe descriptions have been
processed.  And when we walk probe descriptions, we do not have
information about which kernels and instructions are problematic.

The motivating problem pertains to pid probes, but we would like
a provider-agnostic solution.

Introduce a "NEEDED" probe flag.  Set the flag when a probe is explicitly
needed (say, it was specifically named).  We use two such flags:

* In dt_probe.h, DT_PROBE_FLAG_NEEDED is used for the "flags"
  member of dt_probe_t.

* For pid-style probes (including USDT and stapsdt), in
  include/dtrace/pid.h, DT_PID_PSP_FLAG_NEEDED is used for
  the pps_flags member of pid_probespec_t.

Set the "NEEDED" flag for a probe in these situations:

* In dt_tp_probe_insert(), set the flag for each probe we insert.
  This covers the dtrace, fbt, rawtp, sdt, and syscall providers.

* In dt_sdt_populate(), set the flag for each probe we insert.
  This covers, e.g., the io, ip, lockstat, proc, sched, and tcp
  providers.

* For the cpc and profile providers, set the flag for probes created
  with the "provide" functions (explicitly requested by the user)
  but not with the "populate" functions (chosen by default by dtrace).

* For providers that will sit on underlying probes, set the NEEDED
  flag for pid_probespec_t.  Specifically,

  * In dt_pid_per_sym(), set the NEEDED flag to cover entry,
    return, and explicit offsets.  Unset the flag for glob offsets.
    In dt_pid_create_usdt_probes_proc() and dt_stapsdt_parse(),
    set the flag.

  * In dt_prov_uprobe(), pass the pid_probespec_t NEEDED flag
    to dt_probe_t, for both the overlying and underlying probe.

In dt_bpf.c, if the return code from the attach function indicates
failure, return a failure only if the probe was NEEDED.

Remove the old check on "hlt" instructions, since it was not sufficiently
general for other kernels and other problematic instructions.

Existing tests that expose the problem are

    test/unittest/pid/tst.coverage.d
    test/unittest/pid/tst.emptystack.d
    test/unittest/pid/tst.entry_off0.sh
        hlt instruction on x86

    test/unittest/pid/tst.float.d
        multi-byte nop on x86 on newer platforms

    test/unittest/pid/tst.entry_off0.sh
        autiasp instruction on aarch64 on newer platforms
        depending on how the trigger process was built

A new test is added to check that a probe set explicitly on a
problematic instruction will still lead to an error.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 include/dtrace/pid.h                   |  3 +++
 libdtrace/dt_bpf.c                     |  2 +-
 libdtrace/dt_pid.c                     | 10 ++++------
 libdtrace/dt_probe.h                   |  3 +++
 libdtrace/dt_prov_cpc.c                | 10 ++++++++--
 libdtrace/dt_prov_profile.c            | 10 ++++++++--
 libdtrace/dt_prov_uprobe.c             | 18 ++++++++++++++++++
 libdtrace/dt_provider_sdt.c            |  8 ++++++--
 libdtrace/dt_provider_tp.c             |  7 ++++++-
 test/unittest/pid/err.needed.aarch64.x |  3 +++
 test/unittest/pid/err.needed.sh        | 23 +++++++++++++++++++++++
 test/unittest/pid/err.needed.x86_64.x  |  6 ++++++
 test/unittest/pid/tst.entry_off0.sh    |  4 ++--
 13 files changed, 91 insertions(+), 16 deletions(-)
 create mode 100755 test/unittest/pid/err.needed.aarch64.x
 create mode 100755 test/unittest/pid/err.needed.sh
 create mode 100755 test/unittest/pid/err.needed.x86_64.x

diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
index 8ddb1167e..7829e47c0 100644
--- a/include/dtrace/pid.h
+++ b/include/dtrace/pid.h
@@ -47,6 +47,7 @@ typedef struct pid_probespec {
 	size_t pps_xargvlen;			/* (high estimate of) length of array */
 	int8_t *pps_argmap;			/* mapped arg indexes */
 	char *pps_sargv;			/* list of arg sources */
+	int pps_flags;				/* flags */
 
 	/*
 	 * Fields below this point do not apply to underlying probes.
@@ -55,4 +56,6 @@ typedef struct pid_probespec {
 	uint64_t pps_nameoff;			/* offset to use for name */
 } pid_probespec_t;
 
+#define DT_PID_PSP_FLAG_NEEDED	1		/* probe is needed */
+
 #endif /* _DTRACE_PID_H */
diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
index 31781ac9f..dc3d47f56 100644
--- a/libdtrace/dt_bpf.c
+++ b/libdtrace/dt_bpf.c
@@ -1385,7 +1385,7 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
 		if (prp->prov->impl->attach)
 			rc = prp->prov->impl->attach(dtp, prp, fd);
 
-		if (rc < 0) {
+		if (rc < 0 && (prp->flags & DT_PROBE_FLAG_NEEDED)) {
 			close(fd);
 			dt_attach_error(dtp, rc,
 					prp->desc->prv, prp->desc->mod,
diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
index 4af9141aa..6e7fba287 100644
--- a/libdtrace/dt_pid.c
+++ b/libdtrace/dt_pid.c
@@ -184,6 +184,7 @@ dt_pid_per_sym(dt_pid_probe_t *pp, const GElf_Sym *symp, const char *func)
 	psp->pps_fun = (char *) func;
 	psp->pps_nameoff = 0;
 	psp->pps_off = symp->st_value - pp->dpp_vaddr;
+	psp->pps_flags |= DT_PID_PSP_FLAG_NEEDED;
 
 	/*
 	 * The special function "-" means the probe name is an absolute
@@ -386,15 +387,10 @@ dt_pid_per_sym(dt_pid_probe_t *pp, const GElf_Sym *symp, const char *func)
 #define disasm(x, y) 4
 #endif
 
+		psp->pps_flags &= ~DT_PID_PSP_FLAG_NEEDED;
 		for (off = 0; off < symp->st_size; off += disasm(off, &disasm_info)) {
 			char offstr[32];
 
-#if defined(__amd64)
-			/* Newer kernels do not allow uprobes on "hlt" instructions. */
-			if ((unsigned int)disasm_info.buffer[off] == 0xf4)
-				continue;
-#endif
-
 			snprintf(offstr, sizeof(offstr), "%lx", off);
 			if (!gmatch(offstr, pp->dpp_name))
 				continue;
@@ -1064,6 +1060,7 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, pid_t pid, dt_proc_t *dpr,
 			psp.pps_pid = dpr->dpr_pid;
 			psp.pps_off = tp->tracepoint.addr - pmp->pr_file->first_segment->pr_vaddr;
 			psp.pps_nameoff = 0;
+			psp.pps_flags |= DT_PID_PSP_FLAG_NEEDED;
 
 			if (nargv) {
 				psp.pps_nargc = probe->probe.nargc;
@@ -1421,6 +1418,7 @@ dt_stapsdt_parse(dtrace_hdl_t *dtp, dt_proc_t *dpr, dtrace_probedesc_t *pdp,
 		psp.pps_inum = pmp->pr_inum;
 		psp.pps_pid = dpr->dpr_pid;
 		psp.pps_nameoff = 0;
+		psp.pps_flags |= DT_PID_PSP_FLAG_NEEDED;
 
 		if (pvp->impl->provide_probe(dtp, &psp) < 0) {
 			dt_pid_error(dtp, pcb, dpr, D_PROC_USDT,
diff --git a/libdtrace/dt_probe.h b/libdtrace/dt_probe.h
index 2a78cb9ca..ffd994844 100644
--- a/libdtrace/dt_probe.h
+++ b/libdtrace/dt_probe.h
@@ -53,10 +53,13 @@ typedef struct dt_probe {
 	uint8_t *mapping;		/* translated argument mapping */
 	dtrace_typeinfo_t *argv;	/* output argument types */
 	int argc;			/* output argument count */
+	int flags;			/* flags for the probe */
 	dt_probe_instance_t *pr_inst;	/* list of functions and offsets */
 	dtrace_difo_t *difo;		/* BPF probe program */
 } dt_probe_t;
 
+#define DT_PROBE_FLAG_NEEDED	1	/* probe is needed */
+
 extern dt_probe_t *dt_probe_lookup2(dt_provider_t *, const char *);
 extern dt_probe_t *dt_probe_create(dtrace_hdl_t *, dt_ident_t *, int,
     dt_node_t *, uint_t, dt_node_t *, uint_t);
diff --git a/libdtrace/dt_prov_cpc.c b/libdtrace/dt_prov_cpc.c
index 57b11b135..9fb165cb6 100644
--- a/libdtrace/dt_prov_cpc.c
+++ b/libdtrace/dt_prov_cpc.c
@@ -347,6 +347,7 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
 {
 	dt_provider_t *prv;
 	struct perf_event_attr attr;
+	dt_probe_t *prp;
 
 	prv = dt_provider_lookup(dtp, prvname);
 	if (!prv)
@@ -358,17 +359,22 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
 		return 0;
 
 	/* return if we already have this probe */
-	if (dt_probe_lookup(dtp, pdp))
+	prp = dt_probe_lookup(dtp, pdp);
+	if (prp) {
+		prp->flags |= DT_PROBE_FLAG_NEEDED;
 		return 0;
+	}
 
 	/* check if the probe name can be decoded */
 	if (decode_probename(&attr, prv, pdp->prb) == -1)
 		return 0;
 
 	/* try to add this probe */
-	if (cpc_probe_insert(dtp, prv, pdp->prb) == NULL)
+	prp = cpc_probe_insert(dtp, prv, pdp->prb);
+	if (prp == NULL)
 		return 0;
 
+	prp->flags |= DT_PROBE_FLAG_NEEDED;
 	return 1;
 }
 
diff --git a/libdtrace/dt_prov_profile.c b/libdtrace/dt_prov_profile.c
index e1369ca9b..ec32ce83b 100644
--- a/libdtrace/dt_prov_profile.c
+++ b/libdtrace/dt_prov_profile.c
@@ -160,6 +160,7 @@ static uint64_t get_period(const char *name)
 static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
 {
 	dt_provider_t	*prv;
+	dt_probe_t	*prp;
 	int		kind;
 	uint64_t	period;
 
@@ -176,8 +177,11 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
 		return 0;
 
 	/* return if we already have this probe */
-	if (dt_probe_lookup(dtp, pdp))
+	prp = dt_probe_lookup(dtp, pdp);
+	if (prp) {
+		prp->flags |= DT_PROBE_FLAG_NEEDED;
 		return 0;
+	}
 
 	/* get the provider - should have been created in populate() */
 	prv = dt_provider_lookup(dtp, prvname);
@@ -189,9 +193,11 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
 		return 0;
 
 	/* try to add this probe */
-	if (profile_probe_insert(dtp, prv, pdp->prb, kind, period) == NULL)
+	prp = profile_probe_insert(dtp, prv, pdp->prb, kind, period);
+	if (prp == NULL)
 		return 0;
 
+	prp->flags |= DT_PROBE_FLAG_NEEDED;
 	return 1;
 }
 
diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
index 07b26f20f..2396522ce 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -960,6 +960,12 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
 	} else
 		upp = uprp->prv_data;
 
+	/*
+	 * Pass flag info from psp to uprp
+	 */
+	if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
+		uprp->flags |= DT_PROBE_FLAG_NEEDED;
+
 	/*
 	 * Only one USDT probe can correspond to each underlying probe.
 	 */
@@ -1049,6 +1055,12 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
 	/* Look up the overlying probe. */
 	prp = dt_probe_lookup(dtp, &pd);
 	if (prp != NULL) {
+		/*
+		 * Pass flag info from psp to uprp
+		 */
+		if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
+			prp->flags |= DT_PROBE_FLAG_NEEDED;
+
 		/*
 		 * Probe already exists.  If it's already in the underlying
 		 * probe's probe list, there is nothing left to do.
@@ -1091,6 +1103,12 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
 		return -1;
 	}
 
+	/*
+	 * Pass flag info from psp to uprp
+	 */
+	if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
+		prp->flags |= DT_PROBE_FLAG_NEEDED;
+
 	/*
 	 * Add the overlying probe to the list of probes for the underlying probe.
 	 */
diff --git a/libdtrace/dt_provider_sdt.c b/libdtrace/dt_provider_sdt.c
index 962848485..f9a51fa25 100644
--- a/libdtrace/dt_provider_sdt.c
+++ b/libdtrace/dt_provider_sdt.c
@@ -48,10 +48,14 @@ dt_sdt_populate(dtrace_hdl_t *dtp, const char *prvname, const char *modname,
 	 * entries to identify the probe names.
 	 */
 	for (arg = &probe_args[0]; arg->name != NULL; arg++) {
+		dt_probe_t *prp;
+
 		if (arg->argno == 0 &&
-		    dt_probe_insert(dtp, prv, prvname, modname, "", arg->name,
-				    NULL))
+		    (prp = dt_probe_insert(dtp, prv, prvname, modname,
+					   "", arg->name, NULL))) {
+			prp->flags |= DT_PROBE_FLAG_NEEDED;
 			n++;
+		}
 	}
 
 	return n;
diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
index 3d4ec1e03..4531a88a8 100644
--- a/libdtrace/dt_provider_tp.c
+++ b/libdtrace/dt_provider_tp.c
@@ -368,12 +368,17 @@ dt_tp_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
 		   const char *mod, const char *fun, const char *prb)
 {
 	tp_probe_t	*tpp;
+	dt_probe_t	*prp;
 
 	tpp = dt_tp_alloc(dtp);
 	if (tpp == NULL)
 		return NULL;
 
-	return dt_probe_insert(dtp, prov, prv, mod, fun, prb, tpp);
+	prp = dt_probe_insert(dtp, prov, prv, mod, fun, prb, tpp);
+	if (prp)
+		prp->flags |= DT_PROBE_FLAG_NEEDED;
+
+	return prp;
 }
 
 /*
diff --git a/test/unittest/pid/err.needed.aarch64.x b/test/unittest/pid/err.needed.aarch64.x
new file mode 100755
index 000000000..bd5b41a36
--- /dev/null
+++ b/test/unittest/pid/err.needed.aarch64.x
@@ -0,0 +1,3 @@
+#!/bin/sh
+echo "skip on aarch64"
+exit 2
diff --git a/test/unittest/pid/err.needed.sh b/test/unittest/pid/err.needed.sh
new file mode 100755
index 000000000..d6b041b60
--- /dev/null
+++ b/test/unittest/pid/err.needed.sh
@@ -0,0 +1,23 @@
+#!/bin/bash
+#
+# Oracle Linux DTrace.
+# Copyright (c) 2025, 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.
+#
+
+# We should not be able to trace on a hlt instruction.
+#
+# (When the probe is "needed" -- that is, explicitly requested by the user
+# -- we should incur an error.  Test tst.entry_off0.sh should be okay,
+# since dtrace will silently skip over such problematic instructions.)
+
+dtrace=$1
+trig=`pwd`/test/triggers/ustack-tst-basic
+off=`${OBJDUMP} -d $trig | awk '/hlt/ {sub(/:/, ""); print $1}'`
+
+$dtrace $dt_flags -c $trig -n 'pid$target:a.out:-:'$off'
+{
+	trace("hlt instruction");
+	exit(0);
+}'
diff --git a/test/unittest/pid/err.needed.x86_64.x b/test/unittest/pid/err.needed.x86_64.x
new file mode 100755
index 000000000..158336d98
--- /dev/null
+++ b/test/unittest/pid/err.needed.x86_64.x
@@ -0,0 +1,6 @@
+#!/bin/sh
+if ! ${OBJDUMP} -d test/triggers/ustack-tst-basic | grep hlt; then
+    echo "did not find hlt instruction in trigger"
+    exit 2
+fi
+exit 0
diff --git a/test/unittest/pid/tst.entry_off0.sh b/test/unittest/pid/tst.entry_off0.sh
index 1a4c3d207..304be4568 100755
--- a/test/unittest/pid/tst.entry_off0.sh
+++ b/test/unittest/pid/tst.entry_off0.sh
@@ -1,14 +1,14 @@
 #!/bin/bash
 #
 # Oracle Linux DTrace.
-# Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+# Copyright (c) 2024, 2025, 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.
 #
 
 # Although the D script takes only "one second," it takes a long time to
 # shut down.  Until that has been solved, increase the timeout for the test.
-# @@timeout: 120
+# @@timeout: 240
 
 dtrace=$1
 
-- 
2.47.3


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

* Re: [PATCH] Allow probes not to attach if they were not "needed"
  2025-09-04 19:02 [PATCH] Allow probes not to attach if they were not "needed" eugene.loh
@ 2025-09-17 17:43 ` Kris Van Hees
  2025-09-17 20:04   ` Eugene Loh
  0 siblings, 1 reply; 4+ messages in thread
From: Kris Van Hees @ 2025-09-17 17:43 UTC (permalink / raw)
  To: eugene.loh; +Cc: dtrace, dtrace-devel

I think this is actually counter-intuitive.  In general, probes are expected
to be able to be attached to or else an error is to be raised.  There are
exceptional cases where a failure to attach should not be considered an error.

So, I believe that the implementation should by default expect that failure to
attach is an error.  Introducing a flag is certainly an option (though I would
expect it a flag to mark the probe as *optional*), though another approach that
might be worth exploring here is to introduce a 2nd list of enablings, named
dt_opt_enablings, and add optional probes to this list.  That way you could
refactor the construction and loading of programs into functions called for a
given probe, and just loop through the mandatory enablings first, and then the
optional ones.  That avoids adding a flag and also differentiates the two types
of enablings explicitly.

On Thu, Sep 04, 2025 at 03:02:26PM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Various kernels prevent the placement of uprobes on particular
> instructions -- e.g., on x86 on hlt or multi-byte nops, or on aarch64 on
> autiasp.  This means, for example, that one cannot place a pid probe on
> every possible instruction.  If a user explicitly places a probe on a
> forbidden instruction, an error message indicates there is a problem.
> 
> On the other hand, if the user specifies a wildcard in a pid probe
> description, we should skip instructions where no uprobe can be placed.
> 
> The problem is that we do not find out an instruction is problematic
> until we try to attach, which is long after probe descriptions have been
> processed.  And when we walk probe descriptions, we do not have
> information about which kernels and instructions are problematic.
> 
> The motivating problem pertains to pid probes, but we would like
> a provider-agnostic solution.
> 
> Introduce a "NEEDED" probe flag.  Set the flag when a probe is explicitly
> needed (say, it was specifically named).  We use two such flags:
> 
> * In dt_probe.h, DT_PROBE_FLAG_NEEDED is used for the "flags"
>   member of dt_probe_t.
> 
> * For pid-style probes (including USDT and stapsdt), in
>   include/dtrace/pid.h, DT_PID_PSP_FLAG_NEEDED is used for
>   the pps_flags member of pid_probespec_t.
> 
> Set the "NEEDED" flag for a probe in these situations:
> 
> * In dt_tp_probe_insert(), set the flag for each probe we insert.
>   This covers the dtrace, fbt, rawtp, sdt, and syscall providers.
> 
> * In dt_sdt_populate(), set the flag for each probe we insert.
>   This covers, e.g., the io, ip, lockstat, proc, sched, and tcp
>   providers.
> 
> * For the cpc and profile providers, set the flag for probes created
>   with the "provide" functions (explicitly requested by the user)
>   but not with the "populate" functions (chosen by default by dtrace).
> 
> * For providers that will sit on underlying probes, set the NEEDED
>   flag for pid_probespec_t.  Specifically,
> 
>   * In dt_pid_per_sym(), set the NEEDED flag to cover entry,
>     return, and explicit offsets.  Unset the flag for glob offsets.
>     In dt_pid_create_usdt_probes_proc() and dt_stapsdt_parse(),
>     set the flag.
> 
>   * In dt_prov_uprobe(), pass the pid_probespec_t NEEDED flag
>     to dt_probe_t, for both the overlying and underlying probe.
> 
> In dt_bpf.c, if the return code from the attach function indicates
> failure, return a failure only if the probe was NEEDED.
> 
> Remove the old check on "hlt" instructions, since it was not sufficiently
> general for other kernels and other problematic instructions.
> 
> Existing tests that expose the problem are
> 
>     test/unittest/pid/tst.coverage.d
>     test/unittest/pid/tst.emptystack.d
>     test/unittest/pid/tst.entry_off0.sh
>         hlt instruction on x86
> 
>     test/unittest/pid/tst.float.d
>         multi-byte nop on x86 on newer platforms
> 
>     test/unittest/pid/tst.entry_off0.sh
>         autiasp instruction on aarch64 on newer platforms
>         depending on how the trigger process was built
> 
> A new test is added to check that a probe set explicitly on a
> problematic instruction will still lead to an error.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  include/dtrace/pid.h                   |  3 +++
>  libdtrace/dt_bpf.c                     |  2 +-
>  libdtrace/dt_pid.c                     | 10 ++++------
>  libdtrace/dt_probe.h                   |  3 +++
>  libdtrace/dt_prov_cpc.c                | 10 ++++++++--
>  libdtrace/dt_prov_profile.c            | 10 ++++++++--
>  libdtrace/dt_prov_uprobe.c             | 18 ++++++++++++++++++
>  libdtrace/dt_provider_sdt.c            |  8 ++++++--
>  libdtrace/dt_provider_tp.c             |  7 ++++++-
>  test/unittest/pid/err.needed.aarch64.x |  3 +++
>  test/unittest/pid/err.needed.sh        | 23 +++++++++++++++++++++++
>  test/unittest/pid/err.needed.x86_64.x  |  6 ++++++
>  test/unittest/pid/tst.entry_off0.sh    |  4 ++--
>  13 files changed, 91 insertions(+), 16 deletions(-)
>  create mode 100755 test/unittest/pid/err.needed.aarch64.x
>  create mode 100755 test/unittest/pid/err.needed.sh
>  create mode 100755 test/unittest/pid/err.needed.x86_64.x
> 
> diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
> index 8ddb1167e..7829e47c0 100644
> --- a/include/dtrace/pid.h
> +++ b/include/dtrace/pid.h
> @@ -47,6 +47,7 @@ typedef struct pid_probespec {
>  	size_t pps_xargvlen;			/* (high estimate of) length of array */
>  	int8_t *pps_argmap;			/* mapped arg indexes */
>  	char *pps_sargv;			/* list of arg sources */
> +	int pps_flags;				/* flags */
>  
>  	/*
>  	 * Fields below this point do not apply to underlying probes.
> @@ -55,4 +56,6 @@ typedef struct pid_probespec {
>  	uint64_t pps_nameoff;			/* offset to use for name */
>  } pid_probespec_t;
>  
> +#define DT_PID_PSP_FLAG_NEEDED	1		/* probe is needed */
> +
>  #endif /* _DTRACE_PID_H */
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 31781ac9f..dc3d47f56 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -1385,7 +1385,7 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
>  		if (prp->prov->impl->attach)
>  			rc = prp->prov->impl->attach(dtp, prp, fd);
>  
> -		if (rc < 0) {
> +		if (rc < 0 && (prp->flags & DT_PROBE_FLAG_NEEDED)) {
>  			close(fd);
>  			dt_attach_error(dtp, rc,
>  					prp->desc->prv, prp->desc->mod,
> diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> index 4af9141aa..6e7fba287 100644
> --- a/libdtrace/dt_pid.c
> +++ b/libdtrace/dt_pid.c
> @@ -184,6 +184,7 @@ dt_pid_per_sym(dt_pid_probe_t *pp, const GElf_Sym *symp, const char *func)
>  	psp->pps_fun = (char *) func;
>  	psp->pps_nameoff = 0;
>  	psp->pps_off = symp->st_value - pp->dpp_vaddr;
> +	psp->pps_flags |= DT_PID_PSP_FLAG_NEEDED;
>  
>  	/*
>  	 * The special function "-" means the probe name is an absolute
> @@ -386,15 +387,10 @@ dt_pid_per_sym(dt_pid_probe_t *pp, const GElf_Sym *symp, const char *func)
>  #define disasm(x, y) 4
>  #endif
>  
> +		psp->pps_flags &= ~DT_PID_PSP_FLAG_NEEDED;
>  		for (off = 0; off < symp->st_size; off += disasm(off, &disasm_info)) {
>  			char offstr[32];
>  
> -#if defined(__amd64)
> -			/* Newer kernels do not allow uprobes on "hlt" instructions. */
> -			if ((unsigned int)disasm_info.buffer[off] == 0xf4)
> -				continue;
> -#endif
> -
>  			snprintf(offstr, sizeof(offstr), "%lx", off);
>  			if (!gmatch(offstr, pp->dpp_name))
>  				continue;
> @@ -1064,6 +1060,7 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, pid_t pid, dt_proc_t *dpr,
>  			psp.pps_pid = dpr->dpr_pid;
>  			psp.pps_off = tp->tracepoint.addr - pmp->pr_file->first_segment->pr_vaddr;
>  			psp.pps_nameoff = 0;
> +			psp.pps_flags |= DT_PID_PSP_FLAG_NEEDED;
>  
>  			if (nargv) {
>  				psp.pps_nargc = probe->probe.nargc;
> @@ -1421,6 +1418,7 @@ dt_stapsdt_parse(dtrace_hdl_t *dtp, dt_proc_t *dpr, dtrace_probedesc_t *pdp,
>  		psp.pps_inum = pmp->pr_inum;
>  		psp.pps_pid = dpr->dpr_pid;
>  		psp.pps_nameoff = 0;
> +		psp.pps_flags |= DT_PID_PSP_FLAG_NEEDED;
>  
>  		if (pvp->impl->provide_probe(dtp, &psp) < 0) {
>  			dt_pid_error(dtp, pcb, dpr, D_PROC_USDT,
> diff --git a/libdtrace/dt_probe.h b/libdtrace/dt_probe.h
> index 2a78cb9ca..ffd994844 100644
> --- a/libdtrace/dt_probe.h
> +++ b/libdtrace/dt_probe.h
> @@ -53,10 +53,13 @@ typedef struct dt_probe {
>  	uint8_t *mapping;		/* translated argument mapping */
>  	dtrace_typeinfo_t *argv;	/* output argument types */
>  	int argc;			/* output argument count */
> +	int flags;			/* flags for the probe */
>  	dt_probe_instance_t *pr_inst;	/* list of functions and offsets */
>  	dtrace_difo_t *difo;		/* BPF probe program */
>  } dt_probe_t;
>  
> +#define DT_PROBE_FLAG_NEEDED	1	/* probe is needed */
> +
>  extern dt_probe_t *dt_probe_lookup2(dt_provider_t *, const char *);
>  extern dt_probe_t *dt_probe_create(dtrace_hdl_t *, dt_ident_t *, int,
>      dt_node_t *, uint_t, dt_node_t *, uint_t);
> diff --git a/libdtrace/dt_prov_cpc.c b/libdtrace/dt_prov_cpc.c
> index 57b11b135..9fb165cb6 100644
> --- a/libdtrace/dt_prov_cpc.c
> +++ b/libdtrace/dt_prov_cpc.c
> @@ -347,6 +347,7 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>  {
>  	dt_provider_t *prv;
>  	struct perf_event_attr attr;
> +	dt_probe_t *prp;
>  
>  	prv = dt_provider_lookup(dtp, prvname);
>  	if (!prv)
> @@ -358,17 +359,22 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>  		return 0;
>  
>  	/* return if we already have this probe */
> -	if (dt_probe_lookup(dtp, pdp))
> +	prp = dt_probe_lookup(dtp, pdp);
> +	if (prp) {
> +		prp->flags |= DT_PROBE_FLAG_NEEDED;
>  		return 0;
> +	}
>  
>  	/* check if the probe name can be decoded */
>  	if (decode_probename(&attr, prv, pdp->prb) == -1)
>  		return 0;
>  
>  	/* try to add this probe */
> -	if (cpc_probe_insert(dtp, prv, pdp->prb) == NULL)
> +	prp = cpc_probe_insert(dtp, prv, pdp->prb);
> +	if (prp == NULL)
>  		return 0;
>  
> +	prp->flags |= DT_PROBE_FLAG_NEEDED;
>  	return 1;
>  }
>  
> diff --git a/libdtrace/dt_prov_profile.c b/libdtrace/dt_prov_profile.c
> index e1369ca9b..ec32ce83b 100644
> --- a/libdtrace/dt_prov_profile.c
> +++ b/libdtrace/dt_prov_profile.c
> @@ -160,6 +160,7 @@ static uint64_t get_period(const char *name)
>  static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>  {
>  	dt_provider_t	*prv;
> +	dt_probe_t	*prp;
>  	int		kind;
>  	uint64_t	period;
>  
> @@ -176,8 +177,11 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>  		return 0;
>  
>  	/* return if we already have this probe */
> -	if (dt_probe_lookup(dtp, pdp))
> +	prp = dt_probe_lookup(dtp, pdp);
> +	if (prp) {
> +		prp->flags |= DT_PROBE_FLAG_NEEDED;
>  		return 0;
> +	}
>  
>  	/* get the provider - should have been created in populate() */
>  	prv = dt_provider_lookup(dtp, prvname);
> @@ -189,9 +193,11 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>  		return 0;
>  
>  	/* try to add this probe */
> -	if (profile_probe_insert(dtp, prv, pdp->prb, kind, period) == NULL)
> +	prp = profile_probe_insert(dtp, prv, pdp->prb, kind, period);
> +	if (prp == NULL)
>  		return 0;
>  
> +	prp->flags |= DT_PROBE_FLAG_NEEDED;
>  	return 1;
>  }
>  
> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index 07b26f20f..2396522ce 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -960,6 +960,12 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>  	} else
>  		upp = uprp->prv_data;
>  
> +	/*
> +	 * Pass flag info from psp to uprp
> +	 */
> +	if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
> +		uprp->flags |= DT_PROBE_FLAG_NEEDED;
> +
>  	/*
>  	 * Only one USDT probe can correspond to each underlying probe.
>  	 */
> @@ -1049,6 +1055,12 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
>  	/* Look up the overlying probe. */
>  	prp = dt_probe_lookup(dtp, &pd);
>  	if (prp != NULL) {
> +		/*
> +		 * Pass flag info from psp to uprp
> +		 */
> +		if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
> +			prp->flags |= DT_PROBE_FLAG_NEEDED;
> +
>  		/*
>  		 * Probe already exists.  If it's already in the underlying
>  		 * probe's probe list, there is nothing left to do.
> @@ -1091,6 +1103,12 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
>  		return -1;
>  	}
>  
> +	/*
> +	 * Pass flag info from psp to uprp
> +	 */
> +	if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
> +		prp->flags |= DT_PROBE_FLAG_NEEDED;
> +
>  	/*
>  	 * Add the overlying probe to the list of probes for the underlying probe.
>  	 */
> diff --git a/libdtrace/dt_provider_sdt.c b/libdtrace/dt_provider_sdt.c
> index 962848485..f9a51fa25 100644
> --- a/libdtrace/dt_provider_sdt.c
> +++ b/libdtrace/dt_provider_sdt.c
> @@ -48,10 +48,14 @@ dt_sdt_populate(dtrace_hdl_t *dtp, const char *prvname, const char *modname,
>  	 * entries to identify the probe names.
>  	 */
>  	for (arg = &probe_args[0]; arg->name != NULL; arg++) {
> +		dt_probe_t *prp;
> +
>  		if (arg->argno == 0 &&
> -		    dt_probe_insert(dtp, prv, prvname, modname, "", arg->name,
> -				    NULL))
> +		    (prp = dt_probe_insert(dtp, prv, prvname, modname,
> +					   "", arg->name, NULL))) {
> +			prp->flags |= DT_PROBE_FLAG_NEEDED;
>  			n++;
> +		}
>  	}
>  
>  	return n;
> diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
> index 3d4ec1e03..4531a88a8 100644
> --- a/libdtrace/dt_provider_tp.c
> +++ b/libdtrace/dt_provider_tp.c
> @@ -368,12 +368,17 @@ dt_tp_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
>  		   const char *mod, const char *fun, const char *prb)
>  {
>  	tp_probe_t	*tpp;
> +	dt_probe_t	*prp;
>  
>  	tpp = dt_tp_alloc(dtp);
>  	if (tpp == NULL)
>  		return NULL;
>  
> -	return dt_probe_insert(dtp, prov, prv, mod, fun, prb, tpp);
> +	prp = dt_probe_insert(dtp, prov, prv, mod, fun, prb, tpp);
> +	if (prp)
> +		prp->flags |= DT_PROBE_FLAG_NEEDED;
> +
> +	return prp;
>  }
>  
>  /*
> diff --git a/test/unittest/pid/err.needed.aarch64.x b/test/unittest/pid/err.needed.aarch64.x
> new file mode 100755
> index 000000000..bd5b41a36
> --- /dev/null
> +++ b/test/unittest/pid/err.needed.aarch64.x
> @@ -0,0 +1,3 @@
> +#!/bin/sh
> +echo "skip on aarch64"
> +exit 2
> diff --git a/test/unittest/pid/err.needed.sh b/test/unittest/pid/err.needed.sh
> new file mode 100755
> index 000000000..d6b041b60
> --- /dev/null
> +++ b/test/unittest/pid/err.needed.sh
> @@ -0,0 +1,23 @@
> +#!/bin/bash
> +#
> +# Oracle Linux DTrace.
> +# Copyright (c) 2025, 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.
> +#
> +
> +# We should not be able to trace on a hlt instruction.
> +#
> +# (When the probe is "needed" -- that is, explicitly requested by the user
> +# -- we should incur an error.  Test tst.entry_off0.sh should be okay,
> +# since dtrace will silently skip over such problematic instructions.)
> +
> +dtrace=$1
> +trig=`pwd`/test/triggers/ustack-tst-basic
> +off=`${OBJDUMP} -d $trig | awk '/hlt/ {sub(/:/, ""); print $1}'`
> +
> +$dtrace $dt_flags -c $trig -n 'pid$target:a.out:-:'$off'
> +{
> +	trace("hlt instruction");
> +	exit(0);
> +}'
> diff --git a/test/unittest/pid/err.needed.x86_64.x b/test/unittest/pid/err.needed.x86_64.x
> new file mode 100755
> index 000000000..158336d98
> --- /dev/null
> +++ b/test/unittest/pid/err.needed.x86_64.x
> @@ -0,0 +1,6 @@
> +#!/bin/sh
> +if ! ${OBJDUMP} -d test/triggers/ustack-tst-basic | grep hlt; then
> +    echo "did not find hlt instruction in trigger"
> +    exit 2
> +fi
> +exit 0
> diff --git a/test/unittest/pid/tst.entry_off0.sh b/test/unittest/pid/tst.entry_off0.sh
> index 1a4c3d207..304be4568 100755
> --- a/test/unittest/pid/tst.entry_off0.sh
> +++ b/test/unittest/pid/tst.entry_off0.sh
> @@ -1,14 +1,14 @@
>  #!/bin/bash
>  #
>  # Oracle Linux DTrace.
> -# Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
> +# Copyright (c) 2024, 2025, 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.
>  #
>  
>  # Although the D script takes only "one second," it takes a long time to
>  # shut down.  Until that has been solved, increase the timeout for the test.
> -# @@timeout: 120
> +# @@timeout: 240
>  
>  dtrace=$1
>  
> -- 
> 2.47.3
> 

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

* Re: [PATCH] Allow probes not to attach if they were not "needed"
  2025-09-17 17:43 ` Kris Van Hees
@ 2025-09-17 20:04   ` Eugene Loh
  2025-09-17 20:58     ` Kris Van Hees
  0 siblings, 1 reply; 4+ messages in thread
From: Eugene Loh @ 2025-09-17 20:04 UTC (permalink / raw)
  To: Kris Van Hees; +Cc: dtrace, dtrace-devel

On 9/17/25 13:43, Kris Van Hees wrote:

> I think this is actually counter-intuitive.

I certainly agree.  And I wrestled with this a lot.

A "default" is overridden if any alternative is specified.  In the case 
of this patch, if a probe comes from two probe descriptions, one that 
requires the probe and one that does not, then the "required" case 
should prevail.  So it is not a default.  There is something non-default 
about needing a probe to attach.

Another complication is that when we walk probes based on a 
user-specified probe description, which is when we decide whether a 
probe will have to attach, we are simply "inserting" probes;  this is 
well before probes are enabled.  Probes are enabled later, when we no 
longer are dealing with user-specified probe descriptions. (If I 
remember correctly.)  So we have to mark probes well before we enable them.

I agree there is something counterintuitive going on here, but I find 
the second list of enablings to be even harder to fathom.

Further, a second list of enablings strikes me as a funny creature. 
Today, a dt_probe_t is really just a dt_list_t with a bunch of extra 
"probe" stuff hanging onto it.  That is, a dt_probe_t is an item on a 
single, particular linked list.  Clearly, we could change it so that a 
probe could exist on two different lists, but that gets disruptive.

I'm not saying that the patch is the only way of solving the underlying 
problem.  I do think, though, that having a reasonable patch that solves 
the problem and tests well is attractive, and there is no need to 
explore other solutions.  "Good enough" is good enough, and it is not 
worth spending time exploring other solutions.  There is so much to do 
and so little time.  I vote for the patch that is working and reasonable.

> In general, probes are expected
> to be able to be attached to or else an error is to be raised.  There are
> exceptional cases where a failure to attach should not be considered an error.
>
> So, I believe that the implementation should by default expect that failure to
> attach is an error.  Introducing a flag is certainly an option (though I would
> expect it a flag to mark the probe as *optional*), though another approach that
> might be worth exploring here is to introduce a 2nd list of enablings, named
> dt_opt_enablings, and add optional probes to this list.  That way you could
> refactor the construction and loading of programs into functions called for a
> given probe, and just loop through the mandatory enablings first, and then the
> optional ones.  That avoids adding a flag and also differentiates the two types
> of enablings explicitly.
>
> On Thu, Sep 04, 2025 at 03:02:26PM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> Various kernels prevent the placement of uprobes on particular
>> instructions -- e.g., on x86 on hlt or multi-byte nops, or on aarch64 on
>> autiasp.  This means, for example, that one cannot place a pid probe on
>> every possible instruction.  If a user explicitly places a probe on a
>> forbidden instruction, an error message indicates there is a problem.
>>
>> On the other hand, if the user specifies a wildcard in a pid probe
>> description, we should skip instructions where no uprobe can be placed.
>>
>> The problem is that we do not find out an instruction is problematic
>> until we try to attach, which is long after probe descriptions have been
>> processed.  And when we walk probe descriptions, we do not have
>> information about which kernels and instructions are problematic.
>>
>> The motivating problem pertains to pid probes, but we would like
>> a provider-agnostic solution.
>>
>> Introduce a "NEEDED" probe flag.  Set the flag when a probe is explicitly
>> needed (say, it was specifically named).  We use two such flags:
>>
>> * In dt_probe.h, DT_PROBE_FLAG_NEEDED is used for the "flags"
>>    member of dt_probe_t.
>>
>> * For pid-style probes (including USDT and stapsdt), in
>>    include/dtrace/pid.h, DT_PID_PSP_FLAG_NEEDED is used for
>>    the pps_flags member of pid_probespec_t.
>>
>> Set the "NEEDED" flag for a probe in these situations:
>>
>> * In dt_tp_probe_insert(), set the flag for each probe we insert.
>>    This covers the dtrace, fbt, rawtp, sdt, and syscall providers.
>>
>> * In dt_sdt_populate(), set the flag for each probe we insert.
>>    This covers, e.g., the io, ip, lockstat, proc, sched, and tcp
>>    providers.
>>
>> * For the cpc and profile providers, set the flag for probes created
>>    with the "provide" functions (explicitly requested by the user)
>>    but not with the "populate" functions (chosen by default by dtrace).
>>
>> * For providers that will sit on underlying probes, set the NEEDED
>>    flag for pid_probespec_t.  Specifically,
>>
>>    * In dt_pid_per_sym(), set the NEEDED flag to cover entry,
>>      return, and explicit offsets.  Unset the flag for glob offsets.
>>      In dt_pid_create_usdt_probes_proc() and dt_stapsdt_parse(),
>>      set the flag.
>>
>>    * In dt_prov_uprobe(), pass the pid_probespec_t NEEDED flag
>>      to dt_probe_t, for both the overlying and underlying probe.
>>
>> In dt_bpf.c, if the return code from the attach function indicates
>> failure, return a failure only if the probe was NEEDED.
>>
>> Remove the old check on "hlt" instructions, since it was not sufficiently
>> general for other kernels and other problematic instructions.
>>
>> Existing tests that expose the problem are
>>
>>      test/unittest/pid/tst.coverage.d
>>      test/unittest/pid/tst.emptystack.d
>>      test/unittest/pid/tst.entry_off0.sh
>>          hlt instruction on x86
>>
>>      test/unittest/pid/tst.float.d
>>          multi-byte nop on x86 on newer platforms
>>
>>      test/unittest/pid/tst.entry_off0.sh
>>          autiasp instruction on aarch64 on newer platforms
>>          depending on how the trigger process was built
>>
>> A new test is added to check that a probe set explicitly on a
>> problematic instruction will still lead to an error.
>>
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>> ---
>>   include/dtrace/pid.h                   |  3 +++
>>   libdtrace/dt_bpf.c                     |  2 +-
>>   libdtrace/dt_pid.c                     | 10 ++++------
>>   libdtrace/dt_probe.h                   |  3 +++
>>   libdtrace/dt_prov_cpc.c                | 10 ++++++++--
>>   libdtrace/dt_prov_profile.c            | 10 ++++++++--
>>   libdtrace/dt_prov_uprobe.c             | 18 ++++++++++++++++++
>>   libdtrace/dt_provider_sdt.c            |  8 ++++++--
>>   libdtrace/dt_provider_tp.c             |  7 ++++++-
>>   test/unittest/pid/err.needed.aarch64.x |  3 +++
>>   test/unittest/pid/err.needed.sh        | 23 +++++++++++++++++++++++
>>   test/unittest/pid/err.needed.x86_64.x  |  6 ++++++
>>   test/unittest/pid/tst.entry_off0.sh    |  4 ++--
>>   13 files changed, 91 insertions(+), 16 deletions(-)
>>   create mode 100755 test/unittest/pid/err.needed.aarch64.x
>>   create mode 100755 test/unittest/pid/err.needed.sh
>>   create mode 100755 test/unittest/pid/err.needed.x86_64.x
>>
>> diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
>> index 8ddb1167e..7829e47c0 100644
>> --- a/include/dtrace/pid.h
>> +++ b/include/dtrace/pid.h
>> @@ -47,6 +47,7 @@ typedef struct pid_probespec {
>>   	size_t pps_xargvlen;			/* (high estimate of) length of array */
>>   	int8_t *pps_argmap;			/* mapped arg indexes */
>>   	char *pps_sargv;			/* list of arg sources */
>> +	int pps_flags;				/* flags */
>>   
>>   	/*
>>   	 * Fields below this point do not apply to underlying probes.
>> @@ -55,4 +56,6 @@ typedef struct pid_probespec {
>>   	uint64_t pps_nameoff;			/* offset to use for name */
>>   } pid_probespec_t;
>>   
>> +#define DT_PID_PSP_FLAG_NEEDED	1		/* probe is needed */
>> +
>>   #endif /* _DTRACE_PID_H */
>> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
>> index 31781ac9f..dc3d47f56 100644
>> --- a/libdtrace/dt_bpf.c
>> +++ b/libdtrace/dt_bpf.c
>> @@ -1385,7 +1385,7 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
>>   		if (prp->prov->impl->attach)
>>   			rc = prp->prov->impl->attach(dtp, prp, fd);
>>   
>> -		if (rc < 0) {
>> +		if (rc < 0 && (prp->flags & DT_PROBE_FLAG_NEEDED)) {
>>   			close(fd);
>>   			dt_attach_error(dtp, rc,
>>   					prp->desc->prv, prp->desc->mod,
>> diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
>> index 4af9141aa..6e7fba287 100644
>> --- a/libdtrace/dt_pid.c
>> +++ b/libdtrace/dt_pid.c
>> @@ -184,6 +184,7 @@ dt_pid_per_sym(dt_pid_probe_t *pp, const GElf_Sym *symp, const char *func)
>>   	psp->pps_fun = (char *) func;
>>   	psp->pps_nameoff = 0;
>>   	psp->pps_off = symp->st_value - pp->dpp_vaddr;
>> +	psp->pps_flags |= DT_PID_PSP_FLAG_NEEDED;
>>   
>>   	/*
>>   	 * The special function "-" means the probe name is an absolute
>> @@ -386,15 +387,10 @@ dt_pid_per_sym(dt_pid_probe_t *pp, const GElf_Sym *symp, const char *func)
>>   #define disasm(x, y) 4
>>   #endif
>>   
>> +		psp->pps_flags &= ~DT_PID_PSP_FLAG_NEEDED;
>>   		for (off = 0; off < symp->st_size; off += disasm(off, &disasm_info)) {
>>   			char offstr[32];
>>   
>> -#if defined(__amd64)
>> -			/* Newer kernels do not allow uprobes on "hlt" instructions. */
>> -			if ((unsigned int)disasm_info.buffer[off] == 0xf4)
>> -				continue;
>> -#endif
>> -
>>   			snprintf(offstr, sizeof(offstr), "%lx", off);
>>   			if (!gmatch(offstr, pp->dpp_name))
>>   				continue;
>> @@ -1064,6 +1060,7 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, pid_t pid, dt_proc_t *dpr,
>>   			psp.pps_pid = dpr->dpr_pid;
>>   			psp.pps_off = tp->tracepoint.addr - pmp->pr_file->first_segment->pr_vaddr;
>>   			psp.pps_nameoff = 0;
>> +			psp.pps_flags |= DT_PID_PSP_FLAG_NEEDED;
>>   
>>   			if (nargv) {
>>   				psp.pps_nargc = probe->probe.nargc;
>> @@ -1421,6 +1418,7 @@ dt_stapsdt_parse(dtrace_hdl_t *dtp, dt_proc_t *dpr, dtrace_probedesc_t *pdp,
>>   		psp.pps_inum = pmp->pr_inum;
>>   		psp.pps_pid = dpr->dpr_pid;
>>   		psp.pps_nameoff = 0;
>> +		psp.pps_flags |= DT_PID_PSP_FLAG_NEEDED;
>>   
>>   		if (pvp->impl->provide_probe(dtp, &psp) < 0) {
>>   			dt_pid_error(dtp, pcb, dpr, D_PROC_USDT,
>> diff --git a/libdtrace/dt_probe.h b/libdtrace/dt_probe.h
>> index 2a78cb9ca..ffd994844 100644
>> --- a/libdtrace/dt_probe.h
>> +++ b/libdtrace/dt_probe.h
>> @@ -53,10 +53,13 @@ typedef struct dt_probe {
>>   	uint8_t *mapping;		/* translated argument mapping */
>>   	dtrace_typeinfo_t *argv;	/* output argument types */
>>   	int argc;			/* output argument count */
>> +	int flags;			/* flags for the probe */
>>   	dt_probe_instance_t *pr_inst;	/* list of functions and offsets */
>>   	dtrace_difo_t *difo;		/* BPF probe program */
>>   } dt_probe_t;
>>   
>> +#define DT_PROBE_FLAG_NEEDED	1	/* probe is needed */
>> +
>>   extern dt_probe_t *dt_probe_lookup2(dt_provider_t *, const char *);
>>   extern dt_probe_t *dt_probe_create(dtrace_hdl_t *, dt_ident_t *, int,
>>       dt_node_t *, uint_t, dt_node_t *, uint_t);
>> diff --git a/libdtrace/dt_prov_cpc.c b/libdtrace/dt_prov_cpc.c
>> index 57b11b135..9fb165cb6 100644
>> --- a/libdtrace/dt_prov_cpc.c
>> +++ b/libdtrace/dt_prov_cpc.c
>> @@ -347,6 +347,7 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>>   {
>>   	dt_provider_t *prv;
>>   	struct perf_event_attr attr;
>> +	dt_probe_t *prp;
>>   
>>   	prv = dt_provider_lookup(dtp, prvname);
>>   	if (!prv)
>> @@ -358,17 +359,22 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>>   		return 0;
>>   
>>   	/* return if we already have this probe */
>> -	if (dt_probe_lookup(dtp, pdp))
>> +	prp = dt_probe_lookup(dtp, pdp);
>> +	if (prp) {
>> +		prp->flags |= DT_PROBE_FLAG_NEEDED;
>>   		return 0;
>> +	}
>>   
>>   	/* check if the probe name can be decoded */
>>   	if (decode_probename(&attr, prv, pdp->prb) == -1)
>>   		return 0;
>>   
>>   	/* try to add this probe */
>> -	if (cpc_probe_insert(dtp, prv, pdp->prb) == NULL)
>> +	prp = cpc_probe_insert(dtp, prv, pdp->prb);
>> +	if (prp == NULL)
>>   		return 0;
>>   
>> +	prp->flags |= DT_PROBE_FLAG_NEEDED;
>>   	return 1;
>>   }
>>   
>> diff --git a/libdtrace/dt_prov_profile.c b/libdtrace/dt_prov_profile.c
>> index e1369ca9b..ec32ce83b 100644
>> --- a/libdtrace/dt_prov_profile.c
>> +++ b/libdtrace/dt_prov_profile.c
>> @@ -160,6 +160,7 @@ static uint64_t get_period(const char *name)
>>   static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>>   {
>>   	dt_provider_t	*prv;
>> +	dt_probe_t	*prp;
>>   	int		kind;
>>   	uint64_t	period;
>>   
>> @@ -176,8 +177,11 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>>   		return 0;
>>   
>>   	/* return if we already have this probe */
>> -	if (dt_probe_lookup(dtp, pdp))
>> +	prp = dt_probe_lookup(dtp, pdp);
>> +	if (prp) {
>> +		prp->flags |= DT_PROBE_FLAG_NEEDED;
>>   		return 0;
>> +	}
>>   
>>   	/* get the provider - should have been created in populate() */
>>   	prv = dt_provider_lookup(dtp, prvname);
>> @@ -189,9 +193,11 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>>   		return 0;
>>   
>>   	/* try to add this probe */
>> -	if (profile_probe_insert(dtp, prv, pdp->prb, kind, period) == NULL)
>> +	prp = profile_probe_insert(dtp, prv, pdp->prb, kind, period);
>> +	if (prp == NULL)
>>   		return 0;
>>   
>> +	prp->flags |= DT_PROBE_FLAG_NEEDED;
>>   	return 1;
>>   }
>>   
>> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
>> index 07b26f20f..2396522ce 100644
>> --- a/libdtrace/dt_prov_uprobe.c
>> +++ b/libdtrace/dt_prov_uprobe.c
>> @@ -960,6 +960,12 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>>   	} else
>>   		upp = uprp->prv_data;
>>   
>> +	/*
>> +	 * Pass flag info from psp to uprp
>> +	 */
>> +	if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
>> +		uprp->flags |= DT_PROBE_FLAG_NEEDED;
>> +
>>   	/*
>>   	 * Only one USDT probe can correspond to each underlying probe.
>>   	 */
>> @@ -1049,6 +1055,12 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
>>   	/* Look up the overlying probe. */
>>   	prp = dt_probe_lookup(dtp, &pd);
>>   	if (prp != NULL) {
>> +		/*
>> +		 * Pass flag info from psp to uprp
>> +		 */
>> +		if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
>> +			prp->flags |= DT_PROBE_FLAG_NEEDED;
>> +
>>   		/*
>>   		 * Probe already exists.  If it's already in the underlying
>>   		 * probe's probe list, there is nothing left to do.
>> @@ -1091,6 +1103,12 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
>>   		return -1;
>>   	}
>>   
>> +	/*
>> +	 * Pass flag info from psp to uprp
>> +	 */
>> +	if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
>> +		prp->flags |= DT_PROBE_FLAG_NEEDED;
>> +
>>   	/*
>>   	 * Add the overlying probe to the list of probes for the underlying probe.
>>   	 */
>> diff --git a/libdtrace/dt_provider_sdt.c b/libdtrace/dt_provider_sdt.c
>> index 962848485..f9a51fa25 100644
>> --- a/libdtrace/dt_provider_sdt.c
>> +++ b/libdtrace/dt_provider_sdt.c
>> @@ -48,10 +48,14 @@ dt_sdt_populate(dtrace_hdl_t *dtp, const char *prvname, const char *modname,
>>   	 * entries to identify the probe names.
>>   	 */
>>   	for (arg = &probe_args[0]; arg->name != NULL; arg++) {
>> +		dt_probe_t *prp;
>> +
>>   		if (arg->argno == 0 &&
>> -		    dt_probe_insert(dtp, prv, prvname, modname, "", arg->name,
>> -				    NULL))
>> +		    (prp = dt_probe_insert(dtp, prv, prvname, modname,
>> +					   "", arg->name, NULL))) {
>> +			prp->flags |= DT_PROBE_FLAG_NEEDED;
>>   			n++;
>> +		}
>>   	}
>>   
>>   	return n;
>> diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
>> index 3d4ec1e03..4531a88a8 100644
>> --- a/libdtrace/dt_provider_tp.c
>> +++ b/libdtrace/dt_provider_tp.c
>> @@ -368,12 +368,17 @@ dt_tp_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
>>   		   const char *mod, const char *fun, const char *prb)
>>   {
>>   	tp_probe_t	*tpp;
>> +	dt_probe_t	*prp;
>>   
>>   	tpp = dt_tp_alloc(dtp);
>>   	if (tpp == NULL)
>>   		return NULL;
>>   
>> -	return dt_probe_insert(dtp, prov, prv, mod, fun, prb, tpp);
>> +	prp = dt_probe_insert(dtp, prov, prv, mod, fun, prb, tpp);
>> +	if (prp)
>> +		prp->flags |= DT_PROBE_FLAG_NEEDED;
>> +
>> +	return prp;
>>   }
>>   
>>   /*
>> diff --git a/test/unittest/pid/err.needed.aarch64.x b/test/unittest/pid/err.needed.aarch64.x
>> new file mode 100755
>> index 000000000..bd5b41a36
>> --- /dev/null
>> +++ b/test/unittest/pid/err.needed.aarch64.x
>> @@ -0,0 +1,3 @@
>> +#!/bin/sh
>> +echo "skip on aarch64"
>> +exit 2
>> diff --git a/test/unittest/pid/err.needed.sh b/test/unittest/pid/err.needed.sh
>> new file mode 100755
>> index 000000000..d6b041b60
>> --- /dev/null
>> +++ b/test/unittest/pid/err.needed.sh
>> @@ -0,0 +1,23 @@
>> +#!/bin/bash
>> +#
>> +# Oracle Linux DTrace.
>> +# Copyright (c) 2025, 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.
>> +#
>> +
>> +# We should not be able to trace on a hlt instruction.
>> +#
>> +# (When the probe is "needed" -- that is, explicitly requested by the user
>> +# -- we should incur an error.  Test tst.entry_off0.sh should be okay,
>> +# since dtrace will silently skip over such problematic instructions.)
>> +
>> +dtrace=$1
>> +trig=`pwd`/test/triggers/ustack-tst-basic
>> +off=`${OBJDUMP} -d $trig | awk '/hlt/ {sub(/:/, ""); print $1}'`
>> +
>> +$dtrace $dt_flags -c $trig -n 'pid$target:a.out:-:'$off'
>> +{
>> +	trace("hlt instruction");
>> +	exit(0);
>> +}'
>> diff --git a/test/unittest/pid/err.needed.x86_64.x b/test/unittest/pid/err.needed.x86_64.x
>> new file mode 100755
>> index 000000000..158336d98
>> --- /dev/null
>> +++ b/test/unittest/pid/err.needed.x86_64.x
>> @@ -0,0 +1,6 @@
>> +#!/bin/sh
>> +if ! ${OBJDUMP} -d test/triggers/ustack-tst-basic | grep hlt; then
>> +    echo "did not find hlt instruction in trigger"
>> +    exit 2
>> +fi
>> +exit 0
>> diff --git a/test/unittest/pid/tst.entry_off0.sh b/test/unittest/pid/tst.entry_off0.sh
>> index 1a4c3d207..304be4568 100755
>> --- a/test/unittest/pid/tst.entry_off0.sh
>> +++ b/test/unittest/pid/tst.entry_off0.sh
>> @@ -1,14 +1,14 @@
>>   #!/bin/bash
>>   #
>>   # Oracle Linux DTrace.
>> -# Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
>> +# Copyright (c) 2024, 2025, 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.
>>   #
>>   
>>   # Although the D script takes only "one second," it takes a long time to
>>   # shut down.  Until that has been solved, increase the timeout for the test.
>> -# @@timeout: 120
>> +# @@timeout: 240
>>   
>>   dtrace=$1
>>   
>> -- 
>> 2.47.3
>>

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

* Re: [PATCH] Allow probes not to attach if they were not "needed"
  2025-09-17 20:04   ` Eugene Loh
@ 2025-09-17 20:58     ` Kris Van Hees
  0 siblings, 0 replies; 4+ messages in thread
From: Kris Van Hees @ 2025-09-17 20:58 UTC (permalink / raw)
  To: Eugene Loh; +Cc: Kris Van Hees, dtrace, dtrace-devel

I am fine not doing two lists, and using a flag, but then I still believe the
flag should be about marking the probe optional rather than needed.  A probe
attach failure being non-fatal is actually not normal behaviour, but rather an
internal aspect of the implementation.  E.g. we do this with underlying probes
for SDT probes where we may have multiple probes listed that can feed us the
info we need, and we don't mind if some fail to attach.  Your example of the
wildcard instruction offset probes is another.

But all those cases are "known" in the code and thus we can mark probes as
optional when these cases occur.  Any other case that adds one of these probes
to the list of probes to enable (attach to) would require the probe to be able
to attach and else we falg an error.

If a probe is being added and attachment can be optional, we need to check
of the probe is already listed.  If it is, and it is not optional, then we do
not mark it optional.

If a probe is being added and attachment can be optional, we need to check
of the probe is already listed.  If it is, and it is optional, then we do not
need to mark it optional.

If a probe is being added and attachment can be optional, we need to check
of the probe is already listed.  If it is not listed, we add it as optional.

If a probe is being added and attachment is not optional, and the probe already
exists as optional, remove the optional flag.

If a probe is being added and attachment is not optional, and the probe already
exists and is not optional, nothing is to be done.

If a probe is being added and attachment is not optional, and the probe is not
in the list yet, just add it.

I do not see where this would go wrong.

On Wed, Sep 17, 2025 at 04:04:57PM -0400, Eugene Loh wrote:
> On 9/17/25 13:43, Kris Van Hees wrote:
> 
> > I think this is actually counter-intuitive.
> 
> I certainly agree.  And I wrestled with this a lot.
> 
> A "default" is overridden if any alternative is specified.  In the case of
> this patch, if a probe comes from two probe descriptions, one that requires
> the probe and one that does not, then the "required" case should prevail. 
> So it is not a default.  There is something non-default about needing a
> probe to attach.
> 
> Another complication is that when we walk probes based on a user-specified
> probe description, which is when we decide whether a probe will have to
> attach, we are simply "inserting" probes;  this is well before probes are
> enabled.  Probes are enabled later, when we no longer are dealing with
> user-specified probe descriptions. (If I remember correctly.)  So we have to
> mark probes well before we enable them.
> 
> I agree there is something counterintuitive going on here, but I find the
> second list of enablings to be even harder to fathom.
> 
> Further, a second list of enablings strikes me as a funny creature. Today, a
> dt_probe_t is really just a dt_list_t with a bunch of extra "probe" stuff
> hanging onto it.  That is, a dt_probe_t is an item on a single, particular
> linked list.  Clearly, we could change it so that a probe could exist on two
> different lists, but that gets disruptive.
> 
> I'm not saying that the patch is the only way of solving the underlying
> problem.  I do think, though, that having a reasonable patch that solves the
> problem and tests well is attractive, and there is no need to explore other
> solutions.  "Good enough" is good enough, and it is not worth spending time
> exploring other solutions.  There is so much to do and so little time.  I
> vote for the patch that is working and reasonable.
> 
> > In general, probes are expected
> > to be able to be attached to or else an error is to be raised.  There are
> > exceptional cases where a failure to attach should not be considered an error.
> > 
> > So, I believe that the implementation should by default expect that failure to
> > attach is an error.  Introducing a flag is certainly an option (though I would
> > expect it a flag to mark the probe as *optional*), though another approach that
> > might be worth exploring here is to introduce a 2nd list of enablings, named
> > dt_opt_enablings, and add optional probes to this list.  That way you could
> > refactor the construction and loading of programs into functions called for a
> > given probe, and just loop through the mandatory enablings first, and then the
> > optional ones.  That avoids adding a flag and also differentiates the two types
> > of enablings explicitly.
> > 
> > On Thu, Sep 04, 2025 at 03:02:26PM -0400, eugene.loh@oracle.com wrote:
> > > From: Eugene Loh <eugene.loh@oracle.com>
> > > 
> > > Various kernels prevent the placement of uprobes on particular
> > > instructions -- e.g., on x86 on hlt or multi-byte nops, or on aarch64 on
> > > autiasp.  This means, for example, that one cannot place a pid probe on
> > > every possible instruction.  If a user explicitly places a probe on a
> > > forbidden instruction, an error message indicates there is a problem.
> > > 
> > > On the other hand, if the user specifies a wildcard in a pid probe
> > > description, we should skip instructions where no uprobe can be placed.
> > > 
> > > The problem is that we do not find out an instruction is problematic
> > > until we try to attach, which is long after probe descriptions have been
> > > processed.  And when we walk probe descriptions, we do not have
> > > information about which kernels and instructions are problematic.
> > > 
> > > The motivating problem pertains to pid probes, but we would like
> > > a provider-agnostic solution.
> > > 
> > > Introduce a "NEEDED" probe flag.  Set the flag when a probe is explicitly
> > > needed (say, it was specifically named).  We use two such flags:
> > > 
> > > * In dt_probe.h, DT_PROBE_FLAG_NEEDED is used for the "flags"
> > >    member of dt_probe_t.
> > > 
> > > * For pid-style probes (including USDT and stapsdt), in
> > >    include/dtrace/pid.h, DT_PID_PSP_FLAG_NEEDED is used for
> > >    the pps_flags member of pid_probespec_t.
> > > 
> > > Set the "NEEDED" flag for a probe in these situations:
> > > 
> > > * In dt_tp_probe_insert(), set the flag for each probe we insert.
> > >    This covers the dtrace, fbt, rawtp, sdt, and syscall providers.
> > > 
> > > * In dt_sdt_populate(), set the flag for each probe we insert.
> > >    This covers, e.g., the io, ip, lockstat, proc, sched, and tcp
> > >    providers.
> > > 
> > > * For the cpc and profile providers, set the flag for probes created
> > >    with the "provide" functions (explicitly requested by the user)
> > >    but not with the "populate" functions (chosen by default by dtrace).
> > > 
> > > * For providers that will sit on underlying probes, set the NEEDED
> > >    flag for pid_probespec_t.  Specifically,
> > > 
> > >    * In dt_pid_per_sym(), set the NEEDED flag to cover entry,
> > >      return, and explicit offsets.  Unset the flag for glob offsets.
> > >      In dt_pid_create_usdt_probes_proc() and dt_stapsdt_parse(),
> > >      set the flag.
> > > 
> > >    * In dt_prov_uprobe(), pass the pid_probespec_t NEEDED flag
> > >      to dt_probe_t, for both the overlying and underlying probe.
> > > 
> > > In dt_bpf.c, if the return code from the attach function indicates
> > > failure, return a failure only if the probe was NEEDED.
> > > 
> > > Remove the old check on "hlt" instructions, since it was not sufficiently
> > > general for other kernels and other problematic instructions.
> > > 
> > > Existing tests that expose the problem are
> > > 
> > >      test/unittest/pid/tst.coverage.d
> > >      test/unittest/pid/tst.emptystack.d
> > >      test/unittest/pid/tst.entry_off0.sh
> > >          hlt instruction on x86
> > > 
> > >      test/unittest/pid/tst.float.d
> > >          multi-byte nop on x86 on newer platforms
> > > 
> > >      test/unittest/pid/tst.entry_off0.sh
> > >          autiasp instruction on aarch64 on newer platforms
> > >          depending on how the trigger process was built
> > > 
> > > A new test is added to check that a probe set explicitly on a
> > > problematic instruction will still lead to an error.
> > > 
> > > Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> > > ---
> > >   include/dtrace/pid.h                   |  3 +++
> > >   libdtrace/dt_bpf.c                     |  2 +-
> > >   libdtrace/dt_pid.c                     | 10 ++++------
> > >   libdtrace/dt_probe.h                   |  3 +++
> > >   libdtrace/dt_prov_cpc.c                | 10 ++++++++--
> > >   libdtrace/dt_prov_profile.c            | 10 ++++++++--
> > >   libdtrace/dt_prov_uprobe.c             | 18 ++++++++++++++++++
> > >   libdtrace/dt_provider_sdt.c            |  8 ++++++--
> > >   libdtrace/dt_provider_tp.c             |  7 ++++++-
> > >   test/unittest/pid/err.needed.aarch64.x |  3 +++
> > >   test/unittest/pid/err.needed.sh        | 23 +++++++++++++++++++++++
> > >   test/unittest/pid/err.needed.x86_64.x  |  6 ++++++
> > >   test/unittest/pid/tst.entry_off0.sh    |  4 ++--
> > >   13 files changed, 91 insertions(+), 16 deletions(-)
> > >   create mode 100755 test/unittest/pid/err.needed.aarch64.x
> > >   create mode 100755 test/unittest/pid/err.needed.sh
> > >   create mode 100755 test/unittest/pid/err.needed.x86_64.x
> > > 
> > > diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
> > > index 8ddb1167e..7829e47c0 100644
> > > --- a/include/dtrace/pid.h
> > > +++ b/include/dtrace/pid.h
> > > @@ -47,6 +47,7 @@ typedef struct pid_probespec {
> > >   	size_t pps_xargvlen;			/* (high estimate of) length of array */
> > >   	int8_t *pps_argmap;			/* mapped arg indexes */
> > >   	char *pps_sargv;			/* list of arg sources */
> > > +	int pps_flags;				/* flags */
> > >   	/*
> > >   	 * Fields below this point do not apply to underlying probes.
> > > @@ -55,4 +56,6 @@ typedef struct pid_probespec {
> > >   	uint64_t pps_nameoff;			/* offset to use for name */
> > >   } pid_probespec_t;
> > > +#define DT_PID_PSP_FLAG_NEEDED	1		/* probe is needed */
> > > +
> > >   #endif /* _DTRACE_PID_H */
> > > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > > index 31781ac9f..dc3d47f56 100644
> > > --- a/libdtrace/dt_bpf.c
> > > +++ b/libdtrace/dt_bpf.c
> > > @@ -1385,7 +1385,7 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
> > >   		if (prp->prov->impl->attach)
> > >   			rc = prp->prov->impl->attach(dtp, prp, fd);
> > > -		if (rc < 0) {
> > > +		if (rc < 0 && (prp->flags & DT_PROBE_FLAG_NEEDED)) {
> > >   			close(fd);
> > >   			dt_attach_error(dtp, rc,
> > >   					prp->desc->prv, prp->desc->mod,
> > > diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> > > index 4af9141aa..6e7fba287 100644
> > > --- a/libdtrace/dt_pid.c
> > > +++ b/libdtrace/dt_pid.c
> > > @@ -184,6 +184,7 @@ dt_pid_per_sym(dt_pid_probe_t *pp, const GElf_Sym *symp, const char *func)
> > >   	psp->pps_fun = (char *) func;
> > >   	psp->pps_nameoff = 0;
> > >   	psp->pps_off = symp->st_value - pp->dpp_vaddr;
> > > +	psp->pps_flags |= DT_PID_PSP_FLAG_NEEDED;
> > >   	/*
> > >   	 * The special function "-" means the probe name is an absolute
> > > @@ -386,15 +387,10 @@ dt_pid_per_sym(dt_pid_probe_t *pp, const GElf_Sym *symp, const char *func)
> > >   #define disasm(x, y) 4
> > >   #endif
> > > +		psp->pps_flags &= ~DT_PID_PSP_FLAG_NEEDED;
> > >   		for (off = 0; off < symp->st_size; off += disasm(off, &disasm_info)) {
> > >   			char offstr[32];
> > > -#if defined(__amd64)
> > > -			/* Newer kernels do not allow uprobes on "hlt" instructions. */
> > > -			if ((unsigned int)disasm_info.buffer[off] == 0xf4)
> > > -				continue;
> > > -#endif
> > > -
> > >   			snprintf(offstr, sizeof(offstr), "%lx", off);
> > >   			if (!gmatch(offstr, pp->dpp_name))
> > >   				continue;
> > > @@ -1064,6 +1060,7 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, pid_t pid, dt_proc_t *dpr,
> > >   			psp.pps_pid = dpr->dpr_pid;
> > >   			psp.pps_off = tp->tracepoint.addr - pmp->pr_file->first_segment->pr_vaddr;
> > >   			psp.pps_nameoff = 0;
> > > +			psp.pps_flags |= DT_PID_PSP_FLAG_NEEDED;
> > >   			if (nargv) {
> > >   				psp.pps_nargc = probe->probe.nargc;
> > > @@ -1421,6 +1418,7 @@ dt_stapsdt_parse(dtrace_hdl_t *dtp, dt_proc_t *dpr, dtrace_probedesc_t *pdp,
> > >   		psp.pps_inum = pmp->pr_inum;
> > >   		psp.pps_pid = dpr->dpr_pid;
> > >   		psp.pps_nameoff = 0;
> > > +		psp.pps_flags |= DT_PID_PSP_FLAG_NEEDED;
> > >   		if (pvp->impl->provide_probe(dtp, &psp) < 0) {
> > >   			dt_pid_error(dtp, pcb, dpr, D_PROC_USDT,
> > > diff --git a/libdtrace/dt_probe.h b/libdtrace/dt_probe.h
> > > index 2a78cb9ca..ffd994844 100644
> > > --- a/libdtrace/dt_probe.h
> > > +++ b/libdtrace/dt_probe.h
> > > @@ -53,10 +53,13 @@ typedef struct dt_probe {
> > >   	uint8_t *mapping;		/* translated argument mapping */
> > >   	dtrace_typeinfo_t *argv;	/* output argument types */
> > >   	int argc;			/* output argument count */
> > > +	int flags;			/* flags for the probe */
> > >   	dt_probe_instance_t *pr_inst;	/* list of functions and offsets */
> > >   	dtrace_difo_t *difo;		/* BPF probe program */
> > >   } dt_probe_t;
> > > +#define DT_PROBE_FLAG_NEEDED	1	/* probe is needed */
> > > +
> > >   extern dt_probe_t *dt_probe_lookup2(dt_provider_t *, const char *);
> > >   extern dt_probe_t *dt_probe_create(dtrace_hdl_t *, dt_ident_t *, int,
> > >       dt_node_t *, uint_t, dt_node_t *, uint_t);
> > > diff --git a/libdtrace/dt_prov_cpc.c b/libdtrace/dt_prov_cpc.c
> > > index 57b11b135..9fb165cb6 100644
> > > --- a/libdtrace/dt_prov_cpc.c
> > > +++ b/libdtrace/dt_prov_cpc.c
> > > @@ -347,6 +347,7 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
> > >   {
> > >   	dt_provider_t *prv;
> > >   	struct perf_event_attr attr;
> > > +	dt_probe_t *prp;
> > >   	prv = dt_provider_lookup(dtp, prvname);
> > >   	if (!prv)
> > > @@ -358,17 +359,22 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
> > >   		return 0;
> > >   	/* return if we already have this probe */
> > > -	if (dt_probe_lookup(dtp, pdp))
> > > +	prp = dt_probe_lookup(dtp, pdp);
> > > +	if (prp) {
> > > +		prp->flags |= DT_PROBE_FLAG_NEEDED;
> > >   		return 0;
> > > +	}
> > >   	/* check if the probe name can be decoded */
> > >   	if (decode_probename(&attr, prv, pdp->prb) == -1)
> > >   		return 0;
> > >   	/* try to add this probe */
> > > -	if (cpc_probe_insert(dtp, prv, pdp->prb) == NULL)
> > > +	prp = cpc_probe_insert(dtp, prv, pdp->prb);
> > > +	if (prp == NULL)
> > >   		return 0;
> > > +	prp->flags |= DT_PROBE_FLAG_NEEDED;
> > >   	return 1;
> > >   }
> > > diff --git a/libdtrace/dt_prov_profile.c b/libdtrace/dt_prov_profile.c
> > > index e1369ca9b..ec32ce83b 100644
> > > --- a/libdtrace/dt_prov_profile.c
> > > +++ b/libdtrace/dt_prov_profile.c
> > > @@ -160,6 +160,7 @@ static uint64_t get_period(const char *name)
> > >   static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
> > >   {
> > >   	dt_provider_t	*prv;
> > > +	dt_probe_t	*prp;
> > >   	int		kind;
> > >   	uint64_t	period;
> > > @@ -176,8 +177,11 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
> > >   		return 0;
> > >   	/* return if we already have this probe */
> > > -	if (dt_probe_lookup(dtp, pdp))
> > > +	prp = dt_probe_lookup(dtp, pdp);
> > > +	if (prp) {
> > > +		prp->flags |= DT_PROBE_FLAG_NEEDED;
> > >   		return 0;
> > > +	}
> > >   	/* get the provider - should have been created in populate() */
> > >   	prv = dt_provider_lookup(dtp, prvname);
> > > @@ -189,9 +193,11 @@ static int provide(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
> > >   		return 0;
> > >   	/* try to add this probe */
> > > -	if (profile_probe_insert(dtp, prv, pdp->prb, kind, period) == NULL)
> > > +	prp = profile_probe_insert(dtp, prv, pdp->prb, kind, period);
> > > +	if (prp == NULL)
> > >   		return 0;
> > > +	prp->flags |= DT_PROBE_FLAG_NEEDED;
> > >   	return 1;
> > >   }
> > > diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> > > index 07b26f20f..2396522ce 100644
> > > --- a/libdtrace/dt_prov_uprobe.c
> > > +++ b/libdtrace/dt_prov_uprobe.c
> > > @@ -960,6 +960,12 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> > >   	} else
> > >   		upp = uprp->prv_data;
> > > +	/*
> > > +	 * Pass flag info from psp to uprp
> > > +	 */
> > > +	if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
> > > +		uprp->flags |= DT_PROBE_FLAG_NEEDED;
> > > +
> > >   	/*
> > >   	 * Only one USDT probe can correspond to each underlying probe.
> > >   	 */
> > > @@ -1049,6 +1055,12 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
> > >   	/* Look up the overlying probe. */
> > >   	prp = dt_probe_lookup(dtp, &pd);
> > >   	if (prp != NULL) {
> > > +		/*
> > > +		 * Pass flag info from psp to uprp
> > > +		 */
> > > +		if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
> > > +			prp->flags |= DT_PROBE_FLAG_NEEDED;
> > > +
> > >   		/*
> > >   		 * Probe already exists.  If it's already in the underlying
> > >   		 * probe's probe list, there is nothing left to do.
> > > @@ -1091,6 +1103,12 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
> > >   		return -1;
> > >   	}
> > > +	/*
> > > +	 * Pass flag info from psp to uprp
> > > +	 */
> > > +	if (psp->pps_flags & DT_PID_PSP_FLAG_NEEDED)
> > > +		prp->flags |= DT_PROBE_FLAG_NEEDED;
> > > +
> > >   	/*
> > >   	 * Add the overlying probe to the list of probes for the underlying probe.
> > >   	 */
> > > diff --git a/libdtrace/dt_provider_sdt.c b/libdtrace/dt_provider_sdt.c
> > > index 962848485..f9a51fa25 100644
> > > --- a/libdtrace/dt_provider_sdt.c
> > > +++ b/libdtrace/dt_provider_sdt.c
> > > @@ -48,10 +48,14 @@ dt_sdt_populate(dtrace_hdl_t *dtp, const char *prvname, const char *modname,
> > >   	 * entries to identify the probe names.
> > >   	 */
> > >   	for (arg = &probe_args[0]; arg->name != NULL; arg++) {
> > > +		dt_probe_t *prp;
> > > +
> > >   		if (arg->argno == 0 &&
> > > -		    dt_probe_insert(dtp, prv, prvname, modname, "", arg->name,
> > > -				    NULL))
> > > +		    (prp = dt_probe_insert(dtp, prv, prvname, modname,
> > > +					   "", arg->name, NULL))) {
> > > +			prp->flags |= DT_PROBE_FLAG_NEEDED;
> > >   			n++;
> > > +		}
> > >   	}
> > >   	return n;
> > > diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
> > > index 3d4ec1e03..4531a88a8 100644
> > > --- a/libdtrace/dt_provider_tp.c
> > > +++ b/libdtrace/dt_provider_tp.c
> > > @@ -368,12 +368,17 @@ dt_tp_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
> > >   		   const char *mod, const char *fun, const char *prb)
> > >   {
> > >   	tp_probe_t	*tpp;
> > > +	dt_probe_t	*prp;
> > >   	tpp = dt_tp_alloc(dtp);
> > >   	if (tpp == NULL)
> > >   		return NULL;
> > > -	return dt_probe_insert(dtp, prov, prv, mod, fun, prb, tpp);
> > > +	prp = dt_probe_insert(dtp, prov, prv, mod, fun, prb, tpp);
> > > +	if (prp)
> > > +		prp->flags |= DT_PROBE_FLAG_NEEDED;
> > > +
> > > +	return prp;
> > >   }
> > >   /*
> > > diff --git a/test/unittest/pid/err.needed.aarch64.x b/test/unittest/pid/err.needed.aarch64.x
> > > new file mode 100755
> > > index 000000000..bd5b41a36
> > > --- /dev/null
> > > +++ b/test/unittest/pid/err.needed.aarch64.x
> > > @@ -0,0 +1,3 @@
> > > +#!/bin/sh
> > > +echo "skip on aarch64"
> > > +exit 2
> > > diff --git a/test/unittest/pid/err.needed.sh b/test/unittest/pid/err.needed.sh
> > > new file mode 100755
> > > index 000000000..d6b041b60
> > > --- /dev/null
> > > +++ b/test/unittest/pid/err.needed.sh
> > > @@ -0,0 +1,23 @@
> > > +#!/bin/bash
> > > +#
> > > +# Oracle Linux DTrace.
> > > +# Copyright (c) 2025, 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.
> > > +#
> > > +
> > > +# We should not be able to trace on a hlt instruction.
> > > +#
> > > +# (When the probe is "needed" -- that is, explicitly requested by the user
> > > +# -- we should incur an error.  Test tst.entry_off0.sh should be okay,
> > > +# since dtrace will silently skip over such problematic instructions.)
> > > +
> > > +dtrace=$1
> > > +trig=`pwd`/test/triggers/ustack-tst-basic
> > > +off=`${OBJDUMP} -d $trig | awk '/hlt/ {sub(/:/, ""); print $1}'`
> > > +
> > > +$dtrace $dt_flags -c $trig -n 'pid$target:a.out:-:'$off'
> > > +{
> > > +	trace("hlt instruction");
> > > +	exit(0);
> > > +}'
> > > diff --git a/test/unittest/pid/err.needed.x86_64.x b/test/unittest/pid/err.needed.x86_64.x
> > > new file mode 100755
> > > index 000000000..158336d98
> > > --- /dev/null
> > > +++ b/test/unittest/pid/err.needed.x86_64.x
> > > @@ -0,0 +1,6 @@
> > > +#!/bin/sh
> > > +if ! ${OBJDUMP} -d test/triggers/ustack-tst-basic | grep hlt; then
> > > +    echo "did not find hlt instruction in trigger"
> > > +    exit 2
> > > +fi
> > > +exit 0
> > > diff --git a/test/unittest/pid/tst.entry_off0.sh b/test/unittest/pid/tst.entry_off0.sh
> > > index 1a4c3d207..304be4568 100755
> > > --- a/test/unittest/pid/tst.entry_off0.sh
> > > +++ b/test/unittest/pid/tst.entry_off0.sh
> > > @@ -1,14 +1,14 @@
> > >   #!/bin/bash
> > >   #
> > >   # Oracle Linux DTrace.
> > > -# Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
> > > +# Copyright (c) 2024, 2025, 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.
> > >   #
> > >   # Although the D script takes only "one second," it takes a long time to
> > >   # shut down.  Until that has been solved, increase the timeout for the test.
> > > -# @@timeout: 120
> > > +# @@timeout: 240
> > >   dtrace=$1
> > > -- 
> > > 2.47.3
> > > 

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

end of thread, other threads:[~2025-09-17 20:58 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-04 19:02 [PATCH] Allow probes not to attach if they were not "needed" eugene.loh
2025-09-17 17:43 ` Kris Van Hees
2025-09-17 20:04   ` Eugene Loh
2025-09-17 20:58     ` Kris Van Hees

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox