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

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

[Dmitrii Dolgov]

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@gmail.com>
---
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@gmail.com/

 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;
 
 		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)
 			break;
 
+		if (refs->bpf_cookie_set)
+			jsonw_lluint_field(json_writer, "bpf_cookie", refs->bpf_cookie);
+
 		jsonw_name(json_writer, "pids");
 		jsonw_start_array(json_writer);
 		for (i = 0; i < refs->ref_cnt; i++) {
@@ -234,6 +241,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;
+
+	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];
 };
 
-- 
2.32.0

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

[Yonghong Song]

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@gmail.com>
> ---
> 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@gmail.com/
> 
>   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)
[...]

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

[Dmitry Dolgov]

> On Tue, Mar 01, 2022 at 11:53:56PM -0800, Yonghong Song wrote:
>
>
> 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.

Yeah, you're right, it seems there is no need for RFC tag any more. Will
add "bpf-next" tag as well, thanks for the suggestion.

> > 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?

Right, I have the same understanding about a single fixed cookie per
link. But in this particular case the implementation uses
hashmap__for_each_key_entry (which is essentially a loop with a
condition inside) and inside it returns as soon as the first entry was
found. So I guess it will not override the cookie with every new
reference, do I see it correct?

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

[Yonghong Song]

On 3/3/22 8:20 AM, Dmitry Dolgov wrote:
>> On Tue, Mar 01, 2022 at 11:53:56PM -0800, Yonghong Song wrote:
>>
>>
>> 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.
> 
> Yeah, you're right, it seems there is no need for RFC tag any more. Will
> add "bpf-next" tag as well, thanks for the suggestion.
> 
>>> 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?
> 
> Right, I have the same understanding about a single fixed cookie per
> link. But in this particular case the implementation uses
> hashmap__for_each_key_entry (which is essentially a loop with a
> condition inside) and inside it returns as soon as the first entry was
> found. So I guess it will not override the cookie with every new
> reference, do I see it correct?

They are not return if pid is not the same.

Let us say the same link is used for pid1 and pid2.
The pid1 case will have refs->bpf_cookie[_set] set properly.
The pid2 case will trigger the above code, and since for the same
link, cookie is fixed, so the above code is not really needed.

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

[Dmitry Dolgov]

> On Thu, Mar 03, 2022 at 10:24:28AM -0800, Yonghong Song wrote:
>
> > > > 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?
> >
> > Right, I have the same understanding about a single fixed cookie per
> > link. But in this particular case the implementation uses
> > hashmap__for_each_key_entry (which is essentially a loop with a
> > condition inside) and inside it returns as soon as the first entry was
> > found. So I guess it will not override the cookie with every new
> > reference, do I see it correct?
>
> They are not return if pid is not the same.
>
> Let us say the same link is used for pid1 and pid2.
> The pid1 case will have refs->bpf_cookie[_set] set properly.
> The pid2 case will trigger the above code, and since for the same
> link, cookie is fixed, so the above code is not really needed.

Oh, I see, thanks for clarification! Will post a new version soon.