Re: [PATCH bpf-next 1/2] bpf: Fix updating attached freplace prog to PROG_ARRAY map

[Yonghong Song <yonghong.song@linux.dev>]

On 7/25/24 8:27 PM, leon.hwang@linux.dev wrote:
> 26 July 2024 at 04:58, "Yonghong Song" <yonghong.song@linux.dev> wrote:
>
>
>
>> On 7/24/24 5:32 PM, Leon Hwang wrote:
>>
>>> The commit f7866c3587337731 ("bpf: Fix null pointer dereference in
>>>
>>>   resolve_prog_type() for BPF_PROG_TYPE_EXT") fixed the following panic,
>>>
>>>   which was caused by updating attached freplace prog to PROG_ARRAY map.
>>>
>> I am confused here. You mentioned that commit f7866c3587337731
>>
>> fixed the panic below. But looking at commit message:
>>
>>   https://lore.kernel.org/bpf/20240711145819.254178-2-wutengda@huaweicloud.com
>>
>> it does not seem the case.
> The commit fixed this panic meanwhile.
>
> This panic seems confusing. I'll remove it in patch v2.
>
[...]

>>>   ---
>>>
>>>   include/linux/bpf_verifier.h | 4 ++--
>>>
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>>   diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>>>
>>>   index 5cea15c81b8a8..387e034e73d0e 100644
>>>
>>>   --- a/include/linux/bpf_verifier.h
>>>
>>>   +++ b/include/linux/bpf_verifier.h
>>>
>>>   @@ -874,8 +874,8 @@ static inline u32 type_flag(u32 type)
>>>
>>>   /* only use after check_attach_btf_id() */
>>>
>>>   static inline enum bpf_prog_type resolve_prog_type(const struct bpf_prog *prog)
>>>
>>>   {
>>>
>>>   - return (prog->type == BPF_PROG_TYPE_EXT && prog->aux->dst_prog) ?
>>>
>>>   - prog->aux->dst_prog->type : prog->type;
>>>
>>>   + return prog->type == BPF_PROG_TYPE_EXT ?
>>>
>>>   + prog->aux->saved_dst_prog_type : prog->type;
>>>
>> If prog->aux->dst_prog is NULL, is it possible that prog->aux->saved_dst_prog_type
>>
>> (0, corresponding to BPF_PROG_TYPE_UNSPEC) could be returned? Do we need to do
>>
>>   return (prog->type == BPF_PROG_TYPE_EXT && prog->aux->saved_dst_prog_type) ?
>>
>>   prog->aux->saved_dst_prog_type : prog->type;
>>
>> Maybe I missed something here?
> It seems better to check prog->aux->saved_dst_prog_type. But I don't think so.
>
> prog->aux->saved_dst_prog_type is set in check_attach_btf_id(). And there is no
> resolve_prog_type() before check_attach_btf_id() in bpf_check().
>
> Therefore, resolve_prog_type() must be called after check_attach_btf_id().

In check_attach_btf_id(), I see
         if (tgt_prog) {
                 prog->aux->saved_dst_prog_type = tgt_prog->type;
                 prog->aux->saved_dst_attach_type = tgt_prog->expected_attach_type;
         }

So it is possible prog->aux->saved_dst_prog_type is 0 (default value).
I don't know that if tgt_prog is NULL, whether later resolve_prog_type()
will be called or not. Need more checking here.