On 3/17/23 12:23, Martin KaFai Lau wrote:
On 3/15/23 7:36 PM, 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_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);
I don't think it has completely addressed Andrii's comment in v4
regarding BPF_F_REPLACE:
https://lore.kernel.org/bpf/CAEf4BzbK8s+VFG5HefydD7CRLzkRFKg-Er0PKV_-C2-yttfXzA@xxxxxxxxxxxxxx/
I just check with Andrii.
For this, I will add a old_map_fd field in bpf_attr to pass a fd of
the old map. The definition of the type of bpf_link_ops::update_map
will be changed to pass an old_map. So, we can check if it is expected.
For now, tcp_update_congestion_control() enforces the same cc-name.
However, it is still not the same as what BPF_F_REPLACE intented to do:
update only when it is the same old-map. Same cc-name does not
necessarily mean the same old-map.
st_ops->update() will check if old_st_mp->kvalue.data is the same
one registered before. That will enforce that old_st_map is old_map.
+ 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;
nit. init zero is unnecessarily.
+
+ new_map = bpf_map_get(attr->link_update.new_map_fd);
+ if (IS_ERR(new_map))
+ return -EINVAL;
+
+ 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
static int link_update(union bpf_attr *attr)
@@ -4696,6 +4711,11 @@ static int link_update(union bpf_attr *attr)
if (IS_ERR(link))
return PTR_ERR(link);
+ if (link->ops->update_map) {
+ ret = link_update_map(link, attr);
+ goto out_put_link;
+ }
+
new_prog = bpf_prog_get(attr->link_update.new_prog_fd);
if (IS_ERR(new_prog)) {
ret = PTR_ERR(new_prog);
diff --git a/net/bpf/bpf_dummy_struct_ops.c
b/net/bpf/bpf_dummy_struct_ops.c
index ff4f89a2b02a..158f14e240d0 100644
--- 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.
