| Message ID | 20220824222730.1923992-1-kafai@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: net: Remove duplicated code from bpf_getsockopt() | expand |
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@fb.com> > --- > 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
On Thu, Aug 25, 2022 at 11:36:36AM -0700, sdf@google.com wrote: > 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@fb.com> > > --- > > 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)) TCP_CONGESTION (name string) and TCP_SAVED_SYN (raw binary) are not expecting integer optval, so I think it is better to stay away from using integer u16 or 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? Yes, it is the reason. I will add a comment. > > > + 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'; Yep. will do in v2. > > + (*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); > > }
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; 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; + memcpy(optval, tp->saved_syn->data, *optlen); + return 0; + } + + if (optname == TCP_CONGESTION) { + if (!inet_csk(sk)->icsk_ca_ops) + return -EINVAL; + (*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);
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@fb.com> --- include/net/tcp.h | 2 ++ net/core/filter.c | 70 ++++++++++++++++++++++++++--------------------- net/ipv4/tcp.c | 4 +-- 3 files changed, 43 insertions(+), 33 deletions(-)