[bpf-next,v2,2/7] bpf: Support kptrs in local storage maps

Commit Message

Kumar Kartikeya Dwivedi Feb. 21, 2023, 8:06 p.m. UTC

Enable support for kptrs in local storage maps by wiring up the freeing
of these kptrs from map value.

Cc: Martin KaFai Lau <[e-mail]>
Cc: KP Singh <[e-mail]>
Cc: Paul E. McKenney <[e-mail]>
Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
---
 kernel/bpf/bpf_local_storage.c | 35 ++++++++++++++++++++++++++++++----
 kernel/bpf/syscall.c           |  6 +++++-
 kernel/bpf/verifier.c          | 12 ++++++++----
 3 files changed, 44 insertions(+), 9 deletions(-)

Comments

Alexei Starovoitov Feb. 22, 2023, 8:48 p.m. UTC | #1

On Tue, Feb 21, 2023 at 12:06 PM Kumar Kartikeya Dwivedi
<[e-mail]> wrote:
>
> Enable support for kptrs in local storage maps by wiring up the freeing
> of these kptrs from map value.
>
> Cc: Martin KaFai Lau <[e-mail]>
> Cc: KP Singh <[e-mail]>
> Cc: Paul E. McKenney <[e-mail]>
> Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
> ---
>  kernel/bpf/bpf_local_storage.c | 35 ++++++++++++++++++++++++++++++----
>  kernel/bpf/syscall.c           |  6 +++++-
>  kernel/bpf/verifier.c          | 12 ++++++++----
>  3 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> index 35f4138a54dc..2803b85b30b2 100644
> --- a/kernel/bpf/bpf_local_storage.c
> +++ b/kernel/bpf/bpf_local_storage.c
> @@ -75,6 +75,7 @@ bpf_selem_alloc(struct bpf_local_storage_map *smap, void *owner,
>         if (selem) {
>                 if (value)
>                         copy_map_value(&smap->map, SDATA(selem)->data, value);
> +               /* No need to call check_and_init_map_value as memory is zero init */
>                 return selem;
>         }
>
> @@ -103,10 +104,17 @@ static void bpf_selem_free_rcu(struct rcu_head *rcu)
>         struct bpf_local_storage_elem *selem;
>
>         selem = container_of(rcu, struct bpf_local_storage_elem, rcu);
> +       bpf_obj_free_fields(SDATA(selem)->smap->map.record, SDATA(selem)->data);
> +       kfree(selem);

CI is saying that clang compiled kernel crashes here:
https://github.com/kernel-patches/bpf/actions/runs/4239645973/jobs/7368557262

[ 18.596262] BUG: unable to handle page fault for address: 00000000ffffffff
[ 18.599128] RIP: 0010:bpf_obj_free_fields+0x29/0x110 [ 18.605706] <TASK>
[ 18.605844] bpf_selem_free_tasks_trace_rcu+0x22/0x30
[ 18.606171] rcu_tasks_invoke_cbs+0x150/0x210
[ 18.606449] rcu_tasks_one_gp+0x401/0x430
[ 18.606701] rcu_tasks_kthread+0x35/0x50

map.record somehow became (u32)-1 ?

aarch64 failures look related too:
libbpf: prog 'test_ls_map_kptr_ref1': failed to attach: ERROR:
strerror_r(-524)=22
test_map_kptr_success:FAIL:bpf_program__attach ref1 unexpected error: -524
#124/26 map_kptr/success-map:FAIL

Please pay attention to CI in the future.
It's the developer's job to monitor it for their patches.

Patch

diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index 35f4138a54dc..2803b85b30b2 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -75,6 +75,7 @@  bpf_selem_alloc(struct bpf_local_storage_map *smap, void *owner,
 	if (selem) {
 		if (value)
 			copy_map_value(&smap->map, SDATA(selem)->data, value);
+		/* No need to call check_and_init_map_value as memory is zero init */
 		return selem;
 	}
 
@@ -103,10 +104,17 @@  static void bpf_selem_free_rcu(struct rcu_head *rcu)
 	struct bpf_local_storage_elem *selem;
 
 	selem = container_of(rcu, struct bpf_local_storage_elem, rcu);
+	bpf_obj_free_fields(SDATA(selem)->smap->map.record, SDATA(selem)->data);
+	kfree(selem);
+}
+
+static void bpf_selem_free_tasks_trace_rcu(struct rcu_head *rcu)
+{
+	/* Free directly if Tasks Trace RCU GP also implies RCU GP */
 	if (rcu_trace_implies_rcu_gp())
-		kfree(selem);
+		bpf_selem_free_rcu(rcu);
 	else
-		kfree_rcu(selem, rcu);
+		call_rcu(rcu, bpf_selem_free_rcu);
 }
 
 /* local_storage->lock must be held and selem->local_storage == local_storage.
@@ -160,9 +168,9 @@  static bool bpf_selem_unlink_storage_nolock(struct bpf_local_storage *local_stor
 		RCU_INIT_POINTER(local_storage->cache[smap->cache_idx], NULL);
 
 	if (use_trace_rcu)
-		call_rcu_tasks_trace(&selem->rcu, bpf_selem_free_rcu);
+		call_rcu_tasks_trace(&selem->rcu, bpf_selem_free_tasks_trace_rcu);
 	else
-		kfree_rcu(selem, rcu);
+		call_rcu(&selem->rcu, bpf_selem_free_rcu);
 
 	return free_local_storage;
 }
@@ -713,6 +721,25 @@  void bpf_local_storage_map_free(struct bpf_map *map,
 	 */
 	synchronize_rcu();
 
+	/* Only delay freeing of smap, buckets are not needed anymore */
 	kvfree(smap->buckets);
+
+	/* When local storage has special fields, callbacks for
+	 * bpf_selem_free_rcu and bpf_selem_free_tasks_trace_rcu will keep using
+	 * the map BTF record, we need to execute an RCU barrier to wait for
+	 * them as the record will be freed right after our map_free callback.
+	 */
+	if (!IS_ERR_OR_NULL(smap->map.record)) {
+		rcu_barrier_tasks_trace();
+		/* We cannot skip rcu_barrier() when rcu_trace_implies_rcu_gp()
+		 * is true, because while call_rcu invocation is skipped in that
+		 * case in bpf_selem_free_tasks_trace_rcu (and all local storage
+		 * maps pass use_trace_rcu = true), there can be call_rcu
+		 * callbacks based on use_trace_rcu = false in the earlier while
+		 * ((selem = ...)) loop or from bpf_local_storage_unlink_nolock
+		 * called from owner's free path.
+		 */
+		rcu_barrier();
+	}
 	bpf_map_area_free(smap);
 }
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index da117a2a83b2..eb50025b03c1 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1063,7 +1063,11 @@  static int map_check_btf(struct bpf_map *map, const struct btf *btf,
 				    map->map_type != BPF_MAP_TYPE_LRU_HASH &&
 				    map->map_type != BPF_MAP_TYPE_LRU_PERCPU_HASH &&
 				    map->map_type != BPF_MAP_TYPE_ARRAY &&
-				    map->map_type != BPF_MAP_TYPE_PERCPU_ARRAY) {
+				    map->map_type != BPF_MAP_TYPE_PERCPU_ARRAY &&
+				    map->map_type != BPF_MAP_TYPE_SK_STORAGE &&
+				    map->map_type != BPF_MAP_TYPE_INODE_STORAGE &&
+				    map->map_type != BPF_MAP_TYPE_TASK_STORAGE &&
+				    map->map_type != BPF_MAP_TYPE_CGRP_STORAGE) {
 					ret = -EOPNOTSUPP;
 					goto free_map_tab;
 				}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 272563a0b770..9a4e7efaf28f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7126,22 +7126,26 @@  static int check_map_func_compatibility(struct bpf_verifier_env *env,
 		break;
 	case BPF_MAP_TYPE_SK_STORAGE:
 		if (func_id != BPF_FUNC_sk_storage_get &&
-		    func_id != BPF_FUNC_sk_storage_delete)
+		    func_id != BPF_FUNC_sk_storage_delete &&
+		    func_id != BPF_FUNC_kptr_xchg)
 			goto error;
 		break;
 	case BPF_MAP_TYPE_INODE_STORAGE:
 		if (func_id != BPF_FUNC_inode_storage_get &&
-		    func_id != BPF_FUNC_inode_storage_delete)
+		    func_id != BPF_FUNC_inode_storage_delete &&
+		    func_id != BPF_FUNC_kptr_xchg)
 			goto error;
 		break;
 	case BPF_MAP_TYPE_TASK_STORAGE:
 		if (func_id != BPF_FUNC_task_storage_get &&
-		    func_id != BPF_FUNC_task_storage_delete)
+		    func_id != BPF_FUNC_task_storage_delete &&
+		    func_id != BPF_FUNC_kptr_xchg)
 			goto error;
 		break;
 	case BPF_MAP_TYPE_CGRP_STORAGE:
 		if (func_id != BPF_FUNC_cgrp_storage_get &&
-		    func_id != BPF_FUNC_cgrp_storage_delete)
+		    func_id != BPF_FUNC_cgrp_storage_delete &&
+		    func_id != BPF_FUNC_kptr_xchg)
 			goto error;
 		break;
 	case BPF_MAP_TYPE_BLOOM_FILTER: