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

PATCH Superseded Stanislav Fomichev 3 participants 10 messages Aug 12, 2022 – Aug 16, 2022

Subsystems (6)

Networking Kselftest BPF BPF selftests +2 more subsystems

lore

Share & tools

lore

Stanislav Fomichev

author

2022-08-12 19:02 UTC #0 lore raw

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.

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

Signature · 1 line

2.37.1.595.g718a3a8f04-goog

Stanislav Fomichev

author

2022-08-12 19:02 UTC #1 lore raw

PATCH 1/3 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 into kernel/bpf/helpers.c so they are more usable from the core parts.

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

---
 include/linux/bpf.h  | 16 +++++++++
 kernel/bpf/cgroup.c  | 82 +++++++++++++++++++-------------------------
 kernel/bpf/helpers.c | 67 ++++++++++++++++++++++++++++++++++--
 3 files changed, 116 insertions(+), 49 deletions(-)

include/linux/bpf.h +16 -0

index a627a02cf8ab..cdb295c6c728 100644
@@ -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)
 {
 }

kernel/bpf/cgroup.c +35 ~~-47~~

126 lines not on this page · raw message

kernel/bpf/helpers.c +65 -2

85 lines not on this page · raw message

Signature · 1 line

2.37.1.595.g718a3a8f04-goog

Martin KaFai Lau

2022-08-16 05:13 UTC #2 lore raw

Re: [PATCH bpf-next 1/3] bpf: introduce cgroup_{common,current}_func_proto

Quoted 78 lines · Stanislav Fomichev

kernel/bpf/helpers.c +54 -2

index 3c1b9bbcf971..de7d2fabb06d 100644
@@ -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) {

kernel/bpf/helpers.c bpf_base_func_proto(enum bpf_func_id func_id) · line 1790

Quoted 10 lines

kernel/bpf/helpers.c +10 -0

+#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.

Stanislav Fomichev

author

2022-08-16 17:19 UTC #3 lore raw

Re: [PATCH bpf-next 1/3] bpf: introduce cgroup_{common,current}_func_proto

Quoted 86 lines · Martin KaFai Lau

Quoted 84 lines · Stanislav Fomichev

kernel/bpf/helpers.c +60 -2

index 3c1b9bbcf971..de7d2fabb06d 100644
@@ -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

Quoted 8 lines

kernel/bpf/helpers.c bpf_base_func_proto(enum bpf_func_id func_id) · line 1774
Quoted 4 lines
kernel/bpf/helpers.c +4 -0
+     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)

Stanislav Fomichev

author

2022-08-12 19:02 UTC #4 lore raw

PATCH 2/3 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)

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

---
 kernel/bpf/helpers.c | 36 ++++++++++++++++++--
 net/core/filter.c    | 78 ++++++++++++++++++--------------------------
 2 files changed, 66 insertions(+), 48 deletions(-)

kernel/bpf/helpers.c +34 -2

index de7d2fabb06d..87ce47b13b22 100644
@@ -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;

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

125 lines not on this page · raw message

Signature · 1 line

2.37.1.595.g718a3a8f04-goog

kernel test robot

build bot

2022-08-13 06:26 UTC #5 lore raw

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

I love your patch! Yet something to improve:

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

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

Signature · 2 lines

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

kernel test robot

build bot

2022-08-13 18:56 UTC #6 lore raw

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

In reply to Stanislav Fomichev

I love your patch! Yet something to improve:

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

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

> 1817			return &bpf_get_cgroup_classid_curr_proto;

1818 #endif
1819 default:
1820 return NULL;
1821 }
1822 }
1823

Signature · 2 lines

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

Martin KaFai Lau

2022-08-16 02:07 UTC #7 lore raw

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

In reply to Stanislav Fomichev

Quoted 25 lines · Stanislav Fomichev

kernel/bpf/helpers.c +14 -2

index de7d2fabb06d..87ce47b13b22 100644
@@ -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:

kernel/bpf/helpers.c cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) · line 1791

Quoted 10 lines

kernel/bpf/helpers.c +10 -0

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

Stanislav Fomichev

author

2022-08-16 17:19 UTC #8 lore raw

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

Quoted 31 lines · Martin KaFai Lau

Quoted 29 lines · Stanislav Fomichev

kernel/bpf/helpers.c +18 -2

index de7d2fabb06d..87ce47b13b22 100644
@@ -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:

Quoted 8 lines

kernel/bpf/helpers.c cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) · line 1791
Quoted 6 lines
kernel/bpf/helpers.c +6 -0
+             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!

Stanislav Fomichev

author

2022-08-12 19:02 UTC #9 lore raw

PATCH 3/3 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..4a8d2063163d
@@ -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)

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