Re: [DTrace-devel] [PATCH 4/4] rawfbt: selectively allow return() in clauses

[Eugene Loh <eugene.loh@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 9311 bytes --]

I'm late to the party, sorry.

For usability, could an error message point to 
/sys/kernel/debug/error_injection/list?  Might as well provide useful 
information at the time of encounter.

When you parse the list, you expect the function name to be terminated 
with a space.  When I look at my list, only 7 (out of nearly 1000) are 
so terminated.  Shouldn't you check for a space *OR* a tab?

When I run the tests, I get an OL8 UEK6 failure on x86 for 
test/unittest/actions/return/tst.destructive.d:
dtrace: could not enable tracing: Failed to enable 
rawfbt:btrfs:open_ctree:entry: No such file or directory

Also, there should be a test to check that return() actually does the 
right thing.  I'm attaching a proposal.

Typos:  "injexction" in the commit msg and raa as Nick previously noted.

On 7/15/25 01:48, Kris Van Hees via DTrace-devel wrote:

> The return() action is only allowed in clauses associated with a rawfbt
> probe on a function listed in /sys/kernel/debug/error_injexction/list.
> Use a reject_clause() callback in the rawfbt provider to enforce this.
>
> The list of allowed function is only initialized the first time it is
> needed, and its content will be stored in a hashtable for faster
> lookup beyond the first.
>
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
>   libdtrace/dt_prov_fbt.c                       | 134 ++++++++++++++++++
>   .../actions/return/err.not_allowed-1.d        |  27 ++++
>   .../actions/return/err.not_allowed-1.r        |   2 +
>   .../actions/return/err.not_allowed-2.d        |  27 ++++
>   .../actions/return/err.not_allowed-2.r        |   2 +
>   .../actions/return/err.not_allowed-3.d        |  27 ++++
>   .../actions/return/err.not_allowed-3.r        |   2 +
>   7 files changed, 221 insertions(+)
>   create mode 100644 test/unittest/actions/return/err.not_allowed-1.d
>   create mode 100644 test/unittest/actions/return/err.not_allowed-1.r
>   create mode 100644 test/unittest/actions/return/err.not_allowed-2.d
>   create mode 100644 test/unittest/actions/return/err.not_allowed-2.r
>   create mode 100644 test/unittest/actions/return/err.not_allowed-3.d
>   create mode 100644 test/unittest/actions/return/err.not_allowed-3.r
>
> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> index 50fa0d9d..b98d8d87 100644
> --- a/libdtrace/dt_prov_fbt.c
> +++ b/libdtrace/dt_prov_fbt.c
> @@ -591,6 +591,139 @@ static void kprobe_detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
>   		free(tpn);
>   }
>   
> +/*
> + * raafbt only:
> + *
> + * Accept all clauses, except those that use the return() action in a function
> + * that does not allow error rejection.
> + */
> +#define FUNCS_ALLOW_RETURN	"/sys/kernel/debug/error_injection/list"
> +
> +typedef struct allowed_fn {
> +	const char	*mod;
> +	const char	*fun;
> +	dt_hentry_t	he;
> +} allowed_fun_t;
> +
> +static uint32_t
> +fun_hval(const allowed_fun_t *p)
> +{
> +	return str2hval(p->fun, p->mod ? str2hval(p->mod, 0) : 0);
> +}
> +
> +static int
> +fun_cmp(const allowed_fun_t *p, const allowed_fun_t *q) {
> +	int	rc;
> +
> +	if (p->mod != NULL) {
> +		if (q->mod == NULL)
> +			return 1;
> +		else {
> +			rc = strcmp(p->mod, q->mod);
> +			if (rc != 0)
> +				return rc;
> +		}
> +	} else if (q->mod != NULL)
> +		return -1;
> +
> +	return strcmp(p->fun, q->fun);
> +}
> +
> +DEFINE_HE_STD_LINK_FUNCS(fun, allowed_fun_t, he)
> +
> +static void *
> +fun_del_entry(allowed_fun_t *head, allowed_fun_t *p)
> +{
> +	head = fun_del(head, p);
> +
> +	free((char *)p->mod);
> +	free((char *)p->fun);
> +	free(p);
> +
> +	return head;
> +}
> +
> +static dt_htab_ops_t fun_htab_ops = {
> +	.hval = (htab_hval_fn)fun_hval,
> +	.cmp = (htab_cmp_fn)fun_cmp,
> +	.add = (htab_add_fn)fun_add,
> +	.del = (htab_del_fn)fun_del_entry,
> +	.next = (htab_next_fn)fun_next
> +};
> +
> +static void reject_clause(const dt_probe_t *prp, int clsflags)
> +{
> +	dt_htab_t	*atab = prp->prov->prv_data;
> +	allowed_fun_t	tmpl;
> +
> +	/* If the clause does not have a return() action, allow it. */
> +	if (!(clsflags & DT_CLSFLAG_RETURN))
> +		return;
> +
> +	/*
> +	 * If the htab of allowed functions for return() does not exist yet,
> +	 * create it.
> +	 */
> +	if (atab == NULL) {
> +		FILE		*f;
> +		char		*buf = NULL;
> +		size_t		len = 0;
> +		allowed_fun_t	*entry;
> +
> +		atab = dt_htab_create(&fun_htab_ops);
> +		if (atab == NULL)
> +			return;
> +
> +		prp->prov->prv_data = atab;
> +
> +		f = fopen(FUNCS_ALLOW_RETURN, "r");
> +		if (f == NULL)
> +			return;
> +
> +		while (getline(&buf, &len, f) >= 0) {
> +			char	*p;
> +
> +			entry = (allowed_fun_t *)malloc(sizeof(allowed_fun_t));
> +			if (entry == NULL)
> +				break;
> +
> +			p = strchr(buf, ' ');
> +			if (p) {
> +				*p++ = '\0';
> +				if (*p == '[') {
> +					char	*q;
> +
> +					p++;
> +					q = strchr(p, ']');
> +					if (q)
> +						*q = '\0';
> +				} else
> +					p = NULL;
> +			}
> +
> +			entry->fun = strdup(buf);
> +			entry->mod = p != NULL ? strdup(p) : NULL;
> +
> +			if (dt_htab_insert(atab, entry) < 0)
> +				return;
> +		}
> +
> +		fclose(f);
> +	}
> +
> +	tmpl.mod = prp->desc->mod;
> +	tmpl.fun = prp->desc->fun;
> +	if (dt_htab_lookup(atab, &tmpl) != NULL)
> +		return;
> +
> +	tmpl.mod = NULL;
> +	if (dt_htab_lookup(atab, &tmpl) != NULL)
> +		return;
> +
> +	xyerror(D_ACT_RETURN, "return() not allowed for %s:%s:%s:%s\n",
> +		prp->desc->prv, prp->desc->mod, prp->desc->fun, prp->desc->prb);
> +}
> +
>   dt_provimpl_t	dt_fbt_fprobe = {
>   	.name		= prvname,
>   	.prog_type	= BPF_PROG_TYPE_TRACING,
> @@ -628,6 +761,7 @@ dt_provimpl_t	dt_rawfbt = {
>   	.populate	= &populate,
>   	.provide	= &provide,
>   	.load_prog	= &dt_bpf_prog_load,
> +	.reject_clause	= &reject_clause,
>   	.trampoline	= &kprobe_trampoline,
>   	.attach		= &kprobe_attach,
>   	.detach		= &kprobe_detach,
> diff --git a/test/unittest/actions/return/err.not_allowed-1.d b/test/unittest/actions/return/err.not_allowed-1.d
> new file mode 100644
> index 00000000..246a68b1
> --- /dev/null
> +++ b/test/unittest/actions/return/err.not_allowed-1.d
> @@ -0,0 +1,27 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: return() is not allowed for fbt probes
> + *
> + * SECTION: Actions and Subroutines/return()
> + */
> +
> +#pragma D option quiet
> +#pragma D option destructive
> +
> +BEGIN
> +{
> +	ok = 0;
> +	exit(0);
> +}
> +
> +fbt:btrfs:open_ctree:entry
> +/ok/
> +{
> +	return(0);
> +}
> diff --git a/test/unittest/actions/return/err.not_allowed-1.r b/test/unittest/actions/return/err.not_allowed-1.r
> new file mode 100644
> index 00000000..e3443c30
> --- /dev/null
> +++ b/test/unittest/actions/return/err.not_allowed-1.r
> @@ -0,0 +1,2 @@
> +-- @@stderr --
> +dtrace: could not enable tracing: return() not allowed for fbt:btrfs:open_ctree:entry
> diff --git a/test/unittest/actions/return/err.not_allowed-2.d b/test/unittest/actions/return/err.not_allowed-2.d
> new file mode 100644
> index 00000000..e0d303c7
> --- /dev/null
> +++ b/test/unittest/actions/return/err.not_allowed-2.d
> @@ -0,0 +1,27 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: return() is only allowed for select functions
> + *
> + * SECTION: Actions and Subroutines/return()
> + */
> +
> +#pragma D option quiet
> +#pragma D option destructive
> +
> +BEGIN
> +{
> +	ok = 0;
> +	exit(0);
> +}
> +
> +rawfbt:btrfs:close_ctree:entry
> +/ok/
> +{
> +	return(0);
> +}
> diff --git a/test/unittest/actions/return/err.not_allowed-2.r b/test/unittest/actions/return/err.not_allowed-2.r
> new file mode 100644
> index 00000000..758d3477
> --- /dev/null
> +++ b/test/unittest/actions/return/err.not_allowed-2.r
> @@ -0,0 +1,2 @@
> +-- @@stderr --
> +dtrace: could not enable tracing: return() not allowed for rawfbt:btrfs:close_ctree:entry
> diff --git a/test/unittest/actions/return/err.not_allowed-3.d b/test/unittest/actions/return/err.not_allowed-3.d
> new file mode 100644
> index 00000000..770565c9
> --- /dev/null
> +++ b/test/unittest/actions/return/err.not_allowed-3.d
> @@ -0,0 +1,27 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: return() is only allowed for select functions
> + *
> + * SECTION: Actions and Subroutines/return()
> + */
> +
> +#pragma D option quiet
> +#pragma D option destructive
> +
> +BEGIN
> +{
> +	ok = 0;
> +	exit(0);
> +}
> +
> +rawfbt:btrfs:*_ctree:entry
> +/ok/
> +{
> +	return(0);
> +}
> diff --git a/test/unittest/actions/return/err.not_allowed-3.r b/test/unittest/actions/return/err.not_allowed-3.r
> new file mode 100644
> index 00000000..758d3477
> --- /dev/null
> +++ b/test/unittest/actions/return/err.not_allowed-3.r
> @@ -0,0 +1,2 @@
> +-- @@stderr --
> +dtrace: could not enable tracing: return() not allowed for rawfbt:btrfs:close_ctree:entry

[-- Attachment #2: 0001-test.patch --]
[-- Type: text/plain, Size: 2776 bytes --]

From a6a6ceb614fadd5c5d1116cc05fe71da2b2dbff7 Mon Sep 17 00:00:00 2001
From: Eugene Loh <eugene.loh@oracle.com>
Date: Tue, 15 Jul 2025 13:33:04 -0700
Subject: [PATCH] test

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 .../actions/return/tst.destructive-getpid.r   | 12 +++++
 .../actions/return/tst.destructive-getpid.r.p | 10 ++++
 .../actions/return/tst.destructive-getpid.sh  | 48 +++++++++++++++++++
 3 files changed, 70 insertions(+)
 create mode 100644 test/unittest/actions/return/tst.destructive-getpid.r
 create mode 100755 test/unittest/actions/return/tst.destructive-getpid.r.p
 create mode 100755 test/unittest/actions/return/tst.destructive-getpid.sh

diff --git a/test/unittest/actions/return/tst.destructive-getpid.r b/test/unittest/actions/return/tst.destructive-getpid.r
new file mode 100644
index 00000000..b6bdf211
--- /dev/null
+++ b/test/unittest/actions/return/tst.destructive-getpid.r
@@ -0,0 +1,12 @@
+pid is mypid
+mypid
+22222
+mypid
+mypid
+55555
+mypid
+mypid
+mypid
+mypid
+mypid
+
diff --git a/test/unittest/actions/return/tst.destructive-getpid.r.p b/test/unittest/actions/return/tst.destructive-getpid.r.p
new file mode 100755
index 00000000..d6593c46
--- /dev/null
+++ b/test/unittest/actions/return/tst.destructive-getpid.r.p
@@ -0,0 +1,10 @@
+#!/usr/bin/gawk -f
+
+BEGIN { mypid = -1 }
+
+/^pid is [1-9][0-9]*$/ { mypid = $3; }
+
+{
+        if ( $NF == mypid ) $NF = "mypid";
+        print;
+}
diff --git a/test/unittest/actions/return/tst.destructive-getpid.sh b/test/unittest/actions/return/tst.destructive-getpid.sh
new file mode 100755
index 00000000..7c44dde9
--- /dev/null
+++ b/test/unittest/actions/return/tst.destructive-getpid.sh
@@ -0,0 +1,48 @@
+#!/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.
+#
+
+dtrace=$1
+
+# Set up test directory.
+
+DIRNAME=$tmpdir/destructive-getpid.$$.$RANDOM
+mkdir -p $DIRNAME
+cd $DIRNAME
+
+# Make the trigger.  It reports the pid 10 times.
+
+cat << EOF > main.c
+#include <stdio.h>
+#include <unistd.h>
+
+int main(int c, char **v) {
+	int i;
+
+	for (i = 0; i < 10; i++)
+		printf("%d\n", getpid());
+
+	return 0;
+}
+EOF
+
+$CC main.c
+if [ $? -ne 0 ]; then
+	echo ERROR: compile
+	exit 1
+fi
+
+# Trace the trigger.  On the 2nd and 5th getpid() calls, modify the result.
+
+$dtrace -c ./a.out -w -q -n '
+BEGIN { printf("pid is %d\n", $target); n = 0; }
+rawfbt:vmlinux:__*_sys_getpid:entry /pid == $target/ { n++ }
+rawfbt:vmlinux:__*_sys_getpid:entry /pid == $target && n == 2/ { return(22222); }
+rawfbt:vmlinux:__*_sys_getpid:entry /pid == $target && n == 5/ { return(55555); }
+'
+
+exit $?
-- 
2.18.4