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

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

v2 on the way soon - with tests

On Fri, Aug 02, 2024 at 11:58:18AM -0400, 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 ++++++++++++---------
>  3 files changed, 30 insertions(+), 9 deletions(-)
> 
> 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;
>  	}
> -- 
> 2.45.2
>