On Thu, Aug 20, 2020 at 11:42 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 8/20/20 11:31 PM, Andrii Nakryiko wrote:
> > On Thu, Aug 20, 2020 at 3:50 PM Yonghong Song <yhs@xxxxxx> 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@xxxxxx>
> >> ---
> >> 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;
> >> +}
> >> +
> >
> > [...]
> >
