Re: [PATCH bpf-next 1/7] libbpf: add BPF-side of USDT support

[Alan Maguire <alan.maguire@oracle.com>]

On Fri, 25 Mar 2022, Andrii Nakryiko wrote:

> Add BPF-side implementation of libbpf-provided USDT support. This
> consists of single header library, usdt.bpf.h, which is meant to be used
> from user's BPF-side source code. This header is added to the list of
> installed libbpf header, along bpf_helpers.h and others.
>

<snip>

Some suggestions below, but nothing major.

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
 
> diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
> new file mode 100644
> index 000000000000..8ee084b2e6b5
> --- /dev/null
> +++ b/tools/lib/bpf/usdt.bpf.h
> @@ -0,0 +1,228 @@
> +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> +#ifndef __USDT_BPF_H__
> +#define __USDT_BPF_H__
> +
> +#include <linux/errno.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include <bpf/bpf_core_read.h>
> +
> +/* Below types and maps are internal implementation details of libpf's USDT
> + * support and are subjects to change. Also, usdt_xxx() API helpers should be
> + * considered an unstable API as well and might be adjusted based on user
> + * feedback from using libbpf's USDT support in production.
> + */
> +
> +/* User can override BPF_USDT_MAX_SPEC_CNT to change default size of internal
> + * map that keeps track of USDT argument specifications. This might be
> + * necessary if there are a lot of USDT attachments.
> + */
> +#ifndef BPF_USDT_MAX_SPEC_CNT
> +#define BPF_USDT_MAX_SPEC_CNT 256
> +#endif
> +/* User can override BPF_USDT_MAX_IP_CNT to change default size of internal
> + * map that keeps track of IP (memory address) mapping to USDT argument
> + * specification.
> + * Note, if kernel supports BPF cookies, this map is not used and could be
> + * resized all the way to 1 to save a bit of memory.
> + */
> +#ifndef BPF_USDT_MAX_IP_CNT
> +#define BPF_USDT_MAX_IP_CNT 1024
> +#endif

might be no harm to just make this default to a reasonable multiple of 
BPF_USDT_MAX_SPEC_CNT; i.e. n specs X m possible sites. Would allow users
to simply override the MAX_SPEC_CNT in most cases too.

> +/* We use BPF CO-RE to detect support for BPF cookie from BPF side. This is
> + * the only dependency on CO-RE, so if it's undesirable, user can override
> + * BPF_USDT_HAS_BPF_COOKIE to specify whether to BPF cookie is supported or not.
> + */
> +#ifndef BPF_USDT_HAS_BPF_COOKIE
> +#define BPF_USDT_HAS_BPF_COOKIE \
> +	bpf_core_enum_value_exists(enum bpf_func_id___usdt, BPF_FUNC_get_attach_cookie___usdt)
> +#endif
> +
> +enum __bpf_usdt_arg_type {
> +	BPF_USDT_ARG_CONST,
> +	BPF_USDT_ARG_REG,
> +	BPF_USDT_ARG_REG_DEREF,
> +};
> +
> +struct __bpf_usdt_arg_spec {
> +	__u64 val_off;
> +	enum __bpf_usdt_arg_type arg_type;
> +	short reg_off;
> +	bool arg_signed;
> +	char arg_bitshift;

would be no harm having a small comment here or below where the 
bitshifting is done like "for arg sizes less than 8 bytes, this tells
us how many bits to shift to left then right to
remove the unused bits, giving correct arg value".

> +};
> +
> +/* should match USDT_MAX_ARG_CNT in usdt.c exactly */
> +#define BPF_USDT_MAX_ARG_CNT 12
> +struct __bpf_usdt_spec {
> +	struct __bpf_usdt_arg_spec args[BPF_USDT_MAX_ARG_CNT];
> +	__u64 usdt_cookie;
> +	short arg_cnt;
> +};
> +
> +__weak struct {
> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> +	__uint(max_entries, BPF_USDT_MAX_SPEC_CNT);
> +	__type(key, int);
> +	__type(value, struct __bpf_usdt_spec);
> +} __bpf_usdt_specs SEC(".maps");
> +
> +__weak struct {
> +	__uint(type, BPF_MAP_TYPE_HASH);
> +	__uint(max_entries, BPF_USDT_MAX_IP_CNT);
> +	__type(key, long);
> +	__type(value, struct __bpf_usdt_spec);
> +} __bpf_usdt_specs_ip_to_id SEC(".maps");
> +
> +/* don't rely on user's BPF code to have latest definition of bpf_func_id */
> +enum bpf_func_id___usdt {
> +	BPF_FUNC_get_attach_cookie___usdt = 0xBAD, /* value doesn't matter */
> +};
> +
> +static inline int __bpf_usdt_spec_id(struct pt_regs *ctx)
> +{
> +	if (!BPF_USDT_HAS_BPF_COOKIE) {
> +		long ip = PT_REGS_IP(ctx);

Trying to sort of the permutations of features, I _think_ is it possible 
the user has CO-RE support, but the clang version doesn't support the
push of the preserve_access_index attribute? Would it be feasible to
do an explicit "PT_REGS_IP_CORE(ctx);" here?

> +		int *spec_id_ptr;
> +
> +		spec_id_ptr = bpf_map_lookup_elem(&__bpf_usdt_specs_ip_to_id, &ip);
> +		return spec_id_ptr ? *spec_id_ptr : -ESRCH;
> +	}
> +
> +	return bpf_get_attach_cookie(ctx);

should we grab the result in a u64 and handle the 0 case here - 
meaning "not specified" - and return -ESRCH?

> +}
> +
> +/* Return number of USDT arguments defined for currently traced USDT. */
> +__hidden __weak
> +int bpf_usdt_arg_cnt(struct pt_regs *ctx)
> +{
> +	struct __bpf_usdt_spec *spec;
> +	int spec_id;
> +
> +	spec_id = __bpf_usdt_spec_id(ctx);
> +	if (spec_id < 0)
> +		return -EINVAL;

spec_id can be 0 for the "cookie not set" case (see above).

should we pass through the error value from __bpf_usdt_spec_id()? Looking
above it's either -ESRCH or 0, but if we catch the 0 case as above we 
could just pass through the error value.
 
> +
> +	spec = bpf_map_lookup_elem(&__bpf_usdt_specs, &spec_id);
> +	if (!spec)
> +		return -EINVAL;
> +

should this be -ESRCH? we know from the above we had a valid
spec_id.

> +	return spec->arg_cnt;
> +}

