| Message ID | 20210816231716.3824813-2-prankgup@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Add support for bpf_setsockopt and bpf_getsockopt from BPF setsockopt | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 6 maintainers not CCed: songliubraving@fb.com kafai@fb.com netdev@vger.kernel.org john.fastabend@gmail.com kpsingh@kernel.org yhs@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 4 this patch: 4 |
| netdev/kdoc | success | Errors and warnings before: 3 this patch: 3 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 4 this patch: 4 |
| netdev/header_inline | success | Link |
On Mon, Aug 16, 2021 at 4:17 PM Prankur gupta <prankgup@fb.com> wrote: > > Add logic to call bpf_setsockopt and bpf_getsockopt from > setsockopt BPF programs. > Example use case, when the user sets the IPV6_TCLASS socket option > we would also like to change the tcp-cc for that socket. > We don't have any use case for calling bpf_setsockopt from > supposedly read-only sys_getsockopti, so it is made available to > BPF_CGROUP_SETSOCKOPT only. > > Signed-off-by: Prankur gupta <prankgup@fb.com> Acked-by: Song Liu <songliubraving@fb.com>
On Mon, Aug 16, 2021 at 4:17 PM Prankur gupta <prankgup@fb.com> wrote: > > Add logic to call bpf_setsockopt and bpf_getsockopt from > setsockopt BPF programs. > Example use case, when the user sets the IPV6_TCLASS socket option > we would also like to change the tcp-cc for that socket. > We don't have any use case for calling bpf_setsockopt from > supposedly read-only sys_getsockopti, so it is made available to > BPF_CGROUP_SETSOCKOPT only. > > Signed-off-by: Prankur gupta <prankgup@fb.com> > --- > kernel/bpf/cgroup.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c > index a1dedba4c174..9c92eff9af95 100644 > --- a/kernel/bpf/cgroup.c > +++ b/kernel/bpf/cgroup.c > @@ -1873,6 +1873,14 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_sk_storage_get_proto; > case BPF_FUNC_sk_storage_delete: > return &bpf_sk_storage_delete_proto; > + case BPF_FUNC_setsockopt: > + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) > + return &bpf_sk_setsockopt_proto; > + return NULL; > + case BPF_FUNC_getsockopt: > + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) > + return &bpf_sk_getsockopt_proto; Is there any problem enabling bpf_getsockopt() for BPF_CGROUP_GETSOCKOPT program type? > + return NULL; > #endif > #ifdef CONFIG_INET > case BPF_FUNC_tcp_sock: > -- > 2.30.2 >
>> >> Add logic to call bpf_setsockopt and bpf_getsockopt from >> setsockopt BPF programs. >> Example use case, when the user sets the IPV6_TCLASS socket option >> we would also like to change the tcp-cc for that socket. >> We don't have any use case for calling bpf_setsockopt from >> supposedly read-only sys_getsockopti, so it is made available to >> BPF_CGROUP_SETSOCKOPT only. >> >> Signed-off-by: Prankur gupta <prankgup@fb.com> >> --- >> kernel/bpf/cgroup.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c >> index a1dedba4c174..9c92eff9af95 100644 >> --- a/kernel/bpf/cgroup.c >> +++ b/kernel/bpf/cgroup.c >> @@ -1873,6 +1873,14 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) >> return &bpf_sk_storage_get_proto; >> case BPF_FUNC_sk_storage_delete: >> return &bpf_sk_storage_delete_proto; >> + case BPF_FUNC_setsockopt: >> + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) >> + return &bpf_sk_setsockopt_proto; >> + return NULL; >> + case BPF_FUNC_getsockopt: >> + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) >> + return &bpf_sk_getsockopt_proto; > >Is there any problem enabling bpf_getsockopt() for >BPF_CGROUP_GETSOCKOPT program type? > No, there's no problem but there's no usecase (which i can think of) where a user wants to set or get some socket option for a getsockopt call >> + return NULL; >> #endif >> #ifdef CONFIG_INET >> case BPF_FUNC_tcp_sock: >> -- >> 2.30.2 >>
On Tue, Aug 17, 2021 at 12:49:21PM -0700, Prankur gupta wrote: > >> > >> Add logic to call bpf_setsockopt and bpf_getsockopt from > >> setsockopt BPF programs. > >> Example use case, when the user sets the IPV6_TCLASS socket option > >> we would also like to change the tcp-cc for that socket. > >> We don't have any use case for calling bpf_setsockopt from > >> supposedly read-only sys_getsockopti, so it is made available to > >> BPF_CGROUP_SETSOCKOPT only. > >> > >> Signed-off-by: Prankur gupta <prankgup@fb.com> > >> --- > >> kernel/bpf/cgroup.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c > >> index a1dedba4c174..9c92eff9af95 100644 > >> --- a/kernel/bpf/cgroup.c > >> +++ b/kernel/bpf/cgroup.c > >> @@ -1873,6 +1873,14 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > >> return &bpf_sk_storage_get_proto; > >> case BPF_FUNC_sk_storage_delete: > >> return &bpf_sk_storage_delete_proto; > >> + case BPF_FUNC_setsockopt: > >> + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) > >> + return &bpf_sk_setsockopt_proto; > >> + return NULL; > >> + case BPF_FUNC_getsockopt: > >> + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) > >> + return &bpf_sk_getsockopt_proto; > > > >Is there any problem enabling bpf_getsockopt() for > >BPF_CGROUP_GETSOCKOPT program type? > > > > No, there's no problem but there's no usecase (which i can think of) > where a user wants to set or get some socket option for a getsockopt call imo, user usually expects that bpf_getsockopt() and bpf_setsockopt() are available together. It is pretty much the only reason bpf_getsockopt() is made available together with bpf_setsockopt() in this patch. It may actually create usage surprises if it only allows one but not another. To read members from a sk, it is more useful to make the bpf_skc_to_*_sock() helpers available instead of bpf_getsockopt(). Then it can read the sk's members from the returned PTR_TO_BTF_ID.
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c index a1dedba4c174..9c92eff9af95 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c @@ -1873,6 +1873,14 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_sk_storage_get_proto; case BPF_FUNC_sk_storage_delete: return &bpf_sk_storage_delete_proto; + case BPF_FUNC_setsockopt: + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) + return &bpf_sk_setsockopt_proto; + return NULL; + case BPF_FUNC_getsockopt: + if (prog->expected_attach_type == BPF_CGROUP_SETSOCKOPT) + return &bpf_sk_getsockopt_proto; + return NULL; #endif #ifdef CONFIG_INET case BPF_FUNC_tcp_sock:
Add logic to call bpf_setsockopt and bpf_getsockopt from setsockopt BPF programs. Example use case, when the user sets the IPV6_TCLASS socket option we would also like to change the tcp-cc for that socket. We don't have any use case for calling bpf_setsockopt from supposedly read-only sys_getsockopti, so it is made available to BPF_CGROUP_SETSOCKOPT only. Signed-off-by: Prankur gupta <prankgup@fb.com> --- kernel/bpf/cgroup.c | 8 ++++++++ 1 file changed, 8 insertions(+)