Re: [PATCH bpf-next v5] bpftool: Add bpf_cookie to link output

[Yonghong Song <yhs@xxxxxx>]

On 3/4/22 6:36 AM, Dmitrii Dolgov wrote:
> Commit 82e6b1eee6a8 ("bpf: Allow to specify user-provided bpf_cookie for
> BPF perf links") introduced the concept of user specified bpf_cookie,
> which could be accessed by BPF programs using bpf_get_attach_cookie().
> For troubleshooting purposes it is convenient to expose bpf_cookie via
> bpftool as well, so there is no need to meddle with the target BPF
> program itself.
>
> Implemented using the pid iterator BPF program to actually fetch
> bpf_cookies, which allows constraining code changes only to bpftool.
>
> $ bpftool link
> 1: type 7  prog 5
>          bpf_cookie 123
>          pids bootstrap(81)
>
> Signed-off-by: Dmitrii Dolgov <9erthalion6@xxxxxxxxx>

LGTM with a few nits below.

Acked-by: Yonghong Song <yhs@xxxxxx>

> ---
> Changes in v5:
>      - Remove unneeded cookie assigns
>
> Changes in v4:
>      - Fetch cookies only for bpf_perf_link
>      - Signal about bpf_cookie via the flag, instead of deducing it from
>        the object and link type
>      - Reset pid_iter_entry to avoid invalid indirect read from stack
>
> Changes in v3:
>      - Use pid iterator to fetch bpf_cookie
>
> Changes in v2:
>      - Display bpf_cookie in bpftool link command instead perf
>
> Previous discussion: https://lore.kernel.org/bpf/20220225152802.20957-1-9erthalion6@xxxxxxxxx/
>
>
>   tools/bpf/bpftool/main.h                  |  2 ++
>   tools/bpf/bpftool/pids.c                  |  8 ++++++++
>   tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 25 +++++++++++++++++++++++
>   tools/bpf/bpftool/skeleton/pid_iter.h     |  2 ++
>   4 files changed, 37 insertions(+)
>
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 0c3840596b5a..1bb76aa1f3b2 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -114,6 +114,8 @@ struct obj_ref {
>   struct obj_refs {
>   	int ref_cnt;
>   	struct obj_ref *refs;
> +	bool bpf_cookie_set;
> +	__u64 bpf_cookie;
>   };
>   
>   struct btf;
> diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
> index 7c384d10e95f..6c6e7c90cc3d 100644
> --- a/tools/bpf/bpftool/pids.c
> +++ b/tools/bpf/bpftool/pids.c
> @@ -78,6 +78,8 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
>   	ref->pid = e->pid;
>   	memcpy(ref->comm, e->comm, sizeof(ref->comm));
>   	refs->ref_cnt = 1;
> +	refs->bpf_cookie_set = e->bpf_cookie_set;
> +	refs->bpf_cookie = e->bpf_cookie;
>   
>   	err = hashmap__append(map, u32_as_hash_field(e->id), refs);
>   	if (err)
> @@ -205,6 +207,9 @@ void emit_obj_refs_json(struct hashmap *map, __u32 id,
>   		if (refs->ref_cnt == 0)
>   			break;
>   
> +		if (refs->bpf_cookie_set)
> +			jsonw_lluint_field(json_writer, "bpf_cookie", refs->bpf_cookie);

The original motivation for 'bpf_cookie' is for kprobe to get function 
addresses. In that case, printing with llx (0x...) is better than llu
since people can easily search it with /proc/kallsyms to get what the
function it attached to. But on the other hand, other use cases might
be simply just wanting an int.

I don't have a strong opinion here. Just to speak out loud so other
people can comment on this too.

> +
>   		jsonw_name(json_writer, "pids");
>   		jsonw_start_array(json_writer);
>   		for (i = 0; i < refs->ref_cnt; i++) {
> @@ -234,6 +239,9 @@ void emit_obj_refs_plain(struct hashmap *map, __u32 id, const char *prefix)
>   		if (refs->ref_cnt == 0)
>   			break;
>   
> +		if (refs->bpf_cookie_set)
> +			printf("\n\tbpf_cookie %llu", refs->bpf_cookie);
> +
>   		printf("%s", prefix);
>   		for (i = 0; i < refs->ref_cnt; i++) {
>   			struct obj_ref *ref = &refs->refs[i];
> diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> index f70702fcb224..91366ce33717 100644
> --- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> @@ -38,6 +38,18 @@ static __always_inline __u32 get_obj_id(void *ent, enum bpf_obj_type type)
>   	}
>   }
>   
> +/* could be used only with BPF_LINK_TYPE_PERF_EVENT links */
> +static __always_inline __u64 get_bpf_cookie(struct bpf_link *link)
> +{
> +	struct bpf_perf_link *perf_link;
> +	struct perf_event *event;
> +
> +	perf_link = container_of(link, struct bpf_perf_link, link);
> +	event = BPF_CORE_READ(perf_link, perf_file, private_data);
> +	return BPF_CORE_READ(event, bpf_cookie);
> +}
> +
> +
>   SEC("iter/task_file")
>   int iter(struct bpf_iter__task_file *ctx)
>   {
> @@ -69,8 +81,21 @@ int iter(struct bpf_iter__task_file *ctx)
>   	if (file->f_op != fops)
>   		return 0;
>   
> +	__builtin_memset(&e, 0, sizeof(e));
>   	e.pid = task->tgid;
>   	e.id = get_obj_id(file->private_data, obj_type);
> +	e.bpf_cookie = 0;
> +	e.bpf_cookie_set = false;

We already have __builtin_memset(&e, 0, sizeof(e)) in the above, so
the above e.bpf_cookie and e.bpf_cookie_set assignment is not
necessary.

> +
> +	if (obj_type == BPF_OBJ_LINK) {
> +		struct bpf_link *link = (struct bpf_link *) file->private_data;
> +
> +		if (BPF_CORE_READ(link, type) == BPF_LINK_TYPE_PERF_EVENT) {
> +			e.bpf_cookie_set = true;
> +			e.bpf_cookie = get_bpf_cookie(link);
> +		}
> +	}
> +
>   	bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm),
>   				  task->group_leader->comm);
>   	bpf_seq_write(ctx->meta->seq, &e, sizeof(e));
> diff --git a/tools/bpf/bpftool/skeleton/pid_iter.h b/tools/bpf/bpftool/skeleton/pid_iter.h
> index 5692cf257adb..2676cece58d7 100644
> --- a/tools/bpf/bpftool/skeleton/pid_iter.h
> +++ b/tools/bpf/bpftool/skeleton/pid_iter.h
> @@ -6,6 +6,8 @@
>   struct pid_iter_entry {
>   	__u32 id;
>   	int pid;
> +	__u64 bpf_cookie;
> +	bool bpf_cookie_set;
>   	char comm[16];
>   };
>