Re: [DTrace-devel] [PATCH 1/4] parser, cg: Implement the return() action

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

On Tue, Jul 15, 2025 at 11:32:42AM +0100, Nick Alcock wrote:
> On 15 Jul 2025, Kris Van Hees via DTrace-devel outgrape:
> 
> > The return(n) action can be used for error injection by forcing a
> > given return value for a kernel function.
> 
> I am amazed this is even possible! I can see all sorts of uses.
> 
> Presumably we need to tell people to turn on CONFIG_BPF_KPROBE_OVERRIDE
> somewhere... UEK already has it on, good. Not many functions
> covered... I suppose it'll let them trace a *few* functions, and if they
> use this they're probably used to hacking in more error-injection points
> anyway.

Well, it will be mentioned in the release notes of course, but the way it is
implemented, if the kernel is not configured to support it, there will not be
any functions for which it will be allowed (see the rest of the series).

I debated putting it in its own provider but that seemms less useful, even
though it would mean you can list which functions can support this.  That may
need to be something for the future if it is wanted - right now it is easy
enuogh for someone to look in /sys/kernel/debug/error_injection/list for that
info.  People using this feature are expected to really know what they are
doing :)

> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> 
> Reviewed-by: Nick Alcock <nick.alcock@oracle.com>
> 
> modulo the tiny nits below.
> 
> > diff --git a/libdtrace/dt_lex.l b/libdtrace/dt_lex.l
> > index cc165c1e..bdcca415 100644
> > --- a/libdtrace/dt_lex.l
> > +++ b/libdtrace/dt_lex.l
> > @@ -104,7 +104,6 @@ if (yypcb->pcb_token != 0) {
> >  <S0>provider	return DT_KEY_PROVIDER;
> >  <S0>register	return DT_KEY_REGISTER;
> >  <S0>restrict	return DT_KEY_RESTRICT;
> > -<S0>return	return DT_KEY_RETURN;
> >  <S0>self	return DT_KEY_SELF;
> >  <S0>short	return DT_KEY_SHORT;
> >  <S0>signed	return DT_KEY_SIGNED;
> 
> I guess these are here for C/C++ header parsing :) a somewhat hopeful
> approach. I wonder if this change will cause breakage for non-bracketed
> returns in C code in static inlines... and returns of non-integral
> types, and just plain return... ugh. I suppose inlines barely work
> anyway and the right way to do that is to simply have the parser ignore
> static inlines in C headers anyway, not have it try to parse them :)

No, they are there as symbols reserved for future use.  So by removing return
here, we are putting that reserved symbol to use :)

> > diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> > index c67455e7..08394a11 100644
> > --- a/libdtrace/dt_open.c
> > +++ b/libdtrace/dt_open.c
> > @@ -269,6 +269,8 @@ static const dt_ident_t _dtrace_globals[] = {
> >  	&dt_idops_func, "void(int)" },
> >  { "rand", DT_IDENT_FUNC, 0, DIF_SUBR_RAND, DT_ATTR_STABCMN, DT_VERS_1_0,
> >  	&dt_idops_func, "int()" },
> > +{ "return", DT_IDENT_ACTFUNC, 0, DT_ACT_RETURN, DT_ATTR_STABCMN, DT_VERS_2_0,
> > +	&dt_idops_func, "void(int)" },
> 
> The "problem" (if this actually is a problem) is that this is not the C
> prototype, which is not expressible in C :(

Since return is being implemented as an action it needs to be void(int).  If
we were to implement it like its C counterpart, then it would be a language
construct, and in taht case I would expect it to somehow set a return value
for the clause. which doesn't work (clauses do not have return values), and
it would not accomplish what we need here.  Since you can never return from
a clause (perhaps if/when conditional expressions are implemented, we may
need to revisit this).

> > diff --git a/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d b/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d
> > new file mode 100644
> > index 00000000..97ed0762
> > --- /dev/null
> > +++ b/test/unittest/actions/return/err.D_PROTO_LEN.missing_arg.d
> > @@ -0,0 +1,20 @@
> > +/*
> > + * 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: The return() action takes exactly one argument.
> > + *
> > + * SECTION: Actions and Subroutines/return()
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +BEGIN
> > +{
> > +	return();
> > +	exit(0);
> > +}
> 
> You might want to try a straight 'return;' as well :)

Sure.

> > diff --git a/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r b/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r
> > new file mode 100644
> > index 00000000..37ff4d9d
> > --- /dev/null
> > +++ b/test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.r
> > @@ -0,0 +1,2 @@
> > +-- @@stderr --
> > +dtrace: failed to compile script test/unittest/actions/return/err.D_PROTO_LEN.too_many_args.d: [D_PROTO_LEN] line 18: return( ) prototype mismatch: 2 args passed, 1 expected
> > diff --git a/test/unittest/actions/return/err.destructive.d b/test/unittest/actions/return/err.destructive.d
> > new file mode 100644
> > index 00000000..4c4a246a
> > --- /dev/null
> > +++ b/test/unittest/actions/return/err.destructive.d
> > @@ -0,0 +1,26 @@
> > +/*
> > + * 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 allowed when destructive execution is allowed
> 
> return() is forbidden when destructive execution is not allowed.

Thanks.