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

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/08/31 11:50AM, bot+bpf-ci@kernel.org wrote:
> > 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?

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.)

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

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