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

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

On 3/17/23 15:27, Andrii Nakryiko wrote:
> On Wed, Mar 15, 2023 at 7:37 PM Kui-Feng Lee <kuifeng@meta.com> wrote:
>>
>> By improving the BPF_LINK_UPDATE command of bpf(), it should allow you
>> to conveniently switch between different struct_ops on a single
>> bpf_link. This would enable smoother transitions from one struct_ops
>> to another.
>>
>> The struct_ops maps passing along with BPF_LINK_UPDATE should have the
>> BPF_F_LINK flag.
>>
>> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
>> ---
>>   include/linux/bpf.h            |  2 ++
>>   include/uapi/linux/bpf.h       |  8 +++++--
>>   kernel/bpf/bpf_struct_ops.c    | 38 ++++++++++++++++++++++++++++++++++
>>   kernel/bpf/syscall.c           | 20 ++++++++++++++++++
>>   net/bpf/bpf_dummy_struct_ops.c |  6 ++++++
>>   net/ipv4/bpf_tcp_ca.c          |  6 ++++++
>>   tools/include/uapi/linux/bpf.h |  8 +++++--
>>   7 files changed, 84 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 455b14bf8f28..56e6ab7559ef 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1474,6 +1474,7 @@ struct bpf_link_ops {
>>          void (*show_fdinfo)(const struct bpf_link *link, struct seq_file *seq);
>>          int (*fill_link_info)(const struct bpf_link *link,
>>                                struct bpf_link_info *info);
>> +       int (*update_map)(struct bpf_link *link, struct bpf_map *new_map);
>>   };
>>
>>   struct bpf_tramp_link {
>> @@ -1516,6 +1517,7 @@ struct bpf_struct_ops {
>>                             void *kdata, const void *udata);
>>          int (*reg)(void *kdata);
>>          void (*unreg)(void *kdata);
>> +       int (*update)(void *kdata, void *old_kdata);
>>          int (*validate)(void *kdata);
>>          const struct btf_type *type;
>>          const struct btf_type *value_type;
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 42f40ee083bf..24e1dec4ad97 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1555,8 +1555,12 @@ union bpf_attr {
>>
>>          struct { /* struct used by BPF_LINK_UPDATE command */
>>                  __u32           link_fd;        /* link fd */
>> -               /* new program fd to update link with */
>> -               __u32           new_prog_fd;
>> +               union {
>> +                       /* new program fd to update link with */
>> +                       __u32           new_prog_fd;
>> +                       /* new struct_ops map fd to update link with */
>> +                       __u32           new_map_fd;
>> +               };
>>                  __u32           flags;          /* extra flags */
>>                  /* expected link's program fd; is specified only if
>>                   * BPF_F_REPLACE flag is set in flags */
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 8ce6c7581ca3..5a9e10b92423 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -807,10 +807,48 @@ static int bpf_struct_ops_map_link_fill_link_info(const struct bpf_link *link,
>>          return 0;
>>   }
>>
>> +static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map *new_map)
>> +{
>> +       struct bpf_struct_ops_map *st_map, *old_st_map;
>> +       struct bpf_struct_ops_link *st_link;
>> +       struct bpf_map *old_map;
>> +       int err = 0;
>> +
>> +       st_link = container_of(link, struct bpf_struct_ops_link, link);
>> +       st_map = container_of(new_map, struct bpf_struct_ops_map, map);
>> +
>> +       if (!bpf_struct_ops_valid_to_reg(new_map))
>> +               return -EINVAL;
>> +
>> +       mutex_lock(&update_mutex);
>> +
>> +       old_map = rcu_dereference_protected(st_link->map, lockdep_is_held(&update_mutex));
>> +       old_st_map = container_of(old_map, struct bpf_struct_ops_map, map);
>> +       /* The new and old struct_ops must be the same type. */
>> +       if (st_map->st_ops != old_st_map->st_ops) {
>> +               err = -EINVAL;
>> +               goto err_out;
>> +       }
>> +
>> +       err = st_map->st_ops->update(st_map->kvalue.data, old_st_map->kvalue.data);
>> +       if (err)
>> +               goto err_out;
>> +
>> +       bpf_map_inc(new_map);
>> +       rcu_assign_pointer(st_link->map, new_map);
>> +       bpf_map_put(old_map);
>> +
>> +err_out:
>> +       mutex_unlock(&update_mutex);
>> +
>> +       return err;
>> +}
>> +
>>   static const struct bpf_link_ops bpf_struct_ops_map_lops = {
>>          .dealloc = bpf_struct_ops_map_link_dealloc,
>>          .show_fdinfo = bpf_struct_ops_map_link_show_fdinfo,
>>          .fill_link_info = bpf_struct_ops_map_link_fill_link_info,
>> +       .update_map = bpf_struct_ops_map_link_update,
>>   };
>>
>>   int bpf_struct_ops_link_create(union bpf_attr *attr)
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 5a45e3bf34e2..6fa10d108278 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -4676,6 +4676,21 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
>>          return ret;
>>   }
>>
>> +static int link_update_map(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;
> 
> I was expecting a check for the BPF_F_LINK flag here. Isn't it
> necessary to verify that here?


link->ops->update_map != NULL implies BPF_F_LINK.  So, it doesn't do
additional check for BPF_F_LINK here.


> 
> 
> 
>> +
>> +       ret = link->ops->update_map(link, new_map);
>> +
>> +       bpf_map_put(new_map);
>> +       return ret;
>> +}
>> +
>>   #define BPF_LINK_UPDATE_LAST_FIELD link_update.old_prog_fd
>>
> 
> [...]