Re: [RFC bpf-next v2 4/6] bpf: Provide bpf_copy_from_user() and bpf_copy_to_user().

[Kui-Feng Lee <sinquersw@gmail.com>]

On 8/14/23 10:07, Stanislav Fomichev wrote:
> On 08/11, Kui-Feng Lee wrote:
>>
>>
>> On 8/11/23 16:27, Kui-Feng Lee wrote:
>>>
>>>
>>> On 8/11/23 16:05, Stanislav Fomichev wrote:
>>>> On 08/10, thinker.li@gmail.com wrote:
>>>>> From: Kui-Feng Lee <kuifeng@meta.com>
>>>>>
>>>>> Provide bpf_copy_from_user() and bpf_copy_to_user() to the BPF programs
>>>>> attached to cgroup/{set,get}sockopt. bpf_copy_to_user() is a new
>>>>> kfunc to
>>>>> copy data from an kernel space buffer to a user space buffer.
>>>>> They are only
>>>>> available for sleepable BPF programs. bpf_copy_to_user() is only
>>>>> available
>>>>> to the BPF programs attached to cgroup/getsockopt.
>>>>>
>>>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>>>> ---
>>>>>    kernel/bpf/cgroup.c  |  6 ++++++
>>>>>    kernel/bpf/helpers.c | 31 +++++++++++++++++++++++++++++++
>>>>>    2 files changed, 37 insertions(+)
>>>>>
>>>>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>>>>> index 5bf3115b265c..c15a72860d2a 100644
>>>>> --- a/kernel/bpf/cgroup.c
>>>>> +++ b/kernel/bpf/cgroup.c
>>>>> @@ -2461,6 +2461,12 @@ cg_sockopt_func_proto(enum bpf_func_id
>>>>> func_id, const struct bpf_prog *prog)
>>>>>    #endif
>>>>>        case BPF_FUNC_perf_event_output:
>>>>>            return &bpf_event_output_data_proto;
>>>>> +
>>>>> +    case BPF_FUNC_copy_from_user:
>>>>> +        if (prog->aux->sleepable)
>>>>> +            return &bpf_copy_from_user_proto;
>>>>> +        return NULL;
>>>>
>>>> If we just allow copy to/from, I'm not sure I understand how the buffer
>>>> sharing between sleepable/non-sleepable works.
>>>>
>>>> Let's assume I have two progs in the chain:
>>>> 1. non-sleepable - copies the buffer, does some modifications; since
>>>>      we don't copy the buffer back after every prog run, the modifications
>>>>      stay in the kernel buffer
>>>> 2. sleepable - runs and just gets the user pointer? does it mean this
>>>>     sleepable program doesn't see the changes from (1)?
>>
>> It is still visible from sleepable programs.  Sleepable programs
>> will receive a pointer to the buffer in the kernel.
>> And, BPF_SOCKOPT_FLAG_OPTVAL_USER is clear.
>>
>>>>
>>>> IOW, do we need some custom sockopt copy_to/from that handle this
>>>> potential buffer location transparently or am I missing something?
>>>>
>>>> Assuming we want to support this at all. If we do, might deserve a
>>>> selftest.
>>>
>>> It is why BPF_SOCKOPT_FLAG_OPTVAL_USER is there.
>>> It helps programs to make a right decision.
>>> However, I am going to remove bpf_copy_from_user()
>>> since we have bpf_so_optval_copy_to() and bpf_so_optval_copy_to_r().
>>> Does it make sense to you?
> 
> Ah, so that's where it's handled. I didn't read that far :-)
> In this case yes, let's have only those helpers.
> 
> Btw, do we also really need bpf_so_optval_copy_to_r? If we are doing
> dynptr, let's only have bpf_so_optval_copy_to version?

Don't you think bpf_so_optval_copy_to_r() is handy to copy
a simple string to the user space?

> 
> I'd also call them something like bpf_sockopt_copy_{to,from}. That
> "_so_optval_" is not super intuitive imho.

Agree!