Re: [RFC PATCH v4] bpftool: Add bpf_cookie to link output

[Yonghong Song <yhs@xxxxxx>]

On 2/25/22 7:28 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.

Do you still need RFC tag? It looks like we have a consensus
with this bpf_iter approach, right?

Please also add "bpf-next" to the tag for clarity purpose.

> 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>
> ---
> 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/20220218075103.10002-1-9erthalion6@xxxxxxxxx/
>
>   tools/bpf/bpftool/main.h                  |  2 ++
>   tools/bpf/bpftool/pids.c                  | 10 +++++++++
>   tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 25 +++++++++++++++++++++++
>   tools/bpf/bpftool/skeleton/pid_iter.h     |  2 ++
>   4 files changed, 39 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..152502c2d6f9 100644
> --- a/tools/bpf/bpftool/pids.c
> +++ b/tools/bpf/bpftool/pids.c
> @@ -55,6 +55,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++;
> +		refs->bpf_cookie_set = e->bpf_cookie_set;
> +		refs->bpf_cookie = e->bpf_cookie;

Do we need here? It is weird that we overwrite the bpf_cookie with every 
new 'pid' reference.

When you create a link, the cookie is fixed for that link. You could pin
that link in bpffs e.g., /sys/fs/bpf/link1 and other programs can then
get a reference to the link1, but they should still have the same 
cookie. Is that right?

>   
>   		return;
>   	}
> @@ -78,6 +80,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 +209,9 @@ void emit_obj_refs_json(struct hashmap *map, __u32 id,
>   		if (refs->ref_cnt == 0)
[...]