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?
I'd also call them something like bpf_sockopt_copy_{to,from}. That
"_so_optval_" is not super intuitive imho.
