Re: [PATCHv3 bpf-next 1/2] bpf: Do not allow to load sleepable BPF_TRACE_RAW_TP program

[Yonghong Song <yhs@meta.com>]

On 1/17/23 1:20 AM, Jiri Olsa wrote:
> On Mon, Jan 16, 2023 at 11:17:56PM -0800, Yonghong Song wrote:
>>
>>
>> On 1/16/23 5:29 AM, Jiri Olsa wrote:
>>> Currently we allow to load any tracing program as sleepable,
>>> but BPF_TRACE_RAW_TP can't sleep. Making the check explicit
>>> for tracing programs attach types, so sleepable BPF_TRACE_RAW_TP
>>> will fail to load.
>>>
>>> Updating the verifier error to mention iter programs as well.
>>>
>>> Acked-by: Song Liu <song@kernel.org>
>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>
>> Ack with a minor comment below.
>>
>> Acked-by: Yonghong Song <yhs@fb.com>
>>
>>> ---
>>> v3 changes:
>>>     - use switch in can_be_sleepable [Alexei]
>>>     - added acks [Song]
>>>
>>>    kernel/bpf/verifier.c | 22 +++++++++++++++++++---
>>>    1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index fa4c911603e9..966dbfc14288 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -16743,6 +16743,23 @@ BTF_ID(func, rcu_read_unlock_strict)
>>>    #endif
>>>    BTF_SET_END(btf_id_deny)
>>> +static bool can_be_sleepable(struct bpf_prog *prog)
>>> +{
>>> +	if (prog->type == BPF_PROG_TYPE_TRACING) {
>>> +		switch (prog->expected_attach_type) {
>>> +		case BPF_TRACE_FENTRY:
>>> +		case BPF_TRACE_FEXIT:
>>> +		case BPF_MODIFY_RETURN:
>>> +		case BPF_TRACE_ITER:
>>> +			return true;
>>> +		default:
>>> +			return false;
>>> +		}
>>> +	}
>>> +	return prog->type == BPF_PROG_TYPE_LSM ||
>>> +	       prog->type == BPF_PROG_TYPE_KPROBE;
>>> +}
>>> +
>>>    static int check_attach_btf_id(struct bpf_verifier_env *env)
>>>    {
>>>    	struct bpf_prog *prog = env->prog;
>>> @@ -16761,9 +16778,8 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
>>>    		return -EINVAL;
>>>    	}
>>> -	if (prog->aux->sleepable && prog->type != BPF_PROG_TYPE_TRACING &&
>>> -	    prog->type != BPF_PROG_TYPE_LSM && prog->type != BPF_PROG_TYPE_KPROBE) {
>>> -		verbose(env, "Only fentry/fexit/fmod_ret, lsm, and kprobe/uprobe programs can be sleepable\n");
>>> +	if (prog->aux->sleepable && !can_be_sleepable(prog)) {
>>> +		verbose(env, "Only fentry/fexit/fmod_ret, lsm, iter and kprobe/uprobe programs can be sleepable\n");
>>
>> actually kprobe programs cannot be sleepable. See kernel/events/core.c.
>> perf_event_set_bpf_prog(...)
>> ...
>>
>>          if (prog->type == BPF_PROG_TYPE_KPROBE && prog->aux->sleepable &&
>> !is_uprobe)
>>                  /* only uprobe programs are allowed to be sleepable */
>>                  return -EINVAL;
>>
>> So I suggest to add a comment and remove the above 'kprobe' from error
>> message.
> 
> ok, is comment below ok?

Thanks! Sounds good to me.

> 
> jirka
> 
> 
> ---
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fa4c911603e9..ca7db2ce70b9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -16743,6 +16743,23 @@ BTF_ID(func, rcu_read_unlock_strict)
>   #endif
>   BTF_SET_END(btf_id_deny)
>   
> +static bool can_be_sleepable(struct bpf_prog *prog)
> +{
> +	if (prog->type == BPF_PROG_TYPE_TRACING) {
> +		switch (prog->expected_attach_type) {
> +		case BPF_TRACE_FENTRY:
> +		case BPF_TRACE_FEXIT:
> +		case BPF_MODIFY_RETURN:
> +		case BPF_TRACE_ITER:
> +			return true;
> +		default:
> +			return false;
> +		}
> +	}
> +	return prog->type == BPF_PROG_TYPE_LSM ||
> +	       prog->type == BPF_PROG_TYPE_KPROBE; /* only for uprobes */
> +}
> +
>   static int check_attach_btf_id(struct bpf_verifier_env *env)
>   {
>   	struct bpf_prog *prog = env->prog;
> @@ -16761,9 +16778,8 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
>   		return -EINVAL;
>   	}
>   
> -	if (prog->aux->sleepable && prog->type != BPF_PROG_TYPE_TRACING &&
> -	    prog->type != BPF_PROG_TYPE_LSM && prog->type != BPF_PROG_TYPE_KPROBE) {
> -		verbose(env, "Only fentry/fexit/fmod_ret, lsm, and kprobe/uprobe programs can be sleepable\n");
> +	if (prog->aux->sleepable && !can_be_sleepable(prog)) {
> +		verbose(env, "Only fentry/fexit/fmod_ret, lsm, iter and uprobe programs can be sleepable\n");
>   		return -EINVAL;
>   	}
>