[PATCH bpf-next 1/3] bpf: Make bpf_cgroup_acquire() KF_RCU | KF_RET_NULL

[PATCH bpf-next 1/3] bpf: Make bpf_cgroup_acquire() KF_RCU | KF_RET_NULL

[David Vernet]

struct cgroup is already an RCU-safe type in the verifier. We can
therefore update bpf_cgroup_acquire() to be KF_RCU | KF_RET_NULL, and
subsequently remove bpf_cgroup_kptr_get(). This patch does the first of
these by updating bpf_cgroup_acquire() to be KF_RCU | KF_RET_NULL, and
also updates selftests accordingly.

Signed-off-by: David Vernet <void@manifault.com>
---
 kernel/bpf/helpers.c                          |  5 ++-
 .../selftests/bpf/progs/cgrp_kfunc_common.h   |  5 +++
 .../selftests/bpf/progs/cgrp_kfunc_failure.c  | 35 +++++++++++++++----
 .../selftests/bpf/progs/cgrp_kfunc_success.c  |  5 ++-
 4 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index b6a5cda5bb59..71f0604bdc97 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2037,8 +2037,7 @@ __bpf_kfunc void bpf_task_release(struct task_struct *p)
  */
 __bpf_kfunc struct cgroup *bpf_cgroup_acquire(struct cgroup *cgrp)
 {
-	cgroup_get(cgrp);
-	return cgrp;
+	return cgroup_tryget(cgrp) ? cgrp : NULL;
 }
 
 /**
@@ -2314,7 +2313,7 @@ BTF_ID_FLAGS(func, bpf_rbtree_add)
 BTF_ID_FLAGS(func, bpf_rbtree_first, KF_RET_NULL)
 
 #ifdef CONFIG_CGROUPS
-BTF_ID_FLAGS(func, bpf_cgroup_acquire, KF_ACQUIRE | KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_cgroup_acquire, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_cgroup_kptr_get, KF_ACQUIRE | KF_KPTR_GET | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_cgroup_release, KF_RELEASE)
 BTF_ID_FLAGS(func, bpf_cgroup_ancestor, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
index d0b7cd0d09d7..b0e279f4652b 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
+++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
@@ -61,6 +61,11 @@ static inline int cgrps_kfunc_map_insert(struct cgroup *cgrp)
 	}
 
 	acquired = bpf_cgroup_acquire(cgrp);
+	if (!acquired) {
+		bpf_map_delete_elem(&__cgrps_kfunc_map, &id);
+		return -ENOENT;
+	}
+
 	old = bpf_kptr_xchg(&v->cgrp, acquired);
 	if (old) {
 		bpf_cgroup_release(old);
diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c b/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
index 48b2034cadb3..49347f12de39 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
+++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
@@ -41,6 +41,23 @@ int BPF_PROG(cgrp_kfunc_acquire_untrusted, struct cgroup *cgrp, const char *path
 
 	/* Can't invoke bpf_cgroup_acquire() on an untrusted pointer. */
 	acquired = bpf_cgroup_acquire(v->cgrp);
+	if (acquired)
+		bpf_cgroup_release(acquired);
+
+	return 0;
+}
+
+SEC("tp_btf/cgroup_mkdir")
+__failure __msg("Possibly NULL pointer passed to trusted arg0")
+int BPF_PROG(cgrp_kfunc_acquire_no_null_check, struct cgroup *cgrp, const char *path)
+{
+	struct cgroup *acquired;
+
+	acquired = bpf_cgroup_acquire(cgrp);
+	/*
+	 * Can't invoke bpf_cgroup_release() without checking the return value
+	 * of bpf_cgroup_acquire().
+	 */
 	bpf_cgroup_release(acquired);
 
 	return 0;
@@ -54,7 +71,8 @@ int BPF_PROG(cgrp_kfunc_acquire_fp, struct cgroup *cgrp, const char *path)
 
 	/* Can't invoke bpf_cgroup_acquire() on a random frame pointer. */
 	acquired = bpf_cgroup_acquire((struct cgroup *)&stack_cgrp);
-	bpf_cgroup_release(acquired);
+	if (acquired)
+		bpf_cgroup_release(acquired);
 
 	return 0;
 }
