Re: [PATCH v2 bpf-next 4/9] libbpf: refactor internal sec_def handling to enable pluggability

[Dave Marchevsky <davemarchevsky@fb.com>]

On 9/20/21 7:43 PM, Andrii Nakryiko wrote:   
> Refactor internals of libbpf to allow adding custom SEC() handling logic
> easily from outside of libbpf. To that effect, each SEC()-handling
> registration sets mandatory program type/expected attach type for
> a given prefix and can provide three callbacks called at different
> points of BPF program lifetime:
> 
>   - init callback for right after bpf_program is initialized and
>   prog_type/expected_attach_type is set. This happens during
>   bpf_object__open() step, close to the very end of constructing
>   bpf_object, so all the libbpf APIs for querying and updating
>   bpf_program properties should be available;

Do you have a usecase in mind that would set this? USDT? 

>   - pre-load callback is called right before BPF_PROG_LOAD command is
>   called in the kernel. This callbacks has ability to set both
>   bpf_program properties, as well as program load attributes, overriding
>   and augmenting the standard libbpf handling of them;

[...]

> @@ -6094,6 +6100,44 @@ static int bpf_object__sanitize_prog(struct bpf_object *obj, struct bpf_program
>  	return 0;
>  }
>  
> +static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, int *btf_type_id);
> +
> +/* this is called as prog->sec_def->preload_fn for libbpf-supported sec_defs */
> +static int libbpf_preload_prog(struct bpf_program *prog,
> +			       struct bpf_prog_load_params *attr, long cookie)
> +{
> +	/* old kernels might not support specifying expected_attach_type */
> +	if (prog->sec_def->is_exp_attach_type_optional &&
> +	    !kernel_supports(prog->obj, FEAT_EXP_ATTACH_TYPE))
> +		attr->expected_attach_type = 0;
> +
> +	if (prog->sec_def->is_sleepable)
> +		attr->prog_flags |= BPF_F_SLEEPABLE;
> +
> +	if ((prog->type == BPF_PROG_TYPE_TRACING ||
> +	     prog->type == BPF_PROG_TYPE_LSM ||
> +	     prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {
> +		int btf_obj_fd = 0, btf_type_id = 0, err;
> +
> +		err = libbpf_find_attach_btf_id(prog, &btf_obj_fd, &btf_type_id);
> +		if (err)
> +			return err;
> +
> +		/* cache resolved BTF FD and BTF type ID in the prog */
> +		prog->attach_btf_obj_fd = btf_obj_fd;
> +		prog->attach_btf_id = btf_type_id;
> +
> +		/* but by now libbpf common logic is not utilizing
> +		 * prog->atach_btf_obj_fd/prog->attach_btf_id anymore because
> +		 * this callback is called after attrs were populated by
> +		 * libbpf, so this callback has to update attr explicitly here
> +		 */
> +		attr->attach_btf_obj_fd = btf_obj_fd;
> +		attr->attach_btf_id = btf_type_id;
> +	}
> +	return 0;
> +}
> +
We talked on VC about some general approach questions I had here, will
summarize. Discussion touched on changes in patches 5 and 6 as well. I thought 
the pulling of these chunks into libbpf_preload_prog made sense, but wondered
whether some of this prog-type specific functionality would also be useful to
"average" custom sec_def writer even if it's not considered 'standard libbpf
handling', e.g. custom sec_def writer whose SEC produces a PROG_TYPE_TRACING
is likely to want the find_attach_btf_id niceness as well. So perhaps something
like the ability to chain the callbacks so that sec_def writer can use libbpf's
would be useful.

Your response was that you explicitly wanted to avoid doing this because this
would result in libbpf's callbacks becoming part of the API and stability 
requirements following from that. Furthermore, you don't anticipate libbpf's
preload callback becoming very complicated and expect that the average 
custom sec_def writer will be familiar enough with libbpf to be able to pull
out whatever they need.

Response made sense to me, LGTM

Acked-by: Dave Marchevsky <davemarchevsky@fb.com>