Re: [PATCH v3] bpf, fbt: do not try to get a return value for void function return pronbes

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

On Fri, Aug 02, 2024 at 01:45:46PM -0400, Eugene Loh wrote:
> Not a real careful review, since I'm not that familiar with the BTF stuff,
> but:
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> 
> I assume the new tests already pass on kprobes even without the new fixes.
> 
> Might the tests be (theoretically) a little more stringent with .r files?

Hm, yes, good point.  On that for v4.

> On 8/2/24 12:35, Kris Van Hees wrote:
> 
> > We were trying to access a return value for void functions, causing BPF
> > verifier failures.  We can determine the return type when getting the
> > probe info, and set the return probe argc value accordingly.  This then
> > drives the code generation in the trampoline.
> > 
> > Reported-by: Alan Maguire <alan.maguire@oracle.com>
> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> > ---
> >   libdtrace/dt_btf.c                            | 16 +++++++++++++
> >   libdtrace/dt_btf.h                            |  2 ++
> >   libdtrace/dt_prov_fbt.c                       | 21 +++++++++-------
> >   .../fbtprovider/err.D_ARGS_IDX.void-void.d    | 24 +++++++++++++++++++
> >   .../fbtprovider/err.D_ARGS_IDX.void.d         | 24 +++++++++++++++++++
> >   5 files changed, 78 insertions(+), 9 deletions(-)
> >   create mode 100644 test/unittest/fbtprovider/err.D_ARGS_IDX.void-void.d
> >   create mode 100644 test/unittest/fbtprovider/err.D_ARGS_IDX.void.d
> > 
> > diff --git a/libdtrace/dt_btf.c b/libdtrace/dt_btf.c
> > index 77366df6..e905580f 100644
> > --- a/libdtrace/dt_btf.c
> > +++ b/libdtrace/dt_btf.c
> > @@ -980,3 +980,19 @@ dt_btf_func_argc(dtrace_hdl_t *dtp, const dt_btf_t *btf, uint32_t id)
> >   	return -1;
> >   }
> > +
> > +int
> > +dt_btf_func_is_void(dtrace_hdl_t *dtp, const dt_btf_t *btf, uint32_t id)
> > +{
> > +	btf_type_t	*type = dt_btf_type_by_id(dtp, btf, id);
> > +
> > +	/* For functions, move on to the function prototype. */
> > +	if (BTF_INFO_KIND(type->info) == BTF_KIND_FUNC)
> > +		type = dt_btf_type_by_id(dtp, btf, type->type);
> > +
> > +	if (BTF_INFO_KIND(type->info) == BTF_KIND_FUNC_PROTO &&
> > +	    type->type == 0)
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > diff --git a/libdtrace/dt_btf.h b/libdtrace/dt_btf.h
> > index a7cee464..e05b30d0 100644
> > --- a/libdtrace/dt_btf.h
> > +++ b/libdtrace/dt_btf.h
> > @@ -25,6 +25,8 @@ extern int32_t dt_btf_lookup_name_kind(dtrace_hdl_t *, dt_btf_t *,
> >   				       const char *, uint32_t);
> >   extern int dt_btf_func_argc(dtrace_hdl_t *dtp, const dt_btf_t *btf,
> >   			    uint32_t id);
> > +extern int dt_btf_func_is_void(dtrace_hdl_t *dtp, const dt_btf_t *btf,
> > +			       uint32_t id);
> >   extern int dt_btf_get_module_ids(dtrace_hdl_t *);
> >   extern int dt_btf_module_fd(const dt_module_t *);
> > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > index 461d5e35..80c3db20 100644
> > --- a/libdtrace/dt_prov_fbt.c
> > +++ b/libdtrace/dt_prov_fbt.c
> > @@ -199,13 +199,15 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> >   		emit(dlp,  BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
> >   		/*
> > +		 * Only try to retrieve a return value if we know we can.
> > +		 *
> >   		 * The return value is provided by the fexit probe as an
> >   		 * argument slot past the last function argument.  We can get
> >   		 * the number of function arguments using the BTF id that has
> >   		 * been stored as the tracepoint event id.
> >   		 */
> >   		dmp = dt_module_lookup_by_name(dtp, prp->desc->mod);
> > -		if (dmp != NULL) {
> > +		if (dmp && prp->argc == 2) {
> >   			int32_t	btf_id = dt_tp_get_event_id(prp);
> >   			int	i = dt_btf_func_argc(dtp, dmp->dm_btf, btf_id);
> > @@ -240,7 +242,9 @@ static int fprobe_probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> >   	dt_tp_set_event_id(prp, btf_id);
> >   	if (strcmp(desc->prb, "return") == 0) {
> > -		argc = 2;
> > +		/* Void function return probes only provide 1 argument. */
> > +		argc = dt_btf_func_is_void(dtp, dmp->dm_btf, btf_id) ? 1 : 2;
> > +
> >   		argv = dt_calloc(dtp, argc, sizeof(dt_argdesc_t));
> >   		if (argv == NULL)
> >   			return dt_set_errno(dtp, EDT_NOMEM);
> > @@ -250,13 +254,12 @@ static int fprobe_probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> >   		argv[0].native = strdup("uint64_t");
> >   		argv[0].xlate = NULL;
> > -		/*
> > -		 * The return type is a generic uint64_t.  For void functions
> > -		 * the value of arg1 and argv[1] is undefined.
> > -		 */
> > -		argv[1].mapping = 1;
> > -		argv[1].native = strdup("uint64_t");
> > -		argv[1].xlate = NULL;
> > +		if (argc == 2) {
> > +			/* The return type is a generic uint64_t. */
> > +			argv[1].mapping = 1;
> > +			argv[1].native = strdup("uint64_t");
> > +			argv[1].xlate = NULL;
> > +		}
> >   		goto done;
> >   	}
> > diff --git a/test/unittest/fbtprovider/err.D_ARGS_IDX.void-void.d b/test/unittest/fbtprovider/err.D_ARGS_IDX.void-void.d
> > new file mode 100644
> > index 00000000..4bdf36cb
> > --- /dev/null
> > +++ b/test/unittest/fbtprovider/err.D_ARGS_IDX.void-void.d
> > @@ -0,0 +1,24 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2024, 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.
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +/*
> > + * This should fail compilation due to accessing args[1] (not args[0]) for a
> > + * void function without any arguuments.
> > + */
> > +fbt:vmlinux:oops_enter:return
> > +{
> > +	trace(args[0]);
> > +	trace(args[1]);
> > +	exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > +	exit(1);
> > +}
> > diff --git a/test/unittest/fbtprovider/err.D_ARGS_IDX.void.d b/test/unittest/fbtprovider/err.D_ARGS_IDX.void.d
> > new file mode 100644
> > index 00000000..516d733a
> > --- /dev/null
> > +++ b/test/unittest/fbtprovider/err.D_ARGS_IDX.void.d
> > @@ -0,0 +1,24 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2024, 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.
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +/*
> > + * This should fail compilation due to accessing args[1] (not args[0]) for a
> > + * void function with arguments.
> > + */
> > +fbt:vmlinux:exit_creds:return
> > +{
> > +	trace(args[0]);
> > +	trace(args[1]);
> > +	exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > +	exit(1);
> > +}