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

[Kris Van Hees <kris.van.hees@oracle.com>]

On Tue, Jul 15, 2025 at 11:50:21AM +0100, Nick Alcock wrote:
> 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.

I am hoping that perhaps in time a lot more functions may allow this, plus if
someone is going to make use of this feature for error injection they are
likely to use it for multiple functions and then being able to use a hashtable
rather than scanning the list multiple times is nicer.

> > +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 (!).

Hm, ah yes, I did this before I added the clsflags test before, so yes, if
we cannot read the file, then we should also flag a compilation error because
it means that error injection is not configured in the kernel (or for some
other reason won't be possible).

Will fix.

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

Oops, thanks!

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

Yes, indeed.  (See comment 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...)

I used this probe because the other tests for error injection use it as well,
so at least if there is rot, we'll notice it real quick across multiple
related tests.