| Message ID | 20240620-fault-injection-statickeys-v2-5-e23947d3d84b@suse.cz (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Masami Hiramatsu |
| Headers | show |
| Series | static key support for error injection functions | expand |
On Wed, Jun 19, 2024 at 3:49 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > When CONFIG_FUNCTION_ERROR_INJECTION is disabled, > within_error_injection_list() will return false for any address and the > result of check_non_sleepable_error_inject() denylist is thus redundant. > The bpf_non_sleepable_error_inject list thus does not need to be > constructed at all, so #ifdef it out. > > This will allow to inline functions on the list when > CONFIG_FUNCTION_ERROR_INJECTION is disabled as there will be no BTF_ID() > reference for them. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > --- > kernel/bpf/verifier.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 77da1f438bec..5cd93de37d68 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -21044,6 +21044,8 @@ static int check_attach_modify_return(unsigned long addr, const char *func_name) > return -EINVAL; > } > > +#ifdef CONFIG_FUNCTION_ERROR_INJECTION > + > /* list of non-sleepable functions that are otherwise on > * ALLOW_ERROR_INJECTION list > */ > @@ -21061,6 +21063,19 @@ static int check_non_sleepable_error_inject(u32 btf_id) > return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id); > } > > +#else /* CONFIG_FUNCTION_ERROR_INJECTION */ > + > +/* > + * Pretend the denylist is empty, within_error_injection_list() will return > + * false anyway. > + */ > +static int check_non_sleepable_error_inject(u32 btf_id) > +{ > + return 0; > +} > + > +#endif The comment reads like this is an optimization, but it's a mandatory ifdef since should_failslab() might not be found by resolve_btfid during the build. Please make it clear in the comment.
On 6/20/24 3:18 AM, Alexei Starovoitov wrote: > On Wed, Jun 19, 2024 at 3:49 PM Vlastimil Babka <vbabka@suse.cz> wrote: >> >> When CONFIG_FUNCTION_ERROR_INJECTION is disabled, >> within_error_injection_list() will return false for any address and the >> result of check_non_sleepable_error_inject() denylist is thus redundant. >> The bpf_non_sleepable_error_inject list thus does not need to be >> constructed at all, so #ifdef it out. >> >> This will allow to inline functions on the list when >> CONFIG_FUNCTION_ERROR_INJECTION is disabled as there will be no BTF_ID() >> reference for them. >> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >> --- >> kernel/bpf/verifier.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 77da1f438bec..5cd93de37d68 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -21044,6 +21044,8 @@ static int check_attach_modify_return(unsigned long addr, const char *func_name) >> return -EINVAL; >> } >> >> +#ifdef CONFIG_FUNCTION_ERROR_INJECTION >> + >> /* list of non-sleepable functions that are otherwise on >> * ALLOW_ERROR_INJECTION list >> */ >> @@ -21061,6 +21063,19 @@ static int check_non_sleepable_error_inject(u32 btf_id) >> return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id); >> } >> >> +#else /* CONFIG_FUNCTION_ERROR_INJECTION */ >> + >> +/* >> + * Pretend the denylist is empty, within_error_injection_list() will return >> + * false anyway. >> + */ >> +static int check_non_sleepable_error_inject(u32 btf_id) >> +{ >> + return 0; >> +} >> + >> +#endif > > The comment reads like this is an optimization, but it's a mandatory > ifdef since should_failslab() might not be found by resolve_btfid > during the build. > Please make it clear in the comment. The comment just tried to explain why the return value is 0 and not 1 (which would be also somewhat logical) but ok, will make it more clear.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 77da1f438bec..5cd93de37d68 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -21044,6 +21044,8 @@ static int check_attach_modify_return(unsigned long addr, const char *func_name) return -EINVAL; } +#ifdef CONFIG_FUNCTION_ERROR_INJECTION + /* list of non-sleepable functions that are otherwise on * ALLOW_ERROR_INJECTION list */ @@ -21061,6 +21063,19 @@ static int check_non_sleepable_error_inject(u32 btf_id) return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id); } +#else /* CONFIG_FUNCTION_ERROR_INJECTION */ + +/* + * Pretend the denylist is empty, within_error_injection_list() will return + * false anyway. + */ +static int check_non_sleepable_error_inject(u32 btf_id) +{ + return 0; +} + +#endif + int bpf_check_attach_target(struct bpf_verifier_log *log, const struct bpf_prog *prog, const struct bpf_prog *tgt_prog,
When CONFIG_FUNCTION_ERROR_INJECTION is disabled, within_error_injection_list() will return false for any address and the result of check_non_sleepable_error_inject() denylist is thus redundant. The bpf_non_sleepable_error_inject list thus does not need to be constructed at all, so #ifdef it out. This will allow to inline functions on the list when CONFIG_FUNCTION_ERROR_INJECTION is disabled as there will be no BTF_ID() reference for them. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- kernel/bpf/verifier.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)