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

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

On 2/16/23 11:17 AM, Kui-Feng Lee wrote:
>>> +static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct 
>>> bpf_map *new_map)
>>> +{
>>> +    struct bpf_struct_ops_value *kvalue;
>>> +    struct bpf_struct_ops_map *st_map, *old_st_map;
>>> +    struct bpf_map *old_map;
>>> +    int err;
>>> +
>>> +    if (new_map->map_type != BPF_MAP_TYPE_STRUCT_OPS || !(new_map->map_flags 
>>> & BPF_F_LINK))
>>> +        return -EINVAL;
>>> +
>>> +    old_map = link->map;
>>> +
>>> +    /* It does nothing if the new map is the same as the old one.
>>> +     * A struct_ops that backs a bpf_link can not be updated or
>>> +     * its kvalue would be updated and causes inconsistencies.
>>> +     */
>>> +    if (old_map == new_map)
>>> +        return 0;
>>> +
>>> +    /* The new and old struct_ops must be the same type. */
>>> +    st_map = (struct bpf_struct_ops_map *)new_map;
>>> +    old_st_map = (struct bpf_struct_ops_map *)old_map;
>>> +    if (st_map->st_ops != old_st_map->st_ops)
>>> +        return -EINVAL;
>>> +
>>> +    /* Assure the struct_ops is updated (has value) and not
>>> +     * backing any other link.
>>> +     */
>>> +    kvalue = &st_map->kvalue;
>>> +    if (kvalue->state != BPF_STRUCT_OPS_STATE_INUSE ||
>>> +        refcount_read(&kvalue->refcnt) != 0)
>>> +        return -EINVAL;
>>> +
>>> +    bpf_map_inc(new_map);
>>> +    refcount_set(&kvalue->refcnt, 1);
>>> +
>>> +    set_memory_rox((long)st_map->image, 1);
>>> +    err = st_map->st_ops->update(kvalue->data, old_st_map->kvalue.data);
>>> +    if (err) {
>>> +        refcount_set(&kvalue->refcnt, 0);
>>> +
>>> +        set_memory_nx((long)st_map->image, 1);
>>> +        set_memory_rw((long)st_map->image, 1);
>>> +        bpf_map_put(new_map);
>>> +        return err;
>>> +    }
>>> +
>>> +    link->map = new_map;
>>
>> Similar here, does this link_update operation needs a lock?
> 
> The update function of tcp_ca checks if the name is unique with the protection 
> of a lock.  bpf_struct_ops_map_update_elem() also check and update state of the 
> kvalue to prevent changing kvalue.  Only one of thread will success to register 
> or update at any moment.

hmm... meaning the lock inside the "->update()" function?  There are many 
variables outside of update() that this function 
(bpf_struct_ops_map_link_update) is setting and testing without a lock. eg. the 
succeeded thread will set refcnt to 1 while the failed thread will set it back 
to 0...

>>> +
>>> +static int link_update_struct_ops(struct bpf_link *link, union bpf_attr *attr)
>>> +{
>>> +    struct bpf_map *new_map;
>>> +    int ret = 0;
>>> +
>>> +    new_map = bpf_map_get(attr->link_update.new_map_fd);
>>> +    if (IS_ERR(new_map))
>>> +        return -EINVAL;
>>> +
>>> +    if (new_map->map_type != BPF_MAP_TYPE_STRUCT_OPS) {
>>> +        ret = -EINVAL;
>>> +        goto out_put_map;
>>> +    }
>>
>> How about BPF_F_REPLACE?
> 
> Do you mean the new_map should be labeled with BPF_F_REPLACE to replace the old 
> one?

was asking if BPF_F_REPLACE is supported.