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

[Stanislav Fomichev <sdf@google.com>]

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.