@@ -67,7 +85,8 @@ int BPF_PROG(cgrp_kfunc_acquire_unsafe_kretprobe, struct cgroup *cgrp)
 
 	/* Can't acquire an untrusted struct cgroup * pointer. */
 	acquired = bpf_cgroup_acquire(cgrp);
-	bpf_cgroup_release(acquired);
+	if (acquired)
+		bpf_cgroup_release(acquired);
 
 	return 0;
 }
@@ -80,7 +99,8 @@ int BPF_PROG(cgrp_kfunc_acquire_trusted_walked, struct cgroup *cgrp, const char
 
 	/* Can't invoke bpf_cgroup_acquire() on a pointer obtained from walking a trusted cgroup. */
 	acquired = bpf_cgroup_acquire(cgrp->old_dom_cgrp);
-	bpf_cgroup_release(acquired);
+	if (acquired)
+		bpf_cgroup_release(acquired);
 
 	return 0;
 }
@@ -93,9 +113,8 @@ int BPF_PROG(cgrp_kfunc_acquire_null, struct cgroup *cgrp, const char *path)
 
 	/* Can't invoke bpf_cgroup_acquire() on a NULL pointer. */
 	acquired = bpf_cgroup_acquire(NULL);
-	if (!acquired)
-		return 0;
-	bpf_cgroup_release(acquired);
+	if (acquired)
+		bpf_cgroup_release(acquired);
 
 	return 0;
 }
@@ -137,6 +156,8 @@ int BPF_PROG(cgrp_kfunc_get_non_kptr_acquired, struct cgroup *cgrp, const char *
 	struct cgroup *kptr, *acquired;
 
 	acquired = bpf_cgroup_acquire(cgrp);
+	if (!acquired)
+		return 0;
 
 	/* Cannot use bpf_cgroup_kptr_get() on a non-map-value, even if the kptr was acquired. */
 	kptr = bpf_cgroup_kptr_get(&acquired);
@@ -256,6 +277,8 @@ int BPF_PROG(cgrp_kfunc_release_null, struct cgroup *cgrp, const char *path)
 		return -ENOENT;
 
 	acquired = bpf_cgroup_acquire(cgrp);
+	if (!acquired)
+		return -ENOENT;
 
 	old = bpf_kptr_xchg(&v->cgrp, acquired);
 
diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c b/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
index 030aff700084..e9dbd1af05a7 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
+++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
@@ -38,7 +38,10 @@ int BPF_PROG(test_cgrp_acquire_release_argument, struct cgroup *cgrp, const char
 		return 0;
 
 	acquired = bpf_cgroup_acquire(cgrp);
-	bpf_cgroup_release(acquired);
+	if (!acquired)
+		err = 1;
+	else
+		bpf_cgroup_release(acquired);
 
 	return 0;
 }
-- 
2.40.0

[PATCH bpf-next 2/3] bpf: Remove bpf_cgroup_kptr_get() kfunc

[David Vernet]

Now that bpf_cgroup_acquire() is KF_RCU | KF_RET_NULL,
bpf_cgroup_kptr_get() is redundant. Let's remove it, and update
selftests to instead use bpf_cgroup_acquire() where appropriate. The
next patch will update the BPF documentation to not mention
bpf_cgroup_kptr_get().

Signed-off-by: David Vernet <void@manifault.com>
---
 kernel/bpf/helpers.c                          | 32 ---------
 .../selftests/bpf/progs/cgrp_kfunc_common.h   |  3 +-
 .../selftests/bpf/progs/cgrp_kfunc_failure.c  | 68 +++----------------
 .../selftests/bpf/progs/cgrp_kfunc_success.c  | 10 ++-
 4 files changed, 14 insertions(+), 99 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 71f0604bdc97..f04e60a4847f 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2040,37 +2040,6 @@ __bpf_kfunc struct cgroup *bpf_cgroup_acquire(struct cgroup *cgrp)
 	return cgroup_tryget(cgrp) ? cgrp : NULL;
 }
 
