bpf: expose bpf_{g,s}et_retval to more cgroup hooks

PATCH Superseded Stanislav Fomichev 2 participants 12 messages Aug 18, 2022 – Aug 22, 2022

Subsystems (7)

Networking Kselftest BPF BPF selftests +3 more subsystems

lore

Share & tools

lore

Stanislav Fomichev

author

2022-08-18 23:27 UTC #0 lore raw

v3 cover bpf: expose bpf_{g,s}et_retval to more cgroup hooks

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.

v3:
- expose strtol/strtoul everywhere (Martin)
- move helpers declarations from bpf.h to bpf-cgroup.h (Martin)
- revise bpf_{g,s}et_retval documentation (Martin)
- don't expose bpf_{g,s}et_retval to cg_skb hooks (Martin)

v2:
- move everything into kernel/bpf/cgroup.c instead (Martin)
- use cgroup_common_func_proto in lsm (Martin)

Stanislav Fomichev (5):
bpf: Introduce cgroup_{common,current}_func_proto
bpf: Use cgroup_{common,current}_func_proto in more hooks
bpf: expose bpf_strtol and bpf_strtoul to all program types
bpf: update bpf_{g,s}et_retval documentation selftests/bpf: Make sure bpf_{g,s}et_retval is exposed everywhere

 include/linux/bpf-cgroup.h                    |  17 ++
 include/uapi/linux/bpf.h                      |  22 +-
 kernel/bpf/bpf_lsm.c                          |  19 +-
 kernel/bpf/cgroup.c                           | 212 ++++++++++++++++--
 kernel/bpf/helpers.c                          |  82 +------
 net/core/filter.c                             |  92 +++-----
 tools/include/uapi/linux/bpf.h                |  22 +-
 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 ++
 11 files changed, 380 insertions(+), 176 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

Signature · 1 line

2.37.1.595.g718a3a8f04-goog

Stanislav Fomichev

author

2022-08-18 23:27 UTC #1 lore raw

PATCH v3 1/5 bpf: Introduce cgroup_{common,current}_func_proto

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 and other cgroup-related helpers into kernel/bpf/cgroup.c so they closer to where they are being used.

Signed-off-by Stanislav Fomichev <[email protected]>

---
 include/linux/bpf-cgroup.h |  17 ++++
 kernel/bpf/cgroup.c        | 172 +++++++++++++++++++++++++++++++++----
 kernel/bpf/helpers.c       |  77 -----------------
 net/core/filter.c          |  14 +--
 4 files changed, 173 insertions(+), 107 deletions(-)

include/linux/bpf-cgroup.h +17 -0

index 2bd1b5f8de9b..57e9e109257e 100644
@@ -414,6 +414,11 @@ int cgroup_bpf_prog_detach(const union bpf_attr *attr,
 int cgroup_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
 int cgroup_bpf_prog_query(const union bpf_attr *attr,
 			  union bpf_attr __user *uattr);
+
+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);
 #else
 
 static inline int cgroup_bpf_inherit(struct cgroup *cgrp) { return 0; }

@@ -444,6 +449,18 @@ static inline int cgroup_bpf_prog_query(const union bpf_attr *attr,
 	return -EINVAL;
 }
 
+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 int bpf_cgroup_storage_assign(struct bpf_prog_aux *aux,
 					    struct bpf_map *map) { return 0; }
 static inline struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(

kernel/bpf/cgroup.c +153 ~~-19~~

229 lines not on this page · raw message

kernel/bpf/helpers.c +0 ~~-77~~

89 lines not on this page · raw message

net/core/filter.c +3 ~~-11~~

index e8508aaafd27..df6bae0f98a7 100644
@@ -3004,17 +3004,6 @@ static const struct bpf_func_proto bpf_msg_pop_data_proto = {
 };
 
 #ifdef CONFIG_CGROUP_NET_CLASSID
-BPF_CALL_0(bpf_get_cgroup_classid_curr)
-{
-	return __task_get_classid(current);
-}
-
-static const struct bpf_func_proto bpf_get_cgroup_classid_curr_proto = {
-	.func		= bpf_get_cgroup_classid_curr,
-	.gpl_only	= false,
-	.ret_type	= RET_INTEGER,
-};
-
 BPF_CALL_1(bpf_skb_cgroup_classid, const struct sk_buff *, skb)
 {
 	struct sock *sk = skb_to_full_sk(skb);

@@ -8104,6 +8093,9 @@ sock_ops_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 
 const struct bpf_func_proto bpf_msg_redirect_map_proto __weak;
 const struct bpf_func_proto bpf_msg_redirect_hash_proto __weak;
+const struct bpf_func_proto bpf_get_cgroup_classid_curr_proto __weak;
+const struct bpf_func_proto bpf_get_current_cgroup_id_proto __weak;
+const struct bpf_func_proto bpf_get_current_ancestor_cgroup_id_proto __weak;
 
 static const struct bpf_func_proto *
 sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

Signature · 1 line

2.37.1.595.g718a3a8f04-goog

Martin KaFai Lau

2022-08-19 18:18 UTC #2 lore raw

Re: [PATCH bpf-next v3 1/5] bpf: Introduce cgroup_{common,current}_func_proto

Quoted 30 lines · Stanislav Fomichev

kernel/bpf/cgroup.c +30 -0

+BPF_CALL_0(bpf_get_current_cgroup_id)
+{
+	struct cgroup *cgrp;
+	u64 cgrp_id;
+
+	rcu_read_lock();
+	cgrp = task_dfl_cgroup(current);
+	cgrp_id = cgroup_id(cgrp);
+	rcu_read_unlock();
+
+	return cgrp_id;
+}
+
+const struct bpf_func_proto bpf_get_current_cgroup_id_proto = {
+	.func		= bpf_get_current_cgroup_id,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+};
+
+BPF_CALL_1(bpf_get_current_ancestor_cgroup_id, int, ancestor_level)
+{
+	struct cgroup *cgrp;
+	struct cgroup *ancestor;
+	u64 cgrp_id;
+
+	rcu_read_lock();
+	cgrp = task_dfl_cgroup(current);
+	ancestor = cgroup_ancestor(cgrp, ancestor_level);
+	cgrp_id = ancestor ? cgroup_id(ancestor) : 0;
+	rcu_read_unlock();

kernel/bpf/cgroup.c int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor, · line 1570

Quoted 10 lines

kernel/bpf/cgroup.c +10 -0

+
+	return cgrp_id;
+}
+
+const struct bpf_func_proto bpf_get_current_ancestor_cgroup_id_proto = {
+	.func		= bpf_get_current_ancestor_cgroup_id,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_ANYTHING,
+};

The bpf_get_current_cgroup_id_proto and bpf_get_current_ancestor_cgroup_id_proto should stay at helpers.c. Otherwise, those non cgroup hooks will have issue (eg. bpf_trace.c) when CONFIG_CGROUP_BPF not set.

May be in the future cgroup_current_func_proto() can be re-used for non cgroup hooks.

kernel/bpf/cgroup.c const struct bpf_func_proto bpf_set_retval_proto = { · line 1644

Quoted 12 lines

kernel/bpf/cgroup.c +12 -0

+#ifdef CONFIG_CGROUP_NET_CLASSID
+BPF_CALL_0(bpf_get_cgroup_classid_curr)
+{
+	return __task_get_classid(current);
+}
+
+const struct bpf_func_proto bpf_get_cgroup_classid_curr_proto = {
+	.func		= bpf_get_cgroup_classid_curr,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+};
+#endif

Same for this one. eg. sk_msg needs it. probably stay in filter.c as-is.

Stanislav Fomichev

author

2022-08-22 18:44 UTC #3 lore raw

Re: [PATCH bpf-next v3 1/5] bpf: Introduce cgroup_{common,current}_func_proto

Quoted 38 lines · Martin KaFai Lau

Quoted 36 lines · Stanislav Fomichev

kernel/bpf/cgroup.c +36 -0

+BPF_CALL_0(bpf_get_current_cgroup_id)
+{
+     struct cgroup *cgrp;
+     u64 cgrp_id;
+
+     rcu_read_lock();
+     cgrp = task_dfl_cgroup(current);
+     cgrp_id = cgroup_id(cgrp);
+     rcu_read_unlock();
+
+     return cgrp_id;
+}
+
+const struct bpf_func_proto bpf_get_current_cgroup_id_proto = {
+     .func           = bpf_get_current_cgroup_id,
+     .gpl_only       = false,
+     .ret_type       = RET_INTEGER,
+};
+
+BPF_CALL_1(bpf_get_current_ancestor_cgroup_id, int, ancestor_level)
+{
+     struct cgroup *cgrp;
+     struct cgroup *ancestor;
+     u64 cgrp_id;
+
+     rcu_read_lock();
+     cgrp = task_dfl_cgroup(current);
+     ancestor = cgroup_ancestor(cgrp, ancestor_level);
+     cgrp_id = ancestor ? cgroup_id(ancestor) : 0;
+     rcu_read_unlock();
+
+     return cgrp_id;
+}
+
+const struct bpf_func_proto bpf_get_current_ancestor_cgroup_id_proto = {
+     .func           = bpf_get_current_ancestor_cgroup_id,

Quoted 8 lines

kernel/bpf/cgroup.c int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor, · line 1570
Quoted 4 lines
kernel/bpf/cgroup.c +4 -0
+     .gpl_only       = false,
+     .ret_type       = RET_INTEGER,
+     .arg1_type      = ARG_ANYTHING,
+};
The bpf_get_current_cgroup_id_proto and bpf_get_current_ancestor_cgroup_id_proto should stay at helpers.c. Otherwise, those non cgroup hooks will have issue (eg. bpf_trace.c) when CONFIG_CGROUP_BPF not set.

Oh, good point on bpf_trace.c, will keep in helpers.c I'm now a bit surprised I haven't seen a build failure :-/ Thank you for the review! Will address your other point and resend.

Quoted 16 lines

May be in the future cgroup_current_func_proto() can be re-used for non cgroup hooks.
kernel/bpf/cgroup.c const struct bpf_func_proto bpf_set_retval_proto = { · line 1644
Quoted 12 lines
kernel/bpf/cgroup.c +12 -0
+#ifdef CONFIG_CGROUP_NET_CLASSID
+BPF_CALL_0(bpf_get_cgroup_classid_curr)
+{
+     return __task_get_classid(current);
+}
+
+const struct bpf_func_proto bpf_get_cgroup_classid_curr_proto = {
+     .func           = bpf_get_cgroup_classid_curr,
+     .gpl_only       = false,
+     .ret_type       = RET_INTEGER,
+};
+#endif
Same for this one. eg. sk_msg needs it. probably stay in filter.c as-is.

Stanislav Fomichev

author

2022-08-18 23:27 UTC #4 lore raw

PATCH v3 2/5 bpf: Use cgroup_{common,current}_func_proto in more hooks

In reply to Stanislav Fomichev

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)
* BPF_PROG_TYPE_LSM+BPF_LSM_CGROUP

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 ignored 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 <[email protected]>

---
 kernel/bpf/bpf_lsm.c | 19 ++++++-----
 kernel/bpf/cgroup.c  | 40 +++++++++++++++++++++--
 kernel/bpf/helpers.c |  1 +
 net/core/filter.c    | 78 ++++++++++++++++++--------------------------
 4 files changed, 81 insertions(+), 57 deletions(-)

kernel/bpf/bpf_lsm.c +10 -9

index fa71d58b7ded..6eba60248e20 100644
@@ -189,6 +189,16 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = {
 static const struct bpf_func_proto *
 bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
+#ifdef CONFIG_CGROUP_BPF
+	const struct bpf_func_proto *func_proto;
+
+	if (prog->expected_attach_type == BPF_LSM_CGROUP) {
+		func_proto = cgroup_common_func_proto(func_id, prog);
+		if (func_proto)
+			return func_proto;
+	}
+#endif
+
 	switch (func_id) {
 	case BPF_FUNC_inode_storage_get:
 		return &bpf_inode_storage_get_proto;

@@ -212,15 +222,6 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return prog->aux->sleepable ? &bpf_ima_file_hash_proto : NULL;
 	case BPF_FUNC_get_attach_cookie:
 		return bpf_prog_has_trampoline(prog) ? &bpf_get_attach_cookie_proto : NULL;
-	case BPF_FUNC_get_local_storage:
-		return prog->expected_attach_type == BPF_LSM_CGROUP ?
-			&bpf_get_local_storage_proto : NULL;
-	case BPF_FUNC_set_retval:
-		return prog->expected_attach_type == BPF_LSM_CGROUP ?
-			&bpf_set_retval_proto : NULL;
-	case BPF_FUNC_get_retval:
-		return prog->expected_attach_type == BPF_LSM_CGROUP ?
-			&bpf_get_retval_proto : NULL;
 #ifdef CONFIG_NET
 	case BPF_FUNC_setsockopt:
 		if (prog->expected_attach_type != BPF_LSM_CGROUP)

kernel/bpf/cgroup.c +38 -2

index 74b5cc692a36..00988312279f 100644
@@ -2533,9 +2533,35 @@ 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_INET_INGRESS:
+		case BPF_CGROUP_INET_EGRESS:
+		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_INET_INGRESS:
+		case BPF_CGROUP_INET_EGRESS:
+		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;
+		}
 	default:
 		return NULL;
 	}

@@ -2548,8 +2574,18 @@ cgroup_current_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_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;
+#ifdef CONFIG_CGROUP_NET_CLASSID
+	case BPF_FUNC_get_cgroup_classid:
+		return &bpf_get_cgroup_classid_curr_proto;
+#endif
 	default:
 		return NULL;
 	}

kernel/bpf/helpers.c +1 -0

index 71979d870646..53451ea6721c 100644
@@ -1521,6 +1521,7 @@ const struct bpf_func_proto bpf_probe_read_user_str_proto __weak;
 const struct bpf_func_proto bpf_probe_read_kernel_proto __weak;
 const struct bpf_func_proto bpf_probe_read_kernel_str_proto __weak;
 const struct bpf_func_proto bpf_task_pt_regs_proto __weak;
+const struct bpf_func_proto bpf_get_cgroup_classid_curr_proto __weak;
 
 const struct bpf_func_proto *
 bpf_base_func_proto(enum bpf_func_id func_id)

net/core/filter.c +32 ~~-46~~

125 lines not on this page · raw message

Signature · 1 line

2.37.1.595.g718a3a8f04-goog

Martin KaFai Lau

2022-08-19 18:21 UTC #5 lore raw

Re: [PATCH bpf-next v3 2/5] bpf: Use cgroup_{common,current}_func_proto in more hooks

kernel/bpf/bpf_lsm.c static const struct bpf_func_proto bpf_get_attach_cookie_proto = { · line 192

Quoted 9 lines · Stanislav Fomichev

kernel/bpf/bpf_lsm.c +1 -0

index fa71d58b7ded..6eba60248e20 100644
@@ -189,6 +189,16 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = {
 static const struct bpf_func_proto *
 bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
+#ifdef CONFIG_CGROUP_BPF

This probably is not needed. Others lgtm.

Quoted 8 lines

kernel/bpf/bpf_lsm.c +8 -0

+	const struct bpf_func_proto *func_proto;
+
+	if (prog->expected_attach_type == BPF_LSM_CGROUP) {
+		func_proto = cgroup_common_func_proto(func_id, prog);
+		if (func_proto)
+			return func_proto;
+	}
+#endif

Stanislav Fomichev

author

2022-08-18 23:27 UTC #6 lore raw

PATCH v3 3/5 bpf: expose bpf_strtol and bpf_strtoul to all program types

In reply to Stanislav Fomichev

bpf_strncmp is already exposed everywhere. The motivation is to keep those helpers in kernel/bpf/helpers.c. Otherwise it's tempting to move them under kernel/bpf/cgroup.c because they are currently only used by sysctl prog types.

Signed-off-by Stanislav Fomichev <[email protected]>

---
 kernel/bpf/cgroup.c  | 4 ----
 kernel/bpf/helpers.c | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

kernel/bpf/cgroup.c +0 -4

index 00988312279f..47796280232b 100644
@@ -2187,10 +2187,6 @@ sysctl_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return func_proto;
 
 	switch (func_id) {
-	case BPF_FUNC_strtol:
-		return &bpf_strtol_proto;
-	case BPF_FUNC_strtoul:
-		return &bpf_strtoul_proto;
 	case BPF_FUNC_sysctl_get_name:
 		return &bpf_sysctl_get_name_proto;
 	case BPF_FUNC_sysctl_get_current_value:

kernel/bpf/helpers.c +4 -0

index 53451ea6721c..8bb78acfe88d 100644
@@ -1577,6 +1577,10 @@ bpf_base_func_proto(enum bpf_func_id func_id)
 		return &bpf_loop_proto;
 	case BPF_FUNC_strncmp:
 		return &bpf_strncmp_proto;
+	case BPF_FUNC_strtol:
+		return &bpf_strtol_proto;
+	case BPF_FUNC_strtoul:
+		return &bpf_strtoul_proto;
 	case BPF_FUNC_dynptr_from_mem:
 		return &bpf_dynptr_from_mem_proto;
 	case BPF_FUNC_dynptr_read:

Signature · 1 line

2.37.1.595.g718a3a8f04-goog

Martin KaFai Lau

Acked-by on v3 3/5

2022-08-19 18:22 UTC #7 lore raw

Re: [PATCH bpf-next v3 3/5] bpf: expose bpf_strtol and bpf_strtoul to all program types

Quoted 4 lines · Stanislav Fomichev

bpf_strncmp is already exposed everywhere. The motivation is to keep those helpers in kernel/bpf/helpers.c. Otherwise it's tempting to move them under kernel/bpf/cgroup.c because they are currently only used by sysctl prog types.

Acked-by Martin KaFai Lau <[email protected]>

Stanislav Fomichev

author

2022-08-18 23:27 UTC #8 lore raw

PATCH v3 4/5 bpf: update bpf_{g,s}et_retval documentation

In reply to Stanislav Fomichev

* replace 'syscall' with 'upper layers', still mention that it's being exported via syscall errno
* describe what happens in set_retval(-EPERM) + return 1
* describe what happens with bind's 'return 3'

Signed-off-by Stanislav Fomichev <[email protected]>

---
 include/uapi/linux/bpf.h       | 22 +++++++++++++++++-----
 tools/include/uapi/linux/bpf.h | 22 +++++++++++++++++-----
 2 files changed, 34 insertions(+), 10 deletions(-)

include/uapi/linux/bpf.h +17 -5

index 934a2a8beb87..954b897a631e 100644
@@ -5085,17 +5085,29 @@ union bpf_attr {
  *
  * int bpf_get_retval(void)
  *	Description
- *		Get the syscall's return value that will be returned to userspace.
+ *		Get the BPF program's return value that will be returned to the upper layers.
  *
- *		This helper is currently supported by cgroup programs only.
+ *		This helper is currently supported by cgroup programs and only by the hooks
+ *		where BPF program's return value is returned to the userspace via errno.
  *	Return
- *		The syscall's return value.
+ *		The BPF program's return value.
  *
  * int bpf_set_retval(int retval)
  *	Description
- *		Set the syscall's return value that will be returned to userspace.
+ *		Set the BPF program's return value that will be returned to the upper layers.
+ *
+ *		This helper is currently supported by cgroup programs and only by the hooks
+ *		where BPF program's return value is returned to the userspace via errno.
+ *
+ *		Note that there is the following corner case where the program exports an error
+ *		via bpf_set_retval but signals success via 'return 1':
+ *
+ *			bpf_set_retval(-EPERM);
+ *			return 1;
+ *
+ *		In this case, the BPF program's return value will use helper's -EPERM. This
+ *		still holds true for cgroup/bind{4,6} which supports extra 'return 3' success case.
  *
- *		This helper is currently supported by cgroup programs only.
  *	Return
  *		0 on success, or a negative error in case of failure.
  *

tools/include/uapi/linux/bpf.h +17 -5

index 1d6085e15fc8..b99ff5f34c61 100644
@@ -5085,17 +5085,29 @@ union bpf_attr {
  *
  * int bpf_get_retval(void)
  *	Description
- *		Get the syscall's return value that will be returned to userspace.
+ *		Get the BPF program's return value that will be returned to the upper layers.
  *
- *		This helper is currently supported by cgroup programs only.
+ *		This helper is currently supported by cgroup programs and only by the hooks
+ *		where BPF program's return value is returned to the userspace via errno.
  *	Return
- *		The syscall's return value.
+ *		The BPF program's return value.
  *
  * int bpf_set_retval(int retval)
  *	Description
- *		Set the syscall's return value that will be returned to userspace.
+ *		Set the BPF program's return value that will be returned to the upper layers.
+ *
+ *		This helper is currently supported by cgroup programs and only by the hooks
+ *		where BPF program's return value is returned to the userspace via errno.
+ *
+ *		Note that there is the following corner case where the program exports an error
+ *		via bpf_set_retval but signals success via 'return 1':
+ *
+ *			bpf_set_retval(-EPERM);
+ *			return 1;
+ *
+ *		In this case, the BPF program's return value will use helper's -EPERM. This
+ *		still holds true for cgroup/bind{4,6} which supports extra 'return 3' success case.
  *
- *		This helper is currently supported by cgroup programs only.
  *	Return
  *		0 on success, or a negative error in case of failure.
  *

Signature · 1 line

2.37.1.595.g718a3a8f04-goog

Martin KaFai Lau

Acked-by on v3 4/5

2022-08-19 18:25 UTC #9 lore raw

Re: [PATCH bpf-next v3 4/5] bpf: update bpf_{g,s}et_retval documentation

Quoted 4 lines · Stanislav Fomichev

* replace 'syscall' with 'upper layers', still mention that it's being exported via syscall errno
* describe what happens in set_retval(-EPERM) + return 1
* describe what happens with bind's 'return 3'

Acked-by Martin KaFai Lau <[email protected]>

Stanislav Fomichev

author

2022-08-18 23:27 UTC #10 lore raw

PATCH v3 5/5 selftests/bpf: Make sure bpf_{g,s}et_retval is exposed everywhere

In reply to Stanislav Fomichev

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 <[email protected]>

---
 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

tools/testing/selftests/bpf/Makefile +1 -0

index 8d59ec7f4c2d..eecad99f1735 100644
@@ -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

tools/testing/selftests/bpf/cgroup_getset_retval_hooks.h new file +25 -0

new file mode 100644
index 000000000000..a525d3544fd7
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+BPF_RETVAL_HOOK(ingress, "cgroup_skb/ingress", __sk_buff, -EINVAL)
+BPF_RETVAL_HOOK(egress, "cgroup_skb/egress", __sk_buff, -EINVAL)
+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)

tools/testing/selftests/bpf/prog_tests/cgroup_getset_retval.c +48 -0

66 lines not on this page · raw message

tools/testing/selftests/bpf/progs/cgroup_getset_retval_hooks.c new file +16 -0

new file mode 100644
index 000000000000..13dfb4bbfd28
@@ -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

Signature · 1 line

2.37.1.595.g718a3a8f04-goog

Martin KaFai Lau

Acked-by on v3 5/5

2022-08-19 18:34 UTC #11 lore raw

Re: [PATCH bpf-next v3 5/5] selftests/bpf: Make sure bpf_{g,s}et_retval is exposed everywhere

Quoted 8 lines · Stanislav Fomichev

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

and also
- cg_skb ingress and egress

Acked-by Martin KaFai Lau <[email protected]>