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 |
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.
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?
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?
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.
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!
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 --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);