-/**
- * bpf_cgroup_kptr_get - Acquire a reference on a struct cgroup kptr. A cgroup
- * kptr acquired by this kfunc which is not subsequently stored in a map, must
- * be released by calling bpf_cgroup_release().
- * @cgrpp: A pointer to a cgroup kptr on which a reference is being acquired.
- */
-__bpf_kfunc struct cgroup *bpf_cgroup_kptr_get(struct cgroup **cgrpp)
-{
-	struct cgroup *cgrp;
-
-	rcu_read_lock();
-	/* Another context could remove the cgroup from the map and release it
-	 * at any time, including after we've done the lookup above. This is
-	 * safe because we're in an RCU read region, so the cgroup is
-	 * guaranteed to remain valid until at least the rcu_read_unlock()
-	 * below.
-	 */
-	cgrp = READ_ONCE(*cgrpp);
-
-	if (cgrp && !cgroup_tryget(cgrp))
-		/* If the cgroup had been removed from the map and freed as
-		 * described above, cgroup_tryget() will return false. The
-		 * cgroup will be freed at some point after the current RCU gp
-		 * has ended, so just return NULL to the user.
-		 */
-		cgrp = NULL;
-	rcu_read_unlock();
-
-	return cgrp;
-}
-
 /**
  * bpf_cgroup_release - Release the reference acquired on a cgroup.
  * If this kfunc is invoked in an RCU read region, the cgroup is guaranteed to
@@ -2314,7 +2283,6 @@ BTF_ID_FLAGS(func, bpf_rbtree_first, KF_RET_NULL)
 
 #ifdef CONFIG_CGROUPS
 BTF_ID_FLAGS(func, bpf_cgroup_acquire, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
-BTF_ID_FLAGS(func, bpf_cgroup_kptr_get, KF_ACQUIRE | KF_KPTR_GET | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_cgroup_release, KF_RELEASE)
 BTF_ID_FLAGS(func, bpf_cgroup_ancestor, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_cgroup_from_id, KF_ACQUIRE | KF_RET_NULL)
diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
index b0e279f4652b..22914a70db54 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
+++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
@@ -21,10 +21,11 @@ struct hash_map {
 } __cgrps_kfunc_map SEC(".maps");
 
 struct cgroup *bpf_cgroup_acquire(struct cgroup *p) __ksym;
-struct cgroup *bpf_cgroup_kptr_get(struct cgroup **pp) __ksym;
 void bpf_cgroup_release(struct cgroup *p) __ksym;
 struct cgroup *bpf_cgroup_ancestor(struct cgroup *cgrp, int level) __ksym;
 struct cgroup *bpf_cgroup_from_id(u64 cgid) __ksym;
+void bpf_rcu_read_lock(void) __ksym;
+void bpf_rcu_read_unlock(void) __ksym;
 
 static inline struct __cgrps_kfunc_map_value *cgrps_kfunc_map_value_lookup(struct cgroup *cgrp)
 {
diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c b/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
index 49347f12de39..0fa564a5cc5b 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
+++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
@@ -133,59 +133,6 @@ int BPF_PROG(cgrp_kfunc_acquire_unreleased, struct cgroup *cgrp, const char *pat
 	return 0;
 }
 
-SEC("tp_btf/cgroup_mkdir")
-__failure __msg("arg#0 expected pointer to map value")
-int BPF_PROG(cgrp_kfunc_get_non_kptr_param, struct cgroup *cgrp, const char *path)
-{
-	struct cgroup *kptr;
-
-	/* Cannot use bpf_cgroup_kptr_get() on a non-kptr, even on a valid cgroup. */
-	kptr = bpf_cgroup_kptr_get(&cgrp);
-	if (!kptr)
-		return 0;
-
-	bpf_cgroup_release(kptr);
-
-	return 0;
-}
-
-SEC("tp_btf/cgroup_mkdir")
-__failure __msg("arg#0 expected pointer to map value")
-int BPF_PROG(cgrp_kfunc_get_non_kptr_acquired, struct cgroup *cgrp, const char *path)
-{
-	struct cgroup *kptr, *acquired;
-
-	acquired = bpf_cgroup_acquire(cgrp);
-	if (!acquired)
-		return 0;
-
-	/* Cannot use bpf_cgroup_kptr_get() on a non-map-value, even if the kptr was acquired. */
-	kptr = bpf_cgroup_kptr_get(&acquired);
-	bpf_cgroup_release(acquired);
-	if (!kptr)
-		return 0;
-
-	bpf_cgroup_release(kptr);
-
-	return 0;
-}
-
-SEC("tp_btf/cgroup_mkdir")
-__failure __msg("arg#0 expected pointer to map value")
-int BPF_PROG(cgrp_kfunc_get_null, struct cgroup *cgrp, const char *path)
-{
-	struct cgroup *kptr;
-
-	/* Cannot use bpf_cgroup_kptr_get() on a NULL pointer. */
-	kptr = bpf_cgroup_kptr_get(NULL);
-	if (!kptr)
-		return 0;
-
-	bpf_cgroup_release(kptr);
-
-	return 0;
-}
-
 SEC("tp_btf/cgroup_mkdir")
 __failure __msg("Unreleased reference")
 int BPF_PROG(cgrp_kfunc_xchg_unreleased, struct cgroup *cgrp, const char *path)
