| 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
| ||
|
Message-ID: <051dab4c-bbde-320e-c2bf-da63a7994fc4@fb.com>
Date: Tue, 23 Jun 2020 13:46:22 -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>,
Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH bpf-next v3 05/15] bpf: add bpf_skc_to_tcp6_sock() helper
On 6/23/20 1:11 PM, Andrii Nakryiko wrote:
> On Tue, Jun 23, 2020 at 12:47 PM Yonghong Song <yhs@...com> wrote:
>>
>>
>>
>> On 6/23/20 11:23 AM, Andrii Nakryiko wrote:
>>> On Tue, Jun 23, 2020 at 7:52 AM Yonghong Song <yhs@...com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/22/20 11:39 PM, Andrii Nakryiko wrote:
>>>>> On Mon, Jun 22, 2020 at 5:38 PM Yonghong Song <yhs@...com> wrote:
>>>>>>
>>>>>> The helper is used in tracing programs to cast a socket
>>>>>> pointer to a tcp6_sock pointer.
>>>>>> The return value could be NULL if the casting is illegal.
>>>>>>
>>>>>> A new helper return type RET_PTR_TO_BTF_ID_OR_NULL is added
>>>>>> so the verifier is able to deduce proper return types for the helper.
>>>>>>
>>>>>> Different from the previous BTF_ID based helpers,
>>>>>> the bpf_skc_to_tcp6_sock() argument can be several possible
>>>>>> btf_ids. More specifically, all possible socket data structures
>>>>>> with sock_common appearing in the first in the memory layout.
>>>>>> This patch only added socket types related to tcp and udp.
>>>>>>
>>>>>> All possible argument btf_id and return value btf_id
>>>>>> for helper bpf_skc_to_tcp6_sock() are pre-calculcated and
>>>>>> cached. In the future, it is even possible to precompute
>>>>>> these btf_id's at kernel build time.
>>>>>>
>>>>>> Acked-by: Martin KaFai Lau <kafai@...com>
>>>>>> Signed-off-by: Yonghong Song <yhs@...com>
>>>>>> ---
>>>>>
>>>>> Looks good to me as is, but see a few suggestions, they will probably
>>>>> save me time at some point as well :)
>>>>>
>>>>> Acked-by: Andrii Nakryiko <andriin@...com>
>>>>>
>>>>>
>>>>>> include/linux/bpf.h | 12 +++++
>>>>>> include/uapi/linux/bpf.h | 9 +++-
>>>>>> kernel/bpf/btf.c | 1 +
>>>>>> kernel/bpf/verifier.c | 43 +++++++++++++-----
>>>>>> kernel/trace/bpf_trace.c | 2 +
>>>>>> net/core/filter.c | 80 ++++++++++++++++++++++++++++++++++
>>>>>> scripts/bpf_helpers_doc.py | 2 +
>>>>>> tools/include/uapi/linux/bpf.h | 9 +++-
>>>>>> 8 files changed, 146 insertions(+), 12 deletions(-)
>>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>> @@ -4815,6 +4826,18 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>>>>>> regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
>>>>>> regs[BPF_REG_0].id = ++env->id_gen;
>>>>>> regs[BPF_REG_0].mem_size = meta.mem_size;
>>>>>> + } else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL) {
>>>>>> + int ret_btf_id;
>>>>>> +
>>>>>> + mark_reg_known_zero(env, regs, BPF_REG_0);
>>>>>> + regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL;
>>>>>> + ret_btf_id = *fn->ret_btf_id;
[...]
>>>
>>>>
>>>>>
>>>>>> + if (ret_btf_id == 0) {
>>>>>
>>>>> This also has to be struct/union (after typedef/mods stripping, of
>>>>> course). Or are there other cases?
>>>>
>>>> This is an "int". The func_proto difinition is below:
>>>> int *ret_btf_id; /* return value btf_id */
>>>
>>> I meant the BTF type itself that this btf_id points to. Is there any
>>> use case where this won't be a pointer to struct/union and instead
>>> something like a pointer to an int?
>>
>> Maybe you misunderstood. The mechanism is similar to the argument btf_id
>> encoding in func_proto's:
>>
>> static int bpf_seq_printf_btf_ids[5];
>> ...
>> .btf_id = bpf_seq_printf_btf_ids,
>>
>> func_proto->ret_btf_id will be a pointer to int which encodes the
>> btf_id, not the btf_type.
>
> I understand that. Say it points to value 25. BTF type with ID=25 is
> going to be BTF_KIND_PTR -> BTF_KIND_STRUCT. I was wondering if we
> want/need to check that it's always BTF_KIND_PTR -> (modifier)* ->
> BTF_KIND_STRUCT/BTF_KIND_UNION. That's it.
Just to be clear. The ret_btf_id returned here is the btf id is the
type id of the pointee, so in this case it is BTF_KIND_STRUCT/....
These id's are pre-calculated and stored in memory. Unless the whole
thing is mess up, there is no need to check...
>
>>
>>>
>>>>
>>>>>
>>>>>> + verbose(env, "invalid return type %d of func %s#%d\n",
>>>>>> + fn->ret_type, func_id_name(func_id), func_id);
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> + regs[BPF_REG_0].btf_id = ret_btf_id;
>>>>>> } else {
>>>>>> verbose(env, "unknown return type %d of func %s#%d\n",
>>>>>> fn->ret_type, func_id_name(func_id), func_id);
>>>>>
>>>>> [...]
>>>>>
>>>>>> +void init_btf_sock_ids(struct btf *btf)
>>>>>> +{
>>>>>> + int i, btf_id;
>>>>>> +
>>>>>> + for (i = 0; i < MAX_BTF_SOCK_TYPE; i++) {
>>>>>> + btf_id = btf_find_by_name_kind(btf, bpf_sock_types[i],
>>>>>> + BTF_KIND_STRUCT);
>>>>>> + if (btf_id > 0)
>>>>>> + btf_sock_ids[i] = btf_id;
>>>>>> + }
>>>>>> +}
>>>>>
>>>>> This will hopefully go away with Jiri's work on static BTF IDs, right?
>>>>> So looking forward to that :)
>>>>
>>>> Yes. That's the plan.
>>>>
>>>>>
>>>>>> +
>>>>>> +static bool check_arg_btf_id(u32 btf_id, u32 arg)
>>>>>> +{
>>>>>> + int i;
>>>>>> +
>>>>>> + /* only one argument, no need to check arg */
>>>>>> + for (i = 0; i < MAX_BTF_SOCK_TYPE; i++)
>>>>>> + if (btf_sock_ids[i] == btf_id)
>>>>>> + return true;
>>>>>> + return false;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> [...]
>>>>>
Powered by blists - more mailing lists