Re: [PATCH bpf-next 1/3] bpf: Always allow sleepable programs on syscalls

[Leon Hwang <leon.hwang@linux.dev>]

On 5/3/26 23:25, Viktor Malik wrote:
> Sleepable BPF programs can only be attached to selected functions. For
> convenience, the error injection list was originally used, which
> contains syscalls and several other functions.
> 
> When error injection is disabled (CONFIG_FUNCTION_ERROR_INJECTION=n),
> that list is empty and sleepable tracing programs are effectively
> unavailable. In such a case, at least enable sleepable programs on
> syscalls.
> 
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>  kernel/bpf/verifier.c | 54 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d92cf2821657..cb80a3ddb7a4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -24930,6 +24930,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
>   */
> @@ -24951,6 +24953,51 @@ static int check_non_sleepable_error_inject(u32 btf_id)
>  	return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
>  }
>  
> +static int check_attach_sleepable(u32 btf_id, unsigned long addr, const char *func_name)
> +{
> +	/* fentry/fexit/fmod_ret progs can be sleepable if they are
> +	 * attached to ALLOW_ERROR_INJECTION and are not in denylist.
> +	 */
> +	if (!check_non_sleepable_error_inject(btf_id) &&
> +	    within_error_injection_list(addr))
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +
> +#else
> +
> +static bool has_arch_syscall_prefix(const char *func_name)
> +{
> +#if defined(__x86_64__)
> +	return !strncmp(func_name, "__x64_", 6);
> +#elif defined(__i386__)
> +	return !strncmp(func_name, "__ia32_", 7);
> +#elif defined(__s390x__)
> +	return !strncmp(func_name, "__s390x_", 8);
> +#elif defined(__aarch64__)
> +	return !strncmp(func_name, "__arm64_", 8);
> +#elif defined(__riscv)
> +	return !strncmp(func_name, "__riscv_", 8);
> +#elif defined(__powerpc__) || defined(__powerpc64__)
> +	return !strncmp(func_name, "sys", 3);
> +#else
> +	return false;
> +#endif
> +}

At first glance, it is not a good taste to hard code these prefixes here.

Furthermore, the arch_syscall_match_sym_name() of ftrace does the same
thing. Consider it instead.

Thanks,
Leon

> +
> +/* Without error injection, allow sleepable progs on syscalls. */
> +
> +static int check_attach_sleepable(u32 btf_id, unsigned long addr, const char *func_name)
> +{
> +	if (has_arch_syscall_prefix(func_name))
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +
> +#endif /* CONFIG_FUNCTION_ERROR_INJECTION */
> +
>  int bpf_check_attach_target(struct bpf_verifier_log *log,
>  			    const struct bpf_prog *prog,
>  			    const struct bpf_prog *tgt_prog,
> @@ -25230,12 +25277,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>  			ret = -EINVAL;
>  			switch (prog->type) {
>  			case BPF_PROG_TYPE_TRACING:
> -
> -				/* fentry/fexit/fmod_ret progs can be sleepable if they are
> -				 * attached to ALLOW_ERROR_INJECTION and are not in denylist.
> -				 */
> -				if (!check_non_sleepable_error_inject(btf_id) &&
> -				    within_error_injection_list(addr))
> +				if (!check_attach_sleepable(btf_id, addr, tname))
>  					ret = 0;
>  				/* fentry/fexit/fmod_ret progs can also be sleepable if they are
>  				 * in the fmodret id set with the KF_SLEEPABLE flag.