@@ -207,8 +154,8 @@ int BPF_PROG(cgrp_kfunc_xchg_unreleased, struct cgroup *cgrp, const char *path)
 }
 
 SEC("tp_btf/cgroup_mkdir")
-__failure __msg("Unreleased reference")
-int BPF_PROG(cgrp_kfunc_get_unreleased, struct cgroup *cgrp, const char *path)
+__failure __msg("must be referenced or trusted")
+int BPF_PROG(cgrp_kfunc_rcu_get_release, struct cgroup *cgrp, const char *path)
 {
 	struct cgroup *kptr;
 	struct __cgrps_kfunc_map_value *v;
@@ -217,11 +164,12 @@ int BPF_PROG(cgrp_kfunc_get_unreleased, struct cgroup *cgrp, const char *path)
 	if (!v)
 		return 0;
 
-	kptr = bpf_cgroup_kptr_get(&v->cgrp);
-	if (!kptr)
-		return 0;
-
-	/* Kptr acquired above is never released. */
+	bpf_rcu_read_lock();
+	kptr = v->cgrp;
+	if (kptr)
+		/* Can't release a cgroup kptr stored in a map. */
+		bpf_cgroup_release(kptr);
+	bpf_rcu_read_unlock();
 
 	return 0;
 }
diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c b/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
index e9dbd1af05a7..5354455a01be 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
+++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
@@ -126,13 +126,11 @@ int BPF_PROG(test_cgrp_get_release, struct cgroup *cgrp, const char *path)
 		return 0;
 	}
 
-	kptr = bpf_cgroup_kptr_get(&v->cgrp);
-	if (!kptr) {
+	bpf_rcu_read_lock();
+	kptr = v->cgrp;
+	if (!kptr)
 		err = 3;
-		return 0;
-	}
-
-	bpf_cgroup_release(kptr);
+	bpf_rcu_read_unlock();
 
 	return 0;
 }
-- 
2.40.0

[PATCH bpf-next 3/3] bpf,docs: Remove references to bpf_cgroup_kptr_get()

[David Vernet]

The bpf_cgroup_kptr_get() kfunc has been removed, and
bpf_cgroup_acquire() / bpf_cgroup_release() now have the same semantics
as bpf_task_acquire() / bpf_task_release(). This patch updates the BPF
documentation to reflect this.

Signed-off-by: David Vernet <void@manifault.com>
---
 Documentation/bpf/kfuncs.rst | 68 ------------------------------------
 1 file changed, 68 deletions(-)

diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
index d8a16c4bef7f..3b42cfe12437 100644
--- a/Documentation/bpf/kfuncs.rst
+++ b/Documentation/bpf/kfuncs.rst
@@ -572,74 +572,6 @@ bpf_task_release() respectively, so we won't provide examples for them.
 
 ----
 
-You may also acquire a reference to a ``struct cgroup`` kptr that's already
-stored in a map using bpf_cgroup_kptr_get():
-
-.. kernel-doc:: kernel/bpf/helpers.c
-   :identifiers: bpf_cgroup_kptr_get
-
-Here's an example of how it can be used:
-
-.. code-block:: c
-
-	/* struct containing the struct task_struct kptr which is actually stored in the map. */
-	struct __cgroups_kfunc_map_value {
-		struct cgroup __kptr * cgroup;
-	};
-
-	/* The map containing struct __cgroups_kfunc_map_value entries. */
-	struct {
-		__uint(type, BPF_MAP_TYPE_HASH);
-		__type(key, int);
-		__type(value, struct __cgroups_kfunc_map_value);
-		__uint(max_entries, 1);
-	} __cgroups_kfunc_map SEC(".maps");
-
-	/* ... */
-
-	/**
-	 * A simple example tracepoint program showing how a
-	 * struct cgroup kptr that is stored in a map can
-	 * be acquired using the bpf_cgroup_kptr_get() kfunc.
-	 */
-	 SEC("tp_btf/cgroup_mkdir")
-	 int BPF_PROG(cgroup_kptr_get_example, struct cgroup *cgrp, const char *path)
-	 {
-		struct cgroup *kptr;
-		struct __cgroups_kfunc_map_value *v;
-		s32 id = cgrp->self.id;
-
-		/* Assume a cgroup kptr was previously stored in the map. */
-		v = bpf_map_lookup_elem(&__cgroups_kfunc_map, &id);
-		if (!v)
-			return -ENOENT;
-
-		/* Acquire a reference to the cgroup kptr that's already stored in the map. */
-		kptr = bpf_cgroup_kptr_get(&v->cgroup);
-		if (!kptr)
-			/* If no cgroup 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_cgroup_kptr_get().
-			 * bpf_cgroup_kptr_get() internally safely handles this
-			 * race, and will return NULL if the task is no longer
-			 * present in the map by the time we invoke the kfunc.
-			 */
-			return -EBUSY;
-
-		/* Free the reference we just took above. Note that the
-		 * original struct cgroup 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_cgroup_release(kptr);
-
-		return 0;
-        }
-
-----
-
 Other kfuncs available for interacting with ``struct cgroup *`` objects are
 bpf_cgroup_ancestor() and bpf_cgroup_from_id(), allowing callers to access
 the ancestor of a cgroup and find a cgroup by its ID, respectively. Both
-- 
2.40.0

Re: [PATCH bpf-next 1/3] bpf: Make bpf_cgroup_acquire() KF_RCU | KF_RET_NULL

[patchwork-bot+netdevbpf]

Hello:

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

On Mon, 10 Apr 2023 23:16:31 -0500 you wrote:
> struct cgroup is already an RCU-safe type in the verifier. We can
> therefore update bpf_cgroup_acquire() to be KF_RCU | KF_RET_NULL, and
> subsequently remove bpf_cgroup_kptr_get(). This patch does the first of
> these by updating bpf_cgroup_acquire() to be KF_RCU | KF_RET_NULL, and
> also updates selftests accordingly.
> 
> Signed-off-by: David Vernet <void@manifault.com>
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/3] bpf: Make bpf_cgroup_acquire() KF_RCU | KF_RET_NULL
    https://git.kernel.org/bpf/bpf-next/c/1d71283987c7
  - [bpf-next,2/3] bpf: Remove bpf_cgroup_kptr_get() kfunc
    https://git.kernel.org/bpf/bpf-next/c/6499fe6edc4f
  - [bpf-next,3/3] bpf,docs: Remove references to bpf_cgroup_kptr_get()
    https://git.kernel.org/bpf/bpf-next/c/ec48599abee3

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