[PATCH 0/5] bpf: Adjustments for four function implementations

[PATCH 0/5] bpf: Adjustments for four function implementations

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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

[PATCH 1/5] bpf: Improve exception handling in bpf_struct_ops_link_create()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[PATCH 2/5] bpf: Move an assignment for the variable “st_map” in bpf_struct_ops_link_create()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[PATCH 3/5] bpf: Improve exception handling in bpf_core_apply()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[PATCH 4/5] bpf: Return directly after a failed bpf_map_kmalloc_node() in bpf_cgroup_storage_alloc()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[PATCH 5/5] bpf: Improve exception handling in trie_update_elem()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

Re: [PATCH 0/5] bpf: Adjustments for four function implementations

[Alexei Starovoitov]

On Sat, Dec 30, 2023 at 12:04 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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.

Re: [PATCH 0/5] bpf: Adjustments for four function implementations

[Markus Elfring]

>> 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/ed2f5323-390f-4c9d-919d-df43ba1cad2b@web.de/

Regards,
Markus

Re: [PATCH 0/5] bpf: Adjustments for four function implementations

[Song Liu]

On Mon, Jan 1, 2024 at 1:10 AM Markus Elfring <Markus.Elfring@web.de> 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/ed2f5323-390f-4c9d-919d-df43ba1cad2b@web.de/

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