All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/3] bpf: expose bpf_{g,s}et_retval to more cgroup hooks
@ 2022-08-12 19:02 Stanislav Fomichev
  2022-08-12 19:02 ` [PATCH bpf-next 1/3] bpf: introduce cgroup_{common,current}_func_proto Stanislav Fomichev
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2022-08-12 19:02 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa

Apparently, only a small subset of cgroup hooks actually falls
back to cgroup_base_func_proto. This leads to unexpected result
where not all cgroup helpers have bpf_{g,s}et_retval.

It's getting harder and harder to manage which helpers are exported
to which hooks. We now have the following call chains:

- cg_skb_func_proto
  - sk_filter_func_proto
    - bpf_sk_base_func_proto
      - bpf_base_func_proto

So by looking at cg_skb_func_proto it's pretty hard to understand
what's going on.

For cgroup helpers, I'm proposing we do the following instead:

  func_proto = cgroup_common_func_proto();
  if (func_proto) return func_proto;

  /* optional, if hook has 'current' */
  func_proto = cgroup_current_func_proto();
  if (func_proto) return func_proto;

  ...

  switch (func_id) {
  /* hook specific helpers */
  case BPF_FUNC_hook_specific_helper:
    return &xyz;
  default:
    /* always fall back to plain bpf_base_func_proto */
    bpf_base_func_proto(func_id);
  }

If this turns out more workable, we can follow up with converting
the rest to the same pattern.

Stanislav Fomichev (3):
  bpf: introduce cgroup_{common,current}_func_proto
  bpf: use cgroup_{common,current}_func_proto in more hooks
  selftests/bpf: make sure bpf_{g,s}et_retval is exposed everywhere

 include/linux/bpf.h                           | 16 +++
 kernel/bpf/cgroup.c                           | 82 +++++++--------
 kernel/bpf/helpers.c                          | 99 ++++++++++++++++++-
 net/core/filter.c                             | 78 ++++++---------
 tools/testing/selftests/bpf/Makefile          |  1 +
 .../bpf/cgroup_getset_retval_hooks.h          | 25 +++++
 .../bpf/prog_tests/cgroup_getset_retval.c     | 48 +++++++++
 .../bpf/progs/cgroup_getset_retval_hooks.c    | 16 +++
 8 files changed, 270 insertions(+), 95 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/cgroup_getset_retval_hooks.h
 create mode 100644 tools/testing/selftests/bpf/progs/cgroup_getset_retval_hooks.c

-- 
2.37.1.595.g718a3a8f04-goog


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

* [PATCH bpf-next 1/3] bpf: introduce cgroup_{common,current}_func_proto
  2022-08-12 19:02 [PATCH bpf-next 0/3] bpf: expose bpf_{g,s}et_retval to more cgroup hooks Stanislav Fomichev
@ 2022-08-12 19:02 ` Stanislav Fomichev
  2022-08-15 21:26   ` Martin KaFai Lau
  2022-08-12 19:02 ` [PATCH bpf-next 2/3] bpf: use cgroup_{common,current}_func_proto in more hooks Stanislav Fomichev
  2022-08-12 19:02 ` [PATCH bpf-next 3/3] selftests/bpf: make sure bpf_{g,s}et_retval is exposed everywhere Stanislav Fomichev
  2 siblings, 1 reply; 10+ messages in thread
From: Stanislav Fomichev @ 2022-08-12 19:02 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa

Split cgroup_base_func_proto into the following:

* cgroup_common_func_proto - common helpers for all cgroup hooks
* cgroup_current_func_proto - common helpers for all cgroup hooks
  running in the process context (== have meaningful 'current').

Move bpf_{g,s}et_retval into kernel/bpf/helpers.c so they are more
usable from the core parts.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 include/linux/bpf.h  | 16 +++++++++
 kernel/bpf/cgroup.c  | 82 +++++++++++++++++++-------------------------
 kernel/bpf/helpers.c | 67 ++++++++++++++++++++++++++++++++++--
 3 files changed, 116 insertions(+), 49 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a627a02cf8ab..cdb295c6c728 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1948,6 +1948,10 @@ struct bpf_prog *bpf_prog_by_id(u32 id);
 struct bpf_link *bpf_link_by_id(u32 id);
 
 const struct bpf_func_proto *bpf_base_func_proto(enum bpf_func_id func_id);
+const struct bpf_func_proto *
+cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog);
+const struct bpf_func_proto *
+cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog);
 void bpf_task_storage_free(struct task_struct *task);
 bool bpf_prog_has_kfunc_call(const struct bpf_prog *prog);
 const struct btf_func_model *
@@ -2154,6 +2158,18 @@ bpf_base_func_proto(enum bpf_func_id func_id)
 	return NULL;
 }
 
+static inline const struct bpf_func_proto *
+cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+{
+	return NULL;
+}
+
+static inline const struct bpf_func_proto *
+cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+{
+	return NULL;
+}
+
 static inline void bpf_task_storage_free(struct task_struct *task)
 {
 }
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 59b7eb60d5b4..cc0b33b7d4cc 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -1527,63 +1527,27 @@ int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor,
 	return ret;
 }
 
-BPF_CALL_0(bpf_get_retval)
-{
-	struct bpf_cg_run_ctx *ctx =
-		container_of(current->bpf_ctx, struct bpf_cg_run_ctx, run_ctx);
-
-	return ctx->retval;
-}
-
-const struct bpf_func_proto bpf_get_retval_proto = {
-	.func		= bpf_get_retval,
-	.gpl_only	= false,
-	.ret_type	= RET_INTEGER,
-};
-
-BPF_CALL_1(bpf_set_retval, int, retval)
+static const struct bpf_func_proto *
+cgroup_dev_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
-	struct bpf_cg_run_ctx *ctx =
-		container_of(current->bpf_ctx, struct bpf_cg_run_ctx, run_ctx);
+	const struct bpf_func_proto *func_proto;
 
-	ctx->retval = retval;
-	return 0;
-}
+	func_proto = cgroup_common_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
 
-const struct bpf_func_proto bpf_set_retval_proto = {
-	.func		= bpf_set_retval,
-	.gpl_only	= false,
-	.ret_type	= RET_INTEGER,
-	.arg1_type	= ARG_ANYTHING,
-};
+	func_proto = cgroup_current_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
 
-static const struct bpf_func_proto *
-cgroup_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
-{
 	switch (func_id) {
-	case BPF_FUNC_get_current_uid_gid:
-		return &bpf_get_current_uid_gid_proto;
-	case BPF_FUNC_get_local_storage:
-		return &bpf_get_local_storage_proto;
-	case BPF_FUNC_get_current_cgroup_id:
-		return &bpf_get_current_cgroup_id_proto;
 	case BPF_FUNC_perf_event_output:
 		return &bpf_event_output_data_proto;
-	case BPF_FUNC_get_retval:
-		return &bpf_get_retval_proto;
-	case BPF_FUNC_set_retval:
-		return &bpf_set_retval_proto;
 	default:
 		return bpf_base_func_proto(func_id);
 	}
 }
 
-static const struct bpf_func_proto *
-cgroup_dev_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
-{
-	return cgroup_base_func_proto(func_id, prog);
-}
-
 static bool cgroup_dev_is_valid_access(int off, int size,
 				       enum bpf_access_type type,
 				       const struct bpf_prog *prog,
@@ -2096,6 +2060,16 @@ static const struct bpf_func_proto bpf_sysctl_set_new_value_proto = {
 static const struct bpf_func_proto *
 sysctl_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
+	const struct bpf_func_proto *func_proto;
+
+	func_proto = cgroup_common_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
+
+	func_proto = cgroup_current_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
+
 	switch (func_id) {
 	case BPF_FUNC_strtol:
 		return &bpf_strtol_proto;
@@ -2111,8 +2085,10 @@ sysctl_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_sysctl_set_new_value_proto;
 	case BPF_FUNC_ktime_get_coarse_ns:
 		return &bpf_ktime_get_coarse_ns_proto;
+	case BPF_FUNC_perf_event_output:
+		return &bpf_event_output_data_proto;
 	default:
-		return cgroup_base_func_proto(func_id, prog);
+		return bpf_base_func_proto(func_id);
 	}
 }
 
@@ -2233,6 +2209,16 @@ static const struct bpf_func_proto bpf_get_netns_cookie_sockopt_proto = {
 static const struct bpf_func_proto *
 cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
+	const struct bpf_func_proto *func_proto;
+
+	func_proto = cgroup_common_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
+
+	func_proto = cgroup_current_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
+
 	switch (func_id) {
 #ifdef CONFIG_NET
 	case BPF_FUNC_get_netns_cookie:
@@ -2254,8 +2240,10 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	case BPF_FUNC_tcp_sock:
 		return &bpf_tcp_sock_proto;
 #endif
+	case BPF_FUNC_perf_event_output:
+		return &bpf_event_output_data_proto;
 	default:
-		return cgroup_base_func_proto(func_id, prog);
+		return bpf_base_func_proto(func_id);
 	}
 }
 
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 3c1b9bbcf971..de7d2fabb06d 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -429,7 +429,6 @@ const struct bpf_func_proto bpf_get_current_ancestor_cgroup_id_proto = {
 };
 
 #ifdef CONFIG_CGROUP_BPF
-
 BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags)
 {
 	/* flags argument is not used now,
@@ -460,7 +459,37 @@ const struct bpf_func_proto bpf_get_local_storage_proto = {
 	.arg1_type	= ARG_CONST_MAP_PTR,
 	.arg2_type	= ARG_ANYTHING,
 };
-#endif
+
+BPF_CALL_0(bpf_get_retval)
+{
+	struct bpf_cg_run_ctx *ctx =
+		container_of(current->bpf_ctx, struct bpf_cg_run_ctx, run_ctx);
+
+	return ctx->retval;
+}
+
+const struct bpf_func_proto bpf_get_retval_proto = {
+	.func		= bpf_get_retval,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+};
+
+BPF_CALL_1(bpf_set_retval, int, retval)
+{
+	struct bpf_cg_run_ctx *ctx =
+		container_of(current->bpf_ctx, struct bpf_cg_run_ctx, run_ctx);
+
+	ctx->retval = retval;
+	return 0;
+}
+
+const struct bpf_func_proto bpf_set_retval_proto = {
+	.func		= bpf_set_retval,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_ANYTHING,
+};
+#endif /* CONFIG_CGROUP_BPF */
 
 #define BPF_STRTOX_BASE_MASK 0x1F
 
@@ -1726,6 +1755,40 @@ bpf_base_func_proto(enum bpf_func_id func_id)
 	}
 }
 
+/* Common helpers for cgroup hooks. */
+const struct bpf_func_proto *
+cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+{
+	switch (func_id) {
+#ifdef CONFIG_CGROUP_BPF
+	case BPF_FUNC_get_local_storage:
+		return &bpf_get_local_storage_proto;
+	case BPF_FUNC_get_retval:
+		return &bpf_get_retval_proto;
+	case BPF_FUNC_set_retval:
+		return &bpf_set_retval_proto;
+#endif
+	default:
+		return NULL;
+	}
+}
+
+/* Common helpers for cgroup hooks with valid process context. */
+const struct bpf_func_proto *
+cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+{
+	switch (func_id) {
+#ifdef CONFIG_CGROUPS
+	case BPF_FUNC_get_current_uid_gid:
+		return &bpf_get_current_uid_gid_proto;
+	case BPF_FUNC_get_current_cgroup_id:
+		return &bpf_get_current_cgroup_id_proto;
+#endif
+	default:
+		return NULL;
+	}
+}
+
 BTF_SET8_START(tracing_btf_ids)
 #ifdef CONFIG_KEXEC_CORE
 BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
-- 
2.37.1.595.g718a3a8f04-goog


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

* [PATCH bpf-next 2/3] bpf: use cgroup_{common,current}_func_proto in more hooks
  2022-08-12 19:02 [PATCH bpf-next 0/3] bpf: expose bpf_{g,s}et_retval to more cgroup hooks Stanislav Fomichev
  2022-08-12 19:02 ` [PATCH bpf-next 1/3] bpf: introduce cgroup_{common,current}_func_proto Stanislav Fomichev
