Re: [RFC][PATCH 2/4] bpf: Allow BPF_JIT with CONFIG_MODULES=n

[Christophe Leroy]

Le 06/03/2024 à 21:05, Calvin Owens a écrit :
> [Vous ne recevez pas souvent de courriers de jcalvinowens@xxxxxxxxx. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> No BPF code has to change, except in struct_ops (for module refs).
> 
> This conflicts with bpf-next because of this (relevant) series:
> 
>      https://lore.kernel.org/all/20240119225005.668602-1-thinker.li@xxxxxxxxx/
> 
> If something like this is merged down the road, it can go through
> bpf-next at leisure once the module_alloc change is in: it's a one-way
> dependency.
> 
> Signed-off-by: Calvin Owens <jcalvinowens@xxxxxxxxx>
> ---
>   kernel/bpf/Kconfig          |  2 +-
>   kernel/bpf/bpf_struct_ops.c | 28 ++++++++++++++++++++++++----
>   2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/bpf/Kconfig b/kernel/bpf/Kconfig
> index 6a906ff93006..77df483a8925 100644
> --- a/kernel/bpf/Kconfig
> +++ b/kernel/bpf/Kconfig
> @@ -42,7 +42,7 @@ config BPF_JIT
>          bool "Enable BPF Just In Time compiler"
>          depends on BPF
>          depends on HAVE_CBPF_JIT || HAVE_EBPF_JIT
> -       depends on MODULES
> +       select MODULE_ALLOC
>          help
>            BPF programs are normally handled by a BPF interpreter. This option
>            allows the kernel to generate native code when a program is loaded
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 02068bd0e4d9..fbf08a1bb00c 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -108,11 +108,30 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
>   #endif
>   };
> 
> +#if IS_ENABLED(CONFIG_MODULES)

Can you avoid ifdefs as much as possible ?

>   static const struct btf_type *module_type;
> 
> +static int bpf_struct_module_type_init(struct btf *btf)
> +{
> +       s32 module_id;

Could be:

	if (!IS_ENABLED(CONFIG_MODULES))
		return 0;

> +
> +       module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
> +       if (module_id < 0)
> +               return 1;
> +
> +       module_type = btf_type_by_id(btf, module_id);
> +       return 0;
> +}
> +#else
> +static int bpf_struct_module_type_init(struct btf *btf)
> +{
> +       return 0;
> +}
> +#endif
> +
>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>   {
> -       s32 type_id, value_id, module_id;
> +       s32 type_id, value_id;
>          const struct btf_member *member;
>          struct bpf_struct_ops *st_ops;
>          const struct btf_type *t;
> @@ -125,12 +144,10 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>   #include "bpf_struct_ops_types.h"
>   #undef BPF_STRUCT_OPS_TYPE
> 
> -       module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
> -       if (module_id < 0) {
> +       if (bpf_struct_module_type_init(btf)) {
>                  pr_warn("Cannot find struct module in btf_vmlinux\n");
>                  return;
>          }
> -       module_type = btf_type_by_id(btf, module_id);
> 
>          for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
>                  st_ops = bpf_struct_ops[i];
> @@ -433,12 +450,15 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
> 
>                  moff = __btf_member_bit_offset(t, member) / 8;
>                  ptype = btf_type_resolve_ptr(btf_vmlinux, member->type, NULL);
> +
> +#if IS_ENABLED(CONFIG_MODULES)

Can't see anything depending on CONFIG_MODULES here, can you instead do:

		if (IS_ENABLED(CONFIG_MODULES) && ptype == module_type) {

>                  if (ptype == module_type) {
>                          if (*(void **)(udata + moff))
>                                  goto reset_unlock;
>                          *(void **)(kdata + moff) = BPF_MODULE_OWNER;
>                          continue;
>                  }
> +#endif
> 
>                  err = st_ops->init_member(t, member, kdata, udata);
>                  if (err < 0)
> --
> 2.43.0
> 
>