| Message ID | 20210127232853.3753823-5-sdf@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4c3384d7abe58a68d78fd1b8b3bffbf62e1e29a1 |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: expose bpf_{g,s}etsockopt to more bpf_sock_addr hooks | 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 | 10 maintainers not CCed: shuah@kernel.org songliubraving@fb.com linux-kselftest@vger.kernel.org andrii@kernel.org kpsingh@kernel.org davem@davemloft.net john.fastabend@gmail.com kuba@kernel.org kafai@fb.com 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: 45 this patch: 45 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 50 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 45 this patch: 45 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On 1/28/21 12:28 AM, Stanislav Fomichev wrote: > Those hooks run as BPF_CGROUP_RUN_SA_PROG_LOCK and operate on > a locked socket. > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- > net/core/filter.c | 4 ++++ > tools/testing/selftests/bpf/progs/recvmsg4_prog.c | 5 +++++ > tools/testing/selftests/bpf/progs/recvmsg6_prog.c | 5 +++++ > 3 files changed, 14 insertions(+) > > diff --git a/net/core/filter.c b/net/core/filter.c > index ba436b1d70c2..e15d4741719a 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -7023,6 +7023,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > case BPF_CGROUP_INET6_BIND: > case BPF_CGROUP_INET4_CONNECT: > case BPF_CGROUP_INET6_CONNECT: > + case BPF_CGROUP_UDP4_RECVMSG: > + case BPF_CGROUP_UDP6_RECVMSG: > case BPF_CGROUP_UDP4_SENDMSG: > case BPF_CGROUP_UDP6_SENDMSG: > case BPF_CGROUP_INET4_GETPEERNAME: > @@ -7039,6 +7041,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > case BPF_CGROUP_INET6_BIND: > case BPF_CGROUP_INET4_CONNECT: > case BPF_CGROUP_INET6_CONNECT: > + case BPF_CGROUP_UDP4_RECVMSG: > + case BPF_CGROUP_UDP6_RECVMSG: > case BPF_CGROUP_UDP4_SENDMSG: > case BPF_CGROUP_UDP6_SENDMSG: > case BPF_CGROUP_INET4_GETPEERNAME: Looks good overall, also thanks for adding the test cases! I was about to apply, but noticed one small nit that would be good to get resolved before that. Above you now list all the attach hooks for sock_addr ctx, so we should just remove the whole switch that tests on prog->expected_attach_type altogether in this last commit. Thanks, Daniel
On Thu, Jan 28, 2021 at 4:52 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 1/28/21 12:28 AM, Stanislav Fomichev wrote: > > Those hooks run as BPF_CGROUP_RUN_SA_PROG_LOCK and operate on > > a locked socket. > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > net/core/filter.c | 4 ++++ > > tools/testing/selftests/bpf/progs/recvmsg4_prog.c | 5 +++++ > > tools/testing/selftests/bpf/progs/recvmsg6_prog.c | 5 +++++ > > 3 files changed, 14 insertions(+) > > > > diff --git a/net/core/filter.c b/net/core/filter.c > > index ba436b1d70c2..e15d4741719a 100644 > > --- a/net/core/filter.c > > +++ b/net/core/filter.c > > @@ -7023,6 +7023,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > > case BPF_CGROUP_INET6_BIND: > > case BPF_CGROUP_INET4_CONNECT: > > case BPF_CGROUP_INET6_CONNECT: > > + case BPF_CGROUP_UDP4_RECVMSG: > > + case BPF_CGROUP_UDP6_RECVMSG: > > case BPF_CGROUP_UDP4_SENDMSG: > > case BPF_CGROUP_UDP6_SENDMSG: > > case BPF_CGROUP_INET4_GETPEERNAME: > > @@ -7039,6 +7041,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > > case BPF_CGROUP_INET6_BIND: > > case BPF_CGROUP_INET4_CONNECT: > > case BPF_CGROUP_INET6_CONNECT: > > + case BPF_CGROUP_UDP4_RECVMSG: > > + case BPF_CGROUP_UDP6_RECVMSG: > > case BPF_CGROUP_UDP4_SENDMSG: > > case BPF_CGROUP_UDP6_SENDMSG: > > case BPF_CGROUP_INET4_GETPEERNAME: > > Looks good overall, also thanks for adding the test cases! I was about to apply, but noticed one > small nit that would be good to get resolved before that. Above you now list all the attach hooks > for sock_addr ctx, so we should just remove the whole switch that tests on prog->expected_attach_type > altogether in this last commit. Sure, I can resend tomorrow. But do you think it's safe and there won't ever be another sock_addr hook that runs with an unlocked socket?
On 1/29/21 1:59 AM, Stanislav Fomichev wrote: > On Thu, Jan 28, 2021 at 4:52 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 1/28/21 12:28 AM, Stanislav Fomichev wrote: >>> Those hooks run as BPF_CGROUP_RUN_SA_PROG_LOCK and operate on >>> a locked socket. >>> >>> Signed-off-by: Stanislav Fomichev <sdf@google.com> >>> --- >>> net/core/filter.c | 4 ++++ >>> tools/testing/selftests/bpf/progs/recvmsg4_prog.c | 5 +++++ >>> tools/testing/selftests/bpf/progs/recvmsg6_prog.c | 5 +++++ >>> 3 files changed, 14 insertions(+) >>> >>> diff --git a/net/core/filter.c b/net/core/filter.c >>> index ba436b1d70c2..e15d4741719a 100644 >>> --- a/net/core/filter.c >>> +++ b/net/core/filter.c >>> @@ -7023,6 +7023,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) >>> case BPF_CGROUP_INET6_BIND: >>> case BPF_CGROUP_INET4_CONNECT: >>> case BPF_CGROUP_INET6_CONNECT: >>> + case BPF_CGROUP_UDP4_RECVMSG: >>> + case BPF_CGROUP_UDP6_RECVMSG: >>> case BPF_CGROUP_UDP4_SENDMSG: >>> case BPF_CGROUP_UDP6_SENDMSG: >>> case BPF_CGROUP_INET4_GETPEERNAME: >>> @@ -7039,6 +7041,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) >>> case BPF_CGROUP_INET6_BIND: >>> case BPF_CGROUP_INET4_CONNECT: >>> case BPF_CGROUP_INET6_CONNECT: >>> + case BPF_CGROUP_UDP4_RECVMSG: >>> + case BPF_CGROUP_UDP6_RECVMSG: >>> case BPF_CGROUP_UDP4_SENDMSG: >>> case BPF_CGROUP_UDP6_SENDMSG: >>> case BPF_CGROUP_INET4_GETPEERNAME: >> >> Looks good overall, also thanks for adding the test cases! I was about to apply, but noticed one >> small nit that would be good to get resolved before that. Above you now list all the attach hooks >> for sock_addr ctx, so we should just remove the whole switch that tests on prog->expected_attach_type >> altogether in this last commit. > Sure, I can resend tomorrow. > But do you think it's safe and there won't ever be another sock_addr > hook that runs with an unlocked socket? Ok, that rationale seems reasonable to keep the series as is. It probably makes sense to add a small comment at least to the commit log to explain the reasoning, I can do so while applying. So no need for v3, thanks!
On Thu, Jan 28, 2021 at 5:08 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 1/29/21 1:59 AM, Stanislav Fomichev wrote: > > On Thu, Jan 28, 2021 at 4:52 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > >> On 1/28/21 12:28 AM, Stanislav Fomichev wrote: > >>> Those hooks run as BPF_CGROUP_RUN_SA_PROG_LOCK and operate on > >>> a locked socket. > >>> > >>> Signed-off-by: Stanislav Fomichev <sdf@google.com> > >>> --- > >>> net/core/filter.c | 4 ++++ > >>> tools/testing/selftests/bpf/progs/recvmsg4_prog.c | 5 +++++ > >>> tools/testing/selftests/bpf/progs/recvmsg6_prog.c | 5 +++++ > >>> 3 files changed, 14 insertions(+) > >>> > >>> diff --git a/net/core/filter.c b/net/core/filter.c > >>> index ba436b1d70c2..e15d4741719a 100644 > >>> --- a/net/core/filter.c > >>> +++ b/net/core/filter.c > >>> @@ -7023,6 +7023,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > >>> case BPF_CGROUP_INET6_BIND: > >>> case BPF_CGROUP_INET4_CONNECT: > >>> case BPF_CGROUP_INET6_CONNECT: > >>> + case BPF_CGROUP_UDP4_RECVMSG: > >>> + case BPF_CGROUP_UDP6_RECVMSG: > >>> case BPF_CGROUP_UDP4_SENDMSG: > >>> case BPF_CGROUP_UDP6_SENDMSG: > >>> case BPF_CGROUP_INET4_GETPEERNAME: > >>> @@ -7039,6 +7041,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > >>> case BPF_CGROUP_INET6_BIND: > >>> case BPF_CGROUP_INET4_CONNECT: > >>> case BPF_CGROUP_INET6_CONNECT: > >>> + case BPF_CGROUP_UDP4_RECVMSG: > >>> + case BPF_CGROUP_UDP6_RECVMSG: > >>> case BPF_CGROUP_UDP4_SENDMSG: > >>> case BPF_CGROUP_UDP6_SENDMSG: > >>> case BPF_CGROUP_INET4_GETPEERNAME: > >> > >> Looks good overall, also thanks for adding the test cases! I was about to apply, but noticed one > >> small nit that would be good to get resolved before that. Above you now list all the attach hooks > >> for sock_addr ctx, so we should just remove the whole switch that tests on prog->expected_attach_type > >> altogether in this last commit. > > Sure, I can resend tomorrow. > > But do you think it's safe and there won't ever be another sock_addr > > hook that runs with an unlocked socket? > > Ok, that rationale seems reasonable to keep the series as is. It probably makes sense to add a > small comment at least to the commit log to explain the reasoning, I can do so while applying. > So no need for v3, thanks! Sounds good, thank you!
diff --git a/net/core/filter.c b/net/core/filter.c index ba436b1d70c2..e15d4741719a 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -7023,6 +7023,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) case BPF_CGROUP_INET6_BIND: case BPF_CGROUP_INET4_CONNECT: case BPF_CGROUP_INET6_CONNECT: + case BPF_CGROUP_UDP4_RECVMSG: + case BPF_CGROUP_UDP6_RECVMSG: case BPF_CGROUP_UDP4_SENDMSG: case BPF_CGROUP_UDP6_SENDMSG: case BPF_CGROUP_INET4_GETPEERNAME: @@ -7039,6 +7041,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) case BPF_CGROUP_INET6_BIND: case BPF_CGROUP_INET4_CONNECT: case BPF_CGROUP_INET6_CONNECT: + case BPF_CGROUP_UDP4_RECVMSG: + case BPF_CGROUP_UDP6_RECVMSG: case BPF_CGROUP_UDP4_SENDMSG: case BPF_CGROUP_UDP6_SENDMSG: case BPF_CGROUP_INET4_GETPEERNAME: diff --git a/tools/testing/selftests/bpf/progs/recvmsg4_prog.c b/tools/testing/selftests/bpf/progs/recvmsg4_prog.c index fc2fe8a952fa..3d1ae8b3402f 100644 --- a/tools/testing/selftests/bpf/progs/recvmsg4_prog.c +++ b/tools/testing/selftests/bpf/progs/recvmsg4_prog.c @@ -8,6 +8,8 @@ #include <bpf/bpf_helpers.h> #include <bpf/bpf_endian.h> +#include <bpf_sockopt_helpers.h> + #define SERV4_IP 0xc0a801feU /* 192.168.1.254 */ #define SERV4_PORT 4040 @@ -28,6 +30,9 @@ int recvmsg4_prog(struct bpf_sock_addr *ctx) if (ctx->type != SOCK_STREAM && ctx->type != SOCK_DGRAM) return 1; + if (!get_set_sk_priority(ctx)) + return 1; + ctx->user_ip4 = bpf_htonl(SERV4_IP); ctx->user_port = bpf_htons(SERV4_PORT); diff --git a/tools/testing/selftests/bpf/progs/recvmsg6_prog.c b/tools/testing/selftests/bpf/progs/recvmsg6_prog.c index 6060fd63324b..27dfb21b21b4 100644 --- a/tools/testing/selftests/bpf/progs/recvmsg6_prog.c +++ b/tools/testing/selftests/bpf/progs/recvmsg6_prog.c @@ -8,6 +8,8 @@ #include <bpf/bpf_helpers.h> #include <bpf/bpf_endian.h> +#include <bpf_sockopt_helpers.h> + #define SERV6_IP_0 0xfaceb00c /* face:b00c:1234:5678::abcd */ #define SERV6_IP_1 0x12345678 #define SERV6_IP_2 0x00000000 @@ -31,6 +33,9 @@ int recvmsg6_prog(struct bpf_sock_addr *ctx) if (ctx->type != SOCK_STREAM && ctx->type != SOCK_DGRAM) return 1; + if (!get_set_sk_priority(ctx)) + return 1; + ctx->user_ip6[0] = bpf_htonl(SERV6_IP_0); ctx->user_ip6[1] = bpf_htonl(SERV6_IP_1); ctx->user_ip6[2] = bpf_htonl(SERV6_IP_2);
Those hooks run as BPF_CGROUP_RUN_SA_PROG_LOCK and operate on a locked socket. Signed-off-by: Stanislav Fomichev <sdf@google.com> --- net/core/filter.c | 4 ++++ tools/testing/selftests/bpf/progs/recvmsg4_prog.c | 5 +++++ tools/testing/selftests/bpf/progs/recvmsg6_prog.c | 5 +++++ 3 files changed, 14 insertions(+)