| Message ID | 20220810190724.2692127-1-kafai@fb.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | bpf: net: Remove duplicated code from bpf_setsockopt() | expand |
On 08/10, Martin KaFai Lau wrote: > The code in bpf_setsockopt() is mostly a copy-and-paste from > the sock_setsockopt(), do_tcp_setsockopt(), do_ipv6_setsockopt(), > and do_ip_setsockopt(). As the allowed optnames in bpf_setsockopt() > grows, so are the duplicated code. The code between the copies > also slowly drifted. > This set is an effort to clean this up and reuse the existing > {sock,do_tcp,do_ipv6,do_ip}_setsockopt() as much as possible. > After the clean up, this set also adds a few allowed optnames > that we need to the bpf_setsockopt(). > The initial attempt was to clean up both bpf_setsockopt() and > bpf_getsockopt() together. However, the patch set was getting > too long. It is beneficial to leave the bpf_getsockopt() > out for another patch set. Thus, this set is focusing > on the bpf_setsockopt(). > v3: > - s/in_bpf/has_current_bpf_ctx/ (Andrii) > - Add comments to has_current_bpf_ctx() and sockopt_lock_sock() > (Stanislav) > - Use vmlinux.h in selftest and add defines to bpf_tracing_net.h > (Stanislav) > - Use bpf_getsockopt(SO_MARK) in selftest (Stanislav) > - Use BPF_CORE_READ_BITFIELD in selftest (Yonghong) Reviewed-by: Stanislav Fomichev <sdf@google.com> (I didn't go super deep on the selftest)
On 8/11/22 7:04 PM, sdf@google.com wrote: > On 08/10, Martin KaFai Lau wrote: >> The code in bpf_setsockopt() is mostly a copy-and-paste from >> the sock_setsockopt(), do_tcp_setsockopt(), do_ipv6_setsockopt(), >> and do_ip_setsockopt(). As the allowed optnames in bpf_setsockopt() >> grows, so are the duplicated code. The code between the copies >> also slowly drifted. > >> This set is an effort to clean this up and reuse the existing >> {sock,do_tcp,do_ipv6,do_ip}_setsockopt() as much as possible. > >> After the clean up, this set also adds a few allowed optnames >> that we need to the bpf_setsockopt(). > >> The initial attempt was to clean up both bpf_setsockopt() and >> bpf_getsockopt() together. However, the patch set was getting >> too long. It is beneficial to leave the bpf_getsockopt() >> out for another patch set. Thus, this set is focusing >> on the bpf_setsockopt(). > >> v3: >> - s/in_bpf/has_current_bpf_ctx/ (Andrii) >> - Add comments to has_current_bpf_ctx() and sockopt_lock_sock() >> (Stanislav) >> - Use vmlinux.h in selftest and add defines to bpf_tracing_net.h >> (Stanislav) >> - Use bpf_getsockopt(SO_MARK) in selftest (Stanislav) >> - Use BPF_CORE_READ_BITFIELD in selftest (Yonghong) > > Reviewed-by: Stanislav Fomichev <sdf@google.com> > > (I didn't go super deep on the selftest) Looks like that one throws a build error, fwiw: https://github.com/kernel-patches/bpf/runs/7844497492?check_suite_focus=true [...] CLNG-BPF [test_maps] kfunc_call_test_subprog.o CLNG-BPF [test_maps] bpf_iter_test_kern6.o progs/setget_sockopt.c:39:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] { .opt = SO_REUSEADDR, .flip = 1, }, ^ progs/setget_sockopt.c:42:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] { .opt = SO_KEEPALIVE, .flip = 1, }, ^ progs/setget_sockopt.c:44:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] { .opt = SO_REUSEPORT, .flip = 1, }, ^ CLNG-BPF [test_maps] btf__core_reloc_type_id.o progs/setget_sockopt.c:48:32: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] { .opt = SO_TXREHASH, .flip = 1, }, ^ progs/setget_sockopt.c:53:32: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] { .opt = TCP_NODELAY, .flip = 1, }, ^ progs/setget_sockopt.c:61:45: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] { .opt = TCP_THIN_LINEAR_TIMEOUTS, .flip = 1, }, ^ progs/setget_sockopt.c:75:39: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] { .opt = IPV6_AUTOFLOWLABEL, .flip = 1, }, ^ 7 errors generated. make: *** [Makefile:521: /tmp/runner/work/bpf/bpf/tools/testing/selftests/bpf/setget_sockopt.o] Error 1 make: *** Waiting for unfinished jobs.... make: Leaving directory '/tmp/runner/work/bpf/bpf/tools/testing/selftests/bpf' Error: Process completed with exit code 2.
On Tue, Aug 16, 2022 at 12:04:52AM +0200, Daniel Borkmann wrote: > On 8/11/22 7:04 PM, sdf@google.com wrote: > > On 08/10, Martin KaFai Lau wrote: > > > The code in bpf_setsockopt() is mostly a copy-and-paste from > > > the sock_setsockopt(), do_tcp_setsockopt(), do_ipv6_setsockopt(), > > > and do_ip_setsockopt(). As the allowed optnames in bpf_setsockopt() > > > grows, so are the duplicated code. The code between the copies > > > also slowly drifted. > > > > > This set is an effort to clean this up and reuse the existing > > > {sock,do_tcp,do_ipv6,do_ip}_setsockopt() as much as possible. > > > > > After the clean up, this set also adds a few allowed optnames > > > that we need to the bpf_setsockopt(). > > > > > The initial attempt was to clean up both bpf_setsockopt() and > > > bpf_getsockopt() together. However, the patch set was getting > > > too long. It is beneficial to leave the bpf_getsockopt() > > > out for another patch set. Thus, this set is focusing > > > on the bpf_setsockopt(). > > > > > v3: > > > - s/in_bpf/has_current_bpf_ctx/ (Andrii) > > > - Add comments to has_current_bpf_ctx() and sockopt_lock_sock() > > > (Stanislav) > > > - Use vmlinux.h in selftest and add defines to bpf_tracing_net.h > > > (Stanislav) > > > - Use bpf_getsockopt(SO_MARK) in selftest (Stanislav) > > > - Use BPF_CORE_READ_BITFIELD in selftest (Yonghong) > > > > Reviewed-by: Stanislav Fomichev <sdf@google.com> > > > > (I didn't go super deep on the selftest) > > Looks like that one throws a build error, fwiw: > > https://github.com/kernel-patches/bpf/runs/7844497492?check_suite_focus=true > > [...] > CLNG-BPF [test_maps] kfunc_call_test_subprog.o > CLNG-BPF [test_maps] bpf_iter_test_kern6.o > progs/setget_sockopt.c:39:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] > { .opt = SO_REUSEADDR, .flip = 1, }, > ^ > progs/setget_sockopt.c:42:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] > { .opt = SO_KEEPALIVE, .flip = 1, }, > ^ > progs/setget_sockopt.c:44:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] > { .opt = SO_REUSEPORT, .flip = 1, }, > ^ > CLNG-BPF [test_maps] btf__core_reloc_type_id.o > progs/setget_sockopt.c:48:32: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] > { .opt = SO_TXREHASH, .flip = 1, }, > ^ > progs/setget_sockopt.c:53:32: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] > { .opt = TCP_NODELAY, .flip = 1, }, > ^ > progs/setget_sockopt.c:61:45: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] > { .opt = TCP_THIN_LINEAR_TIMEOUTS, .flip = 1, }, > ^ > progs/setget_sockopt.c:75:39: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion] > { .opt = IPV6_AUTOFLOWLABEL, .flip = 1, }, > ^ > 7 errors generated. > make: *** [Makefile:521: /tmp/runner/work/bpf/bpf/tools/testing/selftests/bpf/setget_sockopt.o] Error 1 > make: *** Waiting for unfinished jobs.... > make: Leaving directory '/tmp/runner/work/bpf/bpf/tools/testing/selftests/bpf' > Error: Process completed with exit code 2. Thanks for the report. I also see it after moving from clang 15 to 16. I will address it in v4.
The code in bpf_setsockopt() is mostly a copy-and-paste from the sock_setsockopt(), do_tcp_setsockopt(), do_ipv6_setsockopt(), and do_ip_setsockopt(). As the allowed optnames in bpf_setsockopt() grows, so are the duplicated code. The code between the copies also slowly drifted. This set is an effort to clean this up and reuse the existing {sock,do_tcp,do_ipv6,do_ip}_setsockopt() as much as possible. After the clean up, this set also adds a few allowed optnames that we need to the bpf_setsockopt(). The initial attempt was to clean up both bpf_setsockopt() and bpf_getsockopt() together. However, the patch set was getting too long. It is beneficial to leave the bpf_getsockopt() out for another patch set. Thus, this set is focusing on the bpf_setsockopt(). v3: - s/in_bpf/has_current_bpf_ctx/ (Andrii) - Add comments to has_current_bpf_ctx() and sockopt_lock_sock() (Stanislav) - Use vmlinux.h in selftest and add defines to bpf_tracing_net.h (Stanislav) - Use bpf_getsockopt(SO_MARK) in selftest (Stanislav) - Use BPF_CORE_READ_BITFIELD in selftest (Yonghong) v2: - A major change is to use in_bpf() to test if a setsockopt() is called by a bpf prog and use in_bpf() to skip capable check. Suggested by Stanislav. - Instead of passing is_locked through sockptr_t or through an extra argument to sk_setsockopt, v2 uses in_bpf() to skip the lock_sock() also because bpf prog has the lock acquired. - No change to the current sockptr_t in this revision - s/codes/code/ Martin KaFai Lau (15): net: Add sk_setsockopt() to take the sk ptr instead of the sock ptr bpf: net: Avoid sk_setsockopt() taking sk lock when called from bpf bpf: net: Consider has_current_bpf_ctx() when testing capable() in sk_setsockopt() bpf: net: Change do_tcp_setsockopt() to use the sockopt's lock_sock() and capable() bpf: net: Change do_ip_setsockopt() to use the sockopt's lock_sock() and capable() bpf: net: Change do_ipv6_setsockopt() to use the sockopt's lock_sock() and capable() bpf: Initialize the bpf_run_ctx in bpf_iter_run_prog() bpf: Embed kernel CONFIG check into the if statement in bpf_setsockopt bpf: Change bpf_setsockopt(SOL_SOCKET) to reuse sk_setsockopt() bpf: Refactor bpf specific tcp optnames to a new function bpf: Change bpf_setsockopt(SOL_TCP) to reuse do_tcp_setsockopt() bpf: Change bpf_setsockopt(SOL_IP) to reuse do_ip_setsockopt() bpf: Change bpf_setsockopt(SOL_IPV6) to reuse do_ipv6_setsockopt() bpf: Add a few optnames to bpf_setsockopt selftests/bpf: bpf_setsockopt tests include/linux/bpf.h | 14 + include/net/ip.h | 2 + include/net/ipv6.h | 2 + include/net/ipv6_stubs.h | 2 + include/net/sock.h | 7 + include/net/tcp.h | 2 + kernel/bpf/bpf_iter.c | 5 + net/core/filter.c | 377 ++++++--------- net/core/sock.c | 81 +++- net/ipv4/ip_sockglue.c | 16 +- net/ipv4/tcp.c | 22 +- net/ipv6/af_inet6.c | 1 + net/ipv6/ipv6_sockglue.c | 18 +- .../selftests/bpf/prog_tests/setget_sockopt.c | 125 +++++ .../selftests/bpf/progs/bpf_tracing_net.h | 31 +- .../selftests/bpf/progs/setget_sockopt.c | 451 ++++++++++++++++++ 16 files changed, 883 insertions(+), 273 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/setget_sockopt.c create mode 100644 tools/testing/selftests/bpf/progs/setget_sockopt.c