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

[RFC PATCH v2] 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.

    $ bpftool link
    1: type 7  prog 5  bpf_cookie 123
        pids bootstrap(87)

Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
Changes in v2:
    - Display bpf_cookie in bpftool link command instead perf

    Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com

 include/uapi/linux/bpf.h       |  3 +++
 kernel/bpf/syscall.c           | 13 +++++++++++++
 tools/bpf/bpftool/link.c       |  2 ++
 tools/include/uapi/linux/bpf.h |  3 +++
 4 files changed, 21 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a7f0ddedac1f..600da4496404 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5850,6 +5850,9 @@ struct bpf_link_info {
 			__u32 target_obj_id; /* prog_id for PROG_EXT, otherwise btf object id */
 			__u32 target_btf_id; /* BTF type id inside the object */
 		} tracing;
+		struct {
+			__u64 bpf_cookie;
+		} perf;
 		struct {
 			__u64 cgroup_id;
 			__u32 attach_type;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 72ce1edde950..94b7fa777fc7 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2948,6 +2948,7 @@ static const struct bpf_link_ops bpf_raw_tp_link_lops = {
 struct bpf_perf_link {
 	struct bpf_link link;
 	struct file *perf_file;
+	u64 bpf_cookie;
 };
 
 static void bpf_perf_link_release(struct bpf_link *link)
@@ -2966,9 +2967,20 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
 	kfree(perf_link);
 }
 
+static int bpf_perf_link_fill_link_info(const struct bpf_link *link,
+					  struct bpf_link_info *info)
+{
+	struct bpf_perf_link *perf_link =
+		container_of(link, struct bpf_perf_link, link);
+
+	info->perf.bpf_cookie = perf_link->bpf_cookie;
+	return 0;
+}
+
 static const struct bpf_link_ops bpf_perf_link_lops = {
 	.release = bpf_perf_link_release,
 	.dealloc = bpf_perf_link_dealloc,
+	.fill_link_info = bpf_perf_link_fill_link_info,
 };
 
 static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
@@ -2993,6 +3005,7 @@ static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *pro
 	}
 	bpf_link_init(&link->link, BPF_LINK_TYPE_PERF_EVENT, &bpf_perf_link_lops, prog);
 	link->perf_file = perf_file;
+	link->bpf_cookie = attr->link_create.perf_event.bpf_cookie;
 
 	err = bpf_link_prime(&link->link, &link_primer);
 	if (err) {
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index 97dec81950e5..3ddeacb3593f 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -243,6 +243,8 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
 		printf("\n\tnetns_ino %u  ", info->netns.netns_ino);
 		show_link_attach_type_plain(info->netns.attach_type);
 		break;
+	case BPF_LINK_TYPE_PERF_EVENT:
+		printf("\n\tbpf_cookie %llu  ", info->perf.bpf_cookie);
 	default:
 		break;
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a7f0ddedac1f..600da4496404 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5850,6 +5850,9 @@ struct bpf_link_info {
 			__u32 target_obj_id; /* prog_id for PROG_EXT, otherwise btf object id */
 			__u32 target_btf_id; /* BTF type id inside the object */
 		} tracing;
+		struct {
+			__u64 bpf_cookie;
+		} perf;
 		struct {
 			__u64 cgroup_id;
 			__u32 attach_type;
-- 
2.32.0

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

[Yonghong Song]

On 2/4/22 10:11 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.
> 
>      $ bpftool link
>      1: type 7  prog 5  bpf_cookie 123
>          pids bootstrap(87)
> 
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> ---
> Changes in v2:
>      - Display bpf_cookie in bpftool link command instead perf
> 
>      Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com
> 
>   include/uapi/linux/bpf.h       |  3 +++
>   kernel/bpf/syscall.c           | 13 +++++++++++++
>   tools/bpf/bpftool/link.c       |  2 ++
>   tools/include/uapi/linux/bpf.h |  3 +++

Could you change this patch into two separate ones?
The subject 'bpftool' sounds like it is a bpftool change but
actually it also includes kernel bpf change.
Maybe:
   patch 1: prefix: bpf
      include/uapi/linux/bpf.h
      kernel/bpf/syscall.c
      tools/include/uapi/linux/bpf.h
   patch 2: prefix: bpftool
      tools/bpf/bpftool/link.c

>   4 files changed, 21 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a7f0ddedac1f..600da4496404 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5850,6 +5850,9 @@ struct bpf_link_info {
>   			__u32 target_obj_id; /* prog_id for PROG_EXT, otherwise btf object id */
>   			__u32 target_btf_id; /* BTF type id inside the object */
>   		} tracing;
> +		struct {
> +			__u64 bpf_cookie;
> +		} perf;
>   		struct {
>   			__u64 cgroup_id;
>   			__u32 attach_type;
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 72ce1edde950..94b7fa777fc7 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2948,6 +2948,7 @@ static const struct bpf_link_ops bpf_raw_tp_link_lops = {
>   struct bpf_perf_link {
>   	struct bpf_link link;
>   	struct file *perf_file;
> +	u64 bpf_cookie;
>   };
>   
>   static void bpf_perf_link_release(struct bpf_link *link)
> @@ -2966,9 +2967,20 @@ static void bpf_perf_link_dealloc(struct bpf_link *link)
>   	kfree(perf_link);
>   }
>   
> +static int bpf_perf_link_fill_link_info(const struct bpf_link *link,
> +					  struct bpf_link_info *info)
> +{
> +	struct bpf_perf_link *perf_link =
> +		container_of(link, struct bpf_perf_link, link);
> +
> +	info->perf.bpf_cookie = perf_link->bpf_cookie;

I think we don't need bpf_cookie in bpf_perf_link. This is a low
frequency event. You can get the information from perf_link->perf_file.

Could you check whether the following works or not?

         struct perf_event *event;
         struct file *perf_file;

         perf_file = perf_link->perf_file;
         event = perf_file->private_data;
         info->perf.bpf_cookie = event->bpf_cookie;

> +	return 0;
> +}
> +
>   static const struct bpf_link_ops bpf_perf_link_lops = {
>   	.release = bpf_perf_link_release,
>   	.dealloc = bpf_perf_link_dealloc,
> +	.fill_link_info = bpf_perf_link_fill_link_info,
>   };
>   
>   static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> @@ -2993,6 +3005,7 @@ static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *pro
>   	}
>   	bpf_link_init(&link->link, BPF_LINK_TYPE_PERF_EVENT, &bpf_perf_link_lops, prog);
>   	link->perf_file = perf_file;
> +	link->bpf_cookie = attr->link_create.perf_event.bpf_cookie;
>   
>   	err = bpf_link_prime(&link->link, &link_primer);
>   	if (err) {
> diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
> index 97dec81950e5..3ddeacb3593f 100644
> --- a/tools/bpf/bpftool/link.c
> +++ b/tools/bpf/bpftool/link.c
> @@ -243,6 +243,8 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
>   		printf("\n\tnetns_ino %u  ", info->netns.netns_ino);
>   		show_link_attach_type_plain(info->netns.attach_type);
>   		break;
> +	case BPF_LINK_TYPE_PERF_EVENT:
> +		printf("\n\tbpf_cookie %llu  ", info->perf.bpf_cookie);
>   	default:
>   		break;
>   	}
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index a7f0ddedac1f..600da4496404 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -5850,6 +5850,9 @@ struct bpf_link_info {
>   			__u32 target_obj_id; /* prog_id for PROG_EXT, otherwise btf object id */
>   			__u32 target_btf_id; /* BTF type id inside the object */
>   		} tracing;
> +		struct {
> +			__u64 bpf_cookie;
> +		} perf;
>   		struct {
>   			__u64 cgroup_id;
>   			__u32 attach_type;

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

[Andrii Nakryiko]

On Fri, Feb 4, 2022 at 10:12 AM Dmitrii Dolgov <9erthalion6@gmail.com> 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.
>
>     $ bpftool link
>     1: type 7  prog 5  bpf_cookie 123
>         pids bootstrap(87)
>
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> ---
> Changes in v2:
>     - Display bpf_cookie in bpftool link command instead perf
>
>     Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com


So I think this change is pretty straightforward and I don't mind it,
but I'm not clear how this approach will scale to multi-attach kprobe
and fentry programs. For those, users will be specifying many bpf
cookies, one per each target attach function. At that point we'll have
a bunch of cookies sorted by the attach function IP (not necessarily
in the original order). I don't think it will be all that useful and
interesting to the end user. We won't be storing original function
names (too much memory for storing something that most probably won't
be ever queried), so restoring original order and original function
names will be hard. If we don't think this through, we'll end up with
kernel API that works for just one simple use case.

Can you please describe your use case which motivated this feature in
the first place to better understand the importance of this?

BTW, bpftool can technically implement this today without kernel
changes by fetching such bpf_cookies from the kernel using its pid
iterator BPF program. See skeleton/pid_iter.bpf.c for pointers. I
wonder if it would make more sense to start with doing this purely on
the bpftool side first.

As an aside (and probably something more generally useful), it seems
like we have a bpf_iter__bpf_map iterator, but we don't have prog and
link iterators implemented. Would it be a good idea to add that to the
kernel? Yonghong, Alexei, any thoughts?

>
>  include/uapi/linux/bpf.h       |  3 +++
>  kernel/bpf/syscall.c           | 13 +++++++++++++
>  tools/bpf/bpftool/link.c       |  2 ++
>  tools/include/uapi/linux/bpf.h |  3 +++
>  4 files changed, 21 insertions(+)
>

[...]

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

[Yonghong Song]

On 2/7/22 2:11 PM, Andrii Nakryiko wrote:
> On Fri, Feb 4, 2022 at 10:12 AM Dmitrii Dolgov <9erthalion6@gmail.com> 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.
>>
>>      $ bpftool link
>>      1: type 7  prog 5  bpf_cookie 123
>>          pids bootstrap(87)
>>
>> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
>> ---
>> Changes in v2:
>>      - Display bpf_cookie in bpftool link command instead perf
>>
>>      Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com
> 
> 
> So I think this change is pretty straightforward and I don't mind it,
> but I'm not clear how this approach will scale to multi-attach kprobe
> and fentry programs. For those, users will be specifying many bpf
> cookies, one per each target attach function. At that point we'll have
> a bunch of cookies sorted by the attach function IP (not necessarily
> in the original order). I don't think it will be all that useful and
> interesting to the end user. We won't be storing original function
> names (too much memory for storing something that most probably won't
> be ever queried), so restoring original order and original function
> names will be hard. If we don't think this through, we'll end up with
> kernel API that works for just one simple use case.

The cookie for multi-attachment is indeed a problem. Some of original
cookies may not be available any more.

> 
> Can you please describe your use case which motivated this feature in
> the first place to better understand the importance of this?
> 
> BTW, bpftool can technically implement this today without kernel
> changes by fetching such bpf_cookies from the kernel using its pid
> iterator BPF program. See skeleton/pid_iter.bpf.c for pointers. I
> wonder if it would make more sense to start with doing this purely on
> the bpftool side first.
> 
> As an aside (and probably something more generally useful), it seems
> like we have a bpf_iter__bpf_map iterator, but we don't have prog and
> link iterators implemented. Would it be a good idea to add that to the
> kernel? Yonghong, Alexei, any thoughts?

We already have program iterator. We have discussed link iterators
for sometime. As more and more usages for links, a link iterator
should be good to improve performance compared to generic 'task/file'
iterator.

> 
>>
>>   include/uapi/linux/bpf.h       |  3 +++
>>   kernel/bpf/syscall.c           | 13 +++++++++++++
>>   tools/bpf/bpftool/link.c       |  2 ++
>>   tools/include/uapi/linux/bpf.h |  3 +++
>>   4 files changed, 21 insertions(+)
>>
> 
> [...]

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

[Alexei Starovoitov]

On Mon, Feb 7, 2022 at 9:46 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 2/7/22 2:11 PM, Andrii Nakryiko wrote:
> > On Fri, Feb 4, 2022 at 10:12 AM Dmitrii Dolgov <9erthalion6@gmail.com> 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.
> >>
> >>      $ bpftool link
> >>      1: type 7  prog 5  bpf_cookie 123
> >>          pids bootstrap(87)
> >>
> >> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> >> ---
> >> Changes in v2:
> >>      - Display bpf_cookie in bpftool link command instead perf
> >>
> >>      Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com
> >
> >
> > So I think this change is pretty straightforward and I don't mind it,
> > but I'm not clear how this approach will scale to multi-attach kprobe
> > and fentry programs. For those, users will be specifying many bpf
> > cookies, one per each target attach function. At that point we'll have
> > a bunch of cookies sorted by the attach function IP (not necessarily
> > in the original order). I don't think it will be all that useful and
> > interesting to the end user. We won't be storing original function
> > names (too much memory for storing something that most probably won't
> > be ever queried), so restoring original order and original function
> > names will be hard. If we don't think this through, we'll end up with
> > kernel API that works for just one simple use case.
>
> The cookie for multi-attachment is indeed a problem. Some of original
> cookies may not be available any more.
>
> >
> > Can you please describe your use case which motivated this feature in
> > the first place to better understand the importance of this?
> >
> > BTW, bpftool can technically implement this today without kernel
> > changes by fetching such bpf_cookies from the kernel using its pid
> > iterator BPF program. See skeleton/pid_iter.bpf.c for pointers. I
> > wonder if it would make more sense to start with doing this purely on
> > the bpftool side first.
> >
> > As an aside (and probably something more generally useful), it seems
> > like we have a bpf_iter__bpf_map iterator, but we don't have prog and
> > link iterators implemented. Would it be a good idea to add that to the
> > kernel? Yonghong, Alexei, any thoughts?
>
> We already have program iterator. We have discussed link iterators
> for sometime. As more and more usages for links, a link iterator
> should be good to improve performance compared to generic 'task/file'
> iterator.

Agree. We have iters for progs and maps. The iter for links is missing.

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

[Dmitry Dolgov]

> On Mon, Feb 07, 2022 at 02:11:11PM -0800, Andrii Nakryiko wrote:
> On Fri, Feb 4, 2022 at 10:12 AM Dmitrii Dolgov <9erthalion6@gmail.com> 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.
> >
> >     $ bpftool link
> >     1: type 7  prog 5  bpf_cookie 123
> >         pids bootstrap(87)
> >
> > Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> > ---
> > Changes in v2:
> >     - Display bpf_cookie in bpftool link command instead perf
> >
> >     Previous discussion: https://lore.kernel.org/bpf/20220127082649.12134-1-9erthalion6@gmail.com
>
>
> So I think this change is pretty straightforward and I don't mind it,
> but I'm not clear how this approach will scale to multi-attach kprobe
> and fentry programs. For those, users will be specifying many bpf
> cookies, one per each target attach function. At that point we'll have
> a bunch of cookies sorted by the attach function IP (not necessarily
> in the original order). I don't think it will be all that useful and
> interesting to the end user. We won't be storing original function
> names (too much memory for storing something that most probably won't
> be ever queried), so restoring original order and original function
> names will be hard. If we don't think this through, we'll end up with
> kernel API that works for just one simple use case.
>
> Can you please describe your use case which motivated this feature in
> the first place to better understand the importance of this?

The use case is pretty theoretical at the moment, I'm trying to
understand how to get more visibility about bpf_cookie usage. Let me try
to do the same only in bpftool, while pondering if multi-attach programs
case could be somehow meaningfully addressed as well.

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

[Dmitry Dolgov]

> On Mon, Feb 07, 2022 at 09:46:36PM -0800, Yonghong Song wrote:
> > As an aside (and probably something more generally useful), it seems
> > like we have a bpf_iter__bpf_map iterator, but we don't have prog and
> > link iterators implemented. Would it be a good idea to add that to the
> > kernel? Yonghong, Alexei, any thoughts?
>
> We already have program iterator. We have discussed link iterators
> for sometime. As more and more usages for links, a link iterator
> should be good to improve performance compared to generic 'task/file'
> iterator.

Are those discussions about link iterators captured somewhere in the
mailing list, could you point me to them?

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

[Yonghong Song]

On 2/8/22 12:16 PM, Dmitry Dolgov wrote:
>> On Mon, Feb 07, 2022 at 09:46:36PM -0800, Yonghong Song wrote:
>>> As an aside (and probably something more generally useful), it seems
>>> like we have a bpf_iter__bpf_map iterator, but we don't have prog and
>>> link iterators implemented. Would it be a good idea to add that to the
>>> kernel? Yonghong, Alexei, any thoughts?
>>
>> We already have program iterator. We have discussed link iterators
>> for sometime. As more and more usages for links, a link iterator
>> should be good to improve performance compared to generic 'task/file'
>> iterator.
> 
> Are those discussions about link iterators captured somewhere in the
> mailing list, could you point me to them?

We could have mentioned link iterators in various occasions as it is
natural to extend beyond progs/maps, but I don't remember a dedicated
discussion around this.