| Message ID | 20200820224917.483128-1-yhs@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: implement link_query for bpf iterators | expand |
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; > +} > + [...]
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; >> +} >> + > > [...] >
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; > >> +} > >> + > > > > [...] > >
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; >>>> +} >>>> + >>> >>> [...] >>>
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;
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(+)