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

On Thu, Sep 18, 2025 at 02:02:13AM -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
> name, 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.
> 
> Set an "OPTIONAL" probe flag when a probe is not needed:
> 
> * In dt_probe.h, DT_PROBE_FLAG_OPTIONAL 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_OPTIONAL is used for
>   the pps_flags member of pid_probespec_t.
> 
> 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
> 
> New tests are added to check that a probe set explicitly on a
> problematic instruction will still lead to an error, even if a
> probe description before or after it tolerates attach failure.
> 
> Signed-off-by: Eugene Loh <[email protected]>

Reviewed-by: Kris Van Hees <[email protected]>

> ---
>  include/dtrace/pid.h                          |  3 +++
>  libdtrace/dt_bpf.c                            |  2 +-
>  libdtrace/dt_pid.c                            |  7 +----
>  libdtrace/dt_probe.h                          |  3 +++
>  libdtrace/dt_prov_uprobe.c                    | 26 ++++++++++++++++---
>  .../pid/err.opt_after_needed.aarch64.x        |  3 +++
>  test/unittest/pid/err.opt_after_needed.sh     | 25 ++++++++++++++++++
>  .../pid/err.opt_after_needed.x86_64.x         |  6 +++++
>  .../pid/err.opt_before_needed.aarch64.x       |  3 +++
>  test/unittest/pid/err.opt_before_needed.sh    | 25 ++++++++++++++++++
>  .../pid/err.opt_before_needed.x86_64.x        |  6 +++++
>  test/unittest/pid/err.optional.aarch64.x      |  3 +++
>  test/unittest/pid/err.optional.sh             | 25 ++++++++++++++++++
>  test/unittest/pid/err.optional.x86_64.x       |  6 +++++
>  test/unittest/pid/tst.entry_off0.sh           |  4 +--
>  15 files changed, 135 insertions(+), 12 deletions(-)
>  create mode 100755 test/unittest/pid/err.opt_after_needed.aarch64.x
>  create mode 100755 test/unittest/pid/err.opt_after_needed.sh
>  create mode 100755 test/unittest/pid/err.opt_after_needed.x86_64.x
>  create mode 100755 test/unittest/pid/err.opt_before_needed.aarch64.x
>  create mode 100755 test/unittest/pid/err.opt_before_needed.sh
>  create mode 100755 test/unittest/pid/err.opt_before_needed.x86_64.x
>  create mode 100755 test/unittest/pid/err.optional.aarch64.x
>  create mode 100755 test/unittest/pid/err.optional.sh
>  create mode 100755 test/unittest/pid/err.optional.x86_64.x
> 
> diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
> index 8ddb1167e..4a239b076 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_OPTIONAL	1	/* probe is optional */
> +
>  #endif /* _DTRACE_PID_H */
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 31781ac9f..0223764a6 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_OPTIONAL)) {
>  			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 ffc52132f..7d6cfb4dc 100644
> --- a/libdtrace/dt_pid.c
> +++ b/libdtrace/dt_pid.c
> @@ -386,15 +386,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_OPTIONAL;
>  		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;
> diff --git a/libdtrace/dt_probe.h b/libdtrace/dt_probe.h
> index 2a78cb9ca..637875183 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_OPTIONAL	1	/* probe is optional */
> +
>  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_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index 07b26f20f..3b7145a50 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -957,9 +957,16 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>  				       upp);
>  		if (uprp == NULL)
>  			goto fail;
> -	} else
> +
> +		if (psp->pps_flags & DT_PID_PSP_FLAG_OPTIONAL)
> +			uprp->flags |= DT_PROBE_FLAG_OPTIONAL;
> +	} else {
>  		upp = uprp->prv_data;
>  
> +		if (!(psp->pps_flags & DT_PID_PSP_FLAG_OPTIONAL))
> +			uprp->flags &= ~DT_PROBE_FLAG_OPTIONAL;
> +	}
> +
>  	/*
>  	 * Only one USDT probe can correspond to each underlying probe.
>  	 */
> @@ -1049,6 +1056,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) {
> +		/*
> +		 * If not optional, pass that info on.
> +		 */
> +		if (!(psp->pps_flags & DT_PID_PSP_FLAG_OPTIONAL))
> +			prp->flags &= ~DT_PROBE_FLAG_OPTIONAL;
> +
>  		/*
>  		 * Probe already exists.  If it's already in the underlying
>  		 * probe's probe list, there is nothing left to do.
> @@ -1079,10 +1092,17 @@ static int provide_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
>  	 */
>  
>  	pup->probe = uprp;
> -	if (prp == NULL)
> +	if (prp == NULL) {
>  		prp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
>  				      pup);
> -	else
> +
> +		/*
> +		 * If not optional, pass that info on.
> +		 */
> +		if (psp->pps_flags & DT_PID_PSP_FLAG_OPTIONAL)
> +			prp->flags |= DT_PROBE_FLAG_OPTIONAL;
> +
> +	} else
>  		dt_list_append((dt_list_t *)prp->prv_data, pup);
>  
>  	if (prp == NULL) {
> diff --git a/test/unittest/pid/err.opt_after_needed.aarch64.x b/test/unittest/pid/err.opt_after_needed.aarch64.x
> new file mode 100755
> index 000000000..bd5b41a36
> --- /dev/null
> +++ b/test/unittest/pid/err.opt_after_needed.aarch64.x
> @@ -0,0 +1,3 @@
> +#!/bin/sh
> +echo "skip on aarch64"
> +exit 2
> diff --git a/test/unittest/pid/err.opt_after_needed.sh b/test/unittest/pid/err.opt_after_needed.sh
> new file mode 100755
> index 000000000..3ee65cd93
> --- /dev/null
> +++ b/test/unittest/pid/err.opt_after_needed.sh
> @@ -0,0 +1,25 @@
> +#!/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, which is forbidden
> +# by the kernel.
> +#
> +# (Test tst.entry_off0.sh should be okay, since there the probe on that
> +# instruction is optional, and dtrace will silently skip over such
> +# problematic instructions.  Here, however, the probe on the instruction
> +# is explicitly requested by the user.)
> +
> +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',pid$target:a.out::
> +{
> +	trace("hlt instruction");
> +	exit(0);
> +}'
> diff --git a/test/unittest/pid/err.opt_after_needed.x86_64.x b/test/unittest/pid/err.opt_after_needed.x86_64.x
> new file mode 100755
> index 000000000..158336d98
> --- /dev/null
> +++ b/test/unittest/pid/err.opt_after_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/err.opt_before_needed.aarch64.x b/test/unittest/pid/err.opt_before_needed.aarch64.x
> new file mode 100755
> index 000000000..bd5b41a36
> --- /dev/null
> +++ b/test/unittest/pid/err.opt_before_needed.aarch64.x
> @@ -0,0 +1,3 @@
> +#!/bin/sh
> +echo "skip on aarch64"
> +exit 2
> diff --git a/test/unittest/pid/err.opt_before_needed.sh b/test/unittest/pid/err.opt_before_needed.sh
> new file mode 100755
> index 000000000..f1f172a18
> --- /dev/null
> +++ b/test/unittest/pid/err.opt_before_needed.sh
> @@ -0,0 +1,25 @@
> +#!/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, which is forbidden
> +# by the kernel.
> +#
> +# (Test tst.entry_off0.sh should be okay, since there the probe on that
> +# instruction is optional, and dtrace will silently skip over such
> +# problematic instructions.  Here, however, the probe on the instruction
> +# is explicitly requested by the user.)
> +
> +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::,pid$target:a.out:-:'$off'
> +{
> +	trace("hlt instruction");
> +	exit(0);
> +}'
> diff --git a/test/unittest/pid/err.opt_before_needed.x86_64.x b/test/unittest/pid/err.opt_before_needed.x86_64.x
> new file mode 100755
> index 000000000..158336d98
> --- /dev/null
> +++ b/test/unittest/pid/err.opt_before_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/err.optional.aarch64.x b/test/unittest/pid/err.optional.aarch64.x
> new file mode 100755
> index 000000000..bd5b41a36
> --- /dev/null
> +++ b/test/unittest/pid/err.optional.aarch64.x
> @@ -0,0 +1,3 @@
> +#!/bin/sh
> +echo "skip on aarch64"
> +exit 2
> diff --git a/test/unittest/pid/err.optional.sh b/test/unittest/pid/err.optional.sh
> new file mode 100755
> index 000000000..1dbef4e45
> --- /dev/null
> +++ b/test/unittest/pid/err.optional.sh
> @@ -0,0 +1,25 @@
> +#!/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, which is forbidden
> +# by the kernel.
> +#
> +# (Test tst.entry_off0.sh should be okay, since there the probe on that
> +# instruction is optional, and dtrace will silently skip over such
> +# problematic instructions.  Here, however, the probe on the instruction
> +# is explicitly requested by the user.)
> +
> +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.optional.x86_64.x b/test/unittest/pid/err.optional.x86_64.x
> new file mode 100755
> index 000000000..158336d98
> --- /dev/null
> +++ b/test/unittest/pid/err.optional.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
>