All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe
@ 2023-03-16  5:40 David Vernet
  2023-03-16  5:40 ` [PATCH bpf-next v2 1/5] bpf: Free struct bpf_cpumask in call_rcu handler David Vernet
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: David Vernet @ 2023-03-16  5:40 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, linux-kernel, kernel-team

The struct bpf_cpumask type is currently not RCU safe. It uses the
bpf_mem_cache_{alloc,free}() APIs to allocate and release cpumasks, and
those allocations may be reused before an RCU grace period has elapsed.
We want to be able to enable using this pattern in BPF programs:

private(MASK) static struct bpf_cpumask __kptr *global;

int BPF_PROG(prog, ...)
{
	struct bpf_cpumask *cpumask;

	bpf_rcu_read_lock();
	cpumask = global;
	if (!cpumask) {
		bpf_rcu_read_unlock();
		return -1;
	}
	bpf_cpumask_setall(cpumask);
	...
	bpf_rcu_read_unlock();
}

In other words, to be able to pass a kptr to KF_RCU bpf_cpumask kfuncs
without requiring the acquisition and release of refcounts using
bpf_cpumask_kptr_get(). This patchset enables this by making the struct
bpf_cpumask type RCU safe, and removing the bpf_cpumask_kptr_get()
function.

---
v1: https://lore.kernel.org/all/20230316014122.678082-2-void@manifault.com/

Changelog:
----------
v1 -> v2:
- Add doxygen comment for new @rcu field in struct bpf_cpumask.

David Vernet (5):
  bpf: Free struct bpf_cpumask in call_rcu handler
  bpf: Mark struct bpf_cpumask as rcu protected
  bpf/selftests: Test using global cpumask kptr with RCU
  bpf: Remove bpf_cpumask_kptr_get() kfunc
  bpf,docs: Remove bpf_cpumask_kptr_get() from documentation

 Documentation/bpf/cpumasks.rst                | 30 +++-----
 kernel/bpf/cpumask.c                          | 38 +++--------
 kernel/bpf/verifier.c                         |  1 +
 .../selftests/bpf/prog_tests/cpumask.c        |  2 +-
 .../selftests/bpf/progs/cpumask_common.h      |  7 +-
 .../selftests/bpf/progs/cpumask_failure.c     | 68 +++++++++++++++----
 .../selftests/bpf/progs/cpumask_success.c     | 29 ++++----
 7 files changed, 96 insertions(+), 79 deletions(-)

-- 
2.39.0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH bpf-next v2 1/5] bpf: Free struct bpf_cpumask in call_rcu handler
  2023-03-16  5:40 [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe David Vernet
@ 2023-03-16  5:40 ` David Vernet
  2023-03-16  5:40 ` [PATCH bpf-next v2 2/5] bpf: Mark struct bpf_cpumask as rcu protected David Vernet
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: David Vernet @ 2023-03-16  5:40 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, linux-kernel, kernel-team

The struct bpf_cpumask type uses the bpf_mem_cache_{alloc,free}() APIs
to allocate and free its cpumasks. The bpf_mem allocator may currently
immediately reuse some memory when its freed, without waiting for an RCU
read cycle to elapse. We want to be able to treat struct bpf_cpumask
objects as completely RCU safe.

This is necessary for two reasons:

1. bpf_cpumask_kptr_get() currently does an RCU-protected
   refcnt_inc_not_zero(). This of course assumes that the underlying
   memory is not reused, and is therefore unsafe in its current form.

2. We want to be able to get rid of bpf_cpumask_kptr_get() entirely, and
   intead use the superior kptr RCU semantics now afforded by the
   verifier.

This patch fixes (1), and enables (2), by making struct bpf_cpumask RCU
safe. A subsequent patch will update the verifier to allow struct
bpf_cpumask * pointers to be passed to KF_RCU kfuncs, and then a latter
patch will remove bpf_cpumask_kptr_get().

Fixes: 516f4d3397c9 ("bpf: Enable cpumasks to be queried and used as kptrs")
Signed-off-by: David Vernet <void@manifault.com>
---
 kernel/bpf/cpumask.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c
index b6587ec40f1b..98eea62b6b7b 100644
--- a/kernel/bpf/cpumask.c
+++ b/kernel/bpf/cpumask.c
@@ -9,6 +9,7 @@
 /**
  * struct bpf_cpumask - refcounted BPF cpumask wrapper structure
  * @cpumask:	The actual cpumask embedded in the struct.
+ * @rcu:	The RCU head used to free the cpumask with RCU safety.
  * @usage:	Object reference counter. When the refcount goes to 0, the
  *		memory is released back to the BPF allocator, which provides
  *		RCU safety.
@@ -24,6 +25,7 @@
  */
 struct bpf_cpumask {
 	cpumask_t cpumask;
+	struct rcu_head rcu;
 	refcount_t usage;
 };
 
@@ -108,6 +110,16 @@ __bpf_kfunc struct bpf_cpumask *bpf_cpumask_kptr_get(struct bpf_cpumask **cpumas
 	return cpumask;
 }
 
+static void cpumask_free_cb(struct rcu_head *head)
+{
+	struct bpf_cpumask *cpumask;
+
+	cpumask = container_of(head, struct bpf_cpumask, rcu);
+	migrate_disable();
+	bpf_mem_cache_free(&bpf_cpumask_ma, cpumask);
+	migrate_enable();
+}
+
 /**
  * bpf_cpumask_release() - Release a previously acquired BPF cpumask.
  * @cpumask: The cpumask being released.
@@ -121,11 +133,8 @@ __bpf_kfunc void bpf_cpumask_release(struct bpf_cpumask *cpumask)
 	if (!cpumask)
 		return;
 
-	if (refcount_dec_and_test(&cpumask->usage)) {
-		migrate_disable();
-		bpf_mem_cache_free(&bpf_cpumask_ma, cpumask);
-		migrate_enable();
-	}
+	if (refcount_dec_and_test(&cpumask->usage))
+		call_rcu(&cpumask->rcu, cpumask_free_cb);
 }
 
 /**
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH bpf-next v2 2/5] bpf: Mark struct bpf_cpumask as rcu protected
  2023-03-16  5:40 [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe David Vernet
  2023-03-16  5:40 ` [PATCH bpf-next v2 1/5] bpf: Free struct bpf_cpumask in call_rcu handler David Vernet
