Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes:
> On Thu, Sep 17, 2020 at 3:06 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> 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@xxxxxx>
Will do, thanks!
-Toke
