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

[Nick Alcock <nick.alcock@oracle.com>]

On 15 Jul 2025, Kris Van Hees via DTrace-devel verbalised:

> 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.

Pluralize "function".

> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>

Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

modulo typos and other nits above and below. Nothing serious found, just
misconceptions of mine being squashed.

> +/*
> + * raafbt only:

rawfbt

> + *
> + * 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;

Ugh, that's quite tangled for how little it does :( can't think of a
nicer approach though.

> +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.
> +	 */

Personally I'd move this into a function just to do this initialization,
and call it from this one.

> +	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;

Might want to at least dt_dprintf something here, or more likely
xyerror() out, given that this probably indicates a kernel configuration
problem. Right now this looks exactly like a success (!).

> +		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);

free(buf);

> +	}
> +
> +	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);
> +}

(end of function for comparision with "error" path above...)

>  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,

So unlike what I assumed in the previous commit, we're assigning to
reject_clause() whether or not destructive tracing is on...

... because dt_cg_clsflags() is turning on the destructive flag whenever
the return flag is on. OK.

> 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);
> +}

This seems like code that will rot fast, given that btrfs is one of the
few modules that actually does have error injection. Why not pick some
of the huge mass of the kernel that doesn't have it at all? (I guess the
rot will be easy to see -- new test failures, every time...)

-- 
NULL && (void)