@ 2022-08-12 19:02 ` Stanislav Fomichev
  2022-08-13  6:25   ` kernel test robot
                     ` (2 more replies)
  2022-08-12 19:02 ` [PATCH bpf-next 3/3] selftests/bpf: make sure bpf_{g,s}et_retval is exposed everywhere Stanislav Fomichev
  2 siblings, 3 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2022-08-12 19:02 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa

The following hooks are per-cgroup hooks but they are not
using cgroup_{common,current}_func_proto, fix it:

* BPF_PROG_TYPE_CGROUP_SKB (cg_skb)
* BPF_PROG_TYPE_CGROUP_SOCK_ADDR (cg_sock_addr)
* BPF_PROG_TYPE_CGROUP_SOCK (cg_sock)

Also:

* move common func_proto's into cgroup func_proto handlers
* make sure bpf_{g,s}et_retval are not accessible from recvmsg,
  getpeername and getsockname (return/errno is ignore in these
  places)
* as a side effect, expose get_current_pid_tgid, get_current_comm_proto,
  get_current_ancestor_cgroup_id, get_cgroup_classid to more cgroup
  hooks

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 kernel/bpf/helpers.c | 36 ++++++++++++++++++--
 net/core/filter.c    | 78 ++++++++++++++++++--------------------------
 2 files changed, 66 insertions(+), 48 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index de7d2fabb06d..87ce47b13b22 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1764,9 +1764,31 @@ cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	case BPF_FUNC_get_local_storage:
 		return &bpf_get_local_storage_proto;
 	case BPF_FUNC_get_retval:
