Re: [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 12/16/23 08:48, Martin KaFai Lau wrote:
> On 12/15/23 9:43 PM, Kui-Feng Lee wrote:
>>
>>
>> On 12/15/23 17:19, Martin KaFai Lau wrote:
>>> On 12/15/23 1:42 PM, Kui-Feng Lee wrote:
>>>>
>>>>
>>>> On 12/14/23 18:22, Martin KaFai Lau wrote:
>>>>> On 12/8/23 4:26 PM, thinker.li@gmail.com wrote:
>>>>>> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf 
>>>>>> *btf, u32 *ret_cnt)
>>>>>> +{
>>>>>> +    if (!btf)
>>>>>> +        return NULL;
>>>>>> +    if (!btf->struct_ops_tab)
>>>>>
>>>>>          *ret_cnt = 0;
>>>>>
>>>>> unless the later patch checks the return value NULL before using 
>>>>> *ret_cnt.
>>>>> Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.
>>>>>
>>>>> The same should go for the "!btf" case above but I suspect the 
>>>>> above !btf check is unnecessary also and the caller should have 
>>>>> checked for !btf itself instead of expecting a list of struct_ops 
>>>>> from a NULL btf. Lets continue the review on the later patches for 
>>>>> now to confirm where the above !btf case might happen.
>>>>
>>>> Checking callers, I didn't find anything that make btf here NULL so 
>>>> far.
>>>
>>>> It is safe to remove !btf check. For the same reason as assigning
>>>> *ret_cnt for safe, this check should be fine here as well, right?
>>>
>>> If for safety, why ref_cnt is not checked for NULL also? The 
>>> userspace passed-in btf should have been checked for NULL long time 
>>> before reaching here. There is no need to be over protective here. It 
>>> would really need a BUG_ON instead if btf was NULL here (don't add a 
>>> BUG_ON though).
>>>
>>> afaict, no function in btf.c is checking the btf argument for NULL also.
>>>
>>>>
>>>> I don't have strong opinion here. What I though is to keep the values
>>>> as it is without any side-effect if the function call fails and if
>>>> possible. And, the callers should not expect the callee to set some
>>>> specific values when a call fails.
>>>
>>> For *ref_cnt stay uninit, there is a bug in patch 10 which exactly 
>>> assumes 0 is set in *ret_cnt when there is no struct_ops. It is a 
>>> good signal on how this function will be used.
>>>
>>> I think it is arguable whether returning NULL here is failure. I 
>>> would argue getting a 0 struct_ops_desc array is not a failure. It is 
>>> why the !btf case confuses the return NULL case to mean a never would 
>>> happen error instead of meaning there is no struct_ops. Taking out 
>>> the !btf case, NULL means there is no struct_ops (instead of 
>>> failure), so 0 cnt.
>>>
>>> Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the 
>>> local cnt 0 and write a warning comment here in btf_get_struct_ops() 
>>> saying ret_cnt won't be set when there is no struct_ops in the btf.
>>
>>
>> I will fix at the patch 10 by setting local cnt 0
>>
>>>
>>> When looking at it again, how about moving the 
>>> bpf_struct_ops_find_*() to btf.c. Then it will avoid the need of the 
>>> new btf_get_struct_ops() function. bpf_struct_ops_find_*() can 
>>> directly use the btf->struct_ops_tab.
>>>
>>
>> I prefer to keep them in bpf_struct_ops.c if it is ok to you.
>> Fixing the initialization issue of bpf_struct_ops_find()
>> should be enough.
> 
> If choosing between fixing the bug in patch 10 and moving them to btf.c, 
> I would avoid patch 10 (and future) bug to begin with by moving them to 
> btf.c. Moving them should be cheap unless there is other dependency that 
> I have overlooked.

Ok! Got it!

> 
>>
>>>
>>>>
>>>>>
>>>>>> +        return NULL;
>>>>>> +
>>>>>> +    *ret_cnt = btf->struct_ops_tab->cnt;
>>>>>> +    return (const struct bpf_struct_ops_desc 
>>>>>> *)btf->struct_ops_tab->ops;
>>>>>> +}
>>>>>
>>>
>