On 08/24, Martin KaFai Lau wrote:
This patch changes bpf_getsockopt(SOL_TCP) to reuse
do_tcp_getsockopt(). It removes the duplicated code from
bpf_getsockopt(SOL_TCP).
Before this patch, there were some optnames available to
bpf_setsockopt(SOL_TCP) but missing in bpf_getsockopt(SOL_TCP).
For example, TCP_NODELAY, TCP_MAXSEG, TCP_KEEPIDLE, TCP_KEEPINTVL,
and a few more. It surprises users from time to time. This patch
automatically closes this gap without duplicating more code.
bpf_getsockopt(TCP_SAVED_SYN) does not free the saved_syn,
so it stays in sol_tcp_sockopt().
For string name value like TCP_CONGESTION, bpf expects it
is always null terminated, so sol_tcp_sockopt() decrements
optlen by one before calling do_tcp_getsockopt() and
the 'if (optlen < saved_optlen) memset(..,0,..);'
in __bpf_getsockopt() will always do a null termination.
Signed-off-by: Martin KaFai Lau <kafai@xxxxxx>
---
include/net/tcp.h | 2 ++
net/core/filter.c | 70 ++++++++++++++++++++++++++---------------------
net/ipv4/tcp.c | 4 +--
3 files changed, 43 insertions(+), 33 deletions(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index c03a50c72f40..735e957f7f4b 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -402,6 +402,8 @@ void tcp_init_sock(struct sock *sk);
void tcp_init_transfer(struct sock *sk, int bpf_op, struct sk_buff *skb);
__poll_t tcp_poll(struct file *file, struct socket *sock,
struct poll_table_struct *wait);
+int do_tcp_getsockopt(struct sock *sk, int level,
+ int optname, sockptr_t optval, sockptr_t optlen);
int tcp_getsockopt(struct sock *sk, int level, int optname,
char __user *optval, int __user *optlen);
bool tcp_bpf_bypass_getsockopt(int level, int optname);
diff --git a/net/core/filter.c b/net/core/filter.c
index 68b52243b306..cdbbcec46e8b 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5096,8 +5096,9 @@ static int bpf_sol_tcp_setsockopt(struct sock *sk,
int optname,
return 0;
}
-static int sol_tcp_setsockopt(struct sock *sk, int optname,
- char *optval, int optlen)
+static int sol_tcp_sockopt(struct sock *sk, int optname,
+ char *optval, int *optlen,
+ bool getopt)
{
if (sk->sk_prot->setsockopt != tcp_setsockopt)
return -EINVAL;
@@ -5114,17 +5115,47 @@ static int sol_tcp_setsockopt(struct sock *sk,
int optname,
case TCP_USER_TIMEOUT:
case TCP_NOTSENT_LOWAT:
case TCP_SAVE_SYN:
- if (optlen != sizeof(int))
+ if (*optlen != sizeof(int))
return -EINVAL;
break;
[..]
case TCP_CONGESTION:
+ if (*optlen < 2)
+ return -EINVAL;
+ break;
+ case TCP_SAVED_SYN:
+ if (*optlen < 1)
+ return -EINVAL;
break;
This looks a bit inconsistent vs '*optlen != sizeof(int)' above. Maybe
if (*optlen < sizeof(u16))
if (*optlen < sizeof(u8))
?
default:
- return bpf_sol_tcp_setsockopt(sk, optname, optval, optlen);
+ if (getopt)
+ return -EINVAL;
+ return bpf_sol_tcp_setsockopt(sk, optname, optval, *optlen);
+ }
+
+ if (getopt) {
+ if (optname == TCP_SAVED_SYN) {
+ struct tcp_sock *tp = tcp_sk(sk);
+
+ if (!tp->saved_syn ||
+ *optlen > tcp_saved_syn_len(tp->saved_syn))
+ return -EINVAL;
You mention in the description that bpf doesn't doesn't free saved_syn,
maybe worth putting a comment with the rationale here as well?
I'm assuming we don't free from bpf because we want userspace to
have an opportunity to read it as well?
+ memcpy(optval, tp->saved_syn->data, *optlen);
+ return 0;
+ }
+
+ if (optname == TCP_CONGESTION) {
+ if (!inet_csk(sk)->icsk_ca_ops)
+ return -EINVAL;
Is it worth it doing null termination more explicitly here?
For readability sake:
/* BPF always expects NULL-terminated strings. */
optval[*optlen-1] = '\0';
+ (*optlen)--;
+ }
+
+ return do_tcp_getsockopt(sk, SOL_TCP, optname,
+ KERNEL_SOCKPTR(optval),
+ KERNEL_SOCKPTR(optlen));
}
return do_tcp_setsockopt(sk, SOL_TCP, optname,
- KERNEL_SOCKPTR(optval), optlen);
+ KERNEL_SOCKPTR(optval), *optlen);
}
static int sol_ip_setsockopt(struct sock *sk, int optname,
@@ -5179,7 +5210,7 @@ static int __bpf_setsockopt(struct sock *sk, int
level, int optname,
else if (IS_ENABLED(CONFIG_IPV6) && level == SOL_IPV6)
return sol_ipv6_setsockopt(sk, optname, optval, optlen);
else if (IS_ENABLED(CONFIG_INET) && level == SOL_TCP)
- return sol_tcp_setsockopt(sk, optname, optval, optlen);
+ return sol_tcp_sockopt(sk, optname, optval, &optlen, false);
return -EINVAL;
}
@@ -5202,31 +5233,8 @@ static int __bpf_getsockopt(struct sock *sk, int
level, int optname,
if (level == SOL_SOCKET) {
err = sol_socket_sockopt(sk, optname, optval, &optlen, true);
- } else if (IS_ENABLED(CONFIG_INET) &&
- level == SOL_TCP && sk->sk_prot->getsockopt == tcp_getsockopt) {
- struct inet_connection_sock *icsk;
- struct tcp_sock *tp;
-
- switch (optname) {
- case TCP_CONGESTION:
- icsk = inet_csk(sk);
-
- if (!icsk->icsk_ca_ops || optlen <= 1)
- goto err_clear;
- strncpy(optval, icsk->icsk_ca_ops->name, optlen);
- optval[optlen - 1] = 0;
- break;
- case TCP_SAVED_SYN:
- tp = tcp_sk(sk);
-
- if (optlen <= 0 || !tp->saved_syn ||
- optlen > tcp_saved_syn_len(tp->saved_syn))
- goto err_clear;
- memcpy(optval, tp->saved_syn->data, optlen);
- break;
- default:
- goto err_clear;
- }
+ } else if (IS_ENABLED(CONFIG_INET) && level == SOL_TCP) {
+ err = sol_tcp_sockopt(sk, optname, optval, &optlen, true);
} else if (IS_ENABLED(CONFIG_INET) && level == SOL_IP) {
struct inet_sock *inet = inet_sk(sk);
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index ab8118225797..a47cb5662be6 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4043,8 +4043,8 @@ struct sk_buff
*tcp_get_timestamping_opt_stats(const struct sock *sk,
return stats;
}
-static int do_tcp_getsockopt(struct sock *sk, int level,
- int optname, sockptr_t optval, sockptr_t optlen)
+int do_tcp_getsockopt(struct sock *sk, int level,
+ int optname, sockptr_t optval, sockptr_t optlen)
{
struct inet_connection_sock *icsk = inet_csk(sk);
struct tcp_sock *tp = tcp_sk(sk);
--
2.30.2
