diff mbox series

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

Message ID 20230811043127.1318152-5-thinker.li@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Sleepable BPF programs on cgroup {get,set}sockopt | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 pending Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-25 fail Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 fail Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 fail Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for veristat
bpf/vmtest-bpf-next-VM_Test-26 fail Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-6 success Logs for set-matrix
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 1400 this patch: 1404
netdev/cc_maintainers warning 5 maintainers not CCed: daniel@iogearbox.net kpsingh@kernel.org john.fastabend@gmail.com jolsa@kernel.org haoluo@google.com
netdev/build_clang fail Errors and warnings before: 1365 this patch: 1368
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 1423 this patch: 1427
netdev/checkpatch warning WARNING: From:/Signed-off-by: email address mismatch: 'From: Kui-Feng Lee <kuifeng@meta.com>' != 'Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>' WARNING: line length of 96 exceeds 80 columns
netdev/kdoc fail Errors and warnings before: 12 this patch: 14
netdev/source_inline success Was 0 now: 0

Commit Message

Kui-Feng Lee Aug. 11, 2023, 4:31 a.m. UTC
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(+)

Comments

Stanislav Fomichev Aug. 11, 2023, 11:05 p.m. UTC | #1
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)?

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.
Kui-Feng Lee Aug. 11, 2023, 11:27 p.m. UTC | #2
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)?
> 
> 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?
Kui-Feng Lee Aug. 11, 2023, 11:31 p.m. UTC | #3
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?
Stanislav Fomichev Aug. 14, 2023, 5:07 p.m. UTC | #4
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?

I'd also call them something like bpf_sockopt_copy_{to,from}. That
"_so_optval_" is not super intuitive imho.
Kui-Feng Lee Aug. 14, 2023, 7:20 p.m. UTC | #5
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!
Stanislav Fomichev Aug. 14, 2023, 8:16 p.m. UTC | #6
On Mon, Aug 14, 2023 at 12:20 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>
>
>
> 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?

Yeah, it is convenient, but it feels like something we can avoid if we
are using dynptrs (exclusively)?
Can the programs have a simple wrapper around
bpf_so_optval_from+bpf_so_optval_copy_to to provide the same ptr-based
api?

static inline void bpf_so_optval_copy_to_r(sopt, ptr, sz) {
   bpf_so_optval_alloc(sopt, sz, &dynptr);
   some_dynptr_copy_to(&dynptr, ptr, siz);
   bpf_so_optval_copy_to(sopt, &dynptr);
   bpf_so_optval_release(&dynptr);
}

Or maybe we should instead have a new kfunc that turns sopt ptr+sz
into a dynptr?

static inline void bpf_so_optval_copy_to_r(sopt, ptr, sz) {
   bpf_so_optval_from_user_ptr(sopt, &dynptr);
   bpf_so_optval_copy_to(sopt, &dynptr);
   bpf_so_optval_release(&dynptr);
}

That probably fits better into dynptr world?
diff mbox series

Patch

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;
+
 	default:
 		return bpf_base_func_proto(func_id);
 	}
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index eb91cae0612a..ff240db1512c 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -669,6 +669,26 @@  const struct bpf_func_proto bpf_copy_from_user_proto = {
 	.arg3_type	= ARG_ANYTHING,
 };
 
+/**
+ * int bpf_copy_to_user(void *dst, u32 size, const void *kern_ptr)
+ *     Description
+ *             Read *size* bytes from kernel space address *kern_ptr* and
+ *              store the data in user space address *dst*. This is a
+ *              wrapper of **copy_to_user**\ ().
+ *     Return
+ *             0 on success, or a negative error in case of failure.
+ */
+__bpf_kfunc int bpf_copy_to_user(void *dst__uninit, u32 dst__sz,
+				 const void *src__ign)
+{
+	int ret = copy_to_user(dst__uninit, src__ign, dst__sz);
+
+	if (unlikely(ret))
+		return -EFAULT;
+
+	return ret;
+}
+
 BPF_CALL_5(bpf_copy_from_user_task, void *, dst, u32, size,
 	   const void __user *, user_ptr, struct task_struct *, tsk, u64, flags)
 {
@@ -2456,6 +2476,7 @@  BTF_ID_FLAGS(func, bpf_cgroup_from_id, KF_ACQUIRE | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_task_under_cgroup, KF_RCU)
 #endif
 BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_copy_to_user, KF_SLEEPABLE)
 BTF_SET8_END(generic_btf_ids)
 
 static const struct btf_kfunc_id_set generic_kfunc_set = {
@@ -2494,6 +2515,15 @@  static const struct btf_kfunc_id_set common_kfunc_set = {
 	.set   = &common_btf_ids,
 };
 
+BTF_SET8_START(cgroup_common_btf_ids)
+BTF_ID_FLAGS(func, bpf_copy_to_user, KF_SLEEPABLE)
+BTF_SET8_END(cgroup_common_btf_ids)
+
+static const struct btf_kfunc_id_set cgroup_kfunc_set = {
+	.owner	= THIS_MODULE,
+	.set	= &cgroup_common_btf_ids,
+};
+
 static int __init kfunc_init(void)
 {
 	int ret;
@@ -2513,6 +2543,7 @@  static int __init kfunc_init(void)
 	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &generic_kfunc_set);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &generic_kfunc_set);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &generic_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCKOPT, &cgroup_kfunc_set);
 	ret = ret ?: register_btf_id_dtor_kfuncs(generic_dtors,
 						  ARRAY_SIZE(generic_dtors),
 						  THIS_MODULE);