netdev - Re: [PATCH bpf-next v2 1/3] bpf: implement link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d0b44fee-97f4-05bc-90b7-0b47f238ab37@fb.com>
Date:   Fri, 21 Aug 2020 10:46:34 -0700
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next v2 1/3] bpf: implement link_query for bpf
 iterators



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