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

On 26/08/31 11:50AM, bot+bpf-ci@kernel.org wrote:

quoted

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

[ ... ]

quoted

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?

Yes, it does (intentionally). For new hooks, which aim to be attach
points for BPF programs, there is no real reason to return anything,
but 0 or error. If they are called from a funciton returning a pointer,
the calling funciton should covert the return value appropriately.
(Also, new void hooks do not make sense.)

quoted

 #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

[ ... ]

quoted

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?

Yes, there is no intent to add new void 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