also, since in every case (I think) that we call __bpf_usdt_spec_id()
we co on to look up the spec in the map, would it be easier to
combine both operations and have

struct __bpf_usdt_spec * __bpf_usdt_spec(struct pt_regs *ctx);

?

> +
> +/* Fetch USDT argument *arg* (zero-indexed) and put its value into *res.
> + * Returns 0 on success; negative error, otherwise.
> + * On error *res is guaranteed to be set to zero.
> + */
> +__hidden __weak
> +int bpf_usdt_arg(struct pt_regs *ctx, int arg, long *res)
> +{
> +	struct __bpf_usdt_spec *spec;
> +	struct __bpf_usdt_arg_spec *arg_spec;
> +	unsigned long val;
> +	int err, spec_id;
> +
> +	*res = 0;
> +
> +	spec_id = __bpf_usdt_spec_id(ctx);
> +	if (spec_id < 0)
> +		return -ESRCH;
> +
> +	spec = bpf_map_lookup_elem(&__bpf_usdt_specs, &spec_id);
> +	if (!spec)
> +		return -ESRCH;
> +
> +	if (arg >= spec->arg_cnt)
> +		return -ENOENT;
> +

I'm surprised you didn't need to check for negative values or a hard 
upper bound for the arg index here (to keep the verifier happy for
the later array indexing using arg). Any dangers that an older
LLVM+clang would generate code that might get tripped up on
verification with this?

> +	arg_spec = &spec->args[arg];
> +	switch (arg_spec->arg_type) {
> +	case BPF_USDT_ARG_CONST:
> +		val = arg_spec->val_off;
> +		break;
> +	case BPF_USDT_ARG_REG:
> +		err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec->reg_off);
> +		if (err)
> +			return err;
> +		break;
> +	case BPF_USDT_ARG_REG_DEREF:
> +		err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec->reg_off);
> +		if (err)
> +			return err;
> +		err = bpf_probe_read_user(&val, sizeof(val), (void *)val + arg_spec->val_off);
> +		if (err)
> +			return err;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	val <<= arg_spec->arg_bitshift;
> +	if (arg_spec->arg_signed)
> +		val = ((long)val) >> arg_spec->arg_bitshift;
> +	else
> +		val = val >> arg_spec->arg_bitshift;
> +	*res = val;
> +	return 0;
> +}
> +
> +/* Retrieve user-specified cookie value provided during attach as
> + * bpf_usdt_opts.usdt_cookie. This serves the same purpose as BPF cookie
> + * returned by bpf_get_attach_cookie(). Libbpf's support for USDT is itself
> + * utilizaing BPF cookies internally, so user can't use BPF cookie directly
> + * for USDT programs and has to use bpf_usdt_cookie() API instead.
> + */
> +__hidden __weak
> +long bpf_usdt_cookie(struct pt_regs *ctx)
> +{
> +	struct __bpf_usdt_spec *spec;
> +	int spec_id;
> +
> +	spec_id = __bpf_usdt_spec_id(ctx);
> +	if (spec_id < 0)
> +		return 0;
> +
> +	spec = bpf_map_lookup_elem(&__bpf_usdt_specs, &spec_id);
> +	if (!spec)
> +		return 0;
> +
> +	return spec->usdt_cookie;
> +}
> +
> +/* we rely on ___bpf_apply() and ___bpf_narg() macros already defined in bpf_tracing.h */
> +#define ___bpf_usdt_args0() ctx
> +#define ___bpf_usdt_args1(x) ___bpf_usdt_args0(), ({ long _x; bpf_usdt_arg(ctx, 0, &_x); (void *)_x; })
> +#define ___bpf_usdt_args2(x, args...) ___bpf_usdt_args1(args), ({ long _x; bpf_usdt_arg(ctx, 1, &_x); (void *)_x; })
> +#define ___bpf_usdt_args3(x, args...) ___bpf_usdt_args2(args), ({ long _x; bpf_usdt_arg(ctx, 2, &_x); (void *)_x; })
> +#define ___bpf_usdt_args4(x, args...) ___bpf_usdt_args3(args), ({ long _x; bpf_usdt_arg(ctx, 3, &_x); (void *)_x; })
> +#define ___bpf_usdt_args5(x, args...) ___bpf_usdt_args4(args), ({ long _x; bpf_usdt_arg(ctx, 4, &_x); (void *)_x; })
> +#define ___bpf_usdt_args6(x, args...) ___bpf_usdt_args5(args), ({ long _x; bpf_usdt_arg(ctx, 5, &_x); (void *)_x; })
> +#define ___bpf_usdt_args7(x, args...) ___bpf_usdt_args6(args), ({ long _x; bpf_usdt_arg(ctx, 6, &_x); (void *)_x; })
> +#define ___bpf_usdt_args8(x, args...) ___bpf_usdt_args7(args), ({ long _x; bpf_usdt_arg(ctx, 7, &_x); (void *)_x; })
> +#define ___bpf_usdt_args9(x, args...) ___bpf_usdt_args8(args), ({ long _x; bpf_usdt_arg(ctx, 8, &_x); (void *)_x; })
> +#define ___bpf_usdt_args10(x, args...) ___bpf_usdt_args9(args), ({ long _x; bpf_usdt_arg(ctx, 9, &_x); (void *)_x; })
> +#define ___bpf_usdt_args11(x, args...) ___bpf_usdt_args10(args), ({ long _x; bpf_usdt_arg(ctx, 10, &_x); (void *)_x; })
> +#define ___bpf_usdt_args12(x, args...) ___bpf_usdt_args11(args), ({ long _x; bpf_usdt_arg(ctx, 11, &_x); (void *)_x; })
> +#define ___bpf_usdt_args(args...) ___bpf_apply(___bpf_usdt_args, ___bpf_narg(args))(args)
> +
> +/*
> + * BPF_USDT serves the same purpose for USDT handlers as BPF_PROG for
> + * tp_btf/fentry/fexit BPF programs and BPF_KPROBE for kprobes.
> + * Original struct pt_regs * context is preserved as 'ctx' argument.
> + */
> +#define BPF_USDT(name, args...)						    \
> +name(struct pt_regs *ctx);						    \
> +static __attribute__((always_inline)) typeof(name(0))			    \
> +____##name(struct pt_regs *ctx, ##args);				    \
> +typeof(name(0)) name(struct pt_regs *ctx)				    \
> +{									    \
> +        _Pragma("GCC diagnostic push")					    \
> +        _Pragma("GCC diagnostic ignored \"-Wint-conversion\"")		    \
> +        return ____##name(___bpf_usdt_args(args));			    \
> +        _Pragma("GCC diagnostic pop")					    \
> +}									    \
> +static __attribute__((always_inline)) typeof(name(0))			    \
> +____##name(struct pt_regs *ctx, ##args)
> +
> +#endif /* __USDT_BPF_H__ */
> -- 
> 2.30.2
> 
>