2023-10-16 14:02:23

by Breno Leitao

Subject: [PATCH v7 04/11] net/socket: Break down __sys_getsockopt

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 <[email protected]>
Signed-off-by: Breno Leitao <[email protected]>
---
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(-)

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;
}
--
2.34.1

2023-10-19 00:21:12

by Jakub Kicinski

Subject: Re: [PATCH v7 04/11] net/socket: Break down __sys_getsockopt

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 <[email protected]>

2023-10-19 19:12:35

by Martin KaFai Lau

Subject: Re: [PATCH v7 04/11] net/socket: Break down __sys_getsockopt

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;
> }

2023-10-19 20:05:02

by Jens Axboe

Subject: Re: [PATCH v7 04/11] net/socket: Break down __sys_getsockopt

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;
}

--
Jens Axboe

2023-10-19 20:38:37

by Martin KaFai Lau

Subject: Re: [PATCH v7 04/11] net/socket: Break down __sys_getsockopt

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 <[email protected]>

>
>
> 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;
> }
>

2023-10-19 22:33:04

by Jens Axboe

Subject: Re: [PATCH v7 04/11] net/socket: Break down __sys_getsockopt

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 <[email protected]>

Thanks Martin, I'll add that too.

--
Jens Axboe