@ 2023-03-16  5:40 ` David Vernet
  2023-03-16  5:40 ` [PATCH bpf-next v2 3/5] bpf/selftests: Test using global cpumask kptr with RCU David Vernet
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: David Vernet @ 2023-03-16  5:40 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, linux-kernel, kernel-team

struct bpf_cpumask is a BPF-wrapper around the struct cpumask type which
can be instantiated by a BPF program, and then queried as a cpumask in
similar fashion to normal kernel code. The previous patch in this series
makes the type fully RCU safe, so the type can be included in the
rcu_protected_type BTF ID list.

A subsequent patch will remove bpf_cpumask_kptr_get(), as it's no longer
useful now that we can just treat the type as RCU safe by default and do
our own if check.

Signed-off-by: David Vernet <void@manifault.com>
---
 kernel/bpf/verifier.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 60793f793ca6..15b5c5c729f9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4599,6 +4599,7 @@ static bool in_rcu_cs(struct bpf_verifier_env *env)
 BTF_SET_START(rcu_protected_types)
 BTF_ID(struct, prog_test_ref_kfunc)
 BTF_ID(struct, cgroup)
+BTF_ID(struct, bpf_cpumask)
 BTF_SET_END(rcu_protected_types)
 
 static bool rcu_protected_object(const struct btf *btf, u32 btf_id)
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH bpf-next v2 3/5] bpf/selftests: Test using global cpumask kptr with RCU
  2023-03-16  5:40 [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe David Vernet
  2023-03-16  5:40 ` [PATCH bpf-next v2 1/5] bpf: Free struct bpf_cpumask in call_rcu handler David Vernet
  2023-03-16  5:40 ` [PATCH bpf-next v2 2/5] bpf: Mark struct bpf_cpumask as rcu protected David Vernet
@ 2023-03-16  5:40 ` David Vernet
  2023-03-16  5:40 ` [PATCH bpf-next v2 4/5] bpf: Remove bpf_cpumask_kptr_get() kfunc David Vernet
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: David Vernet @ 2023-03-16  5:40 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, linux-kernel, kernel-team

Now that struct bpf_cpumask * is considered an RCU-safe type according
to the verifier, we should add tests that validate its common usages.
This patch adds those tests to the cpumask test suite. A subsequent
changes will remove bpf_cpumask_kptr_get(), and will adjust the selftest
and BPF documentation accordingly.

Signed-off-by: David Vernet <void@manifault.com>
---
 .../selftests/bpf/prog_tests/cpumask.c        |  1 +
 .../selftests/bpf/progs/cpumask_common.h      |  6 ++
 .../selftests/bpf/progs/cpumask_failure.c     | 62 +++++++++++++++++++
 .../selftests/bpf/progs/cpumask_success.c     | 33 ++++++++++
 4 files changed, 102 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/cpumask.c b/tools/testing/selftests/bpf/prog_tests/cpumask.c
index 5fbe457c4ebe..6c0fe23498c7 100644
--- a/tools/testing/selftests/bpf/prog_tests/cpumask.c
+++ b/tools/testing/selftests/bpf/prog_tests/cpumask.c
@@ -17,6 +17,7 @@ static const char * const cpumask_success_testcases[] = {
 	"test_insert_leave",
 	"test_insert_remove_release",
 	"test_insert_kptr_get_release",
+	"test_global_mask_rcu",
 };
 
 static void verify_success(const char *prog_name)
diff --git a/tools/testing/selftests/bpf/progs/cpumask_common.h b/tools/testing/selftests/bpf/progs/cpumask_common.h
index 65e5496ca1b2..7623782fbd62 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_common.h
+++ b/tools/testing/selftests/bpf/progs/cpumask_common.h
@@ -9,6 +9,9 @@
 
 int err;
 
+#define private(name) SEC(".bss." #name) __hidden __attribute__((aligned(8)))
+private(MASK) static struct bpf_cpumask __kptr * global_mask;
+
 struct __cpumask_map_value {
 	struct bpf_cpumask __kptr * cpumask;
 };
@@ -51,6 +54,9 @@ void bpf_cpumask_copy(struct bpf_cpumask *dst, const struct cpumask *src) __ksym
 u32 bpf_cpumask_any(const struct cpumask *src) __ksym;
 u32 bpf_cpumask_any_and(const struct cpumask *src1, const struct cpumask *src2) __ksym;
 
+void bpf_rcu_read_lock(void) __ksym;
+void bpf_rcu_read_unlock(void) __ksym;
+
 static inline const struct cpumask *cast(struct bpf_cpumask *cpumask)
 {
 	return (const struct cpumask *)cpumask;
diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
index cfe83f0ef9e2..9f726d55f747 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
@@ -127,3 +127,65 @@ int BPF_PROG(test_cpumask_null, struct task_struct *task, u64 clone_flags)
 
 	return 0;
 }
+
+SEC("tp_btf/task_newtask")
+__failure __msg("R2 must be a rcu pointer")
+int BPF_PROG(test_global_mask_out_of_rcu, struct task_struct *task, u64 clone_flags)
+{
+	struct bpf_cpumask *local, *prev;
+
+	local = create_cpumask();
+	if (!local)
+		return 0;
+
+	prev = bpf_kptr_xchg(&global_mask, local);
+	if (prev) {
+		bpf_cpumask_release(prev);
+		err = 3;
+		return 0;
+	}
+
+	bpf_rcu_read_lock();
+	local = global_mask;
+	if (!local) {
+		err = 4;
+		bpf_rcu_read_unlock();
+		return 0;
+	}
+
+	bpf_rcu_read_unlock();
+
+	/* RCU region is exited before calling KF_RCU kfunc. */
+
+	bpf_cpumask_test_cpu(0, (const struct cpumask *)local);
+
+	return 0;
+}
+
+SEC("tp_btf/task_newtask")
+__failure __msg("NULL pointer passed to trusted arg1")
+int BPF_PROG(test_global_mask_no_null_check, struct task_struct *task, u64 clone_flags)
+{
+	struct bpf_cpumask *local, *prev;
+
+	local = create_cpumask();
+	if (!local)
+		return 0;
+
+	prev = bpf_kptr_xchg(&global_mask, local);
+	if (prev) {
+		bpf_cpumask_release(prev);
+		err = 3;
+		return 0;
+	}
+
+	bpf_rcu_read_lock();
+	local = global_mask;
+
+	/* No NULL check is performed on global cpumask kptr. */
+	bpf_cpumask_test_cpu(0, (const struct cpumask *)local);
+
+	bpf_rcu_read_unlock();
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/cpumask_success.c b/tools/testing/selftests/bpf/progs/cpumask_success.c
index 97ed08c4ff03..fe928ff72a06 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_success.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_success.c
@@ -423,3 +423,36 @@ int BPF_PROG(test_insert_kptr_get_release, struct task_struct *task, u64 clone_f
 
 	return 0;
 }
+
+SEC("tp_btf/task_newtask")
+int BPF_PROG(test_global_mask_rcu, struct task_struct *task, u64 clone_flags)
+{
+	struct bpf_cpumask *local, *prev;
+
+	if (!is_test_task())
+		return 0;
+
+	local = create_cpumask();
+	if (!local)
+		return 0;
+
+	prev = bpf_kptr_xchg(&global_mask, local);
+	if (prev) {
+		bpf_cpumask_release(prev);
+		err = 3;
+		return 0;
+	}
+
+	bpf_rcu_read_lock();
+	local = global_mask;
+	if (!local) {
+		err = 4;
+		bpf_rcu_read_unlock();
+		return 0;
+	}
+
+	bpf_cpumask_test_cpu(0, (const struct cpumask *)local);
+	bpf_rcu_read_unlock();
+
+	return 0;
+}
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH bpf-next v2 4/5] bpf: Remove bpf_cpumask_kptr_get() kfunc
  2023-03-16  5:40 [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe David Vernet
                   ` (2 preceding siblings ...)
  2023-03-16  5:40 ` [PATCH bpf-next v2 3/5] bpf/selftests: Test using global cpumask kptr with RCU David Vernet
@ 2023-03-16  5:40 ` David Vernet
  2023-03-16  5:40 ` [PATCH bpf-next v2 5/5] bpf,docs: Remove bpf_cpumask_kptr_get() from documentation David Vernet
  2023-03-16 19:40 ` [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe patchwork-bot+netdevbpf
  5 siblings, 0 replies; 7+ messages in thread
From: David Vernet @ 2023-03-16  5:40 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, linux-kernel, kernel-team

Now that struct bpf_cpumask is RCU safe, there's no need for this kfunc.
Rather than doing the following:

private(MASK) static struct bpf_cpumask __kptr *global;

int BPF_PROG(prog, s32 cpu, ...)
{
	struct bpf_cpumask *cpumask;

	bpf_rcu_read_lock();
	cpumask = bpf_cpumask_kptr_get(&global);
	if (!cpumask) {
		bpf_rcu_read_unlock();
		return -1;
	}
	bpf_cpumask_setall(cpumask);
	...
	bpf_cpumask_release(cpumask);
	bpf_rcu_read_unlock();
}

Programs can instead simply do (assume same global cpumask):

int BPF_PROG(prog, ...)
{
	struct bpf_cpumask *cpumask;

	bpf_rcu_read_lock();
	cpumask = global;
	if (!cpumask) {
		bpf_rcu_read_unlock();
		return -1;
	}
	bpf_cpumask_setall(cpumask);
	...
	bpf_rcu_read_unlock();
}

In other words, no extra atomic acquire / release, and less boilerplate
code.

This patch removes both the kfunc, as well as its selftests and
documentation.

Signed-off-by: David Vernet <void@manifault.com>
---
 kernel/bpf/cpumask.c                          | 29 ------------------
 .../selftests/bpf/prog_tests/cpumask.c        |  1 -
 .../selftests/bpf/progs/cpumask_common.h      |  1 -
 .../selftests/bpf/progs/cpumask_failure.c     | 24 ---------------
 .../selftests/bpf/progs/cpumask_success.c     | 30 -------------------
 5 files changed, 85 deletions(-)

diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c
index 98eea62b6b7b..db9da2194c1a 100644
--- a/kernel/bpf/cpumask.c
+++ b/kernel/bpf/cpumask.c
@@ -82,34 +82,6 @@ __bpf_kfunc struct bpf_cpumask *bpf_cpumask_acquire(struct bpf_cpumask *cpumask)
 	return cpumask;
 }
 
-/**
- * bpf_cpumask_kptr_get() - Attempt to acquire a reference to a BPF cpumask
- *			    stored in a map.
- * @cpumaskp: A pointer to a BPF cpumask map value.
- *
- * Attempts to acquire a reference to a BPF cpumask stored in a map value. The
- * cpumask returned by this function must either be embedded in a map as a
- * kptr, or freed with bpf_cpumask_release(). This function may return NULL if
- * no BPF cpumask was found in the specified map value.
- */
-__bpf_kfunc struct bpf_cpumask *bpf_cpumask_kptr_get(struct bpf_cpumask **cpumaskp)
-{
-	struct bpf_cpumask *cpumask;
-
-	/* The BPF memory allocator frees memory backing its caches in an RCU
-	 * callback. Thus, we can safely use RCU to ensure that the cpumask is
-	 * safe to read.
-	 */
-	rcu_read_lock();
-
-	cpumask = READ_ONCE(*cpumaskp);
-	if (cpumask && !refcount_inc_not_zero(&cpumask->usage))
-		cpumask = NULL;
-
-	rcu_read_unlock();
-	return cpumask;
-}
-
 static void cpumask_free_cb(struct rcu_head *head)
 {
 	struct bpf_cpumask *cpumask;
@@ -435,7 +407,6 @@ BTF_SET8_START(cpumask_kfunc_btf_ids)
 BTF_ID_FLAGS(func, bpf_cpumask_create, KF_ACQUIRE | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_cpumask_release, KF_RELEASE | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_cpumask_acquire, KF_ACQUIRE | KF_TRUSTED_ARGS)
-BTF_ID_FLAGS(func, bpf_cpumask_kptr_get, KF_ACQUIRE | KF_KPTR_GET | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_cpumask_first, KF_RCU)
 BTF_ID_FLAGS(func, bpf_cpumask_first_zero, KF_RCU)
 BTF_ID_FLAGS(func, bpf_cpumask_set_cpu, KF_RCU)
diff --git a/tools/testing/selftests/bpf/prog_tests/cpumask.c b/tools/testing/selftests/bpf/prog_tests/cpumask.c
index 6c0fe23498c7..cdf4acc18e4c 100644
--- a/tools/testing/selftests/bpf/prog_tests/cpumask.c
+++ b/tools/testing/selftests/bpf/prog_tests/cpumask.c
@@ -16,7 +16,6 @@ static const char * const cpumask_success_testcases[] = {
 	"test_copy_any_anyand",
 	"test_insert_leave",
 	"test_insert_remove_release",
-	"test_insert_kptr_get_release",
 	"test_global_mask_rcu",
 };
 
diff --git a/tools/testing/selftests/bpf/progs/cpumask_common.h b/tools/testing/selftests/bpf/progs/cpumask_common.h
index 7623782fbd62..0c5b785a93e4 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_common.h
+++ b/tools/testing/selftests/bpf/progs/cpumask_common.h
@@ -26,7 +26,6 @@ struct array_map {
 struct bpf_cpumask *bpf_cpumask_create(void) __ksym;
 void bpf_cpumask_release(struct bpf_cpumask *cpumask) __ksym;
 struct bpf_cpumask *bpf_cpumask_acquire(struct bpf_cpumask *cpumask) __ksym;
-struct bpf_cpumask *bpf_cpumask_kptr_get(struct bpf_cpumask **cpumask) __ksym;
 u32 bpf_cpumask_first(const struct cpumask *cpumask) __ksym;
 u32 bpf_cpumask_first_zero(const struct cpumask *cpumask) __ksym;
 void bpf_cpumask_set_cpu(u32 cpu, struct bpf_cpumask *cpumask) __ksym;
diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
index 9f726d55f747..db4f94e72b61 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
@@ -94,30 +94,6 @@ int BPF_PROG(test_insert_remove_no_release, struct task_struct *task, u64 clone_
 	return 0;
 }
 
-SEC("tp_btf/task_newtask")
-__failure __msg("Unreleased reference")
-int BPF_PROG(test_kptr_get_no_release, struct task_struct *task, u64 clone_flags)
-{
-	struct bpf_cpumask *cpumask;
-	struct __cpumask_map_value *v;
-
-	cpumask = create_cpumask();
-	if (!cpumask)
-		return 0;
-
-	if (cpumask_map_insert(cpumask))
-		return 0;
-
-	v = cpumask_map_value_lookup();
-	if (!v)
-		return 0;
-
-	cpumask = bpf_cpumask_kptr_get(&v->cpumask);
-
-	/* cpumask is never released. */
-	return 0;
-}
-
 SEC("tp_btf/task_newtask")
 __failure __msg("NULL pointer passed to trusted arg0")
 int BPF_PROG(test_cpumask_null, struct task_struct *task, u64 clone_flags)
diff --git a/tools/testing/selftests/bpf/progs/cpumask_success.c b/tools/testing/selftests/bpf/progs/cpumask_success.c
index fe928ff72a06..2fcdd7f68ac7 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_success.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_success.c
@@ -394,36 +394,6 @@ int BPF_PROG(test_insert_remove_release, struct task_struct *task, u64 clone_fla
 	return 0;
 }
 
-SEC("tp_btf/task_newtask")
-int BPF_PROG(test_insert_kptr_get_release, struct task_struct *task, u64 clone_flags)
-{
-	struct bpf_cpumask *cpumask;
-	struct __cpumask_map_value *v;
-
-	cpumask = create_cpumask();
-	if (!cpumask)
-		return 0;
-
-	if (cpumask_map_insert(cpumask)) {
-		err = 3;
-		return 0;
-	}
-
-	v = cpumask_map_value_lookup();
-	if (!v) {
-		err = 4;
-		return 0;
-	}
-
-	cpumask = bpf_cpumask_kptr_get(&v->cpumask);
-	if (cpumask)
-		bpf_cpumask_release(cpumask);
-	else
-		err = 5;
-
-	return 0;
-}
-
 SEC("tp_btf/task_newtask")
 int BPF_PROG(test_global_mask_rcu, struct task_struct *task, u64 clone_flags)
 {
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH bpf-next v2 5/5] bpf,docs: Remove bpf_cpumask_kptr_get() from documentation
  2023-03-16  5:40 [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe David Vernet
                   ` (3 preceding siblings ...)
  2023-03-16  5:40 ` [PATCH bpf-next v2 4/5] bpf: Remove bpf_cpumask_kptr_get() kfunc David Vernet
@ 2023-03-16  5:40 ` David Vernet
  2023-03-16 19:40 ` [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe patchwork-bot+netdevbpf
  5 siblings, 0 replies; 7+ messages in thread
From: David Vernet @ 2023-03-16  5:40 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, linux-kernel, kernel-team

Now that the kfunc no longer exists, we can remove it and instead
describe how RCU can be used to get a struct bpf_cpumask from a map
value. This patch updates the BPF documentation accordingly.

Signed-off-by: David Vernet <void@manifault.com>
---
 Documentation/bpf/cpumasks.rst | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/Documentation/bpf/cpumasks.rst b/Documentation/bpf/cpumasks.rst
index 75344cd230e5..41efd8874eeb 100644
--- a/Documentation/bpf/cpumasks.rst
+++ b/Documentation/bpf/cpumasks.rst
@@ -117,12 +117,7 @@ For example:
 As mentioned and illustrated above, these ``struct bpf_cpumask *`` objects can
 also be stored in a map and used as kptrs. If a ``struct bpf_cpumask *`` is in
 a map, the reference can be removed from the map with bpf_kptr_xchg(), or
-opportunistically acquired with bpf_cpumask_kptr_get():
-
-.. kernel-doc:: kernel/bpf/cpumask.c
-  :identifiers: bpf_cpumask_kptr_get
-
-Here is an example of a ``struct bpf_cpumask *`` being retrieved from a map:
+opportunistically acquired using RCU:
 
 .. code-block:: c
 
@@ -144,7 +139,7 @@ Here is an example of a ``struct bpf_cpumask *`` being retrieved from a map:
 	/**
 	 * A simple example tracepoint program showing how a
 	 * struct bpf_cpumask * kptr that is stored in a map can
-	 * be acquired using the bpf_cpumask_kptr_get() kfunc.
+	 * be passed to kfuncs using RCU protection.
 	 */
 	SEC("tp_btf/cgroup_mkdir")
 	int BPF_PROG(cgrp_ancestor_example, struct cgroup *cgrp, const char *path)
@@ -158,26 +153,21 @@ Here is an example of a ``struct bpf_cpumask *`` being retrieved from a map:
 		if (!v)
 			return -ENOENT;
 
+		bpf_rcu_read_lock();
 		/* Acquire a reference to the bpf_cpumask * kptr that's already stored in the map. */
-		kptr = bpf_cpumask_kptr_get(&v->cpumask);
-		if (!kptr)
+		kptr = v->cpumask;
+		if (!kptr) {
 			/* If no bpf_cpumask was present in the map, it's because
 			 * we're racing with another CPU that removed it with
 			 * bpf_kptr_xchg() between the bpf_map_lookup_elem()
-			 * above, and our call to bpf_cpumask_kptr_get().
-			 * bpf_cpumask_kptr_get() internally safely handles this
-			 * race, and will return NULL if the cpumask is no longer
-			 * present in the map by the time we invoke the kfunc.
+			 * above, and our load of the pointer from the map.
 			 */
+			bpf_rcu_read_unlock();
 			return -EBUSY;
+		}
 
-		/* Free the reference we just took above. Note that the
-		 * original struct bpf_cpumask * kptr is still in the map. It will
-		 * be freed either at a later time if another context deletes
-		 * it from the map, or automatically by the BPF subsystem if
-		 * it's still present when the map is destroyed.
-		 */
-		bpf_cpumask_release(kptr);
+		bpf_cpumask_setall(kptr);
+		bpf_rcu_read_unlock();
 
 		return 0;
 	}
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe
  2023-03-16  5:40 [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe David Vernet
                   ` (4 preceding siblings ...)
  2023-03-16  5:40 ` [PATCH bpf-next v2 5/5] bpf,docs: Remove bpf_cpumask_kptr_get() from documentation David Vernet
@ 2023-03-16 19:40 ` patchwork-bot+netdevbpf
  5 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-03-16 19:40 UTC (permalink / raw)
  To: David Vernet
  Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, linux-kernel, kernel-team

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Thu, 16 Mar 2023 00:40:23 -0500 you wrote:
> The struct bpf_cpumask type is currently not RCU safe. It uses the
> bpf_mem_cache_{alloc,free}() APIs to allocate and release cpumasks, and
> those allocations may be reused before an RCU grace period has elapsed.
> We want to be able to enable using this pattern in BPF programs:
> 
> private(MASK) static struct bpf_cpumask __kptr *global;
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2,1/5] bpf: Free struct bpf_cpumask in call_rcu handler
    https://git.kernel.org/bpf/bpf-next/c/77473d1a962f
  - [bpf-next,v2,2/5] bpf: Mark struct bpf_cpumask as rcu protected
    https://git.kernel.org/bpf/bpf-next/c/63d2d83d21a6
  - [bpf-next,v2,3/5] bpf/selftests: Test using global cpumask kptr with RCU
    https://git.kernel.org/bpf/bpf-next/c/a5a197df58c4
  - [bpf-next,v2,4/5] bpf: Remove bpf_cpumask_kptr_get() kfunc
    https://git.kernel.org/bpf/bpf-next/c/1b403ce77dfb
  - [bpf-next,v2,5/5] bpf,docs: Remove bpf_cpumask_kptr_get() from documentation
    https://git.kernel.org/bpf/bpf-next/c/fec2c6d14fd5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-03-16 19:40 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-16  5:40 [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe David Vernet
2023-03-16  5:40 ` [PATCH bpf-next v2 1/5] bpf: Free struct bpf_cpumask in call_rcu handler David Vernet
2023-03-16  5:40 ` [PATCH bpf-next v2 2/5] bpf: Mark struct bpf_cpumask as rcu protected David Vernet
2023-03-16  5:40 ` [PATCH bpf-next v2 3/5] bpf/selftests: Test using global cpumask kptr with RCU David Vernet
2023-03-16  5:40 ` [PATCH bpf-next v2 4/5] bpf: Remove bpf_cpumask_kptr_get() kfunc David Vernet
2023-03-16  5:40 ` [PATCH bpf-next v2 5/5] bpf,docs: Remove bpf_cpumask_kptr_get() from documentation David Vernet
2023-03-16 19:40 ` [PATCH bpf-next v2 0/5] Make struct bpf_cpumask RCU safe patchwork-bot+netdevbpf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.