* [PATCH bpf-next v2 1/3] bpf: implement link_query for bpf iterators
2020-08-20 22:49 [PATCH bpf-next v2 0/3] bpf: implement link_query for bpf iterators Yonghong Song
@ 2020-08-20 22:49 ` Yonghong Song
2020-08-21 6:31 ` Andrii Nakryiko
2020-08-20 22:49 ` [PATCH bpf-next v2 2/3] bpf: implement link_query callbacks in map element iterators Yonghong Song
2020-08-20 22:49 ` [PATCH bpf-next v2 3/3] bpftool: implement link_query for bpf iterators Yonghong Song
2 siblings, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2020-08-20 22:49 UTC (permalink / raw)
To: bpf, netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
This patch implemented bpf_link callback functions
show_fdinfo and fill_link_info to support link_query
interface.
The general interface for show_fdinfo and fill_link_info
will print/fill the target_name. Each targets can
register show_fdinfo and fill_link_info callbacks
to print/fill more target specific information.
For example, the below is a fdinfo result for a bpf
task iterator.
$ cat /proc/1749/fdinfo/7
pos: 0
flags: 02000000
mnt_id: 14
link_type: iter
link_id: 11
prog_tag: 990e1f8152f7e54f
prog_id: 59
target_name: task
Signed-off-by: Yonghong Song <yhs@fb.com>
---
include/linux/bpf.h | 6 ++++
include/uapi/linux/bpf.h | 7 ++++
kernel/bpf/bpf_iter.c | 58 ++++++++++++++++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 7 ++++
4 files changed, 78 insertions(+)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a9b7185a6b37..529e9b183eeb 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1218,12 +1218,18 @@ typedef int (*bpf_iter_attach_target_t)(struct bpf_prog *prog,
union bpf_iter_link_info *linfo,
struct bpf_iter_aux_info *aux);
typedef void (*bpf_iter_detach_target_t)(struct bpf_iter_aux_info *aux);
+typedef void (*bpf_iter_show_fdinfo_t) (const struct bpf_iter_aux_info *aux,
+ struct seq_file *seq);
+typedef int (*bpf_iter_fill_link_info_t)(const struct bpf_iter_aux_info *aux,
+ struct bpf_link_info *info);
#define BPF_ITER_CTX_ARG_MAX 2
struct bpf_iter_reg {
const char *target;
bpf_iter_attach_target_t attach_target;
bpf_iter_detach_target_t detach_target;
+ bpf_iter_show_fdinfo_t show_fdinfo;
+ bpf_iter_fill_link_info_t fill_link_info;
u32 ctx_arg_info_size;
struct bpf_ctx_arg_aux ctx_arg_info[BPF_ITER_CTX_ARG_MAX];
const struct bpf_iter_seq_info *seq_info;
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 0480f893facd..a1bbaff7a0af 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4071,6 +4071,13 @@ struct bpf_link_info {
__u64 cgroup_id;
__u32 attach_type;
} cgroup;
+ struct {
+ __aligned_u64 target_name; /* in/out: target_name buffer ptr */
+ __u32 target_name_len; /* in/out: target_name buffer len */
+ union {
+ __u32 map_id;
+ } map;
+ } iter;
struct {
__u32 netns_ino;
__u32 attach_type;
diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index b6715964b685..124e3ce02ce2 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -377,10 +377,68 @@ static int bpf_iter_link_replace(struct bpf_link *link,
return ret;
}
+static void bpf_iter_link_show_fdinfo(const struct bpf_link *link,
+ struct seq_file *seq)
+{
+ struct bpf_iter_link *iter_link =
+ container_of(link, struct bpf_iter_link, link);
+ bpf_iter_show_fdinfo_t show_fdinfo;
+
+ seq_printf(seq,
+ "target_name:\t%s\n",
+ iter_link->tinfo->reg_info->target);
+
+ show_fdinfo = iter_link->tinfo->reg_info->show_fdinfo;
+ if (show_fdinfo)
+ show_fdinfo(&iter_link->aux, seq);
+}
+
+static int bpf_iter_link_fill_link_info(const struct bpf_link *link,
+ struct bpf_link_info *info)
+{
+ struct bpf_iter_link *iter_link =
+ container_of(link, struct bpf_iter_link, link);
+ char __user *ubuf = u64_to_user_ptr(info->iter.target_name);
+ bpf_iter_fill_link_info_t fill_link_info;
+ u32 ulen = info->iter.target_name_len;
+ const char *target_name;
+ u32 target_len;
+
+ if (ulen && !ubuf)
+ return -EINVAL;
+
+ target_name = iter_link->tinfo->reg_info->target;
+ target_len = strlen(target_name);
+ info->iter.target_name_len = target_len + 1;
+ if (!ubuf)
+ return 0;
+
+ if (ulen >= target_len + 1) {
+ if (copy_to_user(ubuf, target_name, target_len + 1))
+ return -EFAULT;
+ } else {
+ char zero = '\0';
+
+ if (copy_to_user(ubuf, target_name, ulen - 1))
+ return -EFAULT;
+ if (put_user(zero, ubuf + ulen - 1))
+ return -EFAULT;
+ return -ENOSPC;
+ }
+
+ fill_link_info = iter_link->tinfo->reg_info->fill_link_info;
+ if (fill_link_info)
+ return fill_link_info(&iter_link->aux, info);
+
+ return 0;
+}
+
static const struct bpf_link_ops bpf_iter_link_lops = {
.release = bpf_iter_link_release,
.dealloc = bpf_iter_link_dealloc,
.update_prog = bpf_iter_link_replace,
+ .show_fdinfo = bpf_iter_link_show_fdinfo,
+ .fill_link_info = bpf_iter_link_fill_link_info,
};
bool bpf_link_is_iter(struct bpf_link *link)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 0480f893facd..a1bbaff7a0af 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4071,6 +4071,13 @@ struct bpf_link_info {
__u64 cgroup_id;
__u32 attach_type;
} cgroup;
+ struct {
+ __aligned_u64 target_name; /* in/out: target_name buffer ptr */
+ __u32 target_name_len; /* in/out: target_name buffer len */
+ union {
+ __u32 map_id;
+ } map;
+ } iter;
struct {
__u32 netns_ino;
__u32 attach_type;
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH bpf-next v2 1/3] bpf: implement link_query for bpf iterators
2020-08-20 22:49 ` [PATCH bpf-next v2 1/3] " Yonghong Song
@ 2020-08-21 6:31 ` Andrii Nakryiko
2020-08-21 6:42 ` Yonghong Song
0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-08-21 6:31 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Thu, Aug 20, 2020 at 3:50 PM Yonghong Song <yhs@fb.com> wrote:
>
> This patch implemented bpf_link callback functions
> show_fdinfo and fill_link_info to support link_query
> interface.
>
> The general interface for show_fdinfo and fill_link_info
> will print/fill the target_name. Each targets can
> register show_fdinfo and fill_link_info callbacks
> to print/fill more target specific information.
>
> For example, the below is a fdinfo result for a bpf
> task iterator.
> $ cat /proc/1749/fdinfo/7
> pos: 0
> flags: 02000000
> mnt_id: 14
> link_type: iter
> link_id: 11
> prog_tag: 990e1f8152f7e54f
> prog_id: 59
> target_name: task
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> include/linux/bpf.h | 6 ++++
> include/uapi/linux/bpf.h | 7 ++++
> kernel/bpf/bpf_iter.c | 58 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 7 ++++
> 4 files changed, 78 insertions(+)
>
[...]
> +
> +static int bpf_iter_link_fill_link_info(const struct bpf_link *link,
> + struct bpf_link_info *info)
> +{
> + struct bpf_iter_link *iter_link =
> + container_of(link, struct bpf_iter_link, link);
> + char __user *ubuf = u64_to_user_ptr(info->iter.target_name);
> + bpf_iter_fill_link_info_t fill_link_info;
> + u32 ulen = info->iter.target_name_len;
> + const char *target_name;
> + u32 target_len;
> +
> + if (ulen && !ubuf)
> + return -EINVAL;
> +
> + target_name = iter_link->tinfo->reg_info->target;
> + target_len = strlen(target_name);
> + info->iter.target_name_len = target_len + 1;
> + if (!ubuf)
> + return 0;
this might return prematurely before fill_link_info() below gets a
chance to fill in some extra info?
> +
> + if (ulen >= target_len + 1) {
> + if (copy_to_user(ubuf, target_name, target_len + 1))
> + return -EFAULT;
> + } else {
> + char zero = '\0';
> +
> + if (copy_to_user(ubuf, target_name, ulen - 1))
> + return -EFAULT;
> + if (put_user(zero, ubuf + ulen - 1))
> + return -EFAULT;
> + return -ENOSPC;
> + }
> +
> + fill_link_info = iter_link->tinfo->reg_info->fill_link_info;
> + if (fill_link_info)
> + return fill_link_info(&iter_link->aux, info);
> +
> + return 0;
> +}
> +
[...]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH bpf-next v2 1/3] bpf: implement link_query for bpf iterators
2020-08-21 6:31 ` Andrii Nakryiko
@ 2020-08-21 6:42 ` Yonghong Song
2020-08-21 16:44 ` Andrii Nakryiko
0 siblings, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2020-08-21 6:42 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On 8/20/20 11:31 PM, Andrii Nakryiko wrote:
> On Thu, Aug 20, 2020 at 3:50 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> This patch implemented bpf_link callback functions
>> show_fdinfo and fill_link_info to support link_query
>> interface.
>>
>> The general interface for show_fdinfo and fill_link_info
>> will print/fill the target_name. Each targets can
>> register show_fdinfo and fill_link_info callbacks
>> to print/fill more target specific information.
>>
>> For example, the below is a fdinfo result for a bpf
>> task iterator.
>> $ cat /proc/1749/fdinfo/7
>> pos: 0
>> flags: 02000000
>> mnt_id: 14
>> link_type: iter
>> link_id: 11
>> prog_tag: 990e1f8152f7e54f
>> prog_id: 59
>> target_name: task
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> include/linux/bpf.h | 6 ++++
>> include/uapi/linux/bpf.h | 7 ++++
>> kernel/bpf/bpf_iter.c | 58 ++++++++++++++++++++++++++++++++++
>> tools/include/uapi/linux/bpf.h | 7 ++++
>> 4 files changed, 78 insertions(+)
>>
>
> [...]
>
>> +
>> +static int bpf_iter_link_fill_link_info(const struct bpf_link *link,
>> + struct bpf_link_info *info)
>> +{
>> + struct bpf_iter_link *iter_link =
>> + container_of(link, struct bpf_iter_link, link);
>> + char __user *ubuf = u64_to_user_ptr(info->iter.target_name);
>> + bpf_iter_fill_link_info_t fill_link_info;
>> + u32 ulen = info->iter.target_name_len;
>> + const char *target_name;
>> + u32 target_len;
>> +
>> + if (ulen && !ubuf)
>> + return -EINVAL;
>> +
>> + target_name = iter_link->tinfo->reg_info->target;
>> + target_len = strlen(target_name);
>> + info->iter.target_name_len = target_len + 1;
>> + if (!ubuf)
>> + return 0;
>
> this might return prematurely before fill_link_info() below gets a
> chance to fill in some extra info?
The extra info filled by below fill_link_info is target specific
and we need a target name to ensure picking right union members.
So it is best to enforce a valid target name before filling
target dependent fields. See below, if there are any errors
for copy_to_user or enospc, we won't copy addition link info
either.
>
>> +
>> + if (ulen >= target_len + 1) {
>> + if (copy_to_user(ubuf, target_name, target_len + 1))
>> + return -EFAULT;
>> + } else {
>> + char zero = '\0';
>> +
>> + if (copy_to_user(ubuf, target_name, ulen - 1))
>> + return -EFAULT;
>> + if (put_user(zero, ubuf + ulen - 1))
>> + return -EFAULT;
>> + return -ENOSPC;
>> + }
>> +
>> + fill_link_info = iter_link->tinfo->reg_info->fill_link_info;
>> + if (fill_link_info)
>> + return fill_link_info(&iter_link->aux, info);
>> +
>> + return 0;
>> +}
>> +
>
> [...]
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH bpf-next v2 1/3] bpf: implement link_query for bpf iterators
2020-08-21 6:42 ` Yonghong Song
@ 2020-08-21 16:44 ` Andrii Nakryiko
2020-08-21 17:46 ` Yonghong Song
0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-08-21 16:44 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Thu, Aug 20, 2020 at 11:42 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 8/20/20 11:31 PM, Andrii Nakryiko wrote:
> > On Thu, Aug 20, 2020 at 3:50 PM Yonghong Song <yhs@fb.com> wrote:
> >>
> >> This patch implemented bpf_link callback functions
> >> show_fdinfo and fill_link_info to support link_query
> >> interface.
> >>
> >> The general interface for show_fdinfo and fill_link_info
> >> will print/fill the target_name. Each targets can
> >> register show_fdinfo and fill_link_info callbacks
> >> to print/fill more target specific information.
> >>
> >> For example, the below is a fdinfo result for a bpf
> >> task iterator.
> >> $ cat /proc/1749/fdinfo/7
> >> pos: 0
> >> flags: 02000000
> >> mnt_id: 14
> >> link_type: iter
> >> link_id: 11
> >> prog_tag: 990e1f8152f7e54f
> >> prog_id: 59
> >> target_name: task
> >>
> >> Signed-off-by: Yonghong Song <yhs@fb.com>
> >> ---
> >> include/linux/bpf.h | 6 ++++
> >> include/uapi/linux/bpf.h | 7 ++++
> >> kernel/bpf/bpf_iter.c | 58 ++++++++++++++++++++++++++++++++++
> >> tools/include/uapi/linux/bpf.h | 7 ++++
> >> 4 files changed, 78 insertions(+)
> >>
> >
> > [...]
> >
> >> +
> >> +static int bpf_iter_link_fill_link_info(const struct bpf_link *link,
> >> + struct bpf_link_info *info)
> >> +{
> >> + struct bpf_iter_link *iter_link =
> >> + container_of(link, struct bpf_iter_link, link);
> >> + char __user *ubuf = u64_to_user_ptr(info->iter.target_name);
> >> + bpf_iter_fill_link_info_t fill_link_info;
> >> + u32 ulen = info->iter.target_name_len;
> >> + const char *target_name;
> >> + u32 target_len;
> >> +
> >> + if (ulen && !ubuf)
> >> + return -EINVAL;
> >> +
> >> + target_name = iter_link->tinfo->reg_info->target;
> >> + target_len = strlen(target_name);
> >> + info->iter.target_name_len = target_len + 1;
> >> + if (!ubuf)
> >> + return 0;
> >
> > this might return prematurely before fill_link_info() below gets a
> > chance to fill in some extra info?
>
> The extra info filled by below fill_link_info is target specific
> and we need a target name to ensure picking right union members.
> So it is best to enforce a valid target name before filling
> target dependent fields. See below, if there are any errors
> for copy_to_user or enospc, we won't copy addition link info
> either.
>
You are making an assumption that the caller doesn't know what time of
link it's requesting info for. That's not generally true. So I think
we just shouldn't make unnecessary assumptions and provide as much
information on the first try. target_name should be treated as an
optional thing to request, that's all.
> >
> >> +
> >> + if (ulen >= target_len + 1) {
> >> + if (copy_to_user(ubuf, target_name, target_len + 1))
> >> + return -EFAULT;
> >> + } else {
> >> + char zero = '\0';
> >> +
> >> + if (copy_to_user(ubuf, target_name, ulen - 1))
> >> + return -EFAULT;
> >> + if (put_user(zero, ubuf + ulen - 1))
> >> + return -EFAULT;
> >> + return -ENOSPC;
> >> + }
> >> +
> >> + fill_link_info = iter_link->tinfo->reg_info->fill_link_info;
> >> + if (fill_link_info)
> >> + return fill_link_info(&iter_link->aux, info);
> >> +
> >> + return 0;
> >> +}
> >> +
> >
> > [...]
> >
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH bpf-next v2 1/3] bpf: implement link_query for bpf iterators
2020-08-21 16:44 ` Andrii Nakryiko
@ 2020-08-21 17:46 ` Yonghong Song
0 siblings, 0 replies; 11+ messages in thread
From: Yonghong Song @ 2020-08-21 17:46 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On 8/21/20 9:44 AM, Andrii Nakryiko wrote:
> On Thu, Aug 20, 2020 at 11:42 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 8/20/20 11:31 PM, Andrii Nakryiko wrote:
>>> On Thu, Aug 20, 2020 at 3:50 PM Yonghong Song <yhs@fb.com> wrote:
>>>>
>>>> This patch implemented bpf_link callback functions
>>>> show_fdinfo and fill_link_info to support link_query
>>>> interface.
>>>>
>>>> The general interface for show_fdinfo and fill_link_info
>>>> will print/fill the target_name. Each targets can
>>>> register show_fdinfo and fill_link_info callbacks
>>>> to print/fill more target specific information.
>>>>
>>>> For example, the below is a fdinfo result for a bpf
>>>> task iterator.
>>>> $ cat /proc/1749/fdinfo/7
>>>> pos: 0
>>>> flags: 02000000
>>>> mnt_id: 14
>>>> link_type: iter
>>>> link_id: 11
>>>> prog_tag: 990e1f8152f7e54f
>>>> prog_id: 59
>>>> target_name: task
>>>>
>>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>>> ---
>>>> include/linux/bpf.h | 6 ++++
>>>> include/uapi/linux/bpf.h | 7 ++++
>>>> kernel/bpf/bpf_iter.c | 58 ++++++++++++++++++++++++++++++++++
>>>> tools/include/uapi/linux/bpf.h | 7 ++++
>>>> 4 files changed, 78 insertions(+)
>>>>
>>>
>>> [...]
>>>
>>>> +
>>>> +static int bpf_iter_link_fill_link_info(const struct bpf_link *link,
>>>> + struct bpf_link_info *info)
>>>> +{
>>>> + struct bpf_iter_link *iter_link =
>>>> + container_of(link, struct bpf_iter_link, link);
>>>> + char __user *ubuf = u64_to_user_ptr(info->iter.target_name);
>>>> + bpf_iter_fill_link_info_t fill_link_info;
>>>> + u32 ulen = info->iter.target_name_len;
>>>> + const char *target_name;
>>>> + u32 target_len;
>>>> +
>>>> + if (ulen && !ubuf)
>>>> + return -EINVAL;
>>>> +
>>>> + target_name = iter_link->tinfo->reg_info->target;
>>>> + target_len = strlen(target_name);
>>>> + info->iter.target_name_len = target_len + 1;
>>>> + if (!ubuf)
>>>> + return 0;
>>>
>>> this might return prematurely before fill_link_info() below gets a
>>> chance to fill in some extra info?
>>
>> The extra info filled by below fill_link_info is target specific
>> and we need a target name to ensure picking right union members.
>> So it is best to enforce a valid target name before filling
>> target dependent fields. See below, if there are any errors
>> for copy_to_user or enospc, we won't copy addition link info
>> either.
>>
>
> You are making an assumption that the caller doesn't know what time of
> link it's requesting info for. That's not generally true. So I think
Based on my understanding, most users for bpf command
BPF_OBJ_GET_INFO_BY_FD is for tools, not the original application
which created the original link.
But I agree there are certain use cases where the caller has
much more knowledge about 'fd' than bpftool and they may just
want to get one particular piece of information.
> we just shouldn't make unnecessary assumptions and provide as much
> information on the first try. target_name should be treated as an
> optional thing to request, that's all.
Okay, will do this.
>>>
>>>> +
>>>> + if (ulen >= target_len + 1) {
>>>> + if (copy_to_user(ubuf, target_name, target_len + 1))
>>>> + return -EFAULT;
>>>> + } else {
>>>> + char zero = '\0';
>>>> +
>>>> + if (copy_to_user(ubuf, target_name, ulen - 1))
>>>> + return -EFAULT;
>>>> + if (put_user(zero, ubuf + ulen - 1))
>>>> + return -EFAULT;
>>>> + return -ENOSPC;
>>>> + }
>>>> +
>>>> + fill_link_info = iter_link->tinfo->reg_info->fill_link_info;
>>>> + if (fill_link_info)
>>>> + return fill_link_info(&iter_link->aux, info);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>
>>> [...]
>>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH bpf-next v2 2/3] bpf: implement link_query callbacks in map element iterators
2020-08-20 22:49 [PATCH bpf-next v2 0/3] bpf: implement link_query for bpf iterators Yonghong Song
2020-08-20 22:49 ` [PATCH bpf-next v2 1/3] " Yonghong Song
@ 2020-08-20 22:49 ` Yonghong Song
2020-08-21 6:33 ` Andrii Nakryiko
2020-08-20 22:49 ` [PATCH bpf-next v2 3/3] bpftool: implement link_query for bpf iterators Yonghong Song
2 siblings, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2020-08-20 22:49 UTC (permalink / raw)
To: bpf, netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
For bpf_map_elem and bpf_sk_local_storage bpf iterators,
additional map_id should be shown for fdinfo and
userspace query. For example, the following is for
a bpf_map_elem iterator.
$ cat /proc/1753/fdinfo/9
pos: 0
flags: 02000000
mnt_id: 14
link_type: iter
link_id: 34
prog_tag: 104be6d3fe45e6aa
prog_id: 173
target_name: bpf_map_elem
map_id: 127
Signed-off-by: Yonghong Song <yhs@fb.com>
---
include/linux/bpf.h | 4 ++++
kernel/bpf/map_iter.c | 15 +++++++++++++++
net/core/bpf_sk_storage.c | 2 ++
3 files changed, 21 insertions(+)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 529e9b183eeb..30c144af894a 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1256,6 +1256,10 @@ int bpf_iter_new_fd(struct bpf_link *link);
bool bpf_link_is_iter(struct bpf_link *link);
struct bpf_prog *bpf_iter_get_info(struct bpf_iter_meta *meta, bool in_stop);
int bpf_iter_run_prog(struct bpf_prog *prog, void *ctx);
+void bpf_iter_map_show_fdinfo(const struct bpf_iter_aux_info *aux,
+ struct seq_file *seq);
+int bpf_iter_map_fill_link_info(const struct bpf_iter_aux_info *aux,
+ struct bpf_link_info *info);
int bpf_percpu_hash_copy(struct bpf_map *map, void *key, void *value);
int bpf_percpu_array_copy(struct bpf_map *map, void *key, void *value);
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index af86048e5afd..714e74556aa2 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -149,6 +149,19 @@ static void bpf_iter_detach_map(struct bpf_iter_aux_info *aux)
bpf_map_put_with_uref(aux->map);
}
+void bpf_iter_map_show_fdinfo(const struct bpf_iter_aux_info *aux,
+ struct seq_file *seq)
+{
+ seq_printf(seq, "map_id:\t\t%u\n", aux->map->id);
+}
+
+int bpf_iter_map_fill_link_info(const struct bpf_iter_aux_info *aux,
+ struct bpf_link_info *info)
+{
+ info->iter.map.map_id = aux->map->id;
+ return 0;
+}
+
DEFINE_BPF_ITER_FUNC(bpf_map_elem, struct bpf_iter_meta *meta,
struct bpf_map *map, void *key, void *value)
@@ -156,6 +169,8 @@ static const struct bpf_iter_reg bpf_map_elem_reg_info = {
.target = "bpf_map_elem",
.attach_target = bpf_iter_attach_map,
.detach_target = bpf_iter_detach_map,
+ .show_fdinfo = bpf_iter_map_show_fdinfo,
+ .fill_link_info = bpf_iter_map_fill_link_info,
.ctx_arg_info_size = 2,
.ctx_arg_info = {
{ offsetof(struct bpf_iter__bpf_map_elem, key),
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index b988f48153a4..281200dc0a01 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -1437,6 +1437,8 @@ static struct bpf_iter_reg bpf_sk_storage_map_reg_info = {
.target = "bpf_sk_storage_map",
.attach_target = bpf_iter_attach_map,
.detach_target = bpf_iter_detach_map,
+ .show_fdinfo = bpf_iter_map_show_fdinfo,
+ .fill_link_info = bpf_iter_map_fill_link_info,
.ctx_arg_info_size = 2,
.ctx_arg_info = {
{ offsetof(struct bpf_iter__bpf_sk_storage_map, sk),
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH bpf-next v2 2/3] bpf: implement link_query callbacks in map element iterators
2020-08-20 22:49 ` [PATCH bpf-next v2 2/3] bpf: implement link_query callbacks in map element iterators Yonghong Song
@ 2020-08-21 6:33 ` Andrii Nakryiko
2020-08-21 6:44 ` Yonghong Song
0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-08-21 6:33 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Thu, Aug 20, 2020 at 3:50 PM Yonghong Song <yhs@fb.com> wrote:
>
> For bpf_map_elem and bpf_sk_local_storage bpf iterators,
> additional map_id should be shown for fdinfo and
> userspace query. For example, the following is for
> a bpf_map_elem iterator.
> $ cat /proc/1753/fdinfo/9
> pos: 0
> flags: 02000000
> mnt_id: 14
> link_type: iter
> link_id: 34
> prog_tag: 104be6d3fe45e6aa
> prog_id: 173
> target_name: bpf_map_elem
> map_id: 127
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> include/linux/bpf.h | 4 ++++
> kernel/bpf/map_iter.c | 15 +++++++++++++++
> net/core/bpf_sk_storage.c | 2 ++
> 3 files changed, 21 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 529e9b183eeb..30c144af894a 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1256,6 +1256,10 @@ int bpf_iter_new_fd(struct bpf_link *link);
> bool bpf_link_is_iter(struct bpf_link *link);
> struct bpf_prog *bpf_iter_get_info(struct bpf_iter_meta *meta, bool in_stop);
> int bpf_iter_run_prog(struct bpf_prog *prog, void *ctx);
> +void bpf_iter_map_show_fdinfo(const struct bpf_iter_aux_info *aux,
> + struct seq_file *seq);
> +int bpf_iter_map_fill_link_info(const struct bpf_iter_aux_info *aux,
> + struct bpf_link_info *info);
>
> int bpf_percpu_hash_copy(struct bpf_map *map, void *key, void *value);
> int bpf_percpu_array_copy(struct bpf_map *map, void *key, void *value);
> diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
> index af86048e5afd..714e74556aa2 100644
> --- a/kernel/bpf/map_iter.c
> +++ b/kernel/bpf/map_iter.c
> @@ -149,6 +149,19 @@ static void bpf_iter_detach_map(struct bpf_iter_aux_info *aux)
> bpf_map_put_with_uref(aux->map);
> }
>
> +void bpf_iter_map_show_fdinfo(const struct bpf_iter_aux_info *aux,
> + struct seq_file *seq)
> +{
> + seq_printf(seq, "map_id:\t\t%u\n", aux->map->id);
nit: I think it's a bad idea to have two tabs here to align everything
visually, might make parsing unnecessarily harder
> +}
> +
> +int bpf_iter_map_fill_link_info(const struct bpf_iter_aux_info *aux,
> + struct bpf_link_info *info)
> +{
> + info->iter.map.map_id = aux->map->id;
> + return 0;
> +}
> +
> DEFINE_BPF_ITER_FUNC(bpf_map_elem, struct bpf_iter_meta *meta,
> struct bpf_map *map, void *key, void *value)
>
> @@ -156,6 +169,8 @@ static const struct bpf_iter_reg bpf_map_elem_reg_info = {
> .target = "bpf_map_elem",
> .attach_target = bpf_iter_attach_map,
> .detach_target = bpf_iter_detach_map,
> + .show_fdinfo = bpf_iter_map_show_fdinfo,
> + .fill_link_info = bpf_iter_map_fill_link_info,
> .ctx_arg_info_size = 2,
> .ctx_arg_info = {
> { offsetof(struct bpf_iter__bpf_map_elem, key),
> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index b988f48153a4..281200dc0a01 100644
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c
> @@ -1437,6 +1437,8 @@ static struct bpf_iter_reg bpf_sk_storage_map_reg_info = {
> .target = "bpf_sk_storage_map",
> .attach_target = bpf_iter_attach_map,
> .detach_target = bpf_iter_detach_map,
> + .show_fdinfo = bpf_iter_map_show_fdinfo,
> + .fill_link_info = bpf_iter_map_fill_link_info,
> .ctx_arg_info_size = 2,
> .ctx_arg_info = {
> { offsetof(struct bpf_iter__bpf_sk_storage_map, sk),
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH bpf-next v2 2/3] bpf: implement link_query callbacks in map element iterators
2020-08-21 6:33 ` Andrii Nakryiko
@ 2020-08-21 6:44 ` Yonghong Song
0 siblings, 0 replies; 11+ messages in thread
From: Yonghong Song @ 2020-08-21 6:44 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On 8/20/20 11:33 PM, Andrii Nakryiko wrote:
> On Thu, Aug 20, 2020 at 3:50 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> For bpf_map_elem and bpf_sk_local_storage bpf iterators,
>> additional map_id should be shown for fdinfo and
>> userspace query. For example, the following is for
>> a bpf_map_elem iterator.
>> $ cat /proc/1753/fdinfo/9
>> pos: 0
>> flags: 02000000
>> mnt_id: 14
>> link_type: iter
>> link_id: 34
>> prog_tag: 104be6d3fe45e6aa
>> prog_id: 173
>> target_name: bpf_map_elem
>> map_id: 127
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> include/linux/bpf.h | 4 ++++
>> kernel/bpf/map_iter.c | 15 +++++++++++++++
>> net/core/bpf_sk_storage.c | 2 ++
>> 3 files changed, 21 insertions(+)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 529e9b183eeb..30c144af894a 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1256,6 +1256,10 @@ int bpf_iter_new_fd(struct bpf_link *link);
>> bool bpf_link_is_iter(struct bpf_link *link);
>> struct bpf_prog *bpf_iter_get_info(struct bpf_iter_meta *meta, bool in_stop);
>> int bpf_iter_run_prog(struct bpf_prog *prog, void *ctx);
>> +void bpf_iter_map_show_fdinfo(const struct bpf_iter_aux_info *aux,
>> + struct seq_file *seq);
>> +int bpf_iter_map_fill_link_info(const struct bpf_iter_aux_info *aux,
>> + struct bpf_link_info *info);
>>
>> int bpf_percpu_hash_copy(struct bpf_map *map, void *key, void *value);
>> int bpf_percpu_array_copy(struct bpf_map *map, void *key, void *value);
>> diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
>> index af86048e5afd..714e74556aa2 100644
>> --- a/kernel/bpf/map_iter.c
>> +++ b/kernel/bpf/map_iter.c
>> @@ -149,6 +149,19 @@ static void bpf_iter_detach_map(struct bpf_iter_aux_info *aux)
>> bpf_map_put_with_uref(aux->map);
>> }
>>
>> +void bpf_iter_map_show_fdinfo(const struct bpf_iter_aux_info *aux,
>> + struct seq_file *seq)
>> +{
>> + seq_printf(seq, "map_id:\t\t%u\n", aux->map->id);
>
> nit: I think it's a bad idea to have two tabs here to align everything
> visually, might make parsing unnecessarily harder
You are 100% correct. This is on purpose to please visual alignment.
I debate with myself to add extra '\t'. But yes, it is bad for tool
parsing. I will fix it in the next revision.
>
>> +}
>> +
>> +int bpf_iter_map_fill_link_info(const struct bpf_iter_aux_info *aux,
>> + struct bpf_link_info *info)
>> +{
>> + info->iter.map.map_id = aux->map->id;
>> + return 0;
>> +}
>> +
>> DEFINE_BPF_ITER_FUNC(bpf_map_elem, struct bpf_iter_meta *meta,
>> struct bpf_map *map, void *key, void *value)
>>
>> @@ -156,6 +169,8 @@ static const struct bpf_iter_reg bpf_map_elem_reg_info = {
>> .target = "bpf_map_elem",
>> .attach_target = bpf_iter_attach_map,
>> .detach_target = bpf_iter_detach_map,
>> + .show_fdinfo = bpf_iter_map_show_fdinfo,
>> + .fill_link_info = bpf_iter_map_fill_link_info,
>> .ctx_arg_info_size = 2,
>> .ctx_arg_info = {
>> { offsetof(struct bpf_iter__bpf_map_elem, key),
>> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
>> index b988f48153a4..281200dc0a01 100644
>> --- a/net/core/bpf_sk_storage.c
>> +++ b/net/core/bpf_sk_storage.c
>> @@ -1437,6 +1437,8 @@ static struct bpf_iter_reg bpf_sk_storage_map_reg_info = {
>> .target = "bpf_sk_storage_map",
>> .attach_target = bpf_iter_attach_map,
>> .detach_target = bpf_iter_detach_map,
>> + .show_fdinfo = bpf_iter_map_show_fdinfo,
>> + .fill_link_info = bpf_iter_map_fill_link_info,
>> .ctx_arg_info_size = 2,
>> .ctx_arg_info = {
>> { offsetof(struct bpf_iter__bpf_sk_storage_map, sk),
>> --
>> 2.24.1
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH bpf-next v2 3/3] bpftool: implement link_query for bpf iterators
2020-08-20 22:49 [PATCH bpf-next v2 0/3] bpf: implement link_query for bpf iterators Yonghong Song
2020-08-20 22:49 ` [PATCH bpf-next v2 1/3] " Yonghong Song
2020-08-20 22:49 ` [PATCH bpf-next v2 2/3] bpf: implement link_query callbacks in map element iterators Yonghong Song
@ 2020-08-20 22:49 ` Yonghong Song
2020-08-21 6:35 ` Andrii Nakryiko
2 siblings, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2020-08-20 22:49 UTC (permalink / raw)
To: bpf, netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
The link query for bpf iterators is implemented.
Besides being shown to the user what bpf iterator
the link represents, the target_name is also used
to filter out what additional information should be
printed out, e.g., whether map_id should be shown or not.
The following is an example of bpf_iter link dump,
plain output or pretty output.
$ bpftool link show
11: iter prog 59 target_name task
pids test_progs(1749)
34: iter prog 173 target_name bpf_map_elem map_id 127
pids test_progs_1(1753)
$ bpftool -p link show
[{
"id": 11,
"type": "iter",
"prog_id": 59,
"target_name": "task",
"pids": [{
"pid": 1749,
"comm": "test_progs"
}
]
},{
"id": 34,
"type": "iter",
"prog_id": 173,
"target_name": "bpf_map_elem",
"map_id": 127,
"pids": [{
"pid": 1753,
"comm": "test_progs_1"
}
]
}
]
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/bpf/bpftool/link.c | 44 +++++++++++++++++++++++++++++++++++++---
1 file changed, 41 insertions(+), 3 deletions(-)
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index a89f09e3c848..e77e1525d20a 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -77,6 +77,22 @@ static void show_link_attach_type_json(__u32 attach_type, json_writer_t *wtr)
jsonw_uint_field(wtr, "attach_type", attach_type);
}
+static bool is_iter_map_target(const char *target_name)
+{
+ return strcmp(target_name, "bpf_map_elem") == 0 ||
+ strcmp(target_name, "bpf_sk_storage_map") == 0;
+}
+
+static void show_iter_json(struct bpf_link_info *info, json_writer_t *wtr)
+{
+ const char *target_name = u64_to_ptr(info->iter.target_name);
+
+ jsonw_string_field(wtr, "target_name", target_name);
+
+ if (is_iter_map_target(target_name))
+ jsonw_uint_field(wtr, "map_id", info->iter.map.map_id);
+}
+
static int get_prog_info(int prog_id, struct bpf_prog_info *info)
{
__u32 len = sizeof(*info);
@@ -128,6 +144,9 @@ static int show_link_close_json(int fd, struct bpf_link_info *info)
info->cgroup.cgroup_id);
show_link_attach_type_json(info->cgroup.attach_type, json_wtr);
break;
+ case BPF_LINK_TYPE_ITER:
+ show_iter_json(info, json_wtr);
+ break;
case BPF_LINK_TYPE_NETNS:
jsonw_uint_field(json_wtr, "netns_ino",
info->netns.netns_ino);
@@ -175,6 +194,16 @@ static void show_link_attach_type_plain(__u32 attach_type)
printf("attach_type %u ", attach_type);
}
+static void show_iter_plain(struct bpf_link_info *info)
+{
+ const char *target_name = u64_to_ptr(info->iter.target_name);
+
+ printf("target_name %s ", target_name);
+
+ if (is_iter_map_target(target_name))
+ printf("map_id %u ", info->iter.map.map_id);
+}
+
static int show_link_close_plain(int fd, struct bpf_link_info *info)
{
struct bpf_prog_info prog_info;
@@ -204,6 +233,9 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
printf("\n\tcgroup_id %zu ", (size_t)info->cgroup.cgroup_id);
show_link_attach_type_plain(info->cgroup.attach_type);
break;
+ case BPF_LINK_TYPE_ITER:
+ show_iter_plain(info);
+ break;
case BPF_LINK_TYPE_NETNS:
printf("\n\tnetns_ino %u ", info->netns.netns_ino);
show_link_attach_type_plain(info->netns.attach_type);
@@ -231,7 +263,7 @@ static int do_show_link(int fd)
{
struct bpf_link_info info;
__u32 len = sizeof(info);
- char raw_tp_name[256];
+ char buf[256];
int err;
memset(&info, 0, sizeof(info));
@@ -245,8 +277,14 @@ static int do_show_link(int fd)
}
if (info.type == BPF_LINK_TYPE_RAW_TRACEPOINT &&
!info.raw_tracepoint.tp_name) {
- info.raw_tracepoint.tp_name = (unsigned long)&raw_tp_name;
- info.raw_tracepoint.tp_name_len = sizeof(raw_tp_name);
+ info.raw_tracepoint.tp_name = (unsigned long)&buf;
+ info.raw_tracepoint.tp_name_len = sizeof(buf);
+ goto again;
+ }
+ if (info.type == BPF_LINK_TYPE_ITER &&
+ !info.iter.target_name) {
+ info.iter.target_name = (unsigned long)&buf;
+ info.iter.target_name_len = sizeof(buf);
goto again;
}
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH bpf-next v2 3/3] bpftool: implement link_query for bpf iterators
2020-08-20 22:49 ` [PATCH bpf-next v2 3/3] bpftool: implement link_query for bpf iterators Yonghong Song
@ 2020-08-21 6:35 ` Andrii Nakryiko
0 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-08-21 6:35 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Thu, Aug 20, 2020 at 3:50 PM Yonghong Song <yhs@fb.com> wrote:
>
> The link query for bpf iterators is implemented.
> Besides being shown to the user what bpf iterator
> the link represents, the target_name is also used
> to filter out what additional information should be
> printed out, e.g., whether map_id should be shown or not.
> The following is an example of bpf_iter link dump,
> plain output or pretty output.
>
> $ bpftool link show
> 11: iter prog 59 target_name task
> pids test_progs(1749)
> 34: iter prog 173 target_name bpf_map_elem map_id 127
> pids test_progs_1(1753)
> $ bpftool -p link show
> [{
> "id": 11,
> "type": "iter",
> "prog_id": 59,
> "target_name": "task",
> "pids": [{
> "pid": 1749,
> "comm": "test_progs"
> }
> ]
> },{
> "id": 34,
> "type": "iter",
> "prog_id": 173,
> "target_name": "bpf_map_elem",
> "map_id": 127,
> "pids": [{
> "pid": 1753,
> "comm": "test_progs_1"
> }
> ]
> }
> ]
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
LGTM.
Acked-by: Andrii Nakryiko <andriin@fb.com>
> tools/bpf/bpftool/link.c | 44 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 41 insertions(+), 3 deletions(-)
>
[...]
^ permalink raw reply [flat|nested] 11+ messages in thread