Re: [PATCH bpf-next v7 5/8] bpf: Update the struct_ops of a bpf_link.

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

On 3/17/23 6:11 PM, Kui-Feng Lee wrote:
>>> --- a/net/bpf/bpf_dummy_struct_ops.c
>>> +++ b/net/bpf/bpf_dummy_struct_ops.c
>>> @@ -222,12 +222,18 @@ static void bpf_dummy_unreg(void *kdata)
>>>   {
>>>   }
>>> +static int bpf_dummy_update(void *kdata, void *old_kdata)
>>> +{
>>> +    return -EOPNOTSUPP;
>>> +}
>>> +
>>>   struct bpf_struct_ops bpf_bpf_dummy_ops = {
>>>       .verifier_ops = &bpf_dummy_verifier_ops,
>>>       .init = bpf_dummy_init,
>>>       .check_member = bpf_dummy_ops_check_member,
>>>       .init_member = bpf_dummy_init_member,
>>>       .reg = bpf_dummy_reg,
>>> +    .update = bpf_dummy_update,
>>
>> When looking at this together in patch 5, the changes in 
>> bpf_dummy_struct_ops.c should not be needed.
> 
> I don't follow you.
> If we don't assign a function to .update, it will fail in
> bpf_struct_ops_map_link_update(). Of course, I can add a check
> in bpf_struct_ops_map_link_update() to return an error if .update
> is NULL.

Yes, test ->update in bpf_struct_ops.c is better but not in 
bpf_struct_ops_map_link_update (more on this later). It does not need the dummy 
struct_ops to test the link. The dummy struct_ops was created to catch the 
trampoline img issue with ops/func having different args and return value.

It is better to enforce the BPF_F_LINK struct_ops must support both ->validate 
and ->update at the beginning and it can be revisited later. The current 
'->validate' testing in bpf_struct_ops_map_update_elem() in patch 3 is too late. 
Being able to '->validate' is particularly important for BPF_F_LINK struct_ops. 
Testing '->validate' and '->update' in bpf_struct_ops_init() will be too strict 
for now though when considering other on-going efforts to support struct_ops in 
other subsystems. A better place to test both '->validate' and '->update' should 
be in bpf_struct_ops_map_alloc(). It should return -ENOTSUPP when trying to 
creating a struct_ops BPF_F_LINK map without st_ops->validate and st_ops->update.