[DTrace-devel] [PATCH v3 10/13] Add BPF helper mapping

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
but it seems some earlier review remarks fell through the cracks. In any 
case...

On 7/27/22 07:21, Kris Van Hees via DTrace-devel wrote:
> BPF helpers can be very specific to kernel versions, and the set of
> available helpers may differ between the system where DTrace was
> compiled and the system where it is being used.
>
> We add runtime checking of specific BPF helpers, with support for
> possible fallback hwlpers.  E.g. if probe_read_user() is not found,

E.g., hwlpers.

> we can use probe_read() instead (though that may not guarantee
> successful execution).

That parenthetical remark strikes me as quite cryptic.  One approach 
would be to explain what probe_read* issues one is trying to address.  
The other (I'm guessing based on previous remarks would be favored by 
you) would simply be to omit the parenthetical remark: the scope of the 
patch is to allow the implementation to refer to alternate helpers.  
Whether or not they are suitable alternatives is beside the point.

> This can also be used to check whether certain helpers exist.  By
> convention, when the BPF helper function id maps to 0, the helper is
> known not to be supported on the runtime system,

The paragraph ends in a comma.  It should be a period.


But how about replacing the paragraph entirely?  How about:
       "When a helper does not exist and has no suitable alternative,
       it can be mapped to BPF_FUNC_unspec."

>
> We include bpf.h for a kernel recent enough to support all the BPF
> features that DTrace can make use of.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> @@ -152,6 +152,65 @@ int dt_bpf_map_update(int fd, const void *key, const void *val)
>   	return bpf(BPF_MAP_UPDATE_ELEM, &attr);
>   }
>   
> +static int
> +have_helper(uint32_t func_id)
> +{
> +	char		log[DT_BPF_LOG_SIZE_SMALL] = { 0, };
> +	char		*ptr = &log[0];
> +	struct bpf_insn	insns[] = {
> +				BPF_CALL_HELPER(func_id),
> +				BPF_RETURN()
> +			};
> +	dtrace_difo_t	dp;
> +
> +	dp.dtdo_buf = insns;
> +	dp.dtdo_len = ARRAY_SIZE(insns);
> +
> +	/* If the program loads, we can use the helper. */
> +	if (dt_bpf_prog_load(BPF_PROG_TYPE_KPROBE, &dp,
> +			     1, log, DT_BPF_LOG_SIZE_SMALL) > 0)

If the program loads, it should be unloaded?

> +		return 1;
> +
> +	/* Permission denied -> helper not available to us */
> +	if (errno == EPERM)
> +		return 0;
> +
> +	/* Sentinel to make the loop and string searches safe. */
> +	log[DT_BPF_LOG_SIZE_SMALL - 2] = ' ';
> +	log[DT_BPF_LOG_SIZE_SMALL - 1] = 0;
> +
> +	while (*ptr == 0)
> +		ptr++;
> +
> +	/*
> +	 * If an 'invalid func' or 'unknown func' failure is reported, we
> +	 * cannot use the helper.
> +	 */
> +	return strstr(ptr, "invalid func") == NULL &&
> +	       strstr(ptr, "unknown func") == NULL;
> +}
> +
> +void
> +dt_bpf_init_helpers(dtrace_hdl_t *dtp)
> +{
> +	uint32_t	i;
> +
> +	/* Assume all helpers are available. */
> +	for (i = 0; i < __BPF_FUNC_MAX_ID; i++)
> +		dtp->dt_bpfhelper[i] = i;
> +
> +	/* Verify the existance of some specific helpers. */

existence

> +#define BPF_HELPER_MAP(orig, alt)	\
> +	if (!have_helper(BPF_FUNC_##orig)) \
> +			dtp->dt_bpfhelper[BPF_FUNC_##orig] = BPF_FUNC_##alt;
> +
> +	BPF_HELPER_MAP(probe_read_user, probe_read);
> +	BPF_HELPER_MAP(probe_read_user_str, probe_read_str);
> +	BPF_HELPER_MAP(probe_read_kernel, probe_read);
> +	BPF_HELPER_MAP(probe_read_kernel_str, probe_read_str);
> +#undef BPF_HELPER_MAP
> +}
> +
>   static int
>   create_gmap(dtrace_hdl_t *dtp, const char *name, enum bpf_map_type type,
>   	    int ksz, int vsz, int size)
>