diff mbox series

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

Message ID 20220812190241.3544528-2-sdf@google.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: expose bpf_{g,s}et_retval to more cgroup hooks | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR fail PR summary
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1399 this patch: 1399
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 163 this patch: 163
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1391 this patch: 1391
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 12 this patch: 12
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-1 fail Logs for Kernel LATEST on ubuntu-latest with gcc
bpf/vmtest-bpf-next-VM_Test-2 fail Logs for Kernel LATEST on ubuntu-latest with llvm-16
bpf/vmtest-bpf-next-VM_Test-3 fail Logs for Kernel LATEST on z15 with gcc

Commit Message

Stanislav Fomichev Aug. 12, 2022, 7:02 p.m. UTC 
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(-)

Comments

Martin KaFai Lau Aug. 15, 2022, 9:26 p.m. UTC | #1 
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.
Stanislav Fomichev Aug. 16, 2022, 5:19 p.m. UTC | #2 
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)
diff mbox series

Patch

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)