| Message ID | 20231016134750.1381153-5-leitao@debian.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: Initial support for {s,g}etsockopt commands | expand |
On Mon, 16 Oct 2023 06:47:42 -0700 Breno Leitao wrote: > Split __sys_getsockopt() into two functions by removing the core > logic into a sub-function (do_sock_getsockopt()). This will avoid > code duplication when doing the same operation in other callers, for > instance. > > do_sock_getsockopt() will be called by io_uring getsockopt() command > operation in the following patch. > > The same was done for the setsockopt pair. Acked-by: Jakub Kicinski <kuba@kernel.org>
On 10/16/23 6:47 AM, Breno Leitao wrote: > diff --git a/net/socket.c b/net/socket.c > index 0087f8c071e7..f4c156a1987e 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -2350,6 +2350,42 @@ SYSCALL_DEFINE5(setsockopt, int, fd, int, level, int, optname, > INDIRECT_CALLABLE_DECLARE(bool tcp_bpf_bypass_getsockopt(int level, > int optname)); > > +int do_sock_getsockopt(struct socket *sock, bool compat, int level, > + int optname, sockptr_t optval, sockptr_t optlen) > +{ > + int max_optlen __maybe_unused; > + const struct proto_ops *ops; > + int err; > + > + err = security_socket_getsockopt(sock, level, optname); > + if (err) > + return err; > + > + ops = READ_ONCE(sock->ops); > + if (level == SOL_SOCKET) { > + err = sk_getsockopt(sock->sk, level, optname, optval, optlen); > + } else if (unlikely(!ops->getsockopt)) { > + err = -EOPNOTSUPP; > + } else { > + if (WARN_ONCE(optval.is_kernel || optlen.is_kernel, > + "Invalid argument type")) > + return -EOPNOTSUPP; > + > + err = ops->getsockopt(sock, level, optname, optval.user, > + optlen.user); > + } > + > + if (!compat) { > + max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen); The max_optlen was done before the above sk_getsockopt. The bpf CI cannot catch it because it cannot apply patch 5 cleanly. I ran the following out of the linux-block tree: $> ./test_progs -t sockopt_sk test_sockopt_sk:PASS:join_cgroup /sockopt_sk 0 nsec run_test:PASS:skel_load 0 nsec run_test:PASS:setsockopt_link 0 nsec run_test:PASS:getsockopt_link 0 nsec (/data/users/kafai/fb-kernel/linux/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c:111: errno: Operation not permitted) Failed to call getsockopt, ret=-1 run_test:FAIL:getsetsockopt unexpected error: -1 (errno 1) #217 sockopt_sk:FAIL > + err = BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock->sk, level, optname, > + optval, optlen, max_optlen, > + err); > + } > + > + return err; > +} > +EXPORT_SYMBOL(do_sock_getsockopt); > + > /* > * Get a socket option. Because we don't know the option lengths we have > * to pass a user mode parameter for the protocols to sort out. > @@ -2357,37 +2393,18 @@ INDIRECT_CALLABLE_DECLARE(bool tcp_bpf_bypass_getsockopt(int level, > int __sys_getsockopt(int fd, int level, int optname, char __user *optval, > int __user *optlen) > { > - int max_optlen __maybe_unused; > - const struct proto_ops *ops; > int err, fput_needed; > struct socket *sock; > + bool compat; > > sock = sockfd_lookup_light(fd, &err, &fput_needed); > if (!sock) > return err; > > - err = security_socket_getsockopt(sock, level, optname); > - if (err) > - goto out_put; > - > - if (!in_compat_syscall()) > - max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen); The old max_optlen was done here. > + compat = in_compat_syscall(); > + err = do_sock_getsockopt(sock, compat, level, optname, > + USER_SOCKPTR(optval), USER_SOCKPTR(optlen)); > > - ops = READ_ONCE(sock->ops); > - if (level == SOL_SOCKET) > - err = sock_getsockopt(sock, level, optname, optval, optlen); > - else if (unlikely(!ops->getsockopt)) > - err = -EOPNOTSUPP; > - else > - err = ops->getsockopt(sock, level, optname, optval, > - optlen); > - > - if (!in_compat_syscall()) > - err = BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock->sk, level, optname, > - USER_SOCKPTR(optval), > - USER_SOCKPTR(optlen), > - max_optlen, err); > -out_put: > fput_light(sock->file, fput_needed); > return err; > }
On 10/19/23 1:12 PM, Martin KaFai Lau wrote: > On 10/16/23 6:47?AM, Breno Leitao wrote: >> diff --git a/net/socket.c b/net/socket.c >> index 0087f8c071e7..f4c156a1987e 100644 >> --- a/net/socket.c >> +++ b/net/socket.c >> @@ -2350,6 +2350,42 @@ SYSCALL_DEFINE5(setsockopt, int, fd, int, level, int, optname, >> INDIRECT_CALLABLE_DECLARE(bool tcp_bpf_bypass_getsockopt(int level, >> int optname)); >> +int do_sock_getsockopt(struct socket *sock, bool compat, int level, >> + int optname, sockptr_t optval, sockptr_t optlen) >> +{ >> + int max_optlen __maybe_unused; >> + const struct proto_ops *ops; >> + int err; >> + >> + err = security_socket_getsockopt(sock, level, optname); >> + if (err) >> + return err; >> + >> + ops = READ_ONCE(sock->ops); >> + if (level == SOL_SOCKET) { >> + err = sk_getsockopt(sock->sk, level, optname, optval, optlen); >> + } else if (unlikely(!ops->getsockopt)) { >> + err = -EOPNOTSUPP; >> + } else { >> + if (WARN_ONCE(optval.is_kernel || optlen.is_kernel, >> + "Invalid argument type")) >> + return -EOPNOTSUPP; >> + >> + err = ops->getsockopt(sock, level, optname, optval.user, >> + optlen.user); >> + } >> + >> + if (!compat) { >> + max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen); > > The max_optlen was done before the above sk_getsockopt. The bpf CI cannot catch it because it cannot apply patch 5 cleanly. I ran the following out of the linux-block tree: > > $> ./test_progs -t sockopt_sk > test_sockopt_sk:PASS:join_cgroup /sockopt_sk 0 nsec > run_test:PASS:skel_load 0 nsec > run_test:PASS:setsockopt_link 0 nsec > run_test:PASS:getsockopt_link 0 nsec > (/data/users/kafai/fb-kernel/linux/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c:111: errno: Operation not permitted) Failed to call getsockopt, ret=-1 > run_test:FAIL:getsetsockopt unexpected error: -1 (errno 1) > #217 sockopt_sk:FAIL Does it work with this incremental? I can fold that in, will rebase anyway to collect acks. diff --git a/net/socket.c b/net/socket.c index bccd257e13fe..eb6960958026 100644 --- a/net/socket.c +++ b/net/socket.c @@ -2344,6 +2344,9 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level, if (err) return err; + if (!compat) + max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen); + ops = READ_ONCE(sock->ops); if (level == SOL_SOCKET) { err = sk_getsockopt(sock->sk, level, optname, optval, optlen); @@ -2358,12 +2361,10 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level, optlen.user); } - if (!compat) { - max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen); + if (!compat) err = BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock->sk, level, optname, optval, optlen, max_optlen, err); - } return err; }
On 10/19/23 1:04 PM, Jens Axboe wrote: > On 10/19/23 1:12 PM, Martin KaFai Lau wrote: >> On 10/16/23 6:47?AM, Breno Leitao wrote: >>> diff --git a/net/socket.c b/net/socket.c >>> index 0087f8c071e7..f4c156a1987e 100644 >>> --- a/net/socket.c >>> +++ b/net/socket.c >>> @@ -2350,6 +2350,42 @@ SYSCALL_DEFINE5(setsockopt, int, fd, int, level, int, optname, >>> INDIRECT_CALLABLE_DECLARE(bool tcp_bpf_bypass_getsockopt(int level, >>> int optname)); >>> +int do_sock_getsockopt(struct socket *sock, bool compat, int level, >>> + int optname, sockptr_t optval, sockptr_t optlen) >>> +{ >>> + int max_optlen __maybe_unused; >>> + const struct proto_ops *ops; >>> + int err; >>> + >>> + err = security_socket_getsockopt(sock, level, optname); >>> + if (err) >>> + return err; >>> + >>> + ops = READ_ONCE(sock->ops); >>> + if (level == SOL_SOCKET) { >>> + err = sk_getsockopt(sock->sk, level, optname, optval, optlen); >>> + } else if (unlikely(!ops->getsockopt)) { >>> + err = -EOPNOTSUPP; >>> + } else { >>> + if (WARN_ONCE(optval.is_kernel || optlen.is_kernel, >>> + "Invalid argument type")) >>> + return -EOPNOTSUPP; >>> + >>> + err = ops->getsockopt(sock, level, optname, optval.user, >>> + optlen.user); >>> + } >>> + >>> + if (!compat) { >>> + max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen); >> >> The max_optlen was done before the above sk_getsockopt. The bpf CI cannot catch it because it cannot apply patch 5 cleanly. I ran the following out of the linux-block tree: >> >> $> ./test_progs -t sockopt_sk >> test_sockopt_sk:PASS:join_cgroup /sockopt_sk 0 nsec >> run_test:PASS:skel_load 0 nsec >> run_test:PASS:setsockopt_link 0 nsec >> run_test:PASS:getsockopt_link 0 nsec >> (/data/users/kafai/fb-kernel/linux/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c:111: errno: Operation not permitted) Failed to call getsockopt, ret=-1 >> run_test:FAIL:getsetsockopt unexpected error: -1 (errno 1) >> #217 sockopt_sk:FAIL > > Does it work with this incremental? I can fold that in, will rebase > anyway to collect acks. Yes, that should work. Acked-by: Martin KaFai Lau <martin.lau@kernel.org> > > > diff --git a/net/socket.c b/net/socket.c > index bccd257e13fe..eb6960958026 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -2344,6 +2344,9 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level, > if (err) > return err; > > + if (!compat) > + max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen); > + > ops = READ_ONCE(sock->ops); > if (level == SOL_SOCKET) { > err = sk_getsockopt(sock->sk, level, optname, optval, optlen); > @@ -2358,12 +2361,10 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level, > optlen.user); > } > > - if (!compat) { > - max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen); > + if (!compat) > err = BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock->sk, level, optname, > optval, optlen, max_optlen, > err); > - } > > return err; > } >
On 10/19/23 2:37 PM, Martin KaFai Lau wrote: > On 10/19/23 1:04?PM, Jens Axboe wrote: >> On 10/19/23 1:12 PM, Martin KaFai Lau wrote: >>> On 10/16/23 6:47?AM, Breno Leitao wrote: >>>> diff --git a/net/socket.c b/net/socket.c >>>> index 0087f8c071e7..f4c156a1987e 100644 >>>> --- a/net/socket.c >>>> +++ b/net/socket.c >>>> @@ -2350,6 +2350,42 @@ SYSCALL_DEFINE5(setsockopt, int, fd, int, level, int, optname, >>>> INDIRECT_CALLABLE_DECLARE(bool tcp_bpf_bypass_getsockopt(int level, >>>> int optname)); >>>> +int do_sock_getsockopt(struct socket *sock, bool compat, int level, >>>> + int optname, sockptr_t optval, sockptr_t optlen) >>>> +{ >>>> + int max_optlen __maybe_unused; >>>> + const struct proto_ops *ops; >>>> + int err; >>>> + >>>> + err = security_socket_getsockopt(sock, level, optname); >>>> + if (err) >>>> + return err; >>>> + >>>> + ops = READ_ONCE(sock->ops); >>>> + if (level == SOL_SOCKET) { >>>> + err = sk_getsockopt(sock->sk, level, optname, optval, optlen); >>>> + } else if (unlikely(!ops->getsockopt)) { >>>> + err = -EOPNOTSUPP; >>>> + } else { >>>> + if (WARN_ONCE(optval.is_kernel || optlen.is_kernel, >>>> + "Invalid argument type")) >>>> + return -EOPNOTSUPP; >>>> + >>>> + err = ops->getsockopt(sock, level, optname, optval.user, >>>> + optlen.user); >>>> + } >>>> + >>>> + if (!compat) { >>>> + max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen); >>> >>> The max_optlen was done before the above sk_getsockopt. The bpf CI cannot catch it because it cannot apply patch 5 cleanly. I ran the following out of the linux-block tree: >>> >>> $> ./test_progs -t sockopt_sk >>> test_sockopt_sk:PASS:join_cgroup /sockopt_sk 0 nsec >>> run_test:PASS:skel_load 0 nsec >>> run_test:PASS:setsockopt_link 0 nsec >>> run_test:PASS:getsockopt_link 0 nsec >>> (/data/users/kafai/fb-kernel/linux/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c:111: errno: Operation not permitted) Failed to call getsockopt, ret=-1 >>> run_test:FAIL:getsetsockopt unexpected error: -1 (errno 1) >>> #217 sockopt_sk:FAIL >> >> Does it work with this incremental? I can fold that in, will rebase >> anyway to collect acks. > > Yes, that should work. > > Acked-by: Martin KaFai Lau <martin.lau@kernel.org> Thanks Martin, I'll add that too.
diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h index 2912dce9144e..a789266feac3 100644 --- a/include/linux/bpf-cgroup.h +++ b/include/linux/bpf-cgroup.h @@ -393,7 +393,7 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk, ({ \ int __ret = 0; \ if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT)) \ - get_user(__ret, optlen); \ + copy_from_sockptr(&__ret, optlen, sizeof(int)); \ __ret; \ }) diff --git a/include/net/sock.h b/include/net/sock.h index 00103e3143c4..1d6931caf0c3 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -1866,11 +1866,11 @@ int sock_setsockopt(struct socket *sock, int level, int op, sockptr_t optval, unsigned int optlen); int do_sock_setsockopt(struct socket *sock, bool compat, int level, int optname, sockptr_t optval, int optlen); +int do_sock_getsockopt(struct socket *sock, bool compat, int level, + int optname, sockptr_t optval, sockptr_t optlen); int sk_getsockopt(struct sock *sk, int level, int optname, sockptr_t optval, sockptr_t optlen); -int sock_getsockopt(struct socket *sock, int level, int op, - char __user *optval, int __user *optlen); int sock_gettstamp(struct socket *sock, void __user *userstamp, bool timeval, bool time32); struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len, diff --git a/net/core/sock.c b/net/core/sock.c index 290165954379..d4cb8d6e75b7 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -2003,14 +2003,6 @@ int sk_getsockopt(struct sock *sk, int level, int optname, return 0; } -int sock_getsockopt(struct socket *sock, int level, int optname, - char __user *optval, int __user *optlen) -{ - return sk_getsockopt(sock->sk, level, optname, - USER_SOCKPTR(optval), - USER_SOCKPTR(optlen)); -} - /* * Initialize an sk_lock. * diff --git a/net/socket.c b/net/socket.c index 0087f8c071e7..f4c156a1987e 100644 --- a/net/socket.c +++ b/net/socket.c @@ -2350,6 +2350,42 @@ SYSCALL_DEFINE5(setsockopt, int, fd, int, level, int, optname, INDIRECT_CALLABLE_DECLARE(bool tcp_bpf_bypass_getsockopt(int level, int optname)); +int do_sock_getsockopt(struct socket *sock, bool compat, int level, + int optname, sockptr_t optval, sockptr_t optlen) +{ + int max_optlen __maybe_unused; + const struct proto_ops *ops; + int err; + + err = security_socket_getsockopt(sock, level, optname); + if (err) + return err; + + ops = READ_ONCE(sock->ops); + if (level == SOL_SOCKET) { + err = sk_getsockopt(sock->sk, level, optname, optval, optlen); + } else if (unlikely(!ops->getsockopt)) { + err = -EOPNOTSUPP; + } else { + if (WARN_ONCE(optval.is_kernel || optlen.is_kernel, + "Invalid argument type")) + return -EOPNOTSUPP; + + err = ops->getsockopt(sock, level, optname, optval.user, + optlen.user); + } + + if (!compat) { + max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen); + err = BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock->sk, level, optname, + optval, optlen, max_optlen, + err); + } + + return err; +} +EXPORT_SYMBOL(do_sock_getsockopt); + /* * Get a socket option. Because we don't know the option lengths we have * to pass a user mode parameter for the protocols to sort out. @@ -2357,37 +2393,18 @@ INDIRECT_CALLABLE_DECLARE(bool tcp_bpf_bypass_getsockopt(int level, int __sys_getsockopt(int fd, int level, int optname, char __user *optval, int __user *optlen) { - int max_optlen __maybe_unused; - const struct proto_ops *ops; int err, fput_needed; struct socket *sock; + bool compat; sock = sockfd_lookup_light(fd, &err, &fput_needed); if (!sock) return err; - err = security_socket_getsockopt(sock, level, optname); - if (err) - goto out_put; - - if (!in_compat_syscall()) - max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen); + compat = in_compat_syscall(); + err = do_sock_getsockopt(sock, compat, level, optname, + USER_SOCKPTR(optval), USER_SOCKPTR(optlen)); - ops = READ_ONCE(sock->ops); - if (level == SOL_SOCKET) - err = sock_getsockopt(sock, level, optname, optval, optlen); - else if (unlikely(!ops->getsockopt)) - err = -EOPNOTSUPP; - else - err = ops->getsockopt(sock, level, optname, optval, - optlen); - - if (!in_compat_syscall()) - err = BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock->sk, level, optname, - USER_SOCKPTR(optval), - USER_SOCKPTR(optlen), - max_optlen, err); -out_put: fput_light(sock->file, fput_needed); return err; }
Split __sys_getsockopt() into two functions by removing the core logic into a sub-function (do_sock_getsockopt()). This will avoid code duplication when doing the same operation in other callers, for instance. do_sock_getsockopt() will be called by io_uring getsockopt() command operation in the following patch. The same was done for the setsockopt pair. Suggested-by: Martin KaFai Lau <martin.lau@linux.dev> Signed-off-by: Breno Leitao <leitao@debian.org> --- include/linux/bpf-cgroup.h | 2 +- include/net/sock.h | 4 +-- net/core/sock.c | 8 ----- net/socket.c | 63 ++++++++++++++++++++++++-------------- 4 files changed, 43 insertions(+), 34 deletions(-)