From: Markus Elfring <[email protected]>
Date: Sat, 30 Dec 2023 20:51:23 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (5):
Improve exception handling in bpf_struct_ops_link_create()
Move an assignment for the variable “st_map”
in bpf_struct_ops_link_create()
Improve exception handling in bpf_core_apply()
Return directly after a failed bpf_map_kmalloc_node()
in bpf_cgroup_storage_alloc()
Improve exception handling in trie_update_elem()
kernel/bpf/bpf_struct_ops.c | 12 ++++++------
kernel/bpf/btf.c | 8 +++++---
kernel/bpf/local_storage.c | 2 +-
kernel/bpf/lpm_trie.c | 24 +++++++++++-------------
4 files changed, 23 insertions(+), 23 deletions(-)
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Sat, 30 Dec 2023 18:50:45 +0100
The kfree() function was called in two cases by
the bpf_struct_ops_link_create() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.
* Thus use another label.
* Reorder function calls at the end.
* Delete an initialisation (for the variable “link”)
which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring <[email protected]>
---
kernel/bpf/bpf_struct_ops.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 02068bd0e4d9..b49ea460d616 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -888,7 +888,7 @@ static const struct bpf_link_ops bpf_struct_ops_map_lops = {
int bpf_struct_ops_link_create(union bpf_attr *attr)
{
- struct bpf_struct_ops_link *link = NULL;
+ struct bpf_struct_ops_link *link;
struct bpf_link_primer link_primer;
struct bpf_struct_ops_map *st_map;
struct bpf_map *map;
@@ -902,13 +902,13 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
if (!bpf_struct_ops_valid_to_reg(map)) {
err = -EINVAL;
- goto err_out;
+ goto put_map;
}
link = kzalloc(sizeof(*link), GFP_USER);
if (!link) {
err = -ENOMEM;
- goto err_out;
+ goto put_map;
}
bpf_link_init(&link->link, BPF_LINK_TYPE_STRUCT_OPS, &bpf_struct_ops_map_lops, NULL);
@@ -927,7 +927,8 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
return bpf_link_settle(&link_primer);
err_out:
- bpf_map_put(map);
kfree(link);
+put_map:
+ bpf_map_put(map);
return err;
}
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Sat, 30 Dec 2023 19:00:12 +0100
Move one assignment for the variable “st_map” closer to the place
where this pointer is used.
Signed-off-by: Markus Elfring <[email protected]>
---
kernel/bpf/bpf_struct_ops.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index b49ea460d616..4133d65c2a28 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -898,8 +898,6 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
- st_map = (struct bpf_struct_ops_map *)map;
-
if (!bpf_struct_ops_valid_to_reg(map)) {
err = -EINVAL;
goto put_map;
@@ -916,6 +914,7 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
if (err)
goto err_out;
+ st_map = (struct bpf_struct_ops_map *)map;
err = st_map->st_ops->reg(st_map->kvalue.data);
if (err) {
bpf_link_cleanup(&link_primer);
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Sat, 30 Dec 2023 19:28:25 +0100
The kfree() function was called in two cases by
the bpf_core_apply() function during error handling
even if the passed data structure member contained a null pointer.
This issue was detected by using the Coccinelle software.
* Thus use another label.
* Reorder function calls at the end.
Signed-off-by: Markus Elfring <[email protected]>
---
kernel/bpf/btf.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 51e8b4bee0c8..e8391025d408 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -8322,13 +8322,13 @@ int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
bpf_log(ctx->log, "target candidate search failed for %d\n",
relo->type_id);
err = PTR_ERR(cc);
- goto out;
+ goto unlock_mutex;
}
if (cc->cnt) {
cands.cands = kcalloc(cc->cnt, sizeof(*cands.cands), GFP_KERNEL);
if (!cands.cands) {
err = -ENOMEM;
- goto out;
+ goto unlock_mutex;
}
}
for (i = 0; i < cc->cnt; i++) {
@@ -8355,13 +8355,15 @@ int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
&targ_res);
out:
- kfree(specs);
if (need_cands) {
kfree(cands.cands);
+unlock_mutex:
mutex_unlock(&cand_cache_mutex);
if (ctx->log->level & BPF_LOG_LEVEL2)
print_cand_cache(ctx->log);
}
+
+ kfree(specs);
return err;
}
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Sat, 30 Dec 2023 20:06:02 +0100
The kfree() function was called in one case by
the bpf_cgroup_storage_alloc() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.
Thus return directly after a call of the function “bpf_map_kmalloc_node”
failed at the beginning.
Signed-off-by: Markus Elfring <[email protected]>
---
kernel/bpf/local_storage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
index a04f505aefe9..e16a80c93cd7 100644
--- a/kernel/bpf/local_storage.c
+++ b/kernel/bpf/local_storage.c
@@ -514,7 +514,7 @@ struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(struct bpf_prog *prog,
storage = bpf_map_kmalloc_node(map, sizeof(struct bpf_cgroup_storage),
gfp, map->numa_node);
if (!storage)
- goto enomem;
+ return ERR_PTR(-ENOMEM);
if (stype == BPF_CGROUP_STORAGE_SHARED) {
storage->buf = bpf_map_kmalloc_node(map, size, gfp,
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Sat, 30 Dec 2023 20:28:11 +0100
The kfree() function was called in some cases by
the trie_update_elem() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.
* Thus adjust jump targets.
* Reorder data processing steps at the end.
* Delete an initialisation (for the variable “new_node”)
and a repeated pointer check which became unnecessary
with this refactoring.
Signed-off-by: Markus Elfring <[email protected]>
---
kernel/bpf/lpm_trie.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
index b32be680da6c..6c372d831d0f 100644
--- a/kernel/bpf/lpm_trie.c
+++ b/kernel/bpf/lpm_trie.c
@@ -307,7 +307,7 @@ static long trie_update_elem(struct bpf_map *map,
void *_key, void *value, u64 flags)
{
struct lpm_trie *trie = container_of(map, struct lpm_trie, map);
- struct lpm_trie_node *node, *im_node = NULL, *new_node = NULL;
+ struct lpm_trie_node *node, *im_node = NULL, *new_node;
struct lpm_trie_node __rcu **slot;
struct bpf_lpm_trie_key *key = _key;
unsigned long irq_flags;
@@ -327,13 +327,13 @@ static long trie_update_elem(struct bpf_map *map,
if (trie->n_entries == trie->map.max_entries) {
ret = -ENOSPC;
- goto out;
+ goto unlock;
}
new_node = lpm_trie_node_alloc(trie, value);
if (!new_node) {
ret = -ENOMEM;
- goto out;
+ goto unlock;
}
trie->n_entries++;
@@ -368,7 +368,7 @@ static long trie_update_elem(struct bpf_map *map,
*/
if (!node) {
rcu_assign_pointer(*slot, new_node);
- goto out;
+ goto decrement_counter;
}
/* If the slot we picked already exists, replace it with @new_node
@@ -384,7 +384,7 @@ static long trie_update_elem(struct bpf_map *map,
rcu_assign_pointer(*slot, new_node);
kfree_rcu(node, rcu);
- goto out;
+ goto decrement_counter;
}
/* If the new node matches the prefix completely, it must be inserted
@@ -394,13 +394,13 @@ static long trie_update_elem(struct bpf_map *map,
next_bit = extract_bit(node->data, matchlen);
rcu_assign_pointer(new_node->child[next_bit], node);
rcu_assign_pointer(*slot, new_node);
- goto out;
+ goto decrement_counter;
}
im_node = lpm_trie_node_alloc(trie, NULL);
if (!im_node) {
ret = -ENOMEM;
- goto out;
+ goto decrement_counter;
}
im_node->prefixlen = matchlen;
@@ -419,15 +419,13 @@ static long trie_update_elem(struct bpf_map *map,
/* Finally, assign the intermediate node to the determined slot */
rcu_assign_pointer(*slot, im_node);
-out:
if (ret) {
- if (new_node)
- trie->n_entries--;
-
- kfree(new_node);
kfree(im_node);
+decrement_counter:
+ trie->n_entries--;
+ kfree(new_node);
}
-
+unlock:
spin_unlock_irqrestore(&trie->lock, irq_flags);
return ret;
--
2.43.0
On Sat, Dec 30, 2023 at 12:04 PM Markus Elfring <[email protected]> wrote:
>
> From: Markus Elfring <[email protected]>
> Date: Sat, 30 Dec 2023 20:51:23 +0100
>
> A few update suggestions were taken into account
> from static source code analysis.
Auto Nack.
Pls don't send such patches. You were told multiple
times that such kfree usage is fine.
>> A few update suggestions were taken into account
>> from static source code analysis.
>
> Auto Nack.
> Pls don't send such patches. You were told multiple
> times that such kfree usage is fine.
Some implementation details are improvable.
Can you find an update step (like the following) helpful?
[PATCH 2/5] bpf: Move an assignment for the variable “st_map” in bpf_struct_ops_link_create()
https://lore.kernel.org/bpf/[email protected]/
Regards,
Markus
On Mon, Jan 1, 2024 at 1:10 AM Markus Elfring <[email protected]> wrote:
>
> >> A few update suggestions were taken into account
> >> from static source code analysis.
> >
> > Auto Nack.
> > Pls don't send such patches. You were told multiple
> > times that such kfree usage is fine.
>
> Some implementation details are improvable.
> Can you find an update step (like the following) helpful?
>
> [PATCH 2/5] bpf: Move an assignment for the variable “st_map” in bpf_struct_ops_link_create()
> https://lore.kernel.org/bpf/[email protected]/
This change is not helpful at all. The use of "st_map" in current code as-is
doesn't cause any confusion, i.e., it is always struct bpf_struct_ops_map *.
OTOH, this patch will make it harder for folks who use git-blame. Therefore,
it adds negative value to the code base.
Thanks,
Song