-		return &bpf_get_retval_proto;
+		switch (prog->expected_attach_type) {
+		case BPF_CGROUP_SOCK_OPS:
+		case BPF_CGROUP_UDP4_RECVMSG:
+		case BPF_CGROUP_UDP6_RECVMSG:
+		case BPF_CGROUP_INET4_GETPEERNAME:
+		case BPF_CGROUP_INET6_GETPEERNAME:
+		case BPF_CGROUP_INET4_GETSOCKNAME:
+		case BPF_CGROUP_INET6_GETSOCKNAME:
+			return NULL;
+		default:
+			return &bpf_get_retval_proto;
+		}
 	case BPF_FUNC_set_retval:
-		return &bpf_set_retval_proto;
+		switch (prog->expected_attach_type) {
+		case BPF_CGROUP_SOCK_OPS:
+		case BPF_CGROUP_UDP4_RECVMSG:
+		case BPF_CGROUP_UDP6_RECVMSG:
+		case BPF_CGROUP_INET4_GETPEERNAME:
+		case BPF_CGROUP_INET6_GETPEERNAME:
+		case BPF_CGROUP_INET4_GETSOCKNAME:
+		case BPF_CGROUP_INET6_GETSOCKNAME:
+			return NULL;
+		default:
+			return &bpf_set_retval_proto;
+		}
 #endif
 	default:
 		return NULL;
@@ -1781,8 +1803,18 @@ cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 #ifdef CONFIG_CGROUPS
 	case BPF_FUNC_get_current_uid_gid:
 		return &bpf_get_current_uid_gid_proto;
+	case BPF_FUNC_get_current_pid_tgid:
+		return &bpf_get_current_pid_tgid_proto;
+	case BPF_FUNC_get_current_comm:
+		return &bpf_get_current_comm_proto;
 	case BPF_FUNC_get_current_cgroup_id:
 		return &bpf_get_current_cgroup_id_proto;
+	case BPF_FUNC_get_current_ancestor_cgroup_id:
+		return &bpf_get_current_ancestor_cgroup_id_proto;
+#endif
+#ifdef CONFIG_CGROUP_NET_CLASSID
+	case BPF_FUNC_get_cgroup_classid:
+		return &bpf_get_cgroup_classid_curr_proto;
 #endif
 	default:
 		return NULL;
diff --git a/net/core/filter.c b/net/core/filter.c
index 5669248aff25..6731e60621e9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7663,34 +7663,23 @@ const struct bpf_func_proto bpf_sk_storage_get_cg_sock_proto __weak;
 static const struct bpf_func_proto *
 sock_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
+	const struct bpf_func_proto *func_proto;
+
+	func_proto = cgroup_common_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
+
+	func_proto = cgroup_current_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
+
 	switch (func_id) {
-	/* inet and inet6 sockets are created in a process
-	 * context so there is always a valid uid/gid
-	 */
-	case BPF_FUNC_get_current_uid_gid:
-		return &bpf_get_current_uid_gid_proto;
-	case BPF_FUNC_get_local_storage:
-		return &bpf_get_local_storage_proto;
 	case BPF_FUNC_get_socket_cookie:
 		return &bpf_get_socket_cookie_sock_proto;
 	case BPF_FUNC_get_netns_cookie:
 		return &bpf_get_netns_cookie_sock_proto;
 	case BPF_FUNC_perf_event_output:
 		return &bpf_event_output_data_proto;
-	case BPF_FUNC_get_current_pid_tgid:
-		return &bpf_get_current_pid_tgid_proto;
-	case BPF_FUNC_get_current_comm:
-		return &bpf_get_current_comm_proto;
-#ifdef CONFIG_CGROUPS
-	case BPF_FUNC_get_current_cgroup_id:
-		return &bpf_get_current_cgroup_id_proto;
-	case BPF_FUNC_get_current_ancestor_cgroup_id:
-		return &bpf_get_current_ancestor_cgroup_id_proto;
-#endif
-#ifdef CONFIG_CGROUP_NET_CLASSID
-	case BPF_FUNC_get_cgroup_classid:
-		return &bpf_get_cgroup_classid_curr_proto;
-#endif
 	case BPF_FUNC_sk_storage_get:
 		return &bpf_sk_storage_get_cg_sock_proto;
 	case BPF_FUNC_ktime_get_coarse_ns:
@@ -7703,12 +7692,17 @@ sock_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 static const struct bpf_func_proto *
 sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
+	const struct bpf_func_proto *func_proto;
+
+	func_proto = cgroup_common_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
+
+	func_proto = cgroup_current_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
+
 	switch (func_id) {
-	/* inet and inet6 sockets are created in a process
-	 * context so there is always a valid uid/gid
-	 */
-	case BPF_FUNC_get_current_uid_gid:
-		return &bpf_get_current_uid_gid_proto;
 	case BPF_FUNC_bind:
 		switch (prog->expected_attach_type) {
 		case BPF_CGROUP_INET4_CONNECT:
@@ -7721,24 +7715,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_get_socket_cookie_sock_addr_proto;
 	case BPF_FUNC_get_netns_cookie:
 		return &bpf_get_netns_cookie_sock_addr_proto;
-	case BPF_FUNC_get_local_storage:
-		return &bpf_get_local_storage_proto;
 	case BPF_FUNC_perf_event_output:
 		return &bpf_event_output_data_proto;
-	case BPF_FUNC_get_current_pid_tgid:
-		return &bpf_get_current_pid_tgid_proto;
-	case BPF_FUNC_get_current_comm:
-		return &bpf_get_current_comm_proto;
-#ifdef CONFIG_CGROUPS
-	case BPF_FUNC_get_current_cgroup_id:
-		return &bpf_get_current_cgroup_id_proto;
-	case BPF_FUNC_get_current_ancestor_cgroup_id:
-		return &bpf_get_current_ancestor_cgroup_id_proto;
-#endif
-#ifdef CONFIG_CGROUP_NET_CLASSID
-	case BPF_FUNC_get_cgroup_classid:
-		return &bpf_get_cgroup_classid_curr_proto;
-#endif
 #ifdef CONFIG_INET
 	case BPF_FUNC_sk_lookup_tcp:
 		return &bpf_sock_addr_sk_lookup_tcp_proto;
@@ -7819,9 +7797,13 @@ const struct bpf_func_proto bpf_sk_storage_delete_proto __weak;
 static const struct bpf_func_proto *
 cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
+	const struct bpf_func_proto *func_proto;
+
+	func_proto = cgroup_common_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
+
 	switch (func_id) {
-	case BPF_FUNC_get_local_storage:
-		return &bpf_get_local_storage_proto;
 	case BPF_FUNC_sk_fullsock:
 		return &bpf_sk_fullsock_proto;
 	case BPF_FUNC_sk_storage_get:
@@ -8061,6 +8043,12 @@ const struct bpf_func_proto bpf_sock_hash_update_proto __weak;
 static const struct bpf_func_proto *
 sock_ops_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
+	const struct bpf_func_proto *func_proto;
+
+	func_proto = cgroup_common_func_proto(func_id, prog);
+	if (func_proto)
+		return func_proto;
+
 	switch (func_id) {
 	case BPF_FUNC_setsockopt:
 		return &bpf_sock_ops_setsockopt_proto;
@@ -8074,8 +8062,6 @@ sock_ops_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_sock_hash_update_proto;
 	case BPF_FUNC_get_socket_cookie:
 		return &bpf_get_socket_cookie_sock_ops_proto;
-	case BPF_FUNC_get_local_storage:
-		return &bpf_get_local_storage_proto;
 	case BPF_FUNC_perf_event_output:
 		return &bpf_event_output_data_proto;
 	case BPF_FUNC_sk_storage_get:
-- 
2.37.1.595.g718a3a8f04-goog


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

* [PATCH bpf-next 3/3] selftests/bpf: make sure bpf_{g,s}et_retval is exposed everywhere
  2022-08-12 19:02 [PATCH bpf-next 0/3] bpf: expose bpf_{g,s}et_retval to more cgroup hooks Stanislav Fomichev
  2022-08-12 19:02 ` [PATCH bpf-next 1/3] bpf: introduce cgroup_{common,current}_func_proto Stanislav Fomichev
  2022-08-12 19:02 ` [PATCH bpf-next 2/3] bpf: use cgroup_{common,current}_func_proto in more hooks Stanislav Fomichev
@ 2022-08-12 19:02 ` Stanislav Fomichev
  2 siblings, 0 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2022-08-12 19:02 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa

For each hook, have a simple bpf_set_retval(bpf_get_retval) program
and make sure it loads for the hooks we want. The exceptions are
the hooks which don't propagate the error to the callers:

- sockops
- recvmsg
- getpeername
- getsockname

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/Makefile          |  1 +
 .../bpf/cgroup_getset_retval_hooks.h          | 25 ++++++++++
 .../bpf/prog_tests/cgroup_getset_retval.c     | 48 +++++++++++++++++++
 .../bpf/progs/cgroup_getset_retval_hooks.c    | 16 +++++++
 4 files changed, 90 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/cgroup_getset_retval_hooks.h
 create mode 100644 tools/testing/selftests/bpf/progs/cgroup_getset_retval_hooks.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 8d59ec7f4c2d..eecad99f1735 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -323,6 +323,7 @@ $(OUTPUT)/test_l4lb_noinline.o: BPF_CFLAGS += -fno-inline
 $(OUTPUT)/test_xdp_noinline.o: BPF_CFLAGS += -fno-inline
 
 $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
+$(OUTPUT)/cgroup_getset_retval_hooks.o: cgroup_getset_retval_hooks.h
 
 # Build BPF object using Clang
 # $1 - input .c file
diff --git a/tools/testing/selftests/bpf/cgroup_getset_retval_hooks.h b/tools/testing/selftests/bpf/cgroup_getset_retval_hooks.h
new file mode 100644
index 000000000000..4a8d2063163d
--- /dev/null
+++ b/tools/testing/selftests/bpf/cgroup_getset_retval_hooks.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+BPF_RETVAL_HOOK(ingress, "cgroup_skb/ingress", __sk_buff, 0)
+BPF_RETVAL_HOOK(egress, "cgroup_skb/egress", __sk_buff, 0)
+BPF_RETVAL_HOOK(sock_create, "cgroup/sock_create", bpf_sock, 0)
+BPF_RETVAL_HOOK(sock_ops, "sockops", bpf_sock_ops, -EINVAL)
+BPF_RETVAL_HOOK(dev, "cgroup/dev", bpf_cgroup_dev_ctx, 0)
+BPF_RETVAL_HOOK(bind4, "cgroup/bind4", bpf_sock_addr, 0)
+BPF_RETVAL_HOOK(bind6, "cgroup/bind6", bpf_sock_addr, 0)
+BPF_RETVAL_HOOK(connect4, "cgroup/connect4", bpf_sock_addr, 0)
+BPF_RETVAL_HOOK(connect6, "cgroup/connect6", bpf_sock_addr, 0)
+BPF_RETVAL_HOOK(post_bind4, "cgroup/post_bind4", bpf_sock_addr, 0)
+BPF_RETVAL_HOOK(post_bind6, "cgroup/post_bind6", bpf_sock_addr, 0)
+BPF_RETVAL_HOOK(sendmsg4, "cgroup/sendmsg4", bpf_sock_addr, 0)
+BPF_RETVAL_HOOK(sendmsg6, "cgroup/sendmsg6", bpf_sock_addr, 0)
+BPF_RETVAL_HOOK(sysctl, "cgroup/sysctl", bpf_sysctl, 0)
+BPF_RETVAL_HOOK(recvmsg4, "cgroup/recvmsg4", bpf_sock_addr, -EINVAL)
+BPF_RETVAL_HOOK(recvmsg6, "cgroup/recvmsg6", bpf_sock_addr, -EINVAL)
+BPF_RETVAL_HOOK(getsockopt, "cgroup/getsockopt", bpf_sockopt, 0)
+BPF_RETVAL_HOOK(setsockopt, "cgroup/setsockopt", bpf_sockopt, 0)
+BPF_RETVAL_HOOK(getpeername4, "cgroup/getpeername4", bpf_sock_addr, -EINVAL)
+BPF_RETVAL_HOOK(getpeername6, "cgroup/getpeername6", bpf_sock_addr, -EINVAL)
+BPF_RETVAL_HOOK(getsockname4, "cgroup/getsockname4", bpf_sock_addr, -EINVAL)
+BPF_RETVAL_HOOK(getsockname6, "cgroup/getsockname6", bpf_sock_addr, -EINVAL)
+BPF_RETVAL_HOOK(sock_release, "cgroup/sock_release", bpf_sock, 0)
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_getset_retval.c b/tools/testing/selftests/bpf/prog_tests/cgroup_getset_retval.c
index 0b47c3c000c7..4d2fa99273d8 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_getset_retval.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_getset_retval.c
@@ -10,6 +10,7 @@
 
 #include "cgroup_getset_retval_setsockopt.skel.h"
 #include "cgroup_getset_retval_getsockopt.skel.h"
+#include "cgroup_getset_retval_hooks.skel.h"
 
 #define SOL_CUSTOM	0xdeadbeef
 
@@ -433,6 +434,50 @@ static void test_getsockopt_retval_sync(int cgroup_fd, int sock_fd)
 	cgroup_getset_retval_getsockopt__destroy(obj);
 }
 
+struct exposed_hook {
+	const char *name;
+	int expected_err;
+} exposed_hooks[] = {
+
+#define BPF_RETVAL_HOOK(NAME, SECTION, CTX, EXPECTED_ERR) \
+	{ \
+		.name = #NAME, \
+		.expected_err = EXPECTED_ERR, \
+	},
+
+#include "cgroup_getset_retval_hooks.h"
+
+#undef BPF_RETVAL_HOOK
+};
+
+static void test_exposed_hooks(int cgroup_fd, int sock_fd)
+{
+	struct cgroup_getset_retval_hooks *skel;
+	struct bpf_program *prog;
+	int err;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(exposed_hooks); i++) {
+		skel = cgroup_getset_retval_hooks__open();
+		if (!ASSERT_OK_PTR(skel, "cgroup_getset_retval_hooks__open"))
+			continue;
+
+		prog = bpf_object__find_program_by_name(skel->obj, exposed_hooks[i].name);
+		if (!ASSERT_NEQ(prog, NULL, "bpf_object__find_program_by_name"))
+			goto close_skel;
+
+		err = bpf_program__set_autoload(prog, true);
+		if (!ASSERT_OK(err, "bpf_program__set_autoload"))
+			goto close_skel;
+
+		err = cgroup_getset_retval_hooks__load(skel);
+		ASSERT_EQ(err, exposed_hooks[i].expected_err, "expected_err");
+
+close_skel:
+		cgroup_getset_retval_hooks__destroy(skel);
+	}
+}
+
 void test_cgroup_getset_retval(void)
 {
 	int cgroup_fd = -1;
@@ -476,6 +521,9 @@ void test_cgroup_getset_retval(void)
 	if (test__start_subtest("getsockopt-retval_sync"))
 		test_getsockopt_retval_sync(cgroup_fd, sock_fd);
 
+	if (test__start_subtest("exposed_hooks"))
+		test_exposed_hooks(cgroup_fd, sock_fd);
+
 close_fd:
 	close(cgroup_fd);
 }
diff --git a/tools/testing/selftests/bpf/progs/cgroup_getset_retval_hooks.c b/tools/testing/selftests/bpf/progs/cgroup_getset_retval_hooks.c
new file mode 100644
index 000000000000..13dfb4bbfd28
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/cgroup_getset_retval_hooks.c
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+#define BPF_RETVAL_HOOK(name, section, ctx, expected_err) \
+	__attribute__((__section__("?" section))) \
+	int name(struct ctx *_ctx) \
+	{ \
+		bpf_set_retval(bpf_get_retval()); \
+		return 1; \
+	}
+
+#include "cgroup_getset_retval_hooks.h"
+
+#undef BPF_RETVAL_HOOK
-- 
2.37.1.595.g718a3a8f04-goog


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

* Re: [PATCH bpf-next 2/3] bpf: use cgroup_{common,current}_func_proto in more hooks
  2022-08-12 19:02 ` [PATCH bpf-next 2/3] bpf: use cgroup_{common,current}_func_proto in more hooks Stanislav Fomichev
@ 2022-08-13  6:25   ` kernel test robot
  2022-08-13 18:55   ` kernel test robot
  2022-08-15 21:57   ` Martin KaFai Lau
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2022-08-13  6:25 UTC (permalink / raw)
  To: Stanislav Fomichev, bpf
  Cc: kbuild-all, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa

Hi Stanislav,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Stanislav-Fomichev/bpf-expose-bpf_-g-s-et_retval-to-more-cgroup-hooks/20220813-030615
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-a013 (https://download.01.org/0day-ci/archive/20220813/202208131415.CkdRZBrY-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/0429824054f7a843ee976d48432e825e493a0a7e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Stanislav-Fomichev/bpf-expose-bpf_-g-s-et_retval-to-more-cgroup-hooks/20220813-030615
        git checkout 0429824054f7a843ee976d48432e825e493a0a7e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   kernel/bpf/helpers.c: In function 'cgroup_current_func_proto':
>> kernel/bpf/helpers.c:1817:25: error: 'bpf_get_cgroup_classid_curr_proto' undeclared (first use in this function)
    1817 |                 return &bpf_get_cgroup_classid_curr_proto;
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   kernel/bpf/helpers.c:1817:25: note: each undeclared identifier is reported only once for each function it appears in


vim +/bpf_get_cgroup_classid_curr_proto +1817 kernel/bpf/helpers.c

  1797	
  1798	/* Common helpers for cgroup hooks with valid process context. */
  1799	const struct bpf_func_proto *
  1800	cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
  1801	{
  1802		switch (func_id) {
  1803	#ifdef CONFIG_CGROUPS
  1804		case BPF_FUNC_get_current_uid_gid:
  1805			return &bpf_get_current_uid_gid_proto;
  1806		case BPF_FUNC_get_current_pid_tgid:
  1807			return &bpf_get_current_pid_tgid_proto;
  1808		case BPF_FUNC_get_current_comm:
  1809			return &bpf_get_current_comm_proto;
  1810		case BPF_FUNC_get_current_cgroup_id:
  1811			return &bpf_get_current_cgroup_id_proto;
  1812		case BPF_FUNC_get_current_ancestor_cgroup_id:
  1813			return &bpf_get_current_ancestor_cgroup_id_proto;
  1814	#endif
  1815	#ifdef CONFIG_CGROUP_NET_CLASSID
  1816		case BPF_FUNC_get_cgroup_classid:
> 1817			return &bpf_get_cgroup_classid_curr_proto;
  1818	#endif
  1819		default:
  1820			return NULL;
  1821		}
  1822	}
  1823	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

* Re: [PATCH bpf-next 2/3] bpf: use cgroup_{common,current}_func_proto in more hooks
  2022-08-12 19:02 ` [PATCH bpf-next 2/3] bpf: use cgroup_{common,current}_func_proto in more hooks Stanislav Fomichev
  2022-08-13  6:25   ` kernel test robot
@ 2022-08-13 18:55   ` kernel test robot
  2022-08-15 21:57   ` Martin KaFai Lau
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2022-08-13 18:55 UTC (permalink / raw)
  To: Stanislav Fomichev, bpf
  Cc: llvm, kbuild-all, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa

Hi Stanislav,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Stanislav-Fomichev/bpf-expose-bpf_-g-s-et_retval-to-more-cgroup-hooks/20220813-030615
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: i386-randconfig-a004 (https://download.01.org/0day-ci/archive/20220814/202208140239.yjzPuMmv-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 3329cec2f79185bafd678f310fafadba2a8c76d2)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/0429824054f7a843ee976d48432e825e493a0a7e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Stanislav-Fomichev/bpf-expose-bpf_-g-s-et_retval-to-more-cgroup-hooks/20220813-030615
        git checkout 0429824054f7a843ee976d48432e825e493a0a7e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> kernel/bpf/helpers.c:1817:11: error: use of undeclared identifier 'bpf_get_cgroup_classid_curr_proto'
                   return &bpf_get_cgroup_classid_curr_proto;
                           ^
   1 error generated.


vim +/bpf_get_cgroup_classid_curr_proto +1817 kernel/bpf/helpers.c

  1797	
  1798	/* Common helpers for cgroup hooks with valid process context. */
  1799	const struct bpf_func_proto *
  1800	cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
  1801	{
  1802		switch (func_id) {
  1803	#ifdef CONFIG_CGROUPS
  1804		case BPF_FUNC_get_current_uid_gid:
  1805			return &bpf_get_current_uid_gid_proto;
  1806		case BPF_FUNC_get_current_pid_tgid:
  1807			return &bpf_get_current_pid_tgid_proto;
  1808		case BPF_FUNC_get_current_comm:
  1809			return &bpf_get_current_comm_proto;
  1810		case BPF_FUNC_get_current_cgroup_id:
  1811			return &bpf_get_current_cgroup_id_proto;
  1812		case BPF_FUNC_get_current_ancestor_cgroup_id:
  1813			return &bpf_get_current_ancestor_cgroup_id_proto;
  1814	#endif
  1815	#ifdef CONFIG_CGROUP_NET_CLASSID
  1816		case BPF_FUNC_get_cgroup_classid:
> 1817			return &bpf_get_cgroup_classid_curr_proto;
  1818	#endif
  1819		default:
  1820			return NULL;
  1821		}
  1822	}
  1823	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

* Re: [PATCH bpf-next 1/3] bpf: introduce cgroup_{common,current}_func_proto
  2022-08-12 19:02 ` [PATCH bpf-next 1/3] bpf: introduce cgroup_{common,current}_func_proto Stanislav Fomichev
@ 2022-08-15 21:26   ` Martin KaFai Lau
  2022-08-16 17:19     ` Stanislav Fomichev
  0 siblings, 1 reply; 10+ messages in thread
From: Martin KaFai Lau @ 2022-08-15 21:26 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, haoluo, jolsa

On Fri, Aug 12, 2022 at 12:02:39PM -0700, Stanislav Fomichev wrote:
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 3c1b9bbcf971..de7d2fabb06d 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -429,7 +429,6 @@ const struct bpf_func_proto bpf_get_current_ancestor_cgroup_id_proto = {
>  };
>  
>  #ifdef CONFIG_CGROUP_BPF
> -
>  BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags)
>  {
>  	/* flags argument is not used now,
> @@ -460,7 +459,37 @@ const struct bpf_func_proto bpf_get_local_storage_proto = {
>  	.arg1_type	= ARG_CONST_MAP_PTR,
>  	.arg2_type	= ARG_ANYTHING,
>  };
> -#endif
> +
> +BPF_CALL_0(bpf_get_retval)
> +{
> +	struct bpf_cg_run_ctx *ctx =
> +		container_of(current->bpf_ctx, struct bpf_cg_run_ctx, run_ctx);
> +
> +	return ctx->retval;
> +}
> +
> +const struct bpf_func_proto bpf_get_retval_proto = {
> +	.func		= bpf_get_retval,
> +	.gpl_only	= false,
> +	.ret_type	= RET_INTEGER,
> +};
> +
> +BPF_CALL_1(bpf_set_retval, int, retval)
> +{
> +	struct bpf_cg_run_ctx *ctx =
> +		container_of(current->bpf_ctx, struct bpf_cg_run_ctx, run_ctx);
> +
> +	ctx->retval = retval;
> +	return 0;
> +}
> +
> +const struct bpf_func_proto bpf_set_retval_proto = {
> +	.func		= bpf_set_retval,
> +	.gpl_only	= false,
> +	.ret_type	= RET_INTEGER,
> +	.arg1_type	= ARG_ANYTHING,
> +};
> +#endif /* CONFIG_CGROUP_BPF */
>  
>  #define BPF_STRTOX_BASE_MASK 0x1F
>  
> @@ -1726,6 +1755,40 @@ bpf_base_func_proto(enum bpf_func_id func_id)
>  	}
>  }
>  
> +/* Common helpers for cgroup hooks. */
> +const struct bpf_func_proto *
> +cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> +{
> +	switch (func_id) {
> +#ifdef CONFIG_CGROUP_BPF
> +	case BPF_FUNC_get_local_storage:
> +		return &bpf_get_local_storage_proto;
> +	case BPF_FUNC_get_retval:
> +		return &bpf_get_retval_proto;
> +	case BPF_FUNC_set_retval:
> +		return &bpf_set_retval_proto;
> +#endif
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +/* Common helpers for cgroup hooks with valid process context. */
> +const struct bpf_func_proto *
> +cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> +{
> +	switch (func_id) {
> +#ifdef CONFIG_CGROUPS
> +	case BPF_FUNC_get_current_uid_gid:
> +		return &bpf_get_current_uid_gid_proto;
> +	case BPF_FUNC_get_current_cgroup_id:
> +		return &bpf_get_current_cgroup_id_proto;
> +#endif
> +	default:
> +		return NULL;
> +	}
> +}
Does it make sense to move all these changes to kernel/bpf/cgroup.c
instead such that there is no need to do 'ifdef CONFIG_CGROUPS*'.
bpf_get_local_storage probably needs to move to kernel/bpf/cgroup.c
also.

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

* Re: [PATCH bpf-next 2/3] bpf: use cgroup_{common,current}_func_proto in more hooks
  2022-08-12 19:02 ` [PATCH bpf-next 2/3] bpf: use cgroup_{common,current}_func_proto in more hooks Stanislav Fomichev
  2022-08-13  6:25   ` kernel test robot
  2022-08-13 18:55   ` kernel test robot
@ 2022-08-15 21:57   ` Martin KaFai Lau
  2022-08-16 17:19     ` Stanislav Fomichev
  2 siblings, 1 reply; 10+ messages in thread
From: Martin KaFai Lau @ 2022-08-15 21:57 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, haoluo, jolsa

On Fri, Aug 12, 2022 at 12:02:40PM -0700, Stanislav Fomichev wrote:
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index de7d2fabb06d..87ce47b13b22 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1764,9 +1764,31 @@ cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  	case BPF_FUNC_get_local_storage:
>  		return &bpf_get_local_storage_proto;
>  	case BPF_FUNC_get_retval:
> -		return &bpf_get_retval_proto;
> +		switch (prog->expected_attach_type) {
> +		case BPF_CGROUP_SOCK_OPS:
> +		case BPF_CGROUP_UDP4_RECVMSG:
> +		case BPF_CGROUP_UDP6_RECVMSG:
> +		case BPF_CGROUP_INET4_GETPEERNAME:
> +		case BPF_CGROUP_INET6_GETPEERNAME:
> +		case BPF_CGROUP_INET4_GETSOCKNAME:
> +		case BPF_CGROUP_INET6_GETSOCKNAME:
> +			return NULL;
> +		default:
> +			return &bpf_get_retval_proto;
> +		}
>  	case BPF_FUNC_set_retval:
> -		return &bpf_set_retval_proto;
> +		switch (prog->expected_attach_type) {
> +		case BPF_CGROUP_SOCK_OPS:
> +		case BPF_CGROUP_UDP4_RECVMSG:
> +		case BPF_CGROUP_UDP6_RECVMSG:
> +		case BPF_CGROUP_INET4_GETPEERNAME:
> +		case BPF_CGROUP_INET6_GETPEERNAME:
> +		case BPF_CGROUP_INET4_GETSOCKNAME:
> +		case BPF_CGROUP_INET6_GETSOCKNAME:
> +			return NULL;
> +		default:
> +			return &bpf_set_retval_proto;
> +		}
Does it make sense to have bpf_lsm_func_proto() calling
cgroup_common_func_proto() also?

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

* Re: [PATCH bpf-next 1/3] bpf: introduce cgroup_{common,current}_func_proto
  2022-08-15 21:26   ` Martin KaFai Lau
@ 2022-08-16 17:19     ` Stanislav Fomichev
  0 siblings, 0 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2022-08-16 17:19 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, haoluo, jolsa

On Mon, Aug 15, 2022 at 2:27 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Fri, Aug 12, 2022 at 12:02:39PM -0700, Stanislav Fomichev wrote:
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 3c1b9bbcf971..de7d2fabb06d 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -429,7 +429,6 @@ const struct bpf_func_proto bpf_get_current_ancestor_cgroup_id_proto = {
> >  };
> >
> >  #ifdef CONFIG_CGROUP_BPF
> > -
> >  BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags)
> >  {
> >       /* flags argument is not used now,
> > @@ -460,7 +459,37 @@ const struct bpf_func_proto bpf_get_local_storage_proto = {
> >       .arg1_type      = ARG_CONST_MAP_PTR,
> >       .arg2_type      = ARG_ANYTHING,
> >  };
> > -#endif
> > +
> > +BPF_CALL_0(bpf_get_retval)
> > +{
> > +     struct bpf_cg_run_ctx *ctx =
> > +             container_of(current->bpf_ctx, struct bpf_cg_run_ctx, run_ctx);
> > +
> > +     return ctx->retval;
> > +}
> > +
> > +const struct bpf_func_proto bpf_get_retval_proto = {
> > +     .func           = bpf_get_retval,
> > +     .gpl_only       = false,
> > +     .ret_type       = RET_INTEGER,
> > +};
> > +
> > +BPF_CALL_1(bpf_set_retval, int, retval)
> > +{
> > +     struct bpf_cg_run_ctx *ctx =
> > +             container_of(current->bpf_ctx, struct bpf_cg_run_ctx, run_ctx);
> > +
> > +     ctx->retval = retval;
> > +     return 0;
> > +}
> > +
> > +const struct bpf_func_proto bpf_set_retval_proto = {
> > +     .func           = bpf_set_retval,
> > +     .gpl_only       = false,
> > +     .ret_type       = RET_INTEGER,
> > +     .arg1_type      = ARG_ANYTHING,
> > +};
> > +#endif /* CONFIG_CGROUP_BPF */
> >
> >  #define BPF_STRTOX_BASE_MASK 0x1F
> >
> > @@ -1726,6 +1755,40 @@ bpf_base_func_proto(enum bpf_func_id func_id)
> >       }
> >  }
> >
> > +/* Common helpers for cgroup hooks. */
> > +const struct bpf_func_proto *
> > +cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > +{
> > +     switch (func_id) {
> > +#ifdef CONFIG_CGROUP_BPF
> > +     case BPF_FUNC_get_local_storage:
> > +             return &bpf_get_local_storage_proto;
> > +     case BPF_FUNC_get_retval:
> > +             return &bpf_get_retval_proto;
> > +     case BPF_FUNC_set_retval:
> > +             return &bpf_set_retval_proto;
> > +#endif
> > +     default:
> > +             return NULL;
> > +     }
> > +}
> > +
> > +/* Common helpers for cgroup hooks with valid process context. */
> > +const struct bpf_func_proto *
> > +cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > +{
> > +     switch (func_id) {
> > +#ifdef CONFIG_CGROUPS
> > +     case BPF_FUNC_get_current_uid_gid:
> > +             return &bpf_get_current_uid_gid_proto;
> > +     case BPF_FUNC_get_current_cgroup_id:
> > +             return &bpf_get_current_cgroup_id_proto;
> > +#endif
> > +     default:
> > +             return NULL;
> > +     }
> > +}
> Does it make sense to move all these changes to kernel/bpf/cgroup.c
> instead such that there is no need to do 'ifdef CONFIG_CGROUPS*'.
> bpf_get_local_storage probably needs to move to kernel/bpf/cgroup.c
> also.

Hm, didn't think about it, let me try moving everything under
bpf/cgroup.c instead..

(saw the build failures but decided not to spam with a v2)

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

* Re: [PATCH bpf-next 2/3] bpf: use cgroup_{common,current}_func_proto in more hooks
  2022-08-15 21:57   ` Martin KaFai Lau
@ 2022-08-16 17:19     ` Stanislav Fomichev
  0 siblings, 0 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2022-08-16 17:19 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, haoluo, jolsa

On Mon, Aug 15, 2022 at 2:58 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Fri, Aug 12, 2022 at 12:02:40PM -0700, Stanislav Fomichev wrote:
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index de7d2fabb06d..87ce47b13b22 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1764,9 +1764,31 @@ cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >       case BPF_FUNC_get_local_storage:
> >               return &bpf_get_local_storage_proto;
> >       case BPF_FUNC_get_retval:
> > -             return &bpf_get_retval_proto;
> > +             switch (prog->expected_attach_type) {
> > +             case BPF_CGROUP_SOCK_OPS:
> > +             case BPF_CGROUP_UDP4_RECVMSG:
> > +             case BPF_CGROUP_UDP6_RECVMSG:
> > +             case BPF_CGROUP_INET4_GETPEERNAME:
> > +             case BPF_CGROUP_INET6_GETPEERNAME:
> > +             case BPF_CGROUP_INET4_GETSOCKNAME:
> > +             case BPF_CGROUP_INET6_GETSOCKNAME:
> > +                     return NULL;
> > +             default:
> > +                     return &bpf_get_retval_proto;
> > +             }
> >       case BPF_FUNC_set_retval:
> > -             return &bpf_set_retval_proto;
> > +             switch (prog->expected_attach_type) {
> > +             case BPF_CGROUP_SOCK_OPS:
> > +             case BPF_CGROUP_UDP4_RECVMSG:
> > +             case BPF_CGROUP_UDP6_RECVMSG:
> > +             case BPF_CGROUP_INET4_GETPEERNAME:
> > +             case BPF_CGROUP_INET6_GETPEERNAME:
> > +             case BPF_CGROUP_INET4_GETSOCKNAME:
> > +             case BPF_CGROUP_INET6_GETSOCKNAME:
> > +                     return NULL;
> > +             default:
> > +                     return &bpf_set_retval_proto;
> > +             }
> Does it make sense to have bpf_lsm_func_proto() calling
> cgroup_common_func_proto() also?

Oh, sure, will do, thanks!

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

end of thread, other threads:[~2022-08-16 17:19 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-12 19:02 [PATCH bpf-next 0/3] bpf: expose bpf_{g,s}et_retval to more cgroup hooks Stanislav Fomichev
2022-08-12 19:02 ` [PATCH bpf-next 1/3] bpf: introduce cgroup_{common,current}_func_proto Stanislav Fomichev
2022-08-15 21:26   ` Martin KaFai Lau
2022-08-16 17:19     ` Stanislav Fomichev
2022-08-12 19:02 ` [PATCH bpf-next 2/3] bpf: use cgroup_{common,current}_func_proto in more hooks Stanislav Fomichev
2022-08-13  6:25   ` kernel test robot
2022-08-13 18:55   ` kernel test robot
2022-08-15 21:57   ` Martin KaFai Lau
2022-08-16 17:19     ` Stanislav Fomichev
2022-08-12 19:02 ` [PATCH bpf-next 3/3] selftests/bpf: make sure bpf_{g,s}et_retval is exposed everywhere Stanislav Fomichev

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.