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

[Martin KaFai Lau <martin.lau@linux.dev>]

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.

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