* [PATCH bpf-next v5 0/8] Transit between BPF TCP congestion controls.
@ 2023-03-08 0:50 Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 1/8] bpf: Retire the struct_ops map kvalue->refcnt Kui-Feng Lee
` (7 more replies)
0 siblings, 8 replies; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 0:50 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf; +Cc: Kui-Feng Lee
Major changes:
- Create bpf_links in the kernel for BPF struct_ops to register and
unregister it.
- Enables switching between implementations of bpf-tcp-cc under a
name instantly by replacing the backing struct_ops map of a
bpf_link.
Previously, BPF struct_ops didn't go off, as even when the user
program creating it was terminated, none of these ever were pinned.
For instance, the TCP congestion control subsystem indirectly
maintains a reference count on the struct_ops of any registered BPF
implemented algorithm. Thus, the algorithm won't be deactivated until
someone deliberately unregisters it. For compatibility with other BPF
programs, bpf_links have been created to work in coordination with
struct_ops maps. This ensures that the registration and unregistration
of these respective maps is carried out at the start and end of the
bpf_link.
We also faced complications when attempting to replace an existing TCP
congestion control algorithm with a new implementation on the fly. A
struct_ops map was used to register a TCP congestion control algorithm
with a unique name. We had to either register the alternative
implementation with a new name and move over or unregister the current
one before being able to reregistration with the same name. To fix
this problem, we can an option to migrate the registration of the
algorithm from struct_ops maps to bpf_links. By modifying the backing
map of a bpf_link, it suddenly becomes possible to replace an existing
TCP congestion control algorithm with ease.
The major differences from v4:
- Rebase.
- Reorder patches and merge part 4 to part 2 of the v4.
The major differences from v3:
- Remove bpf_struct_ops_map_free_rcu(), and use synchronize_rcu().
- Improve the commit log of the part 1.
- Before transitioning to the READY state, we conduct a value check
to ensure that struct_ops can be successfully utilized and links
created later.
The major differences from v2:
- Simplify states
- Remove TOBEUNREG.
- Rename UNREG to READY.
- Stop using the refcnt of the kvalue of a struct_ops. Explicitly
increase and decrease the refcount of struct_ops.
- Prepare kernel vdata during the load phase of libbpf.
The major differences from v1:
- Added bpf_struct_ops_link to replace the previous union-based
approach.
- Added UNREG and TOBEUNREG to the state of bpf_struct_ops_map.
- bpf_struct_ops_transit_state() maintains state transitions.
- Fixed synchronization issue.
- Prepare kernel vdata of struct_ops during the loading phase of
bpf_object.
- Merged previous patch 3 to patch 1.
v4: https://lore.kernel.org/all/20230307232913.576893-1-andrii@kernel.org/
v3: https://lore.kernel.org/all/20230303012122.852654-1-kuifeng@meta.com/
v2: https://lore.kernel.org/bpf/20230223011238.12313-1-kuifeng@meta.com/
v1: https://lore.kernel.org/bpf/20230214221718.503964-1-kuifeng@meta.com/
Kui-Feng Lee (8):
bpf: Retire the struct_ops map kvalue->refcnt.
net: Update an existing TCP congestion control algorithm.
bpf: Create links for BPF struct_ops maps.
libbpf: Create a bpf_link in bpf_map__attach_struct_ops().
bpf: Update the struct_ops of a bpf_link.
libbpf: Update a bpf_link with another struct_ops.
libbpf: Use .struct_ops.link section to indicate a struct_ops with a
link.
selftests/bpf: Test switching TCP Congestion Control algorithms.
include/linux/bpf.h | 16 ++
include/net/tcp.h | 3 +
include/uapi/linux/bpf.h | 20 +-
kernel/bpf/bpf_struct_ops.c | 218 +++++++++++++++---
kernel/bpf/syscall.c | 68 ++++--
net/bpf/bpf_dummy_struct_ops.c | 6 +
net/ipv4/bpf_tcp_ca.c | 14 +-
net/ipv4/tcp_cong.c | 57 ++++-
tools/include/uapi/linux/bpf.h | 19 +-
tools/lib/bpf/libbpf.c | 184 ++++++++++++---
tools/lib/bpf/libbpf.h | 1 +
tools/lib/bpf/libbpf.map | 1 +
.../selftests/bpf/prog_tests/bpf_tcp_ca.c | 38 +++
.../selftests/bpf/progs/tcp_ca_update.c | 62 +++++
14 files changed, 615 insertions(+), 92 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/tcp_ca_update.c
--
2.34.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH bpf-next v5 1/8] bpf: Retire the struct_ops map kvalue->refcnt.
2023-03-08 0:50 [PATCH bpf-next v5 0/8] Transit between BPF TCP congestion controls Kui-Feng Lee
@ 2023-03-08 0:50 ` Kui-Feng Lee
2023-03-08 18:30 ` Martin KaFai Lau
2023-03-08 0:50 ` [PATCH bpf-next v5 2/8] net: Update an existing TCP congestion control algorithm Kui-Feng Lee
` (6 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 0:50 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf; +Cc: Kui-Feng Lee
We have replaced kvalue-refcnt with synchronize_rcu() to wait for an
RCU grace period.
Maintenance of kvalue->refcnt was a complicated task, as we had to
simultaneously keep track of two reference counts: one for the
reference count of bpf_map. When the kvalue->refcnt reaches zero, we
also have to reduce the reference count on bpf_map - yet these steps
are not performed in an atomic manner and require us to be vigilant
when managing them. By eliminating kvalue->refcnt, we can make our
maintenance more straightforward as the refcount of bpf_map is now
solely managed!
To prevent the trampoline image of a struct_ops from being released
while it is still in use, we wait for an RCU grace period. The
setsockopt(TCP_CONGESTION, "...") command allows you to change your
socket's congestion control algorithm and can result in releasing the
old struct_ops implementation. Moreover, since this function is
exposed through bpf_setsockopt(), it may be accessed by BPF programs
as well. To ensure that the trampoline image belonging to struct_op
can be safely called while its method is in use, struct_ops is
safeguarded with rcu_read_lock(). Doing so prevents any destruction of
the associated images before returning from a trampoline and requires
us to wait for an RCU grace period.
Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
---
include/linux/bpf.h | 2 ++
kernel/bpf/bpf_struct_ops.c | 60 ++++++++++++++++++-------------------
kernel/bpf/syscall.c | 2 +-
3 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 6792a7940e1e..50cfc2388cbc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -78,6 +78,7 @@ struct bpf_map_ops {
struct bpf_map *(*map_alloc)(union bpf_attr *attr);
void (*map_release)(struct bpf_map *map, struct file *map_file);
void (*map_free)(struct bpf_map *map);
+ void (*map_free_rcu)(struct bpf_map *map);
int (*map_get_next_key)(struct bpf_map *map, void *key, void *next_key);
void (*map_release_uref)(struct bpf_map *map);
void *(*map_lookup_elem_sys_only)(struct bpf_map *map, void *key);
@@ -1934,6 +1935,7 @@ struct bpf_map *bpf_map_get_with_uref(u32 ufd);
struct bpf_map *__bpf_map_get(struct fd f);
void bpf_map_inc(struct bpf_map *map);
void bpf_map_inc_with_uref(struct bpf_map *map);
+struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref);
struct bpf_map * __must_check bpf_map_inc_not_zero(struct bpf_map *map);
void bpf_map_put_with_uref(struct bpf_map *map);
void bpf_map_put(struct bpf_map *map);
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 38903fb52f98..9e097fcc9cf4 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -58,6 +58,8 @@ struct bpf_struct_ops_map {
struct bpf_struct_ops_value kvalue;
};
+static DEFINE_MUTEX(update_mutex);
+
#define VALUE_PREFIX "bpf_struct_ops_"
#define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
@@ -249,6 +251,7 @@ int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
struct bpf_struct_ops_value *uvalue, *kvalue;
enum bpf_struct_ops_state state;
+ s64 refcnt;
if (unlikely(*(u32 *)key != 0))
return -ENOENT;
@@ -267,7 +270,9 @@ int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
uvalue = value;
memcpy(uvalue, st_map->uvalue, map->value_size);
uvalue->state = state;
- refcount_set(&uvalue->refcnt, refcount_read(&kvalue->refcnt));
+
+ refcnt = atomic64_read(&map->refcnt) - atomic64_read(&map->usercnt);
+ refcount_set(&uvalue->refcnt, max_t(s64, refcnt, 0));
return 0;
}
@@ -491,7 +496,6 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
*(unsigned long *)(udata + moff) = prog->aux->id;
}
- refcount_set(&kvalue->refcnt, 1);
bpf_map_inc(map);
set_memory_rox((long)st_map->image, 1);
@@ -536,8 +540,7 @@ static int bpf_struct_ops_map_delete_elem(struct bpf_map *map, void *key)
switch (prev_state) {
case BPF_STRUCT_OPS_STATE_INUSE:
st_map->st_ops->unreg(&st_map->kvalue.data);
- if (refcount_dec_and_test(&st_map->kvalue.refcnt))
- bpf_map_put(map);
+ bpf_map_put(map);
return 0;
case BPF_STRUCT_OPS_STATE_TOBEFREE:
return -EINPROGRESS;
@@ -574,6 +577,19 @@ static void bpf_struct_ops_map_free(struct bpf_map *map)
{
struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
+ /* The struct_ops's function may switch to another struct_ops.
+ *
+ * For example, bpf_tcp_cc_x->init() may switch to
+ * another tcp_cc_y by calling
+ * setsockopt(TCP_CONGESTION, "tcp_cc_y").
+ * During the switch, bpf_struct_ops_put(tcp_cc_x) is called
+ * and its refcount may reach 0 which then free its
+ * trampoline image while tcp_cc_x is still running.
+ *
+ * Thus, a rcu grace period is needed here.
+ */
+ synchronize_rcu();
+
if (st_map->links)
bpf_struct_ops_map_put_progs(st_map);
bpf_map_area_free(st_map->links);
@@ -676,41 +692,23 @@ const struct bpf_map_ops bpf_struct_ops_map_ops = {
bool bpf_struct_ops_get(const void *kdata)
{
struct bpf_struct_ops_value *kvalue;
+ struct bpf_struct_ops_map *st_map;
+ struct bpf_map *map;
kvalue = container_of(kdata, struct bpf_struct_ops_value, data);
+ st_map = container_of(kvalue, struct bpf_struct_ops_map, kvalue);
- return refcount_inc_not_zero(&kvalue->refcnt);
-}
-
-static void bpf_struct_ops_put_rcu(struct rcu_head *head)
-{
- struct bpf_struct_ops_map *st_map;
-
- st_map = container_of(head, struct bpf_struct_ops_map, rcu);
- bpf_map_put(&st_map->map);
+ map = __bpf_map_inc_not_zero(&st_map->map, false);
+ return !IS_ERR(map);
}
void bpf_struct_ops_put(const void *kdata)
{
struct bpf_struct_ops_value *kvalue;
+ struct bpf_struct_ops_map *st_map;
kvalue = container_of(kdata, struct bpf_struct_ops_value, data);
- if (refcount_dec_and_test(&kvalue->refcnt)) {
- struct bpf_struct_ops_map *st_map;
-
- st_map = container_of(kvalue, struct bpf_struct_ops_map,
- kvalue);
- /* The struct_ops's function may switch to another struct_ops.
- *
- * For example, bpf_tcp_cc_x->init() may switch to
- * another tcp_cc_y by calling
- * setsockopt(TCP_CONGESTION, "tcp_cc_y").
- * During the switch, bpf_struct_ops_put(tcp_cc_x) is called
- * and its map->refcnt may reach 0 which then free its
- * trampoline image while tcp_cc_x is still running.
- *
- * Thus, a rcu grace period is needed here.
- */
- call_rcu(&st_map->rcu, bpf_struct_ops_put_rcu);
- }
+ st_map = container_of(kvalue, struct bpf_struct_ops_map, kvalue);
+
+ bpf_map_put(&st_map->map);
}
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index f406dfa13792..03273cddd6bd 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1288,7 +1288,7 @@ struct bpf_map *bpf_map_get_with_uref(u32 ufd)
}
/* map_idr_lock should have been held */
-static struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref)
+struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref)
{
int refold;
--
2.34.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next v5 2/8] net: Update an existing TCP congestion control algorithm.
2023-03-08 0:50 [PATCH bpf-next v5 0/8] Transit between BPF TCP congestion controls Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 1/8] bpf: Retire the struct_ops map kvalue->refcnt Kui-Feng Lee
@ 2023-03-08 0:50 ` Kui-Feng Lee
2023-03-08 18:59 ` Martin KaFai Lau
2023-03-08 0:50 ` [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps Kui-Feng Lee
` (5 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 0:50 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf; +Cc: Kui-Feng Lee
This feature lets you immediately transition to another congestion
control algorithm or implementation with the same name. Once a name
is updated, new connections will apply this new algorithm.
The 'validate' function pointer has been added to bpf_struct_ops as
well, allowing us to validate a struct_ops without having to go
through the registration process.
Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
---
include/linux/bpf.h | 2 ++
include/net/tcp.h | 3 ++
net/bpf/bpf_dummy_struct_ops.c | 6 ++++
net/ipv4/bpf_tcp_ca.c | 14 +++++++--
net/ipv4/tcp_cong.c | 57 +++++++++++++++++++++++++++++-----
5 files changed, 73 insertions(+), 9 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 50cfc2388cbc..00b6e1a2edaf 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1512,6 +1512,8 @@ 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;
const char *name;
diff --git a/include/net/tcp.h b/include/net/tcp.h
index db9f828e9d1e..2abb755e6a3a 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1117,6 +1117,9 @@ struct tcp_congestion_ops {
int tcp_register_congestion_control(struct tcp_congestion_ops *type);
void tcp_unregister_congestion_control(struct tcp_congestion_ops *type);
+int tcp_update_congestion_control(struct tcp_congestion_ops *type,
+ struct tcp_congestion_ops *old_type);
+int tcp_validate_congestion_control(struct tcp_congestion_ops *ca);
void tcp_assign_congestion_control(struct sock *sk);
void tcp_init_congestion_control(struct sock *sk);
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,
.unreg = bpf_dummy_unreg,
.name = "bpf_dummy_ops",
};
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index 13fc0c185cd9..e8b27826283e 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -239,8 +239,6 @@ static int bpf_tcp_ca_init_member(const struct btf_type *t,
if (bpf_obj_name_cpy(tcp_ca->name, utcp_ca->name,
sizeof(tcp_ca->name)) <= 0)
return -EINVAL;
- if (tcp_ca_find(utcp_ca->name))
- return -EEXIST;
return 1;
}
@@ -266,13 +264,25 @@ static void bpf_tcp_ca_unreg(void *kdata)
tcp_unregister_congestion_control(kdata);
}
+static int bpf_tcp_ca_update(void *kdata, void *old_kdata)
+{
+ return tcp_update_congestion_control(kdata, old_kdata);
+}
+
+static int bpf_tcp_ca_validate(void *kdata)
+{
+ return tcp_validate_congestion_control(kdata);
+}
+
struct bpf_struct_ops bpf_tcp_congestion_ops = {
.verifier_ops = &bpf_tcp_ca_verifier_ops,
.reg = bpf_tcp_ca_reg,
.unreg = bpf_tcp_ca_unreg,
+ .update = bpf_tcp_ca_update,
.check_member = bpf_tcp_ca_check_member,
.init_member = bpf_tcp_ca_init_member,
.init = bpf_tcp_ca_init,
+ .validate = bpf_tcp_ca_validate,
.name = "tcp_congestion_ops",
};
diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index db8b4b488c31..24829390e495 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -75,14 +75,8 @@ struct tcp_congestion_ops *tcp_ca_find_key(u32 key)
return NULL;
}
-/*
- * Attach new congestion control algorithm to the list
- * of available options.
- */
-int tcp_register_congestion_control(struct tcp_congestion_ops *ca)
+int tcp_validate_congestion_control(struct tcp_congestion_ops *ca)
{
- int ret = 0;
-
/* all algorithms must implement these */
if (!ca->ssthresh || !ca->undo_cwnd ||
!(ca->cong_avoid || ca->cong_control)) {
@@ -90,6 +84,20 @@ int tcp_register_congestion_control(struct tcp_congestion_ops *ca)
return -EINVAL;
}
+ return 0;
+}
+
+/* Attach new congestion control algorithm to the list
+ * of available options.
+ */
+int tcp_register_congestion_control(struct tcp_congestion_ops *ca)
+{
+ int ret;
+
+ ret = tcp_validate_congestion_control(ca);
+ if (ret)
+ return ret;
+
ca->key = jhash(ca->name, sizeof(ca->name), strlen(ca->name));
spin_lock(&tcp_cong_list_lock);
@@ -130,6 +138,41 @@ void tcp_unregister_congestion_control(struct tcp_congestion_ops *ca)
}
EXPORT_SYMBOL_GPL(tcp_unregister_congestion_control);
+/* Replace a registered old ca with a new one.
+ *
+ * The new ca must have the same name as the old one, that has been
+ * registered.
+ */
+int tcp_update_congestion_control(struct tcp_congestion_ops *ca, struct tcp_congestion_ops *old_ca)
+{
+ struct tcp_congestion_ops *existing;
+ int ret;
+
+ ret = tcp_validate_congestion_control(ca);
+ if (ret)
+ return ret;
+
+ ca->key = jhash(ca->name, sizeof(ca->name), strlen(ca->name));
+
+ spin_lock(&tcp_cong_list_lock);
+ existing = tcp_ca_find_key(old_ca->key);
+ if (ca->key == TCP_CA_UNSPEC || !existing || strcmp(existing->name, ca->name)) {
+ pr_notice("%s not registered or non-unique key\n",
+ ca->name);
+ ret = -EINVAL;
+ } else if (existing != old_ca) {
+ pr_notice("invalid old congestion control algorithm to replace\n");
+ ret = -EINVAL;
+ } else {
+ list_add_tail_rcu(&ca->list, &tcp_cong_list);
+ list_del_rcu(&existing->list);
+ pr_debug("%s updated\n", ca->name);
+ }
+ spin_unlock(&tcp_cong_list_lock);
+
+ return ret;
+}
+
u32 tcp_ca_get_key_by_name(struct net *net, const char *name, bool *ecn_ca)
{
const struct tcp_congestion_ops *ca;
--
2.34.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps.
2023-03-08 0:50 [PATCH bpf-next v5 0/8] Transit between BPF TCP congestion controls Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 1/8] bpf: Retire the struct_ops map kvalue->refcnt Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 2/8] net: Update an existing TCP congestion control algorithm Kui-Feng Lee
@ 2023-03-08 0:50 ` Kui-Feng Lee
2023-03-08 15:01 ` kernel test robot
` (3 more replies)
2023-03-08 0:50 ` [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops() Kui-Feng Lee
` (4 subsequent siblings)
7 siblings, 4 replies; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 0:50 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf; +Cc: Kui-Feng Lee
BPF struct_ops maps are employed directly to register TCP Congestion
Control algorithms. Unlike other BPF programs that terminate when
their links gone. The link of a BPF struct_ops map provides a uniform
experience akin to other types of BPF programs.
bpf_links are responsible for registering their associated
struct_ops. You can only use a struct_ops that has the BPF_F_LINK flag
set to create a bpf_link, while a structs without this flag behaves in
the same manner as before and is registered upon updating its value.
The BPF_LINK_TYPE_STRUCT_OPS serves a dual purpose. Not only is it
used to craft the links for BPF struct_ops programs, but also to
create links for BPF struct_ops them-self. Since the links of BPF
struct_ops programs are only used to create trampolines internally,
they are never seen in other contexts. Thus, they can be reused for
struct_ops themself.
To maintain a reference to the map supporting this link, we add
bpf_struct_ops_link as an additional type. The pointer of the map is
RCU and won't be necessary until later in the patchset.
Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
---
include/linux/bpf.h | 11 +++
include/uapi/linux/bpf.h | 12 +++-
kernel/bpf/bpf_struct_ops.c | 124 +++++++++++++++++++++++++++++++--
kernel/bpf/syscall.c | 23 +++---
tools/include/uapi/linux/bpf.h | 12 +++-
5 files changed, 168 insertions(+), 14 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 00b6e1a2edaf..afca6c526fe4 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1548,6 +1548,7 @@ static inline void bpf_module_put(const void *data, struct module *owner)
else
module_put(owner);
}
+int bpf_struct_ops_link_create(union bpf_attr *attr);
#ifdef CONFIG_NET
/* Define it here to avoid the use of forward declaration */
@@ -1588,6 +1589,11 @@ static inline int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map,
{
return -EINVAL;
}
+static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
+{
+ return -EOPNOTSUPP;
+}
+
#endif
#if defined(CONFIG_CGROUP_BPF) && defined(CONFIG_BPF_LSM)
@@ -2379,6 +2385,11 @@ static inline void bpf_link_put(struct bpf_link *link)
{
}
+static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
+{
+ return -EOPNOTSUPP;
+}
+
static inline int bpf_obj_get_user(const char __user *pathname, int flags)
{
return -EOPNOTSUPP;
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 976b194eb775..f9fc7b8af3c4 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1033,6 +1033,7 @@ enum bpf_attach_type {
BPF_PERF_EVENT,
BPF_TRACE_KPROBE_MULTI,
BPF_LSM_CGROUP,
+ BPF_STRUCT_OPS,
__MAX_BPF_ATTACH_TYPE
};
@@ -1266,6 +1267,9 @@ enum {
/* Create a map that is suitable to be an inner map with dynamic max entries */
BPF_F_INNER_MAP = (1U << 12),
+
+/* Create a map that will be registered/unregesitered by the backed bpf_link */
+ BPF_F_LINK = (1U << 13),
};
/* Flags for BPF_PROG_QUERY. */
@@ -1507,7 +1511,10 @@ union bpf_attr {
} task_fd_query;
struct { /* struct used by BPF_LINK_CREATE command */
- __u32 prog_fd; /* eBPF program to attach */
+ union {
+ __u32 prog_fd; /* eBPF program to attach */
+ __u32 map_fd; /* struct_ops to attach */
+ };
union {
__u32 target_fd; /* object to attach to */
__u32 target_ifindex; /* target ifindex */
@@ -6379,6 +6386,9 @@ struct bpf_link_info {
struct {
__u32 ifindex;
} xdp;
+ struct {
+ __u32 map_id;
+ } struct_ops;
};
} __attribute__((aligned(8)));
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 9e097fcc9cf4..5a7e86cf67b5 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -16,6 +16,7 @@ enum bpf_struct_ops_state {
BPF_STRUCT_OPS_STATE_INIT,
BPF_STRUCT_OPS_STATE_INUSE,
BPF_STRUCT_OPS_STATE_TOBEFREE,
+ BPF_STRUCT_OPS_STATE_READY,
};
#define BPF_STRUCT_OPS_COMMON_VALUE \
@@ -58,6 +59,11 @@ struct bpf_struct_ops_map {
struct bpf_struct_ops_value kvalue;
};
+struct bpf_struct_ops_link {
+ struct bpf_link link;
+ struct bpf_map __rcu *map;
+};
+
static DEFINE_MUTEX(update_mutex);
#define VALUE_PREFIX "bpf_struct_ops_"
@@ -496,11 +502,24 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
*(unsigned long *)(udata + moff) = prog->aux->id;
}
- bpf_map_inc(map);
-
set_memory_rox((long)st_map->image, 1);
+ if (st_map->map.map_flags & BPF_F_LINK) {
+ if (st_ops->validate) {
+ err = st_ops->validate(kdata);
+ if (err)
+ goto unlock;
+ }
+ /* Let bpf_link handle registration & unregistration.
+ *
+ * Pair with smp_load_acquire() during lookup_elem().
+ */
+ smp_store_release(&kvalue->state, BPF_STRUCT_OPS_STATE_READY);
+ goto unlock;
+ }
+
err = st_ops->reg(kdata);
if (likely(!err)) {
+ bpf_map_inc(map);
/* Pair with smp_load_acquire() during lookup_elem().
* It ensures the above udata updates (e.g. prog->aux->id)
* can be seen once BPF_STRUCT_OPS_STATE_INUSE is set.
@@ -516,7 +535,6 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
*/
set_memory_nx((long)st_map->image, 1);
set_memory_rw((long)st_map->image, 1);
- bpf_map_put(map);
reset_unlock:
bpf_struct_ops_map_put_progs(st_map);
@@ -534,6 +552,9 @@ static int bpf_struct_ops_map_delete_elem(struct bpf_map *map, void *key)
struct bpf_struct_ops_map *st_map;
st_map = (struct bpf_struct_ops_map *)map;
+ if (st_map->map.map_flags & BPF_F_LINK)
+ return -EOPNOTSUPP;
+
prev_state = cmpxchg(&st_map->kvalue.state,
BPF_STRUCT_OPS_STATE_INUSE,
BPF_STRUCT_OPS_STATE_TOBEFREE);
@@ -601,7 +622,7 @@ static void bpf_struct_ops_map_free(struct bpf_map *map)
static int bpf_struct_ops_map_alloc_check(union bpf_attr *attr)
{
if (attr->key_size != sizeof(unsigned int) || attr->max_entries != 1 ||
- attr->map_flags || !attr->btf_vmlinux_value_type_id)
+ (attr->map_flags & ~BPF_F_LINK) || !attr->btf_vmlinux_value_type_id)
return -EINVAL;
return 0;
}
@@ -712,3 +733,98 @@ void bpf_struct_ops_put(const void *kdata)
bpf_map_put(&st_map->map);
}
+
+static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
+{
+ struct bpf_struct_ops_link *st_link;
+ struct bpf_struct_ops_map *st_map;
+
+ st_link = container_of(link, struct bpf_struct_ops_link, link);
+ st_map = (struct bpf_struct_ops_map *)st_link->map;
+ st_map->st_ops->unreg(&st_map->kvalue.data);
+ bpf_map_put(st_link->map);
+ kfree(st_link);
+}
+
+static void bpf_struct_ops_map_link_show_fdinfo(const struct bpf_link *link,
+ struct seq_file *seq)
+{
+ struct bpf_struct_ops_link *st_link;
+ struct bpf_map *map;
+
+ st_link = container_of(link, struct bpf_struct_ops_link, link);
+ rcu_read_lock();
+ map = rcu_dereference(st_link->map);
+ if (map)
+ seq_printf(seq, "map_id:\t%d\n", map->id);
+ rcu_read_unlock();
+}
+
+static int bpf_struct_ops_map_link_fill_link_info(const struct bpf_link *link,
+ struct bpf_link_info *info)
+{
+ struct bpf_struct_ops_link *st_link;
+ struct bpf_map *map;
+
+ st_link = container_of(link, struct bpf_struct_ops_link, link);
+ rcu_read_lock();
+ map = rcu_dereference(st_link->map);
+ if (map)
+ info->struct_ops.map_id = map->id;
+ rcu_read_unlock();
+ return 0;
+}
+
+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,
+};
+
+int bpf_struct_ops_link_create(union bpf_attr *attr)
+{
+ struct bpf_struct_ops_link *link = NULL;
+ struct bpf_link_primer link_primer;
+ struct bpf_struct_ops_map *st_map;
+ struct bpf_map *map;
+ int err;
+
+ map = bpf_map_get(attr->link_create.map_fd);
+ if (!map)
+ return -EINVAL;
+
+ st_map = (struct bpf_struct_ops_map *)map;
+
+ if (map->map_type != BPF_MAP_TYPE_STRUCT_OPS || !(map->map_flags & BPF_F_LINK) ||
+ /* Pair with smp_store_release() during map_update */
+ smp_load_acquire(&st_map->kvalue.state) != BPF_STRUCT_OPS_STATE_READY) {
+ err = -EINVAL;
+ goto err_out;
+ }
+
+ link = kzalloc(sizeof(*link), GFP_USER);
+ if (!link) {
+ err = -ENOMEM;
+ goto err_out;
+ }
+ bpf_link_init(&link->link, BPF_LINK_TYPE_STRUCT_OPS, &bpf_struct_ops_map_lops, NULL);
+ RCU_INIT_POINTER(link->map, map);
+
+ err = bpf_link_prime(&link->link, &link_primer);
+ if (err)
+ goto err_out;
+
+ err = st_map->st_ops->reg(st_map->kvalue.data);
+ if (err) {
+ bpf_link_cleanup(&link_primer);
+ goto err_out;
+ }
+
+ return bpf_link_settle(&link_primer);
+
+err_out:
+ bpf_map_put(map);
+ kfree(link);
+ return err;
+}
+
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 03273cddd6bd..3a4503987a48 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2806,16 +2806,19 @@ static void bpf_link_show_fdinfo(struct seq_file *m, struct file *filp)
const struct bpf_prog *prog = link->prog;
char prog_tag[sizeof(prog->tag) * 2 + 1] = { };
- bin2hex(prog_tag, prog->tag, sizeof(prog->tag));
seq_printf(m,
"link_type:\t%s\n"
- "link_id:\t%u\n"
- "prog_tag:\t%s\n"
- "prog_id:\t%u\n",
+ "link_id:\t%u\n",
bpf_link_type_strs[link->type],
- link->id,
- prog_tag,
- prog->aux->id);
+ link->id);
+ if (prog) {
+ bin2hex(prog_tag, prog->tag, sizeof(prog->tag));
+ seq_printf(m,
+ "prog_tag:\t%s\n"
+ "prog_id:\t%u\n",
+ prog_tag,
+ prog->aux->id);
+ }
if (link->ops->show_fdinfo)
link->ops->show_fdinfo(link, m);
}
@@ -4290,7 +4293,8 @@ static int bpf_link_get_info_by_fd(struct file *file,
info.type = link->type;
info.id = link->id;
- info.prog_id = link->prog->aux->id;
+ if (link->prog)
+ info.prog_id = link->prog->aux->id;
if (link->ops->fill_link_info) {
err = link->ops->fill_link_info(link, &info);
@@ -4553,6 +4557,9 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
if (CHECK_ATTR(BPF_LINK_CREATE))
return -EINVAL;
+ if (attr->link_create.attach_type == BPF_STRUCT_OPS)
+ return bpf_struct_ops_link_create(attr);
+
prog = bpf_prog_get(attr->link_create.prog_fd);
if (IS_ERR(prog))
return PTR_ERR(prog);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 976b194eb775..051b85525302 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1033,6 +1033,7 @@ enum bpf_attach_type {
BPF_PERF_EVENT,
BPF_TRACE_KPROBE_MULTI,
BPF_LSM_CGROUP,
+ BPF_STRUCT_OPS,
__MAX_BPF_ATTACH_TYPE
};
@@ -1266,6 +1267,9 @@ enum {
/* Create a map that is suitable to be an inner map with dynamic max entries */
BPF_F_INNER_MAP = (1U << 12),
+
+/* Create a map that will be registered/unregesitered by the backed bpf_link */
+ BPF_F_LINK = (1U << 13),
};
/* Flags for BPF_PROG_QUERY. */
@@ -1507,7 +1511,10 @@ union bpf_attr {
} task_fd_query;
struct { /* struct used by BPF_LINK_CREATE command */
- __u32 prog_fd; /* eBPF program to attach */
+ union {
+ __u32 prog_fd; /* eBPF program to attach */
+ __u32 map_fd; /* eBPF struct_ops to attach */
+ };
union {
__u32 target_fd; /* object to attach to */
__u32 target_ifindex; /* target ifindex */
@@ -6379,6 +6386,9 @@ struct bpf_link_info {
struct {
__u32 ifindex;
} xdp;
+ struct {
+ __u32 map_id;
+ } struct_ops;
};
} __attribute__((aligned(8)));
--
2.34.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops().
2023-03-08 0:50 [PATCH bpf-next v5 0/8] Transit between BPF TCP congestion controls Kui-Feng Lee
` (2 preceding siblings ...)
2023-03-08 0:50 ` [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps Kui-Feng Lee
@ 2023-03-08 0:50 ` Kui-Feng Lee
2023-03-08 21:42 ` Martin KaFai Lau
2023-03-08 0:50 ` [PATCH bpf-next v5 5/8] bpf: Update the struct_ops of a bpf_link Kui-Feng Lee
` (3 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 0:50 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf; +Cc: Kui-Feng Lee
bpf_map__attach_struct_ops() was creating a dummy bpf_link as a
placeholder, but now it is constructing an authentic one by calling
bpf_link_create() if the map has the BPF_F_LINK flag.
You can flag a struct_ops map with BPF_F_LINK by calling
bpf_map__set_map_flags().
Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
---
tools/lib/bpf/libbpf.c | 84 +++++++++++++++++++++++++++++++-----------
1 file changed, 62 insertions(+), 22 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index a557718401e4..f70b55c0f40e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -116,6 +116,7 @@ static const char * const attach_type_name[] = {
[BPF_SK_REUSEPORT_SELECT_OR_MIGRATE] = "sk_reuseport_select_or_migrate",
[BPF_PERF_EVENT] = "perf_event",
[BPF_TRACE_KPROBE_MULTI] = "trace_kprobe_multi",
+ [BPF_STRUCT_OPS] = "struct_ops",
};
static const char * const link_type_name[] = {
@@ -7677,6 +7678,26 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
return 0;
}
+static void bpf_map_prepare_vdata(const struct bpf_map *map)
+{
+ struct bpf_struct_ops *st_ops;
+ __u32 i;
+
+ st_ops = map->st_ops;
+ for (i = 0; i < btf_vlen(st_ops->type); i++) {
+ struct bpf_program *prog = st_ops->progs[i];
+ void *kern_data;
+ int prog_fd;
+
+ if (!prog)
+ continue;
+
+ prog_fd = bpf_program__fd(prog);
+ kern_data = st_ops->kern_vdata + st_ops->kern_func_off[i];
+ *(unsigned long *)kern_data = prog_fd;
+ }
+}
+
static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const char *target_btf_path)
{
int err, i;
@@ -7728,6 +7749,10 @@ static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const ch
btf__free(obj->btf_vmlinux);
obj->btf_vmlinux = NULL;
+ for (i = 0; i < obj->nr_maps; i++)
+ if (bpf_map__is_struct_ops(&obj->maps[i]))
+ bpf_map_prepare_vdata(&obj->maps[i]);
+
obj->loaded = true; /* doesn't matter if successfully or not */
if (err)
@@ -11566,22 +11591,34 @@ struct bpf_link *bpf_program__attach(const struct bpf_program *prog)
return link;
}
+struct bpf_link_struct_ops {
+ struct bpf_link link;
+ int map_fd;
+};
+
static int bpf_link__detach_struct_ops(struct bpf_link *link)
{
+ struct bpf_link_struct_ops *st_link;
__u32 zero = 0;
- if (bpf_map_delete_elem(link->fd, &zero))
- return -errno;
+ st_link = container_of(link, struct bpf_link_struct_ops, link);
- return 0;
+ if (st_link->map_fd < 0) {
+ /* Fake bpf_link */
+ if (bpf_map_delete_elem(link->fd, &zero))
+ return -errno;
+ return 0;
+ }
+
+ /* Doesn't support detaching. */
+ return -EOPNOTSUPP;
}
struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
{
- struct bpf_struct_ops *st_ops;
- struct bpf_link *link;
- __u32 i, zero = 0;
- int err;
+ struct bpf_link_struct_ops *link;
+ __u32 zero = 0;
+ int err, fd;
if (!bpf_map__is_struct_ops(map) || map->fd == -1)
return libbpf_err_ptr(-EINVAL);
@@ -11590,31 +11627,34 @@ struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
if (!link)
return libbpf_err_ptr(-EINVAL);
- st_ops = map->st_ops;
- for (i = 0; i < btf_vlen(st_ops->type); i++) {
- struct bpf_program *prog = st_ops->progs[i];
- void *kern_data;
- int prog_fd;
+ /* kern_vdata should be prepared during the loading phase. */
+ err = bpf_map_update_elem(map->fd, &zero, map->st_ops->kern_vdata, 0);
+ if (err) {
+ err = -errno;
+ free(link);
+ return libbpf_err_ptr(err);
+ }
- if (!prog)
- continue;
- prog_fd = bpf_program__fd(prog);
- kern_data = st_ops->kern_vdata + st_ops->kern_func_off[i];
- *(unsigned long *)kern_data = prog_fd;
+ if (!(map->def.map_flags & BPF_F_LINK)) {
+ /* Fake bpf_link */
+ link->link.fd = map->fd;
+ link->map_fd = -1;
+ link->link.detach = bpf_link__detach_struct_ops;
+ return &link->link;
}
- err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0);
- if (err) {
+ fd = bpf_link_create(map->fd, -1, BPF_STRUCT_OPS, NULL);
+ if (fd < 0) {
err = -errno;
free(link);
return libbpf_err_ptr(err);
}
- link->detach = bpf_link__detach_struct_ops;
- link->fd = map->fd;
+ link->link.fd = fd;
+ link->map_fd = map->fd;
- return link;
+ return &link->link;
}
typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct perf_event_header *hdr,
--
2.34.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next v5 5/8] bpf: Update the struct_ops of a bpf_link.
2023-03-08 0:50 [PATCH bpf-next v5 0/8] Transit between BPF TCP congestion controls Kui-Feng Lee
` (3 preceding siblings ...)
2023-03-08 0:50 ` [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops() Kui-Feng Lee
@ 2023-03-08 0:50 ` Kui-Feng Lee
2023-03-08 22:21 ` Martin KaFai Lau
2023-03-08 0:50 ` [PATCH bpf-next v5 6/8] libbpf: Update a bpf_link with another struct_ops Kui-Feng Lee
` (2 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 0:50 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf; +Cc: Kui-Feng Lee
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 | 1 +
include/uapi/linux/bpf.h | 8 ++++--
kernel/bpf/bpf_struct_ops.c | 46 ++++++++++++++++++++++++++++++++++
kernel/bpf/syscall.c | 43 ++++++++++++++++++++++++++++---
tools/include/uapi/linux/bpf.h | 7 +++++-
5 files changed, 98 insertions(+), 7 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index afca6c526fe4..29d555a82bad 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1470,6 +1470,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 {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f9fc7b8af3c4..edef9cf7d596 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 5a7e86cf67b5..79e663869e51 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -775,10 +775,56 @@ 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_value *kvalue;
+ 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;
+
+ if (new_map->map_type != BPF_MAP_TYPE_STRUCT_OPS ||
+ !(new_map->map_flags & BPF_F_LINK))
+ return -EINVAL;
+
+ mutex_lock(&update_mutex);
+
+ st_link = container_of(link, struct bpf_struct_ops_link, link);
+
+ /* The new and old struct_ops must be the same type. */
+ st_map = container_of(new_map, struct bpf_struct_ops_map, map);
+
+ old_map = st_link->map;
+ old_st_map = container_of(old_map, struct bpf_struct_ops_map, map);
+ if (st_map->st_ops != old_st_map->st_ops ||
+ /* Pair with smp_store_release() during map_update */
+ smp_load_acquire(&st_map->kvalue.state) != BPF_STRUCT_OPS_STATE_READY) {
+ err = -EINVAL;
+ goto err_out;
+ }
+
+ kvalue = &st_map->kvalue;
+
+ err = st_map->st_ops->update(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 3a4503987a48..c087dd2e2c08 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4658,6 +4658,30 @@ 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;
+
+ if (new_map->map_type != BPF_MAP_TYPE_STRUCT_OPS) {
+ ret = -EINVAL;
+ goto out_put_map;
+ }
+
+ if (link->ops->update_map)
+ ret = link->ops->update_map(link, new_map);
+ else
+ ret = -EINVAL;
+
+out_put_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)
@@ -4670,14 +4694,25 @@ static int link_update(union bpf_attr *attr)
if (CHECK_ATTR(BPF_LINK_UPDATE))
return -EINVAL;
- flags = attr->link_update.flags;
- if (flags & ~BPF_F_REPLACE)
- return -EINVAL;
-
link = bpf_link_get_from_fd(attr->link_update.link_fd);
if (IS_ERR(link))
return PTR_ERR(link);
+ flags = attr->link_update.flags;
+
+ if (link->ops->update_map) {
+ if (flags) /* always replace the existing one */
+ ret = -EINVAL;
+ else
+ ret = link_update_map(link, attr);
+ goto out_put_link;
+ }
+
+ if (flags & ~BPF_F_REPLACE) {
+ ret = -EINVAL;
+ 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/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 051b85525302..cfd1d2a9898d 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1556,7 +1556,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 */
--
2.34.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next v5 6/8] libbpf: Update a bpf_link with another struct_ops.
2023-03-08 0:50 [PATCH bpf-next v5 0/8] Transit between BPF TCP congestion controls Kui-Feng Lee
` (4 preceding siblings ...)
2023-03-08 0:50 ` [PATCH bpf-next v5 5/8] bpf: Update the struct_ops of a bpf_link Kui-Feng Lee
@ 2023-03-08 0:50 ` Kui-Feng Lee
2023-03-08 22:32 ` Martin KaFai Lau
2023-03-08 0:50 ` [PATCH bpf-next v5 7/8] libbpf: Use .struct_ops.link section to indicate a struct_ops with a link Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 8/8] selftests/bpf: Test switching TCP Congestion Control algorithms Kui-Feng Lee
7 siblings, 1 reply; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 0:50 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf; +Cc: Kui-Feng Lee
Introduce bpf_link__update_struct_ops(), which will allow you to
effortlessly transition the struct_ops map of any given bpf_link into
an alternative.
Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
---
tools/lib/bpf/libbpf.c | 36 ++++++++++++++++++++++++++++++++++++
tools/lib/bpf/libbpf.h | 1 +
tools/lib/bpf/libbpf.map | 1 +
3 files changed, 38 insertions(+)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index f70b55c0f40e..0406d0e00e1f 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -11657,6 +11657,42 @@ struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
return &link->link;
}
+/*
+ * Swap the back struct_ops of a link with a new struct_ops map.
+ */
+int bpf_link__update_map(struct bpf_link *link, const struct bpf_map *map)
+{
+ struct bpf_link_struct_ops *st_ops_link;
+ __u32 zero = 0;
+ int err, fd;
+
+ if (!bpf_map__is_struct_ops(map) || map->fd == -1)
+ return -EINVAL;
+
+ st_ops_link = container_of(link, struct bpf_link_struct_ops, link);
+ /* Ensure the type of a link is correct */
+ if (st_ops_link->map_fd < 0)
+ return -EINVAL;
+
+ err = bpf_map_update_elem(map->fd, &zero, map->st_ops->kern_vdata, 0);
+ if (err && errno != EBUSY) {
+ err = -errno;
+ free(link);
+ return err;
+ }
+
+ fd = bpf_link_update(link->fd, map->fd, NULL);
+ if (fd < 0) {
+ err = -errno;
+ free(link);
+ return err;
+ }
+
+ st_ops_link->map_fd = map->fd;
+
+ return 0;
+}
+
typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct perf_event_header *hdr,
void *private_data);
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index db4992a036f8..1615e55e2e79 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -719,6 +719,7 @@ bpf_program__attach_freplace(const struct bpf_program *prog,
struct bpf_map;
LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map);
+LIBBPF_API int bpf_link__update_map(struct bpf_link *link, const struct bpf_map *map);
struct bpf_iter_attach_opts {
size_t sz; /* size of this struct for forward/backward compatibility */
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 50dde1f6521e..cc05be376257 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -387,6 +387,7 @@ LIBBPF_1.2.0 {
global:
bpf_btf_get_info_by_fd;
bpf_link_get_info_by_fd;
+ bpf_link__update_map;
bpf_map_get_info_by_fd;
bpf_prog_get_info_by_fd;
} LIBBPF_1.1.0;
--
2.34.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next v5 7/8] libbpf: Use .struct_ops.link section to indicate a struct_ops with a link.
2023-03-08 0:50 [PATCH bpf-next v5 0/8] Transit between BPF TCP congestion controls Kui-Feng Lee
` (5 preceding siblings ...)
2023-03-08 0:50 ` [PATCH bpf-next v5 6/8] libbpf: Update a bpf_link with another struct_ops Kui-Feng Lee
@ 2023-03-08 0:50 ` Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 8/8] selftests/bpf: Test switching TCP Congestion Control algorithms Kui-Feng Lee
7 siblings, 0 replies; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 0:50 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf; +Cc: Kui-Feng Lee
Flags a struct_ops is to back a bpf_link by putting it to the
".struct_ops.link" section. Once it is flagged, the created
struct_ops can be used to create a bpf_link or update a bpf_link that
has been backed by another struct_ops.
Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
---
tools/lib/bpf/libbpf.c | 64 +++++++++++++++++++++++++++++++++---------
1 file changed, 50 insertions(+), 14 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 0406d0e00e1f..589eea158d95 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -468,6 +468,7 @@ struct bpf_struct_ops {
#define KCONFIG_SEC ".kconfig"
#define KSYMS_SEC ".ksyms"
#define STRUCT_OPS_SEC ".struct_ops"
+#define STRUCT_OPS_LINK_SEC ".struct_ops.link"
enum libbpf_map_type {
LIBBPF_MAP_UNSPEC,
@@ -597,6 +598,7 @@ struct elf_state {
Elf64_Ehdr *ehdr;
Elf_Data *symbols;
Elf_Data *st_ops_data;
+ Elf_Data *st_ops_link_data;
size_t shstrndx; /* section index for section name strings */
size_t strtabidx;
struct elf_sec_desc *secs;
@@ -606,6 +608,7 @@ struct elf_state {
int text_shndx;
int symbols_shndx;
int st_ops_shndx;
+ int st_ops_link_shndx;
};
struct usdt_manager;
@@ -1119,7 +1122,7 @@ static int bpf_object__init_kern_struct_ops_maps(struct bpf_object *obj)
return 0;
}
-static int bpf_object__init_struct_ops_maps(struct bpf_object *obj)
+static int bpf_object__init_struct_ops_maps_link(struct bpf_object *obj, bool link)
{
const struct btf_type *type, *datasec;
const struct btf_var_secinfo *vsi;
@@ -1127,18 +1130,33 @@ static int bpf_object__init_struct_ops_maps(struct bpf_object *obj)
const char *tname, *var_name;
__s32 type_id, datasec_id;
const struct btf *btf;
+ const char *sec_name;
struct bpf_map *map;
- __u32 i;
+ __u32 i, map_flags;
+ Elf_Data *data;
+ int shndx;
- if (obj->efile.st_ops_shndx == -1)
+ if (link) {
+ sec_name = STRUCT_OPS_LINK_SEC;
+ shndx = obj->efile.st_ops_link_shndx;
+ data = obj->efile.st_ops_link_data;
+ map_flags = BPF_F_LINK;
+ } else {
+ sec_name = STRUCT_OPS_SEC;
+ shndx = obj->efile.st_ops_shndx;
+ data = obj->efile.st_ops_data;
+ map_flags = 0;
+ }
+
+ if (shndx == -1)
return 0;
btf = obj->btf;
- datasec_id = btf__find_by_name_kind(btf, STRUCT_OPS_SEC,
+ datasec_id = btf__find_by_name_kind(btf, sec_name,
BTF_KIND_DATASEC);
if (datasec_id < 0) {
pr_warn("struct_ops init: DATASEC %s not found\n",
- STRUCT_OPS_SEC);
+ sec_name);
return -EINVAL;
}
@@ -1151,7 +1169,7 @@ static int bpf_object__init_struct_ops_maps(struct bpf_object *obj)
type_id = btf__resolve_type(obj->btf, vsi->type);
if (type_id < 0) {
pr_warn("struct_ops init: Cannot resolve var type_id %u in DATASEC %s\n",
- vsi->type, STRUCT_OPS_SEC);
+ vsi->type, sec_name);
return -EINVAL;
}
@@ -1170,7 +1188,7 @@ static int bpf_object__init_struct_ops_maps(struct bpf_object *obj)
if (IS_ERR(map))
return PTR_ERR(map);
- map->sec_idx = obj->efile.st_ops_shndx;
+ map->sec_idx = shndx;
map->sec_offset = vsi->offset;
map->name = strdup(var_name);
if (!map->name)
@@ -1180,6 +1198,7 @@ static int bpf_object__init_struct_ops_maps(struct bpf_object *obj)
map->def.key_size = sizeof(int);
map->def.value_size = type->size;
map->def.max_entries = 1;
+ map->def.map_flags = map_flags;
map->st_ops = calloc(1, sizeof(*map->st_ops));
if (!map->st_ops)
@@ -1192,14 +1211,14 @@ static int bpf_object__init_struct_ops_maps(struct bpf_object *obj)
if (!st_ops->data || !st_ops->progs || !st_ops->kern_func_off)
return -ENOMEM;
- if (vsi->offset + type->size > obj->efile.st_ops_data->d_size) {
+ if (vsi->offset + type->size > data->d_size) {
pr_warn("struct_ops init: var %s is beyond the end of DATASEC %s\n",
- var_name, STRUCT_OPS_SEC);
+ var_name, sec_name);
return -EINVAL;
}
memcpy(st_ops->data,
- obj->efile.st_ops_data->d_buf + vsi->offset,
+ data->d_buf + vsi->offset,
type->size);
st_ops->tname = tname;
st_ops->type = type;
@@ -1212,6 +1231,15 @@ static int bpf_object__init_struct_ops_maps(struct bpf_object *obj)
return 0;
}
+static int bpf_object__init_struct_ops_maps(struct bpf_object *obj)
+{
+ int err;
+
+ err = bpf_object__init_struct_ops_maps_link(obj, false);
+ err = err ?: bpf_object__init_struct_ops_maps_link(obj, true);
+ return err;
+}
+
static struct bpf_object *bpf_object__new(const char *path,
const void *obj_buf,
size_t obj_buf_sz,
@@ -1248,6 +1276,7 @@ static struct bpf_object *bpf_object__new(const char *path,
obj->efile.obj_buf_sz = obj_buf_sz;
obj->efile.btf_maps_shndx = -1;
obj->efile.st_ops_shndx = -1;
+ obj->efile.st_ops_link_shndx = -1;
obj->kconfig_map_idx = -1;
obj->kern_version = get_kernel_version();
@@ -1265,6 +1294,7 @@ static void bpf_object__elf_finish(struct bpf_object *obj)
obj->efile.elf = NULL;
obj->efile.symbols = NULL;
obj->efile.st_ops_data = NULL;
+ obj->efile.st_ops_link_data = NULL;
zfree(&obj->efile.secs);
obj->efile.sec_cnt = 0;
@@ -2753,12 +2783,13 @@ static bool libbpf_needs_btf(const struct bpf_object *obj)
{
return obj->efile.btf_maps_shndx >= 0 ||
obj->efile.st_ops_shndx >= 0 ||
+ obj->efile.st_ops_link_shndx >= 0 ||
obj->nr_extern > 0;
}
static bool kernel_needs_btf(const struct bpf_object *obj)
{
- return obj->efile.st_ops_shndx >= 0;
+ return obj->efile.st_ops_shndx >= 0 || obj->efile.st_ops_link_shndx >= 0;
}
static int bpf_object__init_btf(struct bpf_object *obj,
@@ -3451,6 +3482,9 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
} else if (strcmp(name, STRUCT_OPS_SEC) == 0) {
obj->efile.st_ops_data = data;
obj->efile.st_ops_shndx = idx;
+ } else if (strcmp(name, STRUCT_OPS_LINK_SEC) == 0) {
+ obj->efile.st_ops_link_data = data;
+ obj->efile.st_ops_link_shndx = idx;
} else {
pr_info("elf: skipping unrecognized data section(%d) %s\n",
idx, name);
@@ -3465,6 +3499,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
/* Only do relo for section with exec instructions */
if (!section_have_execinstr(obj, targ_sec_idx) &&
strcmp(name, ".rel" STRUCT_OPS_SEC) &&
+ strcmp(name, ".rel" STRUCT_OPS_LINK_SEC) &&
strcmp(name, ".rel" MAPS_ELF_SEC)) {
pr_info("elf: skipping relo section(%d) %s for section(%d) %s\n",
idx, name, targ_sec_idx,
@@ -6611,7 +6646,7 @@ static int bpf_object__collect_relos(struct bpf_object *obj)
return -LIBBPF_ERRNO__INTERNAL;
}
- if (idx == obj->efile.st_ops_shndx)
+ if (idx == obj->efile.st_ops_shndx || idx == obj->efile.st_ops_link_shndx)
err = bpf_object__collect_st_ops_relos(obj, shdr, data);
else if (idx == obj->efile.btf_maps_shndx)
err = bpf_object__collect_map_relos(obj, shdr, data);
@@ -8954,8 +8989,9 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
}
/* struct_ops BPF prog can be re-used between multiple
- * .struct_ops as long as it's the same struct_ops struct
- * definition and the same function pointer field
+ * .struct_ops & .struct_ops.link as long as it's the
+ * same struct_ops struct definition and the same
+ * function pointer field
*/
if (prog->attach_btf_id != st_ops->type_id ||
prog->expected_attach_type != member_idx) {
--
2.34.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next v5 8/8] selftests/bpf: Test switching TCP Congestion Control algorithms.
2023-03-08 0:50 [PATCH bpf-next v5 0/8] Transit between BPF TCP congestion controls Kui-Feng Lee
` (6 preceding siblings ...)
2023-03-08 0:50 ` [PATCH bpf-next v5 7/8] libbpf: Use .struct_ops.link section to indicate a struct_ops with a link Kui-Feng Lee
@ 2023-03-08 0:50 ` Kui-Feng Lee
2023-03-08 23:10 ` Martin KaFai Lau
7 siblings, 1 reply; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 0:50 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, sdf; +Cc: Kui-Feng Lee
Create a pair of sockets that utilize the congestion control algorithm
under a particular name. Then switch up this congestion control
algorithm to another implementation and check whether newly created
connections using the same cc name now run the new implementation.
Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
---
.../selftests/bpf/prog_tests/bpf_tcp_ca.c | 38 ++++++++++++
.../selftests/bpf/progs/tcp_ca_update.c | 62 +++++++++++++++++++
2 files changed, 100 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/tcp_ca_update.c
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
index e980188d4124..caaa9175ee36 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
@@ -8,6 +8,7 @@
#include "bpf_dctcp.skel.h"
#include "bpf_cubic.skel.h"
#include "bpf_tcp_nogpl.skel.h"
+#include "tcp_ca_update.skel.h"
#include "bpf_dctcp_release.skel.h"
#include "tcp_ca_write_sk_pacing.skel.h"
#include "tcp_ca_incompl_cong_ops.skel.h"
@@ -381,6 +382,41 @@ static void test_unsupp_cong_op(void)
libbpf_set_print(old_print_fn);
}
+static void test_update_ca(void)
+{
+ struct tcp_ca_update *skel;
+ struct bpf_link *link;
+ int saved_ca1_cnt;
+ int err;
+
+ skel = tcp_ca_update__open();
+ if (!ASSERT_OK_PTR(skel, "open"))
+ return;
+
+ err = tcp_ca_update__load(skel);
+ if (!ASSERT_OK(err, "load")) {
+ tcp_ca_update__destroy(skel);
+ return;
+ }
+
+ link = bpf_map__attach_struct_ops(skel->maps.ca_update_1);
+ ASSERT_OK_PTR(link, "attach_struct_ops");
+
+ do_test("tcp_ca_update", NULL);
+ saved_ca1_cnt = skel->bss->ca1_cnt;
+ ASSERT_GT(saved_ca1_cnt, 0, "ca1_ca1_cnt");
+
+ err = bpf_link__update_map(link, skel->maps.ca_update_2);
+ ASSERT_OK(err, "update_struct_ops");
+
+ do_test("tcp_ca_update", NULL);
+ ASSERT_EQ(skel->bss->ca1_cnt, saved_ca1_cnt, "ca2_ca1_cnt");
+ ASSERT_GT(skel->bss->ca2_cnt, 0, "ca2_ca2_cnt");
+
+ bpf_link__destroy(link);
+ tcp_ca_update__destroy(skel);
+}
+
void test_bpf_tcp_ca(void)
{
if (test__start_subtest("dctcp"))
@@ -399,4 +435,6 @@ void test_bpf_tcp_ca(void)
test_incompl_cong_ops();
if (test__start_subtest("unsupp_cong_op"))
test_unsupp_cong_op();
+ if (test__start_subtest("update_ca"))
+ test_update_ca();
}
diff --git a/tools/testing/selftests/bpf/progs/tcp_ca_update.c b/tools/testing/selftests/bpf/progs/tcp_ca_update.c
new file mode 100644
index 000000000000..36a04be95df5
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tcp_ca_update.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+int ca1_cnt = 0;
+int ca2_cnt = 0;
+
+#define USEC_PER_SEC 1000000UL
+
+#define min(a, b) ((a) < (b) ? (a) : (b))
+
+static inline struct tcp_sock *tcp_sk(const struct sock *sk)
+{
+ return (struct tcp_sock *)sk;
+}
+
+SEC("struct_ops/ca_update_1_cong_control")
+void BPF_PROG(ca_update_1_cong_control, struct sock *sk,
+ const struct rate_sample *rs)
+{
+ ca1_cnt++;
+}
+
+SEC("struct_ops/ca_update_2_cong_control")
+void BPF_PROG(ca_update_2_cong_control, struct sock *sk,
+ const struct rate_sample *rs)
+{
+ ca2_cnt++;
+}
+
+SEC("struct_ops/ca_update_ssthresh")
+__u32 BPF_PROG(ca_update_ssthresh, struct sock *sk)
+{
+ return tcp_sk(sk)->snd_ssthresh;
+}
+
+SEC("struct_ops/ca_update_undo_cwnd")
+__u32 BPF_PROG(ca_update_undo_cwnd, struct sock *sk)
+{
+ return tcp_sk(sk)->snd_cwnd;
+}
+
+SEC(".struct_ops.link")
+struct tcp_congestion_ops ca_update_1 = {
+ .cong_control = (void *)ca_update_1_cong_control,
+ .ssthresh = (void *)ca_update_ssthresh,
+ .undo_cwnd = (void *)ca_update_undo_cwnd,
+ .name = "tcp_ca_update",
+};
+
+SEC(".struct_ops.link")
+struct tcp_congestion_ops ca_update_2 = {
+ .cong_control = (void *)ca_update_2_cong_control,
+ .ssthresh = (void *)ca_update_ssthresh,
+ .undo_cwnd = (void *)ca_update_undo_cwnd,
+ .name = "tcp_ca_update",
+};
--
2.34.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps.
2023-03-08 0:50 ` [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps Kui-Feng Lee
@ 2023-03-08 15:01 ` kernel test robot
2023-03-08 15:32 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 28+ messages in thread
From: kernel test robot @ 2023-03-08 15:01 UTC (permalink / raw)
To: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii,
sdf
Cc: oe-kbuild-all, Kui-Feng Lee
Hi Kui-Feng,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Kui-Feng-Lee/bpf-Retire-the-struct_ops-map-kvalue-refcnt/20230308-085434
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20230308005050.255859-4-kuifeng%40meta.com
patch subject: [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps.
config: microblaze-randconfig-r005-20230306 (https://download.01.org/0day-ci/archive/20230308/202303082224.rf1Z7y3o-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/de9e43a5ac82dde718d80d8347e867a8fc935e0a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kui-Feng-Lee/bpf-Retire-the-struct_ops-map-kvalue-refcnt/20230308-085434
git checkout de9e43a5ac82dde718d80d8347e867a8fc935e0a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=microblaze olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/hid/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303082224.rf1Z7y3o-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/hid_bpf.h:6,
from include/linux/hid.h:29,
from drivers/hid/hid-prodikeys.c:21:
include/linux/bpf.h:2388:19: error: redefinition of 'bpf_struct_ops_link_create'
2388 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bpf.h:1592:19: note: previous definition of 'bpf_struct_ops_link_create' with type 'int(union bpf_attr *)'
1592 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/bits.h:21,
from include/linux/ratelimit_types.h:5,
from include/linux/ratelimit.h:5,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from drivers/hid/hid-prodikeys.c:17:
>> include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
include/linux/compiler.h:232:33: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
232 | #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
| ^~~~~~~~~~~~~~~~~
include/linux/kernel.h:55:59: note: in expansion of macro '__must_be_array'
55 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
| ^~~~~~~~~~~~~~~
include/linux/moduleparam.h:517:20: note: in expansion of macro 'ARRAY_SIZE'
517 | = { .max = ARRAY_SIZE(array), .num = nump, \
| ^~~~~~~~~~
include/linux/moduleparam.h:501:9: note: in expansion of macro 'module_param_array_named'
501 | module_param_array_named(name, name, type, nump, perm)
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/hid/hid-prodikeys.c:90:1: note: in expansion of macro 'module_param_array'
90 | module_param_array(index, int, NULL, 0444);
| ^~~~~~~~~~~~~~~~~~
drivers/hid/hid-prodikeys.c:86:12: warning: 'index' defined but not used [-Wunused-variable]
86 | static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
| ^~~~~
vim +16 include/linux/build_bug.h
bc6245e5efd70c Ian Abbott 2017-07-10 6
bc6245e5efd70c Ian Abbott 2017-07-10 7 #ifdef __CHECKER__
bc6245e5efd70c Ian Abbott 2017-07-10 8 #define BUILD_BUG_ON_ZERO(e) (0)
bc6245e5efd70c Ian Abbott 2017-07-10 9 #else /* __CHECKER__ */
bc6245e5efd70c Ian Abbott 2017-07-10 10 /*
bc6245e5efd70c Ian Abbott 2017-07-10 11 * Force a compilation error if condition is true, but also produce a
8788994376d84d Rikard Falkeborn 2019-12-04 12 * result (of value 0 and type int), so the expression can be used
bc6245e5efd70c Ian Abbott 2017-07-10 13 * e.g. in a structure initializer (or where-ever else comma expressions
bc6245e5efd70c Ian Abbott 2017-07-10 14 * aren't permitted).
bc6245e5efd70c Ian Abbott 2017-07-10 15 */
8788994376d84d Rikard Falkeborn 2019-12-04 @16 #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
527edbc18a70e7 Masahiro Yamada 2019-01-03 17 #endif /* __CHECKER__ */
527edbc18a70e7 Masahiro Yamada 2019-01-03 18
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps.
2023-03-08 0:50 ` [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps Kui-Feng Lee
2023-03-08 15:01 ` kernel test robot
@ 2023-03-08 15:32 ` kernel test robot
2023-03-08 16:03 ` kernel test robot
2023-03-08 20:04 ` Martin KaFai Lau
3 siblings, 0 replies; 28+ messages in thread
From: kernel test robot @ 2023-03-08 15:32 UTC (permalink / raw)
To: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii,
sdf
Cc: llvm, oe-kbuild-all, Kui-Feng Lee
Hi Kui-Feng,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Kui-Feng-Lee/bpf-Retire-the-struct_ops-map-kvalue-refcnt/20230308-085434
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20230308005050.255859-4-kuifeng%40meta.com
patch subject: [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps.
config: i386-randconfig-a012-20230306 (https://download.01.org/0day-ci/archive/20230308/202303082344.mxuydKat-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/de9e43a5ac82dde718d80d8347e867a8fc935e0a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kui-Feng-Lee/bpf-Retire-the-struct_ops-map-kvalue-refcnt/20230308-085434
git checkout de9e43a5ac82dde718d80d8347e867a8fc935e0a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/ethernet/intel/fm10k/ drivers/net/ethernet/intel/ixgbe/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303082344.mxuydKat-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/net/ethernet/intel/fm10k/fm10k_main.c:8:
In file included from include/net/tcp.h:35:
In file included from include/net/sock_reuseport.h:5:
In file included from include/linux/filter.h:9:
include/linux/bpf.h:2388:19: error: redefinition of 'bpf_struct_ops_link_create'
static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
^
include/linux/bpf.h:1592:19: note: previous definition is here
static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
^
>> drivers/net/ethernet/intel/fm10k/fm10k_main.c:886:16: warning: division by zero is undefined [-Wdivision-by-zero]
desc_flags |= FM10K_SET_FLAG(tx_flags, FM10K_TX_FLAGS_CSUM,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/fm10k/fm10k_main.c:878:26: note: expanded from macro 'FM10K_SET_FLAG'
((u32)(_input & _flag) / (_flag / _result)))
^ ~~~~~~~~~~~~~~~~~
1 warning and 1 error generated.
--
In file included from drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:27:
include/linux/bpf.h:2388:19: error: redefinition of 'bpf_struct_ops_link_create'
static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
^
include/linux/bpf.h:1592:19: note: previous definition is here
static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
^
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8223:14: warning: division by zero is undefined [-Wdivision-by-zero]
cmd_type |= IXGBE_SET_FLAG(tx_flags, IXGBE_TX_FLAGS_HW_VLAN,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8213:26: note: expanded from macro 'IXGBE_SET_FLAG'
((u32)(_input & _flag) / (_flag / _result)))
^ ~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8227:14: warning: division by zero is undefined [-Wdivision-by-zero]
cmd_type |= IXGBE_SET_FLAG(tx_flags, IXGBE_TX_FLAGS_TSO,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8213:26: note: expanded from macro 'IXGBE_SET_FLAG'
((u32)(_input & _flag) / (_flag / _result)))
^ ~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8231:14: warning: division by zero is undefined [-Wdivision-by-zero]
cmd_type |= IXGBE_SET_FLAG(tx_flags, IXGBE_TX_FLAGS_TSTAMP,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8213:26: note: expanded from macro 'IXGBE_SET_FLAG'
((u32)(_input & _flag) / (_flag / _result)))
^ ~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8235:14: warning: division by zero is undefined [-Wdivision-by-zero]
cmd_type ^= IXGBE_SET_FLAG(skb->no_fcs, 1, IXGBE_ADVTXD_DCMD_IFCS);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8213:26: note: expanded from macro 'IXGBE_SET_FLAG'
((u32)(_input & _flag) / (_flag / _result)))
^ ~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8246:19: warning: division by zero is undefined [-Wdivision-by-zero]
olinfo_status |= IXGBE_SET_FLAG(tx_flags,
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8213:26: note: expanded from macro 'IXGBE_SET_FLAG'
((u32)(_input & _flag) / (_flag / _result)))
^ ~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8251:19: warning: division by zero is undefined [-Wdivision-by-zero]
olinfo_status |= IXGBE_SET_FLAG(tx_flags,
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8213:26: note: expanded from macro 'IXGBE_SET_FLAG'
((u32)(_input & _flag) / (_flag / _result)))
^ ~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8256:19: warning: division by zero is undefined [-Wdivision-by-zero]
olinfo_status |= IXGBE_SET_FLAG(tx_flags,
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8213:26: note: expanded from macro 'IXGBE_SET_FLAG'
((u32)(_input & _flag) / (_flag / _result)))
^ ~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8264:19: warning: division by zero is undefined [-Wdivision-by-zero]
olinfo_status |= IXGBE_SET_FLAG(tx_flags,
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8213:26: note: expanded from macro 'IXGBE_SET_FLAG'
((u32)(_input & _flag) / (_flag / _result)))
^ ~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:10807:46: warning: shift count >= width of type [-Wshift-count-overflow]
err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
^~~~~~~~~~~~~~~~
include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
#define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
^ ~~~
9 warnings and 1 error generated.
vim +886 drivers/net/ethernet/intel/fm10k/fm10k_main.c
76a540d4728a37 Alexander Duyck 2014-09-20 874
76a540d4728a37 Alexander Duyck 2014-09-20 875 #define FM10K_SET_FLAG(_input, _flag, _result) \
76a540d4728a37 Alexander Duyck 2014-09-20 876 ((_flag <= _result) ? \
76a540d4728a37 Alexander Duyck 2014-09-20 877 ((u32)(_input & _flag) * (_result / _flag)) : \
76a540d4728a37 Alexander Duyck 2014-09-20 878 ((u32)(_input & _flag) / (_flag / _result)))
76a540d4728a37 Alexander Duyck 2014-09-20 879
76a540d4728a37 Alexander Duyck 2014-09-20 880 static u8 fm10k_tx_desc_flags(struct sk_buff *skb, u32 tx_flags)
76a540d4728a37 Alexander Duyck 2014-09-20 881 {
76a540d4728a37 Alexander Duyck 2014-09-20 882 /* set type for advanced descriptor with frame checksum insertion */
76a540d4728a37 Alexander Duyck 2014-09-20 883 u32 desc_flags = 0;
76a540d4728a37 Alexander Duyck 2014-09-20 884
76a540d4728a37 Alexander Duyck 2014-09-20 885 /* set checksum offload bits */
76a540d4728a37 Alexander Duyck 2014-09-20 @886 desc_flags |= FM10K_SET_FLAG(tx_flags, FM10K_TX_FLAGS_CSUM,
76a540d4728a37 Alexander Duyck 2014-09-20 887 FM10K_TXD_FLAG_CSUM);
76a540d4728a37 Alexander Duyck 2014-09-20 888
76a540d4728a37 Alexander Duyck 2014-09-20 889 return desc_flags;
76a540d4728a37 Alexander Duyck 2014-09-20 890 }
76a540d4728a37 Alexander Duyck 2014-09-20 891
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps.
2023-03-08 0:50 ` [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps Kui-Feng Lee
2023-03-08 15:01 ` kernel test robot
2023-03-08 15:32 ` kernel test robot
@ 2023-03-08 16:03 ` kernel test robot
2023-03-08 20:04 ` Martin KaFai Lau
3 siblings, 0 replies; 28+ messages in thread
From: kernel test robot @ 2023-03-08 16:03 UTC (permalink / raw)
To: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii,
sdf
Cc: oe-kbuild-all, Kui-Feng Lee
Hi Kui-Feng,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Kui-Feng-Lee/bpf-Retire-the-struct_ops-map-kvalue-refcnt/20230308-085434
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20230308005050.255859-4-kuifeng%40meta.com
patch subject: [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps.
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230308/202303082340.qYFHo45I-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/de9e43a5ac82dde718d80d8347e867a8fc935e0a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kui-Feng-Lee/bpf-Retire-the-struct_ops-map-kvalue-refcnt/20230308-085434
git checkout de9e43a5ac82dde718d80d8347e867a8fc935e0a
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303082340.qYFHo45I-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/net/virtio_net.c:13:
>> include/linux/bpf.h:2388:19: error: redefinition of 'bpf_struct_ops_link_create'
2388 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bpf.h:1592:19: note: previous definition of 'bpf_struct_ops_link_create' with type 'int(union bpf_attr *)'
1592 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
--
In file included from include/linux/filter.h:9,
from include/net/sock_reuseport.h:5,
from include/net/tcp.h:35,
from net/ipv4/netfilter/nf_reject_ipv4.c:8:
>> include/linux/bpf.h:2388:19: error: redefinition of 'bpf_struct_ops_link_create'
2388 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bpf.h:1592:19: note: previous definition of 'bpf_struct_ops_link_create' with type 'int(union bpf_attr *)'
1592 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
net/ipv4/netfilter/nf_reject_ipv4.c: In function 'nf_send_reset':
net/ipv4/netfilter/nf_reject_ipv4.c:244:23: warning: variable 'niph' set but not used [-Wunused-but-set-variable]
244 | struct iphdr *niph;
| ^~~~
--
In file included from include/linux/filter.h:9,
from kernel/bpf/core.c:21:
>> include/linux/bpf.h:2388:19: error: redefinition of 'bpf_struct_ops_link_create'
2388 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bpf.h:1592:19: note: previous definition of 'bpf_struct_ops_link_create' with type 'int(union bpf_attr *)'
1592 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/bpf/core.c:1632:12: warning: no previous prototype for 'bpf_probe_read_kernel' [-Wmissing-prototypes]
1632 | u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
| ^~~~~~~~~~~~~~~~~~~~~
kernel/bpf/core.c:2069:6: warning: no previous prototype for 'bpf_patch_call_args' [-Wmissing-prototypes]
2069 | void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth)
| ^~~~~~~~~~~~~~~~~~~
--
In file included from include/linux/filter.h:9,
from kernel/kallsyms.c:25:
>> include/linux/bpf.h:2388:19: error: redefinition of 'bpf_struct_ops_link_create'
2388 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bpf.h:1592:19: note: previous definition of 'bpf_struct_ops_link_create' with type 'int(union bpf_attr *)'
1592 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/kallsyms.c:663:12: warning: no previous prototype for 'arch_get_kallsym' [-Wmissing-prototypes]
663 | int __weak arch_get_kallsym(unsigned int symnum, unsigned long *value,
| ^~~~~~~~~~~~~~~~
--
In file included from include/linux/bpf-cgroup.h:5,
from net/socket.c:55:
>> include/linux/bpf.h:2388:19: error: redefinition of 'bpf_struct_ops_link_create'
2388 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bpf.h:1592:19: note: previous definition of 'bpf_struct_ops_link_create' with type 'int(union bpf_attr *)'
1592 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
net/socket.c: In function '__sys_getsockopt':
net/socket.c:2300:13: warning: variable 'max_optlen' set but not used [-Wunused-but-set-variable]
2300 | int max_optlen;
| ^~~~~~~~~~
--
In file included from net/ipv6/ip6_fib.c:18:
>> include/linux/bpf.h:2388:19: error: redefinition of 'bpf_struct_ops_link_create'
2388 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bpf.h:1592:19: note: previous definition of 'bpf_struct_ops_link_create' with type 'int(union bpf_attr *)'
1592 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
net/ipv6/ip6_fib.c: In function 'fib6_add':
net/ipv6/ip6_fib.c:1378:32: warning: variable 'pn' set but not used [-Wunused-but-set-variable]
1378 | struct fib6_node *fn, *pn = NULL;
| ^~
--
In file included from include/linux/filter.h:9,
from include/net/sock_reuseport.h:5,
from include/net/tcp.h:35,
from include/linux/netfilter_ipv6.h:11,
from net/ipv6/netfilter/nf_reject_ipv6.c:12:
>> include/linux/bpf.h:2388:19: error: redefinition of 'bpf_struct_ops_link_create'
2388 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bpf.h:1592:19: note: previous definition of 'bpf_struct_ops_link_create' with type 'int(union bpf_attr *)'
1592 | static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
net/ipv6/netfilter/nf_reject_ipv6.c: In function 'nf_send_reset6':
net/ipv6/netfilter/nf_reject_ipv6.c:287:25: warning: variable 'ip6h' set but not used [-Wunused-but-set-variable]
287 | struct ipv6hdr *ip6h;
| ^~~~
vim +/bpf_struct_ops_link_create +2388 include/linux/bpf.h
2387
> 2388 static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
2389 {
2390 return -EOPNOTSUPP;
2391 }
2392
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 1/8] bpf: Retire the struct_ops map kvalue->refcnt.
2023-03-08 0:50 ` [PATCH bpf-next v5 1/8] bpf: Retire the struct_ops map kvalue->refcnt Kui-Feng Lee
@ 2023-03-08 18:30 ` Martin KaFai Lau
2023-03-08 22:30 ` Kui-Feng Lee
0 siblings, 1 reply; 28+ messages in thread
From: Martin KaFai Lau @ 2023-03-08 18:30 UTC (permalink / raw)
To: Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 6792a7940e1e..50cfc2388cbc 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -78,6 +78,7 @@ struct bpf_map_ops {
> struct bpf_map *(*map_alloc)(union bpf_attr *attr);
> void (*map_release)(struct bpf_map *map, struct file *map_file);
> void (*map_free)(struct bpf_map *map);
> + void (*map_free_rcu)(struct bpf_map *map);
This is no longer needed...
> int (*map_get_next_key)(struct bpf_map *map, void *key, void *next_key);
> void (*map_release_uref)(struct bpf_map *map);
> void *(*map_lookup_elem_sys_only)(struct bpf_map *map, void *key);
> @@ -1934,6 +1935,7 @@ struct bpf_map *bpf_map_get_with_uref(u32 ufd);
> struct bpf_map *__bpf_map_get(struct fd f);
> void bpf_map_inc(struct bpf_map *map);
> void bpf_map_inc_with_uref(struct bpf_map *map);
> +struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref);
> struct bpf_map * __must_check bpf_map_inc_not_zero(struct bpf_map *map);
> void bpf_map_put_with_uref(struct bpf_map *map);
> void bpf_map_put(struct bpf_map *map);
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 38903fb52f98..9e097fcc9cf4 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -58,6 +58,8 @@ struct bpf_struct_ops_map {
> struct bpf_struct_ops_value kvalue;
> };
>
> +static DEFINE_MUTEX(update_mutex);
Please address or reply to the earlier review comments. Stan had mentioned in v3
that this mutex is unused in this patch.
This is only used in patch 5 of this set. Please move it there.
> +
> #define VALUE_PREFIX "bpf_struct_ops_"
> #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
>
> @@ -249,6 +251,7 @@ int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
> struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
> struct bpf_struct_ops_value *uvalue, *kvalue;
> enum bpf_struct_ops_state state;
> + s64 refcnt;
>
> if (unlikely(*(u32 *)key != 0))
> return -ENOENT;
> @@ -267,7 +270,9 @@ int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
> uvalue = value;
> memcpy(uvalue, st_map->uvalue, map->value_size);
> uvalue->state = state;
> - refcount_set(&uvalue->refcnt, refcount_read(&kvalue->refcnt));
> +
> + refcnt = atomic64_read(&map->refcnt) - atomic64_read(&map->usercnt);
> + refcount_set(&uvalue->refcnt, max_t(s64, refcnt, 0));
Please explain a few words that why it will work good enough and no need to be
very accurate (eg. it is for introspection purpose to give an idea on how many
refcnts are held by a subsystem, eg tcp_sock ...). This was also a comment given
in v3.
>
> return 0;
> }
> @@ -491,7 +496,6 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
> *(unsigned long *)(udata + moff) = prog->aux->id;
> }
>
> - refcount_set(&kvalue->refcnt, 1);
> bpf_map_inc(map);
>
> set_memory_rox((long)st_map->image, 1);
> @@ -536,8 +540,7 @@ static int bpf_struct_ops_map_delete_elem(struct bpf_map *map, void *key)
> switch (prev_state) {
> case BPF_STRUCT_OPS_STATE_INUSE:
> st_map->st_ops->unreg(&st_map->kvalue.data);
> - if (refcount_dec_and_test(&st_map->kvalue.refcnt))
> - bpf_map_put(map);
> + bpf_map_put(map);
> return 0;
> case BPF_STRUCT_OPS_STATE_TOBEFREE:
> return -EINPROGRESS;
> @@ -574,6 +577,19 @@ static void bpf_struct_ops_map_free(struct bpf_map *map)
> {
> struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
>
> + /* The struct_ops's function may switch to another struct_ops.
> + *
> + * For example, bpf_tcp_cc_x->init() may switch to
> + * another tcp_cc_y by calling
> + * setsockopt(TCP_CONGESTION, "tcp_cc_y").
> + * During the switch, bpf_struct_ops_put(tcp_cc_x) is called
> + * and its refcount may reach 0 which then free its
> + * trampoline image while tcp_cc_x is still running.
> + *
> + * Thus, a rcu grace period is needed here.
> + */
> + synchronize_rcu();
> +
> if (st_map->links)
> bpf_struct_ops_map_put_progs(st_map);
> bpf_map_area_free(st_map->links);
> @@ -676,41 +692,23 @@ const struct bpf_map_ops bpf_struct_ops_map_ops = {
> bool bpf_struct_ops_get(const void *kdata)
> {
> struct bpf_struct_ops_value *kvalue;
> + struct bpf_struct_ops_map *st_map;
> + struct bpf_map *map;
>
> kvalue = container_of(kdata, struct bpf_struct_ops_value, data);
> + st_map = container_of(kvalue, struct bpf_struct_ops_map, kvalue);
>
> - return refcount_inc_not_zero(&kvalue->refcnt);
> -}
> -
> -static void bpf_struct_ops_put_rcu(struct rcu_head *head)
> -{
> - struct bpf_struct_ops_map *st_map;
> -
> - st_map = container_of(head, struct bpf_struct_ops_map, rcu);
> - bpf_map_put(&st_map->map);
> + map = __bpf_map_inc_not_zero(&st_map->map, false);
> + return !IS_ERR(map);
> }
>
> void bpf_struct_ops_put(const void *kdata)
> {
> struct bpf_struct_ops_value *kvalue;
> + struct bpf_struct_ops_map *st_map;
>
> kvalue = container_of(kdata, struct bpf_struct_ops_value, data);
> - if (refcount_dec_and_test(&kvalue->refcnt)) {
> - struct bpf_struct_ops_map *st_map;
> -
> - st_map = container_of(kvalue, struct bpf_struct_ops_map,
> - kvalue);
> - /* The struct_ops's function may switch to another struct_ops.
> - *
> - * For example, bpf_tcp_cc_x->init() may switch to
> - * another tcp_cc_y by calling
> - * setsockopt(TCP_CONGESTION, "tcp_cc_y").
> - * During the switch, bpf_struct_ops_put(tcp_cc_x) is called
> - * and its map->refcnt may reach 0 which then free its
> - * trampoline image while tcp_cc_x is still running.
> - *
> - * Thus, a rcu grace period is needed here.
> - */
> - call_rcu(&st_map->rcu, bpf_struct_ops_put_rcu);
> - }
> + st_map = container_of(kvalue, struct bpf_struct_ops_map, kvalue);
> +
> + bpf_map_put(&st_map->map);
> }
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index f406dfa13792..03273cddd6bd 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1288,7 +1288,7 @@ struct bpf_map *bpf_map_get_with_uref(u32 ufd)
> }
>
> /* map_idr_lock should have been held */
This comment needs to change, eg.
map_idr_lock should have been held or the map is protected by rcu gp....
> -static struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref)
> +struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref)
> {
> int refold;
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 2/8] net: Update an existing TCP congestion control algorithm.
2023-03-08 0:50 ` [PATCH bpf-next v5 2/8] net: Update an existing TCP congestion control algorithm Kui-Feng Lee
@ 2023-03-08 18:59 ` Martin KaFai Lau
2023-03-08 22:51 ` Kui-Feng Lee
0 siblings, 1 reply; 28+ messages in thread
From: Martin KaFai Lau @ 2023-03-08 18:59 UTC (permalink / raw)
To: Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 50cfc2388cbc..00b6e1a2edaf 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1512,6 +1512,8 @@ 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;
> const char *name;
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index db9f828e9d1e..2abb755e6a3a 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1117,6 +1117,9 @@ struct tcp_congestion_ops {
>
> int tcp_register_congestion_control(struct tcp_congestion_ops *type);
> void tcp_unregister_congestion_control(struct tcp_congestion_ops *type);
> +int tcp_update_congestion_control(struct tcp_congestion_ops *type,
> + struct tcp_congestion_ops *old_type);
> +int tcp_validate_congestion_control(struct tcp_congestion_ops *ca);
>
> void tcp_assign_congestion_control(struct sock *sk);
> void tcp_init_congestion_control(struct sock *sk);
> 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,
> .unreg = bpf_dummy_unreg,
> .name = "bpf_dummy_ops",
> };
> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> index 13fc0c185cd9..e8b27826283e 100644
> --- a/net/ipv4/bpf_tcp_ca.c
> +++ b/net/ipv4/bpf_tcp_ca.c
> @@ -239,8 +239,6 @@ static int bpf_tcp_ca_init_member(const struct btf_type *t,
> if (bpf_obj_name_cpy(tcp_ca->name, utcp_ca->name,
> sizeof(tcp_ca->name)) <= 0)
> return -EINVAL;
> - if (tcp_ca_find(utcp_ca->name))
> - return -EEXIST;
This belongs to patch 3 where BPF_F_LINK needs this. move it closer to where it
is actually used.
> return 1;
> }
>
> @@ -266,13 +264,25 @@ static void bpf_tcp_ca_unreg(void *kdata)
> tcp_unregister_congestion_control(kdata);
> }
>
> +static int bpf_tcp_ca_update(void *kdata, void *old_kdata)
> +{
> + return tcp_update_congestion_control(kdata, old_kdata);
> +}
> +
> +static int bpf_tcp_ca_validate(void *kdata)
> +{
> + return tcp_validate_congestion_control(kdata);
> +}
> +
> struct bpf_struct_ops bpf_tcp_congestion_ops = {
> .verifier_ops = &bpf_tcp_ca_verifier_ops,
> .reg = bpf_tcp_ca_reg,
> .unreg = bpf_tcp_ca_unreg,
> + .update = bpf_tcp_ca_update,
> .check_member = bpf_tcp_ca_check_member,
> .init_member = bpf_tcp_ca_init_member,
> .init = bpf_tcp_ca_init,
> + .validate = bpf_tcp_ca_validate,
> .name = "tcp_congestion_ops",
> };
In general, please move "validate" related bpf changes to patch 3 and "update"
related changes to patch 5. They are bpf specific (including the changes in
bpf_tcp_ca.c) and closer to where it will be used.
Then patch 2 should only have changes in tcp_cong.c as the preparation work for
the later bpf needs. Please cc netdev for patch 2 in the next re-spin.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps.
2023-03-08 0:50 ` [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps Kui-Feng Lee
` (2 preceding siblings ...)
2023-03-08 16:03 ` kernel test robot
@ 2023-03-08 20:04 ` Martin KaFai Lau
2023-03-08 23:46 ` Kui-Feng Lee
3 siblings, 1 reply; 28+ messages in thread
From: Martin KaFai Lau @ 2023-03-08 20:04 UTC (permalink / raw)
To: Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 00b6e1a2edaf..afca6c526fe4 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1548,6 +1548,7 @@ static inline void bpf_module_put(const void *data, struct module *owner)
> else
> module_put(owner);
> }
> +int bpf_struct_ops_link_create(union bpf_attr *attr);
>
> #ifdef CONFIG_NET
> /* Define it here to avoid the use of forward declaration */
> @@ -1588,6 +1589,11 @@ static inline int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map,
> {
> return -EINVAL;
> }
> +static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> #endif
>
> #if defined(CONFIG_CGROUP_BPF) && defined(CONFIG_BPF_LSM)
> @@ -2379,6 +2385,11 @@ static inline void bpf_link_put(struct bpf_link *link)
> {
> }
>
> +static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
> +{
> + return -EOPNOTSUPP;
> +}
The inline version is double defined. It does not look right. Please double check.
> +
> static inline int bpf_obj_get_user(const char __user *pathname, int flags)
> {
> return -EOPNOTSUPP;
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 976b194eb775..f9fc7b8af3c4 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1033,6 +1033,7 @@ enum bpf_attach_type {
> BPF_PERF_EVENT,
> BPF_TRACE_KPROBE_MULTI,
> BPF_LSM_CGROUP,
> + BPF_STRUCT_OPS,
> __MAX_BPF_ATTACH_TYPE
> };
>
> @@ -1266,6 +1267,9 @@ enum {
>
> /* Create a map that is suitable to be an inner map with dynamic max entries */
> BPF_F_INNER_MAP = (1U << 12),
> +
> +/* Create a map that will be registered/unregesitered by the backed bpf_link */
> + BPF_F_LINK = (1U << 13),
> };
>
> /* Flags for BPF_PROG_QUERY. */
> @@ -1507,7 +1511,10 @@ union bpf_attr {
> } task_fd_query;
>
> struct { /* struct used by BPF_LINK_CREATE command */
> - __u32 prog_fd; /* eBPF program to attach */
> + union {
> + __u32 prog_fd; /* eBPF program to attach */
> + __u32 map_fd; /* struct_ops to attach */
> + };
> union {
> __u32 target_fd; /* object to attach to */
> __u32 target_ifindex; /* target ifindex */
> @@ -6379,6 +6386,9 @@ struct bpf_link_info {
> struct {
> __u32 ifindex;
> } xdp;
> + struct {
> + __u32 map_id;
> + } struct_ops;
> };
> } __attribute__((aligned(8)));
>
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 9e097fcc9cf4..5a7e86cf67b5 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -16,6 +16,7 @@ enum bpf_struct_ops_state {
> BPF_STRUCT_OPS_STATE_INIT,
> BPF_STRUCT_OPS_STATE_INUSE,
> BPF_STRUCT_OPS_STATE_TOBEFREE,
> + BPF_STRUCT_OPS_STATE_READY,
> };
>
> #define BPF_STRUCT_OPS_COMMON_VALUE \
> @@ -58,6 +59,11 @@ struct bpf_struct_ops_map {
> struct bpf_struct_ops_value kvalue;
> };
>
> +struct bpf_struct_ops_link {
> + struct bpf_link link;
> + struct bpf_map __rcu *map;
> +};
> +
> static DEFINE_MUTEX(update_mutex);
>
> #define VALUE_PREFIX "bpf_struct_ops_"
> @@ -496,11 +502,24 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
> *(unsigned long *)(udata + moff) = prog->aux->id;
> }
>
> - bpf_map_inc(map);
> -
> set_memory_rox((long)st_map->image, 1);
> + if (st_map->map.map_flags & BPF_F_LINK) {
> + if (st_ops->validate) {
> + err = st_ops->validate(kdata);
> + if (err)
> + goto unlock;
This should at least be 'goto reset_unlock' to release the progs.
set_memory_rox(..., 1) should also be done after validate?
> + }
> + /* Let bpf_link handle registration & unregistration.
> + *
> + * Pair with smp_load_acquire() during lookup_elem().
> + */
> + smp_store_release(&kvalue->state, BPF_STRUCT_OPS_STATE_READY);
> + goto unlock;
> + }
> +
> err = st_ops->reg(kdata);
> if (likely(!err)) {
> + bpf_map_inc(map);
> /* Pair with smp_load_acquire() during lookup_elem().
> * It ensures the above udata updates (e.g. prog->aux->id)
> * can be seen once BPF_STRUCT_OPS_STATE_INUSE is set.
> @@ -516,7 +535,6 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
> */
> set_memory_nx((long)st_map->image, 1);
> set_memory_rw((long)st_map->image, 1);
> - bpf_map_put(map);
>
> reset_unlock:
> bpf_struct_ops_map_put_progs(st_map);
> @@ -534,6 +552,9 @@ static int bpf_struct_ops_map_delete_elem(struct bpf_map *map, void *key)
> struct bpf_struct_ops_map *st_map;
>
> st_map = (struct bpf_struct_ops_map *)map;
> + if (st_map->map.map_flags & BPF_F_LINK)
> + return -EOPNOTSUPP;
> +
> prev_state = cmpxchg(&st_map->kvalue.state,
> BPF_STRUCT_OPS_STATE_INUSE,
> BPF_STRUCT_OPS_STATE_TOBEFREE);
> @@ -601,7 +622,7 @@ static void bpf_struct_ops_map_free(struct bpf_map *map)
> static int bpf_struct_ops_map_alloc_check(union bpf_attr *attr)
> {
> if (attr->key_size != sizeof(unsigned int) || attr->max_entries != 1 ||
> - attr->map_flags || !attr->btf_vmlinux_value_type_id)
> + (attr->map_flags & ~BPF_F_LINK) || !attr->btf_vmlinux_value_type_id)
> return -EINVAL;
> return 0;
> }
> @@ -712,3 +733,98 @@ void bpf_struct_ops_put(const void *kdata)
>
> bpf_map_put(&st_map->map);
> }
> +
> +static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
> +{
> + struct bpf_struct_ops_link *st_link;
> + struct bpf_struct_ops_map *st_map;
> +
> + st_link = container_of(link, struct bpf_struct_ops_link, link);
> + st_map = (struct bpf_struct_ops_map *)st_link->map;
/* protected by refcnt and no one is replacing it */
rcu_dereference_protected(st_link->map, true);
st_link->map is with __rcu. It should have warning when compile with 'make C=1
...'. Patchwork also reports this:
https://patchwork.kernel.org/project/netdevbpf/patch/20230308005050.255859-4-kuifeng@meta.com/.
Please pay attention to patchwork for errors.
> + st_map->st_ops->unreg(&st_map->kvalue.data);
> + bpf_map_put(st_link->map);
Same here. Reading __rcu pointer without rcu_dereference_xxx.
or simply use &st_map->map here. Otherwise, it will also have type mismatch warning.
> + kfree(st_link);
> +}
> +
> +static void bpf_struct_ops_map_link_show_fdinfo(const struct bpf_link *link,
> + struct seq_file *seq)
> +{
> + struct bpf_struct_ops_link *st_link;
> + struct bpf_map *map;
> +
> + st_link = container_of(link, struct bpf_struct_ops_link, link);
> + rcu_read_lock();
> + map = rcu_dereference(st_link->map);
> + if (map)
map cannot be NULL?
> + seq_printf(seq, "map_id:\t%d\n", map->id);
> + rcu_read_unlock();
> +}
> +
> +static int bpf_struct_ops_map_link_fill_link_info(const struct bpf_link *link,
> + struct bpf_link_info *info)
> +{
> + struct bpf_struct_ops_link *st_link;
> + struct bpf_map *map;
> +
> + st_link = container_of(link, struct bpf_struct_ops_link, link);
> + rcu_read_lock();
> + map = rcu_dereference(st_link->map);
> + if (map)
Same here.
> + info->struct_ops.map_id = map->id;
> + rcu_read_unlock();
> + return 0;
> +}
> +
> +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,
> +};
> +
> +int bpf_struct_ops_link_create(union bpf_attr *attr)
> +{
> + struct bpf_struct_ops_link *link = NULL;
> + struct bpf_link_primer link_primer;
> + struct bpf_struct_ops_map *st_map;
> + struct bpf_map *map;
> + int err;
> +
> + map = bpf_map_get(attr->link_create.map_fd);
> + if (!map)
> + return -EINVAL;
> +
> + st_map = (struct bpf_struct_ops_map *)map;
> +
> + if (map->map_type != BPF_MAP_TYPE_STRUCT_OPS || !(map->map_flags & BPF_F_LINK) ||
> + /* Pair with smp_store_release() during map_update */
> + smp_load_acquire(&st_map->kvalue.state) != BPF_STRUCT_OPS_STATE_READY) {
> + err = -EINVAL;
> + goto err_out;
> + }
> +
> + link = kzalloc(sizeof(*link), GFP_USER);
> + if (!link) {
> + err = -ENOMEM;
> + goto err_out;
> + }
> + bpf_link_init(&link->link, BPF_LINK_TYPE_STRUCT_OPS, &bpf_struct_ops_map_lops, NULL);
> + RCU_INIT_POINTER(link->map, map);
> +
> + err = bpf_link_prime(&link->link, &link_primer);
> + if (err)
> + goto err_out;
> +
> + err = st_map->st_ops->reg(st_map->kvalue.data);
> + if (err) {
> + bpf_link_cleanup(&link_primer);
> + goto err_out;
> + }
> +
> + return bpf_link_settle(&link_primer);
> +
> +err_out:
> + bpf_map_put(map);
> + kfree(link);
> + return err;
> +}
> +
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops().
2023-03-08 0:50 ` [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops() Kui-Feng Lee
@ 2023-03-08 21:42 ` Martin KaFai Lau
2023-03-09 0:22 ` Kui-Feng Lee
0 siblings, 1 reply; 28+ messages in thread
From: Martin KaFai Lau @ 2023-03-08 21:42 UTC (permalink / raw)
To: Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
> @@ -11566,22 +11591,34 @@ struct bpf_link *bpf_program__attach(const struct bpf_program *prog)
> return link;
> }
>
> +struct bpf_link_struct_ops {
> + struct bpf_link link;
> + int map_fd;
> +};
> +
> static int bpf_link__detach_struct_ops(struct bpf_link *link)
> {
> + struct bpf_link_struct_ops *st_link;
> __u32 zero = 0;
>
> - if (bpf_map_delete_elem(link->fd, &zero))
> - return -errno;
> + st_link = container_of(link, struct bpf_link_struct_ops, link);
>
> - return 0;
> + if (st_link->map_fd < 0) {
map_fd < 0 should always be true?
> + /* Fake bpf_link */
> + if (bpf_map_delete_elem(link->fd, &zero))
> + return -errno;
> + return 0;
> + }
> +
> + /* Doesn't support detaching. */
> + return -EOPNOTSUPP;
> }
>
> struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
> {
> - struct bpf_struct_ops *st_ops;
> - struct bpf_link *link;
> - __u32 i, zero = 0;
> - int err;
> + struct bpf_link_struct_ops *link;
> + __u32 zero = 0;
> + int err, fd;
>
> if (!bpf_map__is_struct_ops(map) || map->fd == -1)
> return libbpf_err_ptr(-EINVAL);
> @@ -11590,31 +11627,34 @@ struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
> if (!link)
> return libbpf_err_ptr(-EINVAL);
>
> - st_ops = map->st_ops;
> - for (i = 0; i < btf_vlen(st_ops->type); i++) {
> - struct bpf_program *prog = st_ops->progs[i];
> - void *kern_data;
> - int prog_fd;
> + /* kern_vdata should be prepared during the loading phase. */
> + err = bpf_map_update_elem(map->fd, &zero, map->st_ops->kern_vdata, 0);
> + if (err) {
> + err = -errno;
> + free(link);
> + return libbpf_err_ptr(err);
> + }
>
> - if (!prog)
> - continue;
>
> - prog_fd = bpf_program__fd(prog);
> - kern_data = st_ops->kern_vdata + st_ops->kern_func_off[i];
> - *(unsigned long *)kern_data = prog_fd;
> + if (!(map->def.map_flags & BPF_F_LINK)) {
> + /* Fake bpf_link */
> + link->link.fd = map->fd;
> + link->map_fd = -1;
> + link->link.detach = bpf_link__detach_struct_ops;
> + return &link->link;
> }
>
> - err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0);
> - if (err) {
> + fd = bpf_link_create(map->fd, -1, BPF_STRUCT_OPS, NULL);
> + if (fd < 0) {
> err = -errno;
> free(link);
> return libbpf_err_ptr(err);
> }
>
> - link->detach = bpf_link__detach_struct_ops;
> - link->fd = map->fd;
> + link->link.fd = fd;
> + link->map_fd = map->fd;
Does it need to set link->link.detach?
>
> - return link;
> + return &link->link;
> }
>
> typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct perf_event_header *hdr,
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 5/8] bpf: Update the struct_ops of a bpf_link.
2023-03-08 0:50 ` [PATCH bpf-next v5 5/8] bpf: Update the struct_ops of a bpf_link Kui-Feng Lee
@ 2023-03-08 22:21 ` Martin KaFai Lau
2023-03-09 3:09 ` Kui-Feng Lee
0 siblings, 1 reply; 28+ messages in thread
From: Martin KaFai Lau @ 2023-03-08 22:21 UTC (permalink / raw)
To: Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/7/23 4:50 PM, Kui-Feng Lee 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 | 1 +
> include/uapi/linux/bpf.h | 8 ++++--
> kernel/bpf/bpf_struct_ops.c | 46 ++++++++++++++++++++++++++++++++++
> kernel/bpf/syscall.c | 43 ++++++++++++++++++++++++++++---
> tools/include/uapi/linux/bpf.h | 7 +++++-
> 5 files changed, 98 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index afca6c526fe4..29d555a82bad 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1470,6 +1470,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 {
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index f9fc7b8af3c4..edef9cf7d596 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 5a7e86cf67b5..79e663869e51 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -775,10 +775,56 @@ 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_value *kvalue;
> + 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;
> +
> + if (new_map->map_type != BPF_MAP_TYPE_STRUCT_OPS ||
> + !(new_map->map_flags & BPF_F_LINK))
> + return -EINVAL;
> +
> + mutex_lock(&update_mutex);
> +
> + st_link = container_of(link, struct bpf_struct_ops_link, link);
> +
> + /* The new and old struct_ops must be the same type. */
> + st_map = container_of(new_map, struct bpf_struct_ops_map, map);
nit. move the st_link and st_map init out of the lock.
> +
> + old_map = st_link->map;
rcu_dereference_protected(...)
> + old_st_map = container_of(old_map, struct bpf_struct_ops_map, map);
> + if (st_map->st_ops != old_st_map->st_ops ||
> + /* Pair with smp_store_release() during map_update */
> + smp_load_acquire(&st_map->kvalue.state) != BPF_STRUCT_OPS_STATE_READY) {
nit. test the smp_load_acquire(&st_map...) outside of the lock.
Do it together with the new_map checking at the beginning of the func.
> + err = -EINVAL;
> + goto err_out;
> + }
> +
> + kvalue = &st_map->kvalue;
> +
> + err = st_map->st_ops->update(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 3a4503987a48..c087dd2e2c08 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -4658,6 +4658,30 @@ 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;
> +
> + if (new_map->map_type != BPF_MAP_TYPE_STRUCT_OPS) {
This is unnecessary test. The individual '.update_map()' should test for its own
map_type and the new bpf_struct_ops_map_link_update() does test it.
> + ret = -EINVAL;
> + goto out_put_map;
> + }
> +
> + if (link->ops->update_map)
This has just been tested in link_update() before calling link_update_map().
> + ret = link->ops->update_map(link, new_map);
> + else
> + ret = -EINVAL;
> +
> +out_put_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)
> @@ -4670,14 +4694,25 @@ static int link_update(union bpf_attr *attr)
> if (CHECK_ATTR(BPF_LINK_UPDATE))
> return -EINVAL;
>
> - flags = attr->link_update.flags;
> - if (flags & ~BPF_F_REPLACE)
> - return -EINVAL;
> -
> link = bpf_link_get_from_fd(attr->link_update.link_fd);
> if (IS_ERR(link))
> return PTR_ERR(link);
>
> + flags = attr->link_update.flags;
> +
> + if (link->ops->update_map) {
> + if (flags) /* always replace the existing one */
> + ret = -EINVAL;
> + else
> + ret = link_update_map(link, attr);
> + goto out_put_link;
> + }
> +
> + if (flags & ~BPF_F_REPLACE) {
> + ret = -EINVAL;
> + 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);
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 1/8] bpf: Retire the struct_ops map kvalue->refcnt.
2023-03-08 18:30 ` Martin KaFai Lau
@ 2023-03-08 22:30 ` Kui-Feng Lee
0 siblings, 0 replies; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 22:30 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/8/23 10:30, Martin KaFai Lau wrote:
> On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 6792a7940e1e..50cfc2388cbc 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -78,6 +78,7 @@ struct bpf_map_ops {
>> struct bpf_map *(*map_alloc)(union bpf_attr *attr);
>> void (*map_release)(struct bpf_map *map, struct file *map_file);
>> void (*map_free)(struct bpf_map *map);
>> + void (*map_free_rcu)(struct bpf_map *map);
>
> This is no longer needed...
>
>> int (*map_get_next_key)(struct bpf_map *map, void *key, void
>> *next_key);
>> void (*map_release_uref)(struct bpf_map *map);
>> void *(*map_lookup_elem_sys_only)(struct bpf_map *map, void *key);
>> @@ -1934,6 +1935,7 @@ struct bpf_map *bpf_map_get_with_uref(u32 ufd);
>> struct bpf_map *__bpf_map_get(struct fd f);
>> void bpf_map_inc(struct bpf_map *map);
>> void bpf_map_inc_with_uref(struct bpf_map *map);
>> +struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref);
>> struct bpf_map * __must_check bpf_map_inc_not_zero(struct bpf_map
>> *map);
>> void bpf_map_put_with_uref(struct bpf_map *map);
>> void bpf_map_put(struct bpf_map *map);
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 38903fb52f98..9e097fcc9cf4 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -58,6 +58,8 @@ struct bpf_struct_ops_map {
>> struct bpf_struct_ops_value kvalue;
>> };
>> +static DEFINE_MUTEX(update_mutex);
>
> Please address or reply to the earlier review comments. Stan had
> mentioned in v3 that this mutex is unused in this patch.
>
> This is only used in patch 5 of this set. Please move it there.
I will address this.
>
>> +
>> #define VALUE_PREFIX "bpf_struct_ops_"
>> #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
>> @@ -249,6 +251,7 @@ int bpf_struct_ops_map_sys_lookup_elem(struct
>> bpf_map *map, void *key,
>> struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map
>> *)map;
>> struct bpf_struct_ops_value *uvalue, *kvalue;
>> enum bpf_struct_ops_state state;
>> + s64 refcnt;
>> if (unlikely(*(u32 *)key != 0))
>> return -ENOENT;
>> @@ -267,7 +270,9 @@ int bpf_struct_ops_map_sys_lookup_elem(struct
>> bpf_map *map, void *key,
>> uvalue = value;
>> memcpy(uvalue, st_map->uvalue, map->value_size);
>> uvalue->state = state;
>> - refcount_set(&uvalue->refcnt, refcount_read(&kvalue->refcnt));
>> +
>> + refcnt = atomic64_read(&map->refcnt) - atomic64_read(&map->usercnt);
>> + refcount_set(&uvalue->refcnt, max_t(s64, refcnt, 0));
>
> Please explain a few words that why it will work good enough and no need
> to be very accurate (eg. it is for introspection purpose to give an idea
> on how many refcnts are held by a subsystem, eg tcp_sock ...). This was
> also a comment given in v3.
Sure
>
>> return 0;
>> }
>> @@ -491,7 +496,6 @@ static int bpf_struct_ops_map_update_elem(struct
>> bpf_map *map, void *key,
>> *(unsigned long *)(udata + moff) = prog->aux->id;
>> }
>> - refcount_set(&kvalue->refcnt, 1);
>> bpf_map_inc(map);
>> set_memory_rox((long)st_map->image, 1);
>> @@ -536,8 +540,7 @@ static int bpf_struct_ops_map_delete_elem(struct
>> bpf_map *map, void *key)
>> switch (prev_state) {
>> case BPF_STRUCT_OPS_STATE_INUSE:
>> st_map->st_ops->unreg(&st_map->kvalue.data);
>> - if (refcount_dec_and_test(&st_map->kvalue.refcnt))
>> - bpf_map_put(map);
>> + bpf_map_put(map);
>> return 0;
>> case BPF_STRUCT_OPS_STATE_TOBEFREE:
>> return -EINPROGRESS;
>> @@ -574,6 +577,19 @@ static void bpf_struct_ops_map_free(struct
>> bpf_map *map)
>> {
>> struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map
>> *)map;
>> + /* The struct_ops's function may switch to another struct_ops.
>> + *
>> + * For example, bpf_tcp_cc_x->init() may switch to
>> + * another tcp_cc_y by calling
>> + * setsockopt(TCP_CONGESTION, "tcp_cc_y").
>> + * During the switch, bpf_struct_ops_put(tcp_cc_x) is called
>> + * and its refcount may reach 0 which then free its
>> + * trampoline image while tcp_cc_x is still running.
>> + *
>> + * Thus, a rcu grace period is needed here.
>> + */
>> + synchronize_rcu();
>> +
>> if (st_map->links)
>> bpf_struct_ops_map_put_progs(st_map);
>> bpf_map_area_free(st_map->links);
>> @@ -676,41 +692,23 @@ const struct bpf_map_ops bpf_struct_ops_map_ops = {
>> bool bpf_struct_ops_get(const void *kdata)
>> {
>> struct bpf_struct_ops_value *kvalue;
>> + struct bpf_struct_ops_map *st_map;
>> + struct bpf_map *map;
>> kvalue = container_of(kdata, struct bpf_struct_ops_value, data);
>> + st_map = container_of(kvalue, struct bpf_struct_ops_map, kvalue);
>> - return refcount_inc_not_zero(&kvalue->refcnt);
>> -}
>> -
>> -static void bpf_struct_ops_put_rcu(struct rcu_head *head)
>> -{
>> - struct bpf_struct_ops_map *st_map;
>> -
>> - st_map = container_of(head, struct bpf_struct_ops_map, rcu);
>> - bpf_map_put(&st_map->map);
>> + map = __bpf_map_inc_not_zero(&st_map->map, false);
>> + return !IS_ERR(map);
>> }
>> void bpf_struct_ops_put(const void *kdata)
>> {
>> struct bpf_struct_ops_value *kvalue;
>> + struct bpf_struct_ops_map *st_map;
>> kvalue = container_of(kdata, struct bpf_struct_ops_value, data);
>> - if (refcount_dec_and_test(&kvalue->refcnt)) {
>> - struct bpf_struct_ops_map *st_map;
>> -
>> - st_map = container_of(kvalue, struct bpf_struct_ops_map,
>> - kvalue);
>> - /* The struct_ops's function may switch to another struct_ops.
>> - *
>> - * For example, bpf_tcp_cc_x->init() may switch to
>> - * another tcp_cc_y by calling
>> - * setsockopt(TCP_CONGESTION, "tcp_cc_y").
>> - * During the switch, bpf_struct_ops_put(tcp_cc_x) is called
>> - * and its map->refcnt may reach 0 which then free its
>> - * trampoline image while tcp_cc_x is still running.
>> - *
>> - * Thus, a rcu grace period is needed here.
>> - */
>> - call_rcu(&st_map->rcu, bpf_struct_ops_put_rcu);
>> - }
>> + st_map = container_of(kvalue, struct bpf_struct_ops_map, kvalue);
>> +
>> + bpf_map_put(&st_map->map);
>> }
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index f406dfa13792..03273cddd6bd 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -1288,7 +1288,7 @@ struct bpf_map *bpf_map_get_with_uref(u32 ufd)
>> }
>> /* map_idr_lock should have been held */
>
> This comment needs to change, eg.
> map_idr_lock should have been held or the map is protected by rcu gp....
>
Sure.
>> -static struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map,
>> bool uref)
>> +struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref)
>> {
>> int refold;
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 6/8] libbpf: Update a bpf_link with another struct_ops.
2023-03-08 0:50 ` [PATCH bpf-next v5 6/8] libbpf: Update a bpf_link with another struct_ops Kui-Feng Lee
@ 2023-03-08 22:32 ` Martin KaFai Lau
0 siblings, 0 replies; 28+ messages in thread
From: Martin KaFai Lau @ 2023-03-08 22:32 UTC (permalink / raw)
To: Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
> Introduce bpf_link__update_struct_ops(), which will allow you to
> effortlessly transition the struct_ops map of any given bpf_link into
> an alternative.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
> ---
> tools/lib/bpf/libbpf.c | 36 ++++++++++++++++++++++++++++++++++++
> tools/lib/bpf/libbpf.h | 1 +
> tools/lib/bpf/libbpf.map | 1 +
> 3 files changed, 38 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index f70b55c0f40e..0406d0e00e1f 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -11657,6 +11657,42 @@ struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
> return &link->link;
> }
>
> +/*
> + * Swap the back struct_ops of a link with a new struct_ops map.
> + */
> +int bpf_link__update_map(struct bpf_link *link, const struct bpf_map *map)
> +{
> + struct bpf_link_struct_ops *st_ops_link;
> + __u32 zero = 0;
> + int err, fd;
> +
> + if (!bpf_map__is_struct_ops(map) || map->fd == -1)
> + return -EINVAL;
> +
> + st_ops_link = container_of(link, struct bpf_link_struct_ops, link);
> + /* Ensure the type of a link is correct */
> + if (st_ops_link->map_fd < 0)
> + return -EINVAL;
> +
> + err = bpf_map_update_elem(map->fd, &zero, map->st_ops->kern_vdata, 0);
> + if (err && errno != EBUSY) {
> + err = -errno;
> + free(link);
hmm... this looks wrong to free here.
> + return err;
> + }
> +
> + fd = bpf_link_update(link->fd, map->fd, NULL);
> + if (fd < 0) {
> + err = -errno;
> + free(link);
same here.
> + return err;
> + }
> +
> + st_ops_link->map_fd = map->fd;
> +
> + return 0;
> +}
> +
> typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct perf_event_header *hdr,
> void *private_data);
>
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index db4992a036f8..1615e55e2e79 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -719,6 +719,7 @@ bpf_program__attach_freplace(const struct bpf_program *prog,
> struct bpf_map;
>
> LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map);
> +LIBBPF_API int bpf_link__update_map(struct bpf_link *link, const struct bpf_map *map);
>
> struct bpf_iter_attach_opts {
> size_t sz; /* size of this struct for forward/backward compatibility */
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 50dde1f6521e..cc05be376257 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -387,6 +387,7 @@ LIBBPF_1.2.0 {
> global:
> bpf_btf_get_info_by_fd;
> bpf_link_get_info_by_fd;
> + bpf_link__update_map;
> bpf_map_get_info_by_fd;
> bpf_prog_get_info_by_fd;
> } LIBBPF_1.1.0;
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 2/8] net: Update an existing TCP congestion control algorithm.
2023-03-08 18:59 ` Martin KaFai Lau
@ 2023-03-08 22:51 ` Kui-Feng Lee
0 siblings, 0 replies; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 22:51 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/8/23 10:59, Martin KaFai Lau wrote:
> On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 50cfc2388cbc..00b6e1a2edaf 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1512,6 +1512,8 @@ 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;
>> const char *name;
>> diff --git a/include/net/tcp.h b/include/net/tcp.h
>> index db9f828e9d1e..2abb755e6a3a 100644
>> --- a/include/net/tcp.h
>> +++ b/include/net/tcp.h
>> @@ -1117,6 +1117,9 @@ struct tcp_congestion_ops {
>> int tcp_register_congestion_control(struct tcp_congestion_ops *type);
>> void tcp_unregister_congestion_control(struct tcp_congestion_ops
>> *type);
>> +int tcp_update_congestion_control(struct tcp_congestion_ops *type,
>> + struct tcp_congestion_ops *old_type);
>> +int tcp_validate_congestion_control(struct tcp_congestion_ops *ca);
>> void tcp_assign_congestion_control(struct sock *sk);
>> void tcp_init_congestion_control(struct sock *sk);
>> 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,
>> .unreg = bpf_dummy_unreg,
>> .name = "bpf_dummy_ops",
>> };
>> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
>> index 13fc0c185cd9..e8b27826283e 100644
>> --- a/net/ipv4/bpf_tcp_ca.c
>> +++ b/net/ipv4/bpf_tcp_ca.c
>> @@ -239,8 +239,6 @@ static int bpf_tcp_ca_init_member(const struct
>> btf_type *t,
>> if (bpf_obj_name_cpy(tcp_ca->name, utcp_ca->name,
>> sizeof(tcp_ca->name)) <= 0)
>> return -EINVAL;
>> - if (tcp_ca_find(utcp_ca->name))
>> - return -EEXIST;
>
> This belongs to patch 3 where BPF_F_LINK needs this. move it closer to
> where it is actually used.
Got it!
>
>> return 1;
>> }
>> @@ -266,13 +264,25 @@ static void bpf_tcp_ca_unreg(void *kdata)
>> tcp_unregister_congestion_control(kdata);
>> }
>> +static int bpf_tcp_ca_update(void *kdata, void *old_kdata)
>> +{
>> + return tcp_update_congestion_control(kdata, old_kdata);
>> +}
>> +
>> +static int bpf_tcp_ca_validate(void *kdata)
>> +{
>> + return tcp_validate_congestion_control(kdata);
>> +}
>> +
>> struct bpf_struct_ops bpf_tcp_congestion_ops = {
>> .verifier_ops = &bpf_tcp_ca_verifier_ops,
>> .reg = bpf_tcp_ca_reg,
>> .unreg = bpf_tcp_ca_unreg,
>> + .update = bpf_tcp_ca_update,
>> .check_member = bpf_tcp_ca_check_member,
>> .init_member = bpf_tcp_ca_init_member,
>> .init = bpf_tcp_ca_init,
>> + .validate = bpf_tcp_ca_validate,
>> .name = "tcp_congestion_ops",
>> };
>
> In general, please move "validate" related bpf changes to patch 3 and
> "update" related changes to patch 5. They are bpf specific (including
> the changes in bpf_tcp_ca.c) and closer to where it will be used.
>
> Then patch 2 should only have changes in tcp_cong.c as the preparation
> work for the later bpf needs. Please cc netdev for patch 2 in the next
> re-spin.
>
Sure
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 8/8] selftests/bpf: Test switching TCP Congestion Control algorithms.
2023-03-08 0:50 ` [PATCH bpf-next v5 8/8] selftests/bpf: Test switching TCP Congestion Control algorithms Kui-Feng Lee
@ 2023-03-08 23:10 ` Martin KaFai Lau
2023-03-09 3:34 ` Kui-Feng Lee
0 siblings, 1 reply; 28+ messages in thread
From: Martin KaFai Lau @ 2023-03-08 23:10 UTC (permalink / raw)
To: Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
> Create a pair of sockets that utilize the congestion control algorithm
> under a particular name. Then switch up this congestion control
> algorithm to another implementation and check whether newly created
> connections using the same cc name now run the new implementation.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
> ---
> .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 38 ++++++++++++
> .../selftests/bpf/progs/tcp_ca_update.c | 62 +++++++++++++++++++
> 2 files changed, 100 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/tcp_ca_update.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
> index e980188d4124..caaa9175ee36 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
> @@ -8,6 +8,7 @@
> #include "bpf_dctcp.skel.h"
> #include "bpf_cubic.skel.h"
> #include "bpf_tcp_nogpl.skel.h"
> +#include "tcp_ca_update.skel.h"
> #include "bpf_dctcp_release.skel.h"
> #include "tcp_ca_write_sk_pacing.skel.h"
> #include "tcp_ca_incompl_cong_ops.skel.h"
> @@ -381,6 +382,41 @@ static void test_unsupp_cong_op(void)
> libbpf_set_print(old_print_fn);
> }
>
> +static void test_update_ca(void)
> +{
> + struct tcp_ca_update *skel;
> + struct bpf_link *link;
> + int saved_ca1_cnt;
> + int err;
> +
> + skel = tcp_ca_update__open();
> + if (!ASSERT_OK_PTR(skel, "open"))
> + return;
> +
> + err = tcp_ca_update__load(skel);
nit. Use tcp_ca_update__open_and_load().
> + if (!ASSERT_OK(err, "load")) {
> + tcp_ca_update__destroy(skel);
> + return;
> + }
> +
> + link = bpf_map__attach_struct_ops(skel->maps.ca_update_1);
> + ASSERT_OK_PTR(link, "attach_struct_ops");
> +
> + do_test("tcp_ca_update", NULL);
> + saved_ca1_cnt = skel->bss->ca1_cnt;
> + ASSERT_GT(saved_ca1_cnt, 0, "ca1_ca1_cnt");
> +
> + err = bpf_link__update_map(link, skel->maps.ca_update_2);
> + ASSERT_OK(err, "update_struct_ops");
> +
> + do_test("tcp_ca_update", NULL);
> + ASSERT_EQ(skel->bss->ca1_cnt, saved_ca1_cnt, "ca2_ca1_cnt");
> + ASSERT_GT(skel->bss->ca2_cnt, 0, "ca2_ca2_cnt");
> +
> + bpf_link__destroy(link);
> + tcp_ca_update__destroy(skel);
> +}
> +
> void test_bpf_tcp_ca(void)
> {
> if (test__start_subtest("dctcp"))
> @@ -399,4 +435,6 @@ void test_bpf_tcp_ca(void)
> test_incompl_cong_ops();
> if (test__start_subtest("unsupp_cong_op"))
> test_unsupp_cong_op();
> + if (test__start_subtest("update_ca"))
> + test_update_ca();
> }
> diff --git a/tools/testing/selftests/bpf/progs/tcp_ca_update.c b/tools/testing/selftests/bpf/progs/tcp_ca_update.c
> new file mode 100644
> index 000000000000..36a04be95df5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tcp_ca_update.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "vmlinux.h"
> +
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +int ca1_cnt = 0;
> +int ca2_cnt = 0;
> +
> +#define USEC_PER_SEC 1000000UL
Not used.
> +
> +#define min(a, b) ((a) < (b) ? (a) : (b))
Not used.
> +
> +static inline struct tcp_sock *tcp_sk(const struct sock *sk)
> +{
> + return (struct tcp_sock *)sk;
> +}
> +
> +SEC("struct_ops/ca_update_1_cong_control")
> +void BPF_PROG(ca_update_1_cong_control, struct sock *sk,
> + const struct rate_sample *rs)
> +{
> + ca1_cnt++;
> +}
> +
> +SEC("struct_ops/ca_update_2_cong_control")
> +void BPF_PROG(ca_update_2_cong_control, struct sock *sk,
> + const struct rate_sample *rs)
> +{
> + ca2_cnt++;
> +}
> +
> +SEC("struct_ops/ca_update_ssthresh")
> +__u32 BPF_PROG(ca_update_ssthresh, struct sock *sk)
> +{
> + return tcp_sk(sk)->snd_ssthresh;
> +}
> +
> +SEC("struct_ops/ca_update_undo_cwnd")
> +__u32 BPF_PROG(ca_update_undo_cwnd, struct sock *sk)
> +{
> + return tcp_sk(sk)->snd_cwnd;
> +}
> +
> +SEC(".struct_ops.link")
> +struct tcp_congestion_ops ca_update_1 = {
> + .cong_control = (void *)ca_update_1_cong_control,
> + .ssthresh = (void *)ca_update_ssthresh,
> + .undo_cwnd = (void *)ca_update_undo_cwnd,
> + .name = "tcp_ca_update",
> +};
> +
> +SEC(".struct_ops.link")
> +struct tcp_congestion_ops ca_update_2 = {
> + .cong_control = (void *)ca_update_2_cong_control,
nit. I think it is more future proof to use '.init' to bump the ca1_cnt and
ca2_cnt. '.init' must be called. Just in case for some unlikely stack change
that may not call cong_control and then need to adjust the test accordingly.
It also needs a few negative tests like creating a link with a map without
BPF_F_LINK. delete_elem on BPF_F_LINK map. Replace a link with a different
tcp-cc-name which is not supported now, ...etc.
> + .ssthresh = (void *)ca_update_ssthresh,
> + .undo_cwnd = (void *)ca_update_undo_cwnd,
> + .name = "tcp_ca_update",
> +};
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps.
2023-03-08 20:04 ` Martin KaFai Lau
@ 2023-03-08 23:46 ` Kui-Feng Lee
0 siblings, 0 replies; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-08 23:46 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/8/23 12:04, Martin KaFai Lau wrote:
> On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 00b6e1a2edaf..afca6c526fe4 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1548,6 +1548,7 @@ static inline void bpf_module_put(const void
>> *data, struct module *owner)
>> else
>> module_put(owner);
>> }
>> +int bpf_struct_ops_link_create(union bpf_attr *attr);
>> #ifdef CONFIG_NET
>> /* Define it here to avoid the use of forward declaration */
>> @@ -1588,6 +1589,11 @@ static inline int
>> bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map,
>> {
>> return -EINVAL;
>> }
>> +static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +
>> #endif
>> #if defined(CONFIG_CGROUP_BPF) && defined(CONFIG_BPF_LSM)
>> @@ -2379,6 +2385,11 @@ static inline void bpf_link_put(struct bpf_link
>> *link)
>> {
>> }
>> +static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>
> The inline version is double defined. It does not look right. Please
> double check.
Removed!
>
>> +
>> static inline int bpf_obj_get_user(const char __user *pathname, int
>> flags)
>> {
>> return -EOPNOTSUPP;
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 976b194eb775..f9fc7b8af3c4 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1033,6 +1033,7 @@ enum bpf_attach_type {
>> BPF_PERF_EVENT,
>> BPF_TRACE_KPROBE_MULTI,
>> BPF_LSM_CGROUP,
>> + BPF_STRUCT_OPS,
>> __MAX_BPF_ATTACH_TYPE
>> };
>> @@ -1266,6 +1267,9 @@ enum {
>> /* Create a map that is suitable to be an inner map with dynamic max
>> entries */
>> BPF_F_INNER_MAP = (1U << 12),
>> +
>> +/* Create a map that will be registered/unregesitered by the backed
>> bpf_link */
>> + BPF_F_LINK = (1U << 13),
>> };
>> /* Flags for BPF_PROG_QUERY. */
>> @@ -1507,7 +1511,10 @@ union bpf_attr {
>> } task_fd_query;
>> struct { /* struct used by BPF_LINK_CREATE command */
>> - __u32 prog_fd; /* eBPF program to attach */
>> + union {
>> + __u32 prog_fd; /* eBPF program to attach */
>> + __u32 map_fd; /* struct_ops to attach */
>> + };
>> union {
>> __u32 target_fd; /* object to attach to */
>> __u32 target_ifindex; /* target ifindex */
>> @@ -6379,6 +6386,9 @@ struct bpf_link_info {
>> struct {
>> __u32 ifindex;
>> } xdp;
>> + struct {
>> + __u32 map_id;
>> + } struct_ops;
>> };
>> } __attribute__((aligned(8)));
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 9e097fcc9cf4..5a7e86cf67b5 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -16,6 +16,7 @@ enum bpf_struct_ops_state {
>> BPF_STRUCT_OPS_STATE_INIT,
>> BPF_STRUCT_OPS_STATE_INUSE,
>> BPF_STRUCT_OPS_STATE_TOBEFREE,
>> + BPF_STRUCT_OPS_STATE_READY,
>> };
>> #define BPF_STRUCT_OPS_COMMON_VALUE \
>> @@ -58,6 +59,11 @@ struct bpf_struct_ops_map {
>> struct bpf_struct_ops_value kvalue;
>> };
>> +struct bpf_struct_ops_link {
>> + struct bpf_link link;
>> + struct bpf_map __rcu *map;
>> +};
>> +
>> static DEFINE_MUTEX(update_mutex);
>> #define VALUE_PREFIX "bpf_struct_ops_"
>> @@ -496,11 +502,24 @@ static int bpf_struct_ops_map_update_elem(struct
>> bpf_map *map, void *key,
>> *(unsigned long *)(udata + moff) = prog->aux->id;
>> }
>> - bpf_map_inc(map);
>> -
>> set_memory_rox((long)st_map->image, 1);
>> + if (st_map->map.map_flags & BPF_F_LINK) {
>> + if (st_ops->validate) {
>> + err = st_ops->validate(kdata);
>> + if (err)
>> + goto unlock;
>
> This should at least be 'goto reset_unlock' to release the progs.
>
> set_memory_rox(..., 1) should also be done after validate?
Yes, it should go to reset_unlock. In that case, set_memory_rox()
should be called after the check.
>
>> + }
>> + /* Let bpf_link handle registration & unregistration.
>> + *
>> + * Pair with smp_load_acquire() during lookup_elem().
>> + */
>> + smp_store_release(&kvalue->state, BPF_STRUCT_OPS_STATE_READY);
>> + goto unlock;
>> + }
>> +
>> err = st_ops->reg(kdata);
>> if (likely(!err)) {
>> + bpf_map_inc(map);
>> /* Pair with smp_load_acquire() during lookup_elem().
>> * It ensures the above udata updates (e.g. prog->aux->id)
>> * can be seen once BPF_STRUCT_OPS_STATE_INUSE is set.
>> @@ -516,7 +535,6 @@ static int bpf_struct_ops_map_update_elem(struct
>> bpf_map *map, void *key,
>> */
>> set_memory_nx((long)st_map->image, 1);
>> set_memory_rw((long)st_map->image, 1);
>> - bpf_map_put(map);
>> reset_unlock:
>> bpf_struct_ops_map_put_progs(st_map);
>> @@ -534,6 +552,9 @@ static int bpf_struct_ops_map_delete_elem(struct
>> bpf_map *map, void *key)
>> struct bpf_struct_ops_map *st_map;
>> st_map = (struct bpf_struct_ops_map *)map;
>> + if (st_map->map.map_flags & BPF_F_LINK)
>> + return -EOPNOTSUPP;
>> +
>> prev_state = cmpxchg(&st_map->kvalue.state,
>> BPF_STRUCT_OPS_STATE_INUSE,
>> BPF_STRUCT_OPS_STATE_TOBEFREE);
>> @@ -601,7 +622,7 @@ static void bpf_struct_ops_map_free(struct bpf_map
>> *map)
>> static int bpf_struct_ops_map_alloc_check(union bpf_attr *attr)
>> {
>> if (attr->key_size != sizeof(unsigned int) || attr->max_entries
>> != 1 ||
>> - attr->map_flags || !attr->btf_vmlinux_value_type_id)
>> + (attr->map_flags & ~BPF_F_LINK) ||
>> !attr->btf_vmlinux_value_type_id)
>> return -EINVAL;
>> return 0;
>> }
>> @@ -712,3 +733,98 @@ void bpf_struct_ops_put(const void *kdata)
>> bpf_map_put(&st_map->map);
>> }
>> +
>> +static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
>> +{
>> + struct bpf_struct_ops_link *st_link;
>> + struct bpf_struct_ops_map *st_map;
>> +
>> + st_link = container_of(link, struct bpf_struct_ops_link, link);
>> + st_map = (struct bpf_struct_ops_map *)st_link->map;
>
> /* protected by refcnt and no one is replacing it */
> rcu_dereference_protected(st_link->map, true);
>
> st_link->map is with __rcu. It should have warning when compile with
> 'make C=1 ...'. Patchwork also reports this:
> https://patchwork.kernel.org/project/netdevbpf/patch/20230308005050.255859-4-kuifeng@meta.com/. Please pay attention to patchwork for errors.
>
>> + st_map->st_ops->unreg(&st_map->kvalue.data);
>> + bpf_map_put(st_link->map);
>
> Same here. Reading __rcu pointer without rcu_dereference_xxx.
According to the discussion offline, rcu_dereference_protected() will be
called.
>
> or simply use &st_map->map here. Otherwise, it will also have type
> mismatch warning.
>
>> + kfree(st_link);
>> +}
>> +
>> +static void bpf_struct_ops_map_link_show_fdinfo(const struct bpf_link
>> *link,
>> + struct seq_file *seq)
>> +{
>> + struct bpf_struct_ops_link *st_link;
>> + struct bpf_map *map;
>> +
>> + st_link = container_of(link, struct bpf_struct_ops_link, link);
>> + rcu_read_lock();
>> + map = rcu_dereference(st_link->map);
>> + if (map)
>
> map cannot be NULL?
It should not be now after removing detach feature.
>
>> + seq_printf(seq, "map_id:\t%d\n", map->id);
>> + rcu_read_unlock();
>> +}
>> +
>> +static int bpf_struct_ops_map_link_fill_link_info(const struct
>> bpf_link *link,
>> + struct bpf_link_info *info)
>> +{
>> + struct bpf_struct_ops_link *st_link;
>> + struct bpf_map *map;
>> +
>> + st_link = container_of(link, struct bpf_struct_ops_link, link);
>> + rcu_read_lock();
>> + map = rcu_dereference(st_link->map);
>> + if (map)
>
> Same here.
Ack.
>
>> + info->struct_ops.map_id = map->id;
>> + rcu_read_unlock();
>> + return 0;
>> +}
>> +
>> +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,
>> +};
>> +
>> +int bpf_struct_ops_link_create(union bpf_attr *attr)
>> +{
>> + struct bpf_struct_ops_link *link = NULL;
>> + struct bpf_link_primer link_primer;
>> + struct bpf_struct_ops_map *st_map;
>> + struct bpf_map *map;
>> + int err;
>> +
>> + map = bpf_map_get(attr->link_create.map_fd);
>> + if (!map)
>> + return -EINVAL;
>> +
>> + st_map = (struct bpf_struct_ops_map *)map;
>> +
>> + if (map->map_type != BPF_MAP_TYPE_STRUCT_OPS || !(map->map_flags
>> & BPF_F_LINK) ||
>> + /* Pair with smp_store_release() during map_update */
>> + smp_load_acquire(&st_map->kvalue.state) !=
>> BPF_STRUCT_OPS_STATE_READY) {
>> + err = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + link = kzalloc(sizeof(*link), GFP_USER);
>> + if (!link) {
>> + err = -ENOMEM;
>> + goto err_out;
>> + }
>> + bpf_link_init(&link->link, BPF_LINK_TYPE_STRUCT_OPS,
>> &bpf_struct_ops_map_lops, NULL);
>> + RCU_INIT_POINTER(link->map, map);
>> +
>> + err = bpf_link_prime(&link->link, &link_primer);
>> + if (err)
>> + goto err_out;
>> +
>> + err = st_map->st_ops->reg(st_map->kvalue.data);
>> + if (err) {
>> + bpf_link_cleanup(&link_primer);
>> + goto err_out;
>> + }
>> +
>> + return bpf_link_settle(&link_primer);
>> +
>> +err_out:
>> + bpf_map_put(map);
>> + kfree(link);
>> + return err;
>> +}
>> +
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops().
2023-03-08 21:42 ` Martin KaFai Lau
@ 2023-03-09 0:22 ` Kui-Feng Lee
2023-03-09 17:09 ` Martin KaFai Lau
0 siblings, 1 reply; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-09 0:22 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/8/23 13:42, Martin KaFai Lau wrote:
> On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
>> @@ -11566,22 +11591,34 @@ struct bpf_link *bpf_program__attach(const
>> struct bpf_program *prog)
>> return link;
>> }
>> +struct bpf_link_struct_ops {
>> + struct bpf_link link;
>> + int map_fd;
>> +};
>> +
>> static int bpf_link__detach_struct_ops(struct bpf_link *link)
>> {
>> + struct bpf_link_struct_ops *st_link;
>> __u32 zero = 0;
>> - if (bpf_map_delete_elem(link->fd, &zero))
>> - return -errno;
>> + st_link = container_of(link, struct bpf_link_struct_ops, link);
>> - return 0;
>> + if (st_link->map_fd < 0) {
>
> map_fd < 0 should always be true?
If the user pass a wrong link, it can fail.
I check it here explicitly even the kernel returns
an error for deleting an element of a struct_ops w/ link.
>
>> + /* Fake bpf_link */
>> + if (bpf_map_delete_elem(link->fd, &zero))
>> + return -errno;
>> + return 0;
>> + }
>> +
>> + /* Doesn't support detaching. */
>> + return -EOPNOTSUPP;
>> }
>> struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
>> {
>> - struct bpf_struct_ops *st_ops;
>> - struct bpf_link *link;
>> - __u32 i, zero = 0;
>> - int err;
>> + struct bpf_link_struct_ops *link;
>> + __u32 zero = 0;
>> + int err, fd;
>> if (!bpf_map__is_struct_ops(map) || map->fd == -1)
>> return libbpf_err_ptr(-EINVAL);
>> @@ -11590,31 +11627,34 @@ struct bpf_link
>> *bpf_map__attach_struct_ops(const struct bpf_map *map)
>> if (!link)
>> return libbpf_err_ptr(-EINVAL);
>> - st_ops = map->st_ops;
>> - for (i = 0; i < btf_vlen(st_ops->type); i++) {
>> - struct bpf_program *prog = st_ops->progs[i];
>> - void *kern_data;
>> - int prog_fd;
>> + /* kern_vdata should be prepared during the loading phase. */
>> + err = bpf_map_update_elem(map->fd, &zero,
>> map->st_ops->kern_vdata, 0);
>> + if (err) {
>> + err = -errno;
>> + free(link);
>> + return libbpf_err_ptr(err);
>> + }
>> - if (!prog)
>> - continue;
>> - prog_fd = bpf_program__fd(prog);
>> - kern_data = st_ops->kern_vdata + st_ops->kern_func_off[i];
>> - *(unsigned long *)kern_data = prog_fd;
>> + if (!(map->def.map_flags & BPF_F_LINK)) {
>> + /* Fake bpf_link */
>> + link->link.fd = map->fd;
>> + link->map_fd = -1;
>> + link->link.detach = bpf_link__detach_struct_ops;
>> + return &link->link;
>> }
>> - err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0);
>> - if (err) {
>> + fd = bpf_link_create(map->fd, -1, BPF_STRUCT_OPS, NULL);
>> + if (fd < 0) {
>> err = -errno;
>> free(link);
>> return libbpf_err_ptr(err);
>> }
>> - link->detach = bpf_link__detach_struct_ops;
>> - link->fd = map->fd;
>> + link->link.fd = fd;
>> + link->map_fd = map->fd;
>
> Does it need to set link->link.detach?
Yes, I have made some changes to this part. The new code will set
link->link.detach for BPF_F_LINK as well to cleanup fd.
>
>> - return link;
>> + return &link->link;
>> }
>> typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct
>> perf_event_header *hdr,
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 5/8] bpf: Update the struct_ops of a bpf_link.
2023-03-08 22:21 ` Martin KaFai Lau
@ 2023-03-09 3:09 ` Kui-Feng Lee
0 siblings, 0 replies; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-09 3:09 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/8/23 14:21, Martin KaFai Lau wrote:
> On 3/7/23 4:50 PM, Kui-Feng Lee 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 | 1 +
>> include/uapi/linux/bpf.h | 8 ++++--
>> kernel/bpf/bpf_struct_ops.c | 46 ++++++++++++++++++++++++++++++++++
>> kernel/bpf/syscall.c | 43 ++++++++++++++++++++++++++++---
>> tools/include/uapi/linux/bpf.h | 7 +++++-
>> 5 files changed, 98 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index afca6c526fe4..29d555a82bad 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1470,6 +1470,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 {
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index f9fc7b8af3c4..edef9cf7d596 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 5a7e86cf67b5..79e663869e51 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -775,10 +775,56 @@ 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_value *kvalue;
>> + 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;
>> +
>> + if (new_map->map_type != BPF_MAP_TYPE_STRUCT_OPS ||
>> + !(new_map->map_flags & BPF_F_LINK))
>> + return -EINVAL;
>> +
>> + mutex_lock(&update_mutex);
>> +
>> + st_link = container_of(link, struct bpf_struct_ops_link, link);
>> +
>> + /* The new and old struct_ops must be the same type. */
>> + st_map = container_of(new_map, struct bpf_struct_ops_map, map);
>
> nit. move the st_link and st_map init out of the lock.
>
Ok
>> +
>> + old_map = st_link->map;
>
> rcu_dereference_protected(...)
>
>> + old_st_map = container_of(old_map, struct bpf_struct_ops_map, map);
>> + if (st_map->st_ops != old_st_map->st_ops ||
>> + /* Pair with smp_store_release() during map_update */
>> + smp_load_acquire(&st_map->kvalue.state) !=
>> BPF_STRUCT_OPS_STATE_READY) {
>
> nit. test the smp_load_acquire(&st_map...) outside of the lock.
> Do it together with the new_map checking at the beginning of the func.
Ok
>
>> + err = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + kvalue = &st_map->kvalue;
>> +
>> + err = st_map->st_ops->update(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 3a4503987a48..c087dd2e2c08 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -4658,6 +4658,30 @@ 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;
>> +
>> + if (new_map->map_type != BPF_MAP_TYPE_STRUCT_OPS) {
>
> This is unnecessary test. The individual '.update_map()' should test for
> its own map_type and the new bpf_struct_ops_map_link_update() does test it.
>
Will remove it.
>> + ret = -EINVAL;
>> + goto out_put_map;
>> + }
>> +
>> + if (link->ops->update_map)
>
> This has just been tested in link_update() before calling
> link_update_map().
Sure
>
>> + ret = link->ops->update_map(link, new_map);
>> + else
>> + ret = -EINVAL;
>> +
>> +out_put_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)
>> @@ -4670,14 +4694,25 @@ static int link_update(union bpf_attr *attr)
>> if (CHECK_ATTR(BPF_LINK_UPDATE))
>> return -EINVAL;
>> - flags = attr->link_update.flags;
>> - if (flags & ~BPF_F_REPLACE)
>> - return -EINVAL;
>> -
>> link = bpf_link_get_from_fd(attr->link_update.link_fd);
>> if (IS_ERR(link))
>> return PTR_ERR(link);
>> + flags = attr->link_update.flags;
>> +
>> + if (link->ops->update_map) {
>> + if (flags) /* always replace the existing one */
>> + ret = -EINVAL;
>> + else
>> + ret = link_update_map(link, attr);
>> + goto out_put_link;
>> + }
>> +
>> + if (flags & ~BPF_F_REPLACE) {
>> + ret = -EINVAL;
>> + 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);
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 8/8] selftests/bpf: Test switching TCP Congestion Control algorithms.
2023-03-08 23:10 ` Martin KaFai Lau
@ 2023-03-09 3:34 ` Kui-Feng Lee
0 siblings, 0 replies; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-09 3:34 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/8/23 15:10, Martin KaFai Lau wrote:
> On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
>> Create a pair of sockets that utilize the congestion control algorithm
>> under a particular name. Then switch up this congestion control
>> algorithm to another implementation and check whether newly created
>> connections using the same cc name now run the new implementation.
>>
>> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
>> ---
>> .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 38 ++++++++++++
>> .../selftests/bpf/progs/tcp_ca_update.c | 62 +++++++++++++++++++
>> 2 files changed, 100 insertions(+)
>> create mode 100644 tools/testing/selftests/bpf/progs/tcp_ca_update.c
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
>> b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
>> index e980188d4124..caaa9175ee36 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
>> @@ -8,6 +8,7 @@
>> #include "bpf_dctcp.skel.h"
>> #include "bpf_cubic.skel.h"
>> #include "bpf_tcp_nogpl.skel.h"
>> +#include "tcp_ca_update.skel.h"
>> #include "bpf_dctcp_release.skel.h"
>> #include "tcp_ca_write_sk_pacing.skel.h"
>> #include "tcp_ca_incompl_cong_ops.skel.h"
>> @@ -381,6 +382,41 @@ static void test_unsupp_cong_op(void)
>> libbpf_set_print(old_print_fn);
>> }
>> +static void test_update_ca(void)
>> +{
>> + struct tcp_ca_update *skel;
>> + struct bpf_link *link;
>> + int saved_ca1_cnt;
>> + int err;
>> +
>> + skel = tcp_ca_update__open();
>> + if (!ASSERT_OK_PTR(skel, "open"))
>> + return;
>> +
>> + err = tcp_ca_update__load(skel);
>
> nit. Use tcp_ca_update__open_and_load().
>
>> + if (!ASSERT_OK(err, "load")) {
>> + tcp_ca_update__destroy(skel);
>> + return;
>> + }
>> +
>> + link = bpf_map__attach_struct_ops(skel->maps.ca_update_1);
>> + ASSERT_OK_PTR(link, "attach_struct_ops");
>> +
>> + do_test("tcp_ca_update", NULL);
>> + saved_ca1_cnt = skel->bss->ca1_cnt;
>> + ASSERT_GT(saved_ca1_cnt, 0, "ca1_ca1_cnt");
>> +
>> + err = bpf_link__update_map(link, skel->maps.ca_update_2);
>> + ASSERT_OK(err, "update_struct_ops");
>> +
>> + do_test("tcp_ca_update", NULL);
>> + ASSERT_EQ(skel->bss->ca1_cnt, saved_ca1_cnt, "ca2_ca1_cnt");
>> + ASSERT_GT(skel->bss->ca2_cnt, 0, "ca2_ca2_cnt");
>> +
>> + bpf_link__destroy(link);
>> + tcp_ca_update__destroy(skel);
>> +}
>> +
>> void test_bpf_tcp_ca(void)
>> {
>> if (test__start_subtest("dctcp"))
>> @@ -399,4 +435,6 @@ void test_bpf_tcp_ca(void)
>> test_incompl_cong_ops();
>> if (test__start_subtest("unsupp_cong_op"))
>> test_unsupp_cong_op();
>> + if (test__start_subtest("update_ca"))
>> + test_update_ca();
>> }
>> diff --git a/tools/testing/selftests/bpf/progs/tcp_ca_update.c
>> b/tools/testing/selftests/bpf/progs/tcp_ca_update.c
>> new file mode 100644
>> index 000000000000..36a04be95df5
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/tcp_ca_update.c
>> @@ -0,0 +1,62 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include "vmlinux.h"
>> +
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +int ca1_cnt = 0;
>> +int ca2_cnt = 0;
>> +
>> +#define USEC_PER_SEC 1000000UL
>
> Not used.
>
>> +
>> +#define min(a, b) ((a) < (b) ? (a) : (b))
>
> Not used.
>
>> +
>> +static inline struct tcp_sock *tcp_sk(const struct sock *sk)
>> +{
>> + return (struct tcp_sock *)sk;
>> +}
>> +
>> +SEC("struct_ops/ca_update_1_cong_control")
>> +void BPF_PROG(ca_update_1_cong_control, struct sock *sk,
>> + const struct rate_sample *rs)
>> +{
>> + ca1_cnt++;
>> +}
>> +
>> +SEC("struct_ops/ca_update_2_cong_control")
>> +void BPF_PROG(ca_update_2_cong_control, struct sock *sk,
>> + const struct rate_sample *rs)
>> +{
>> + ca2_cnt++;
>> +}
>> +
>> +SEC("struct_ops/ca_update_ssthresh")
>> +__u32 BPF_PROG(ca_update_ssthresh, struct sock *sk)
>> +{
>> + return tcp_sk(sk)->snd_ssthresh;
>> +}
>> +
>> +SEC("struct_ops/ca_update_undo_cwnd")
>> +__u32 BPF_PROG(ca_update_undo_cwnd, struct sock *sk)
>> +{
>> + return tcp_sk(sk)->snd_cwnd;
>> +}
>> +
>> +SEC(".struct_ops.link")
>> +struct tcp_congestion_ops ca_update_1 = {
>> + .cong_control = (void *)ca_update_1_cong_control,
>> + .ssthresh = (void *)ca_update_ssthresh,
>> + .undo_cwnd = (void *)ca_update_undo_cwnd,
>> + .name = "tcp_ca_update",
>> +};
>> +
>> +SEC(".struct_ops.link")
>> +struct tcp_congestion_ops ca_update_2 = {
>> + .cong_control = (void *)ca_update_2_cong_control,
>
> nit. I think it is more future proof to use '.init' to bump the ca1_cnt
> and ca2_cnt. '.init' must be called. Just in case for some unlikely
> stack change that may not call cong_control and then need to adjust the
> test accordingly.
Got it!
>
> It also needs a few negative tests like creating a link with a map
> without BPF_F_LINK. delete_elem on BPF_F_LINK map. Replace a link with a
> different tcp-cc-name which is not supported now, ...etc.
I have added a few more today. Replaceing a link with a different name
is something missing. Thanks!
>
>> + .ssthresh = (void *)ca_update_ssthresh,
>> + .undo_cwnd = (void *)ca_update_undo_cwnd,
>> + .name = "tcp_ca_update",
>> +};
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops().
2023-03-09 0:22 ` Kui-Feng Lee
@ 2023-03-09 17:09 ` Martin KaFai Lau
2023-03-09 18:16 ` Kui-Feng Lee
0 siblings, 1 reply; 28+ messages in thread
From: Martin KaFai Lau @ 2023-03-09 17:09 UTC (permalink / raw)
To: Kui-Feng Lee, Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/8/23 4:22 PM, Kui-Feng Lee wrote:
>
>
> On 3/8/23 13:42, Martin KaFai Lau wrote:
>> On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
>>> @@ -11566,22 +11591,34 @@ struct bpf_link *bpf_program__attach(const struct
>>> bpf_program *prog)
>>> return link;
>>> }
>>> +struct bpf_link_struct_ops {
>>> + struct bpf_link link;
>>> + int map_fd;
>>> +};
>>> +
>>> static int bpf_link__detach_struct_ops(struct bpf_link *link)
>>> {
>>> + struct bpf_link_struct_ops *st_link;
>>> __u32 zero = 0;
>>> - if (bpf_map_delete_elem(link->fd, &zero))
>>> - return -errno;
>>> + st_link = container_of(link, struct bpf_link_struct_ops, link);
>>> - return 0;
>>> + if (st_link->map_fd < 0) {
>>
>> map_fd < 0 should always be true?
>
> If the user pass a wrong link, it can fail.
I may have missed something. How can user directly pass a link to this static
function?
> I check it here explicitly even the kernel returns
> an error for deleting an element of a struct_ops w/ link.
Yep, the kernel should have stopped the delete if the user somehow corrupted the
map_fd to -1.
>
>>
>>> + /* Fake bpf_link */
>>> + if (bpf_map_delete_elem(link->fd, &zero))
>>> + return -errno;
>>> + return 0;
>>> + }
>>> +
>>> + /* Doesn't support detaching. */
>>> + return -EOPNOTSUPP;
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops().
2023-03-09 17:09 ` Martin KaFai Lau
@ 2023-03-09 18:16 ` Kui-Feng Lee
2023-03-09 18:19 ` Kui-Feng Lee
0 siblings, 1 reply; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-09 18:16 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/9/23 09:09, Martin KaFai Lau wrote:
> On 3/8/23 4:22 PM, Kui-Feng Lee wrote:
>>
>>
>> On 3/8/23 13:42, Martin KaFai Lau wrote:
>>> On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
>>>> @@ -11566,22 +11591,34 @@ struct bpf_link *bpf_program__attach(const
>>>> struct bpf_program *prog)
>>>> return link;
>>>> }
>>>> +struct bpf_link_struct_ops {
>>>> + struct bpf_link link;
>>>> + int map_fd;
>>>> +};
>>>> +
>>>> static int bpf_link__detach_struct_ops(struct bpf_link *link)
>>>> {
>>>> + struct bpf_link_struct_ops *st_link;
>>>> __u32 zero = 0;
>>>> - if (bpf_map_delete_elem(link->fd, &zero))
>>>> - return -errno;
>>>> + st_link = container_of(link, struct bpf_link_struct_ops, link);
>>>> - return 0;
>>>> + if (st_link->map_fd < 0) {
>>>
>>> map_fd < 0 should always be true?
>>
>> If the user pass a wrong link, it can fail.
>
> I may have missed something. How can user directly pass a link to this
> static function?
Ouch! You are right. This check is not necessary. I mixed it with the
old detach feature.
>
>> I check it here explicitly even the kernel returns
>> an error for deleting an element of a struct_ops w/ link.
> Yep, the kernel should have stopped the delete if the user somehow
> corrupted the map_fd to -1.
>
>>
>>>
>>>> + /* Fake bpf_link */
>>>> + if (bpf_map_delete_elem(link->fd, &zero))
>>>> + return -errno;
>>>> + return 0;
>>>> + }
>>>> +
>>>> + /* Doesn't support detaching. */
>>>> + return -EOPNOTSUPP;
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops().
2023-03-09 18:16 ` Kui-Feng Lee
@ 2023-03-09 18:19 ` Kui-Feng Lee
0 siblings, 0 replies; 28+ messages in thread
From: Kui-Feng Lee @ 2023-03-09 18:19 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sdf
On 3/9/23 10:16, Kui-Feng Lee wrote:
>
>
> On 3/9/23 09:09, Martin KaFai Lau wrote:
>> On 3/8/23 4:22 PM, Kui-Feng Lee wrote:
>>>
>>>
>>> On 3/8/23 13:42, Martin KaFai Lau wrote:
>>>> On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
>>>>> @@ -11566,22 +11591,34 @@ struct bpf_link
>>>>> *bpf_program__attach(const struct bpf_program *prog)
>>>>> return link;
>>>>> }
>>>>> +struct bpf_link_struct_ops {
>>>>> + struct bpf_link link;
>>>>> + int map_fd;
>>>>> +};
>>>>> +
>>>>> static int bpf_link__detach_struct_ops(struct bpf_link *link)
>>>>> {
>>>>> + struct bpf_link_struct_ops *st_link;
>>>>> __u32 zero = 0;
>>>>> - if (bpf_map_delete_elem(link->fd, &zero))
>>>>> - return -errno;
>>>>> + st_link = container_of(link, struct bpf_link_struct_ops, link);
>>>>> - return 0;
>>>>> + if (st_link->map_fd < 0) {
>>>>
>>>> map_fd < 0 should always be true?
>>>
>>> If the user pass a wrong link, it can fail.
>>
>> I may have missed something. How can user directly pass a link to this
>> static function?
>
> Ouch! You are right. This check is not necessary. I mixed it with the
> old detach feature.
By the way, I will keep this test here since this function will handle
the case w/o a link as well.
>
>
>>
>>> I check it here explicitly even the kernel returns
>>> an error for deleting an element of a struct_ops w/ link.
>> Yep, the kernel should have stopped the delete if the user somehow
>> corrupted the map_fd to -1.
>>
>>>
>>>>
>>>>> + /* Fake bpf_link */
>>>>> + if (bpf_map_delete_elem(link->fd, &zero))
>>>>> + return -errno;
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + /* Doesn't support detaching. */
>>>>> + return -EOPNOTSUPP;
>>
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2023-03-09 18:20 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-08 0:50 [PATCH bpf-next v5 0/8] Transit between BPF TCP congestion controls Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 1/8] bpf: Retire the struct_ops map kvalue->refcnt Kui-Feng Lee
2023-03-08 18:30 ` Martin KaFai Lau
2023-03-08 22:30 ` Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 2/8] net: Update an existing TCP congestion control algorithm Kui-Feng Lee
2023-03-08 18:59 ` Martin KaFai Lau
2023-03-08 22:51 ` Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps Kui-Feng Lee
2023-03-08 15:01 ` kernel test robot
2023-03-08 15:32 ` kernel test robot
2023-03-08 16:03 ` kernel test robot
2023-03-08 20:04 ` Martin KaFai Lau
2023-03-08 23:46 ` Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops() Kui-Feng Lee
2023-03-08 21:42 ` Martin KaFai Lau
2023-03-09 0:22 ` Kui-Feng Lee
2023-03-09 17:09 ` Martin KaFai Lau
2023-03-09 18:16 ` Kui-Feng Lee
2023-03-09 18:19 ` Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 5/8] bpf: Update the struct_ops of a bpf_link Kui-Feng Lee
2023-03-08 22:21 ` Martin KaFai Lau
2023-03-09 3:09 ` Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 6/8] libbpf: Update a bpf_link with another struct_ops Kui-Feng Lee
2023-03-08 22:32 ` Martin KaFai Lau
2023-03-08 0:50 ` [PATCH bpf-next v5 7/8] libbpf: Use .struct_ops.link section to indicate a struct_ops with a link Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 8/8] selftests/bpf: Test switching TCP Congestion Control algorithms Kui-Feng Lee
2023-03-08 23:10 ` Martin KaFai Lau
2023-03-09 3:34 ` Kui-Feng Lee
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox