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