Re: [PATCH] Allow probes not to attach if they were not "needed"

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, [email protected] wrote:
> From: Eugene Loh <[email protected]>
> 
> 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 <[email protected]>
> ---
>  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
>