Re: [PATCH bpf-next v2 1/3] bpf: implement link_query for bpf iterators

[Yonghong Song <yhs@fb.com>]

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;
>> +}
>> +
> 
> [...]
>