Re: [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf_id()

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Sep 17, 2020 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> >>
>> >> +int bpf_check_attach_target(struct bpf_verifier_log *log,
>> >> +                           const struct bpf_prog *prog,
>> >> +                           const struct bpf_prog *tgt_prog,
>> >> +                           u32 btf_id,
>> >> +                           struct btf_func_model *fmodel,
>> >> +                           long *tgt_addr,
>> >> +                           const char **tgt_name,
>> >> +                           const struct btf_type **tgt_type);
>> >
>> > So this is obviously an abomination of a function signature,
>> > especially for a one exported to other files.
>> >
>> > One candidate to remove would be tgt_type, which is supposed to be a
>> > derivative of target BTF (vmlinux or tgt_prog->btf) + btf_id,
>> > **except** (and that's how I found the bug below), in case of
>> > fentry/fexit programs attaching to "conservative" BPF functions, in
>> > which case what's stored in aux->attach_func_proto is different from
>> > what is passed into btf_distill_func_proto. So that's a bug already
>> > (you'll return NULL in some cases for tgt_type, while it has to always
>> > be non-NULL).
>>
>> Okay, looked at this in more detail, and I don't think the refactored
>> code is doing anything different from the pre-refactor version?
>>
>> Before we had this:
>>
>>                 if (tgt_prog && conservative) {
>>                         prog->aux->attach_func_proto = NULL;
>>                         t = NULL;
>>                 }
>>
>> and now we just have
>>
>>                 if (tgt_prog && conservative)
>>                         t = NULL;
>>
>> in bpf_check_attach_target(), which gets returned as tgt_type and
>> subsequently assigned to prog->aux->attach_func_proto.
>
> Yeah, you are totally right, I don't know how I missed that
> `prog->aux->attach_func_proto = NULL;`, sorry about that.

No worries - this was certainly not the easiest to review; thanks for
sticking with it! :)

[..]

> Please add my ack when you post a new version:
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>

Will do, thanks!

-Toke