Re: [PATCH bpf-next 1/7] bpf: Allow BPF LSM programs to attach to more hooks

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4295 bytes --]

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 460cb72688450..d01dd1f096fc1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5035,6 +5035,7 @@ L:	bpf@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/bpf/prog_lsm.rst
>  F:	include/linux/bpf_lsm.h
> +F:	include/linux/bpf_lsm_hook_defs.h
>  F:	kernel/bpf/bpf_lsm.c
>  F:	kernel/bpf/bpf_lsm_proto.c
>  F:	kernel/trace/bpf_trace.c

[ ... ]

> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> index dda272d78f013..1e54c7cca27ad 100644
> --- a/include/linux/bpf_lsm.h
> +++ b/include/linux/bpf_lsm.h
> @@ -16,9 +16,19 @@
>
>  extern bool bpf_lsm_initialized __ro_after_init;
>
> +/*
> + * Technically, checking bpf_lsm_initialized is not necessary.
> + * But if it is off, then this means that all security_* calls
> + * do not call BPF, and it doesn't look reasonable to enable
> + * only "non-LSM" bpf hooks...
> + */
> +#define bpf_lsm_hook(NAME, ...) \
> +	(bpf_lsm_initialized ? bpf_lsm_##NAME(__VA_ARGS__) : 0)

Does the new bpf_lsm_hook() macro constrain what hook declarations
can appear in bpf_lsm_hook_defs.h? The macro hardcodes 0 as the return
value in two places:

1. When bpf_lsm_initialized is false, the ternary returns 0, but when
   it is true with no program attached, the __weak nop returns the hook's
   declared DEFAULT. This means LSM_HOOK(int, 0, ...) behaves consistently,
   but LSM_HOOK(int, -EOPNOTSUPP, ...) would return different values
   depending on whether 'bpf' appears in lsm= - 0 before initialization,
   -EOPNOTSUPP after.

2. Because of the ternary, LSM_HOOK(void, ...) cannot compile at call
   sites, since the expression `cond ? void_expr : 0` is invalid.

3. The CONFIG_BPF_LSM=n stub at line 128:

   #define bpf_lsm_hook(NAME, ...) 0

   discards its arguments entirely, so call-site argument checking only
   happens on =y builds. Does this create a risk that =n configurations
   would fail to catch type mismatches or side-effect issues?

>  #define LSM_HOOK(RET, DEFAULT, NAME, ...) \
>  	RET bpf_lsm_##NAME(__VA_ARGS__);
>  #include <linux/lsm_hook_defs.h>
> +#include <linux/bpf_lsm_hook_defs.h>
>  #undef LSM_HOOK

[ ... ]

> diff --git a/include/linux/bpf_lsm_hook_defs.h b/include/linux/bpf_lsm_hook_defs.h
> new file mode 100644
> index 0000000000000..29bc0b514d168
> --- /dev/null
> +++ b/include/linux/bpf_lsm_hook_defs.h
> @@ -0,0 +1,6 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * This is a set of BPF LSM hooks, which are _not_ fully implemented
> + * as LSM hooks. Thus, they only can be used by BPF LSM programs.
> + */
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index 82c5988417a0c..add344ea26914 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -28,11 +28,13 @@ __weak noinline RET bpf_lsm_##NAME(__VA_ARGS__)	\
>  }
>
>  #include <linux/lsm_hook_defs.h>
> +#include <linux/bpf_lsm_hook_defs.h>
>  #undef LSM_HOOK
>
>  #define LSM_HOOK(RET, DEFAULT, NAME, ...) BTF_ID(func, bpf_lsm_##NAME)
>  BTF_SET_START(bpf_lsm_hooks)
>  #include <linux/lsm_hook_defs.h>
> +#include <linux/bpf_lsm_hook_defs.h>
>  #undef LSM_HOOK
>  BTF_SET_END(bpf_lsm_hooks)

The patch adds bpf_lsm_hook_defs.h to three of the four BPF-owned
LSM_HOOK expansion sites, but not the fourth. Does this need updating?

Updated:
  include/linux/bpf_lsm.h (prototypes, lines 30-31)
  kernel/bpf/bpf_lsm.c (__weak nop definitions, lines 30-31)
  kernel/bpf/bpf_lsm.c (bpf_lsm_hooks BTF set, lines 36-37)

Not updated:
  kernel/bpf/bpf_lsm.c (void_lsm_hooks BTF set, further down in the file)

The void_lsm_hooks set is consumed by bpf_lsm_hook_returns_errno() in
bpf_lsm.c, which feeds cgroup_lsm_atype[i].returns_errno. If a hook
declared as LSM_HOOK(void, ...) appeared in the new header, wouldn't it
be omitted from void_lsm_hooks and incorrectly reported as errno-returning?

This is latent today because the four hooks added later in the series all
return int, but is there a reason the fourth site shouldn't include the
new header, or should the header document that it only supports int hooks?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33386073074