On Mon, Aug 14, 2023 at 12:20 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> 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@xxxxxxxxx wrote:
> >>>>> From: Kui-Feng Lee <kuifeng@xxxxxxxx>
> >>>>>
> >>>>> 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@xxxxxxxxx>
> >>>>> ---
> >>>>> 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?
