* [PATCH bpf-next] bpf: add SO_KEEPALIVE and related options to bpf_setsockopt
@ 2020-05-27 15:05 Dmitry Yakunin
2020-05-27 16:42 ` Eric Dumazet
2020-05-27 17:02 ` Martin KaFai Lau
0 siblings, 2 replies; 6+ messages in thread
From: Dmitry Yakunin @ 2020-05-27 15:05 UTC (permalink / raw)
To: davem, brakmo; +Cc: bpf, netdev
This patch adds support of SO_KEEPALIVE flag and TCP related options
to bpf_setsockopt() routine. This is helpful if we want to enable or tune
TCP keepalive for applications which don't do it in the userspace code.
In order to avoid copy-paste, common code from classic setsockopt was moved
to auxiliary functions in the headers.
Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
---
include/net/sock.h | 9 +++++++++
include/net/tcp.h | 18 ++++++++++++++++++
net/core/filter.c | 39 ++++++++++++++++++++++++++++++++++++++-
net/core/sock.c | 9 ---------
net/ipv4/tcp.c | 15 ++-------------
5 files changed, 67 insertions(+), 23 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 3e8c6d4..ee35dea 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -879,6 +879,15 @@ static inline void sock_reset_flag(struct sock *sk, enum sock_flags flag)
__clear_bit(flag, &sk->sk_flags);
}
+static inline void sock_valbool_flag(struct sock *sk, enum sock_flags bit,
+ int valbool)
+{
+ if (valbool)
+ sock_set_flag(sk, bit);
+ else
+ sock_reset_flag(sk, bit);
+}
+
static inline bool sock_flag(const struct sock *sk, enum sock_flags flag)
{
return test_bit(flag, &sk->sk_flags);
diff --git a/include/net/tcp.h b/include/net/tcp.h
index b681338..ae6a495 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1465,6 +1465,24 @@ static inline u32 keepalive_time_elapsed(const struct tcp_sock *tp)
tcp_jiffies32 - tp->rcv_tstamp);
}
+/* val must be validated at the top level function */
+static inline void keepalive_time_set(struct tcp_sock *tp, int val)
+{
+ struct sock *sk = (struct sock *)tp;
+
+ tp->keepalive_time = val * HZ;
+ if (sock_flag(sk, SOCK_KEEPOPEN) &&
+ !((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN))) {
+ u32 elapsed = keepalive_time_elapsed(tp);
+
+ if (tp->keepalive_time > elapsed)
+ elapsed = tp->keepalive_time - elapsed;
+ else
+ elapsed = 0;
+ inet_csk_reset_keepalive_timer(sk, elapsed);
+ }
+}
+
static inline int tcp_fin_time(const struct sock *sk)
{
int fin_timeout = tcp_sk(sk)->linger2 ? : sock_net(sk)->ipv4.sysctl_tcp_fin_timeout;
diff --git a/net/core/filter.c b/net/core/filter.c
index a6fc234..1035e43 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4248,8 +4248,8 @@ static const struct bpf_func_proto bpf_get_socket_uid_proto = {
static int _bpf_setsockopt(struct sock *sk, int level, int optname,
char *optval, int optlen, u32 flags)
{
+ int val, valbool;
int ret = 0;
- int val;
if (!sk_fullsock(sk))
return -EINVAL;
@@ -4260,6 +4260,7 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
if (optlen != sizeof(int))
return -EINVAL;
val = *((int *)optval);
+ valbool = val ? 1 : 0;
/* Only some socketops are supported */
switch (optname) {
@@ -4298,6 +4299,11 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
sk_dst_reset(sk);
}
break;
+ case SO_KEEPALIVE:
+ if (sk->sk_prot->keepalive)
+ sk->sk_prot->keepalive(sk, valbool);
+ sock_valbool_flag(sk, SOCK_KEEPOPEN, valbool);
+ break;
default:
ret = -EINVAL;
}
@@ -4358,6 +4364,7 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
ret = tcp_set_congestion_control(sk, name, false,
reinit, true);
} else {
+ struct inet_connection_sock *icsk = inet_csk(sk);
struct tcp_sock *tp = tcp_sk(sk);
if (optlen != sizeof(int))
@@ -4386,6 +4393,36 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
else
tp->save_syn = val;
break;
+ case TCP_KEEPIDLE:
+ if (val < 1 || val > MAX_TCP_KEEPIDLE)
+ ret = -EINVAL;
+ else
+ keepalive_time_set(tp, val);
+ break;
+ case TCP_KEEPINTVL:
+ if (val < 1 || val > MAX_TCP_KEEPINTVL)
+ ret = -EINVAL;
+ else
+ tp->keepalive_intvl = val * HZ;
+ break;
+ case TCP_KEEPCNT:
+ if (val < 1 || val > MAX_TCP_KEEPCNT)
+ ret = -EINVAL;
+ else
+ tp->keepalive_probes = val;
+ break;
+ case TCP_SYNCNT:
+ if (val < 1 || val > MAX_TCP_SYNCNT)
+ ret = -EINVAL;
+ else
+ icsk->icsk_syn_retries = val;
+ break;
+ case TCP_USER_TIMEOUT:
+ if (val < 0)
+ ret = -EINVAL;
+ else
+ icsk->icsk_user_timeout = val;
+ break;
default:
ret = -EINVAL;
}
diff --git a/net/core/sock.c b/net/core/sock.c
index fd85e65..9836b01 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -684,15 +684,6 @@ static int sock_getbindtodevice(struct sock *sk, char __user *optval,
return ret;
}
-static inline void sock_valbool_flag(struct sock *sk, enum sock_flags bit,
- int valbool)
-{
- if (valbool)
- sock_set_flag(sk, bit);
- else
- sock_reset_flag(sk, bit);
-}
-
bool sk_mc_loop(struct sock *sk)
{
if (dev_recursion_level())
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 9700649..7b239e8 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3003,19 +3003,8 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
case TCP_KEEPIDLE:
if (val < 1 || val > MAX_TCP_KEEPIDLE)
err = -EINVAL;
- else {
- tp->keepalive_time = val * HZ;
- if (sock_flag(sk, SOCK_KEEPOPEN) &&
- !((1 << sk->sk_state) &
- (TCPF_CLOSE | TCPF_LISTEN))) {
- u32 elapsed = keepalive_time_elapsed(tp);
- if (tp->keepalive_time > elapsed)
- elapsed = tp->keepalive_time - elapsed;
- else
- elapsed = 0;
- inet_csk_reset_keepalive_timer(sk, elapsed);
- }
- }
+ else
+ keepalive_time_set(tp, val);
break;
case TCP_KEEPINTVL:
if (val < 1 || val > MAX_TCP_KEEPINTVL)
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: add SO_KEEPALIVE and related options to bpf_setsockopt
2020-05-27 15:05 [PATCH bpf-next] bpf: add SO_KEEPALIVE and related options to bpf_setsockopt Dmitry Yakunin
@ 2020-05-27 16:42 ` Eric Dumazet
2020-05-27 20:17 ` Dmitry Yakunin
2020-05-29 12:41 ` David Laight
2020-05-27 17:02 ` Martin KaFai Lau
1 sibling, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2020-05-27 16:42 UTC (permalink / raw)
To: Dmitry Yakunin, davem, brakmo; +Cc: bpf, netdev
On 5/27/20 8:05 AM, Dmitry Yakunin wrote:
> This patch adds support of SO_KEEPALIVE flag and TCP related options
> to bpf_setsockopt() routine. This is helpful if we want to enable or tune
> TCP keepalive for applications which don't do it in the userspace code.
> In order to avoid copy-paste, common code from classic setsockopt was moved
> to auxiliary functions in the headers.
Please split this in two patches :
- one adding the helpers, a pure TCP patch.
- one for BPF additions.
>
> Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
> ---
> include/net/sock.h | 9 +++++++++
> include/net/tcp.h | 18 ++++++++++++++++++
> net/core/filter.c | 39 ++++++++++++++++++++++++++++++++++++++-
> net/core/sock.c | 9 ---------
> net/ipv4/tcp.c | 15 ++-------------
> 5 files changed, 67 insertions(+), 23 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 3e8c6d4..ee35dea 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -879,6 +879,15 @@ static inline void sock_reset_flag(struct sock *sk, enum sock_flags flag)
> __clear_bit(flag, &sk->sk_flags);
> }
>
> +static inline void sock_valbool_flag(struct sock *sk, enum sock_flags bit,
> + int valbool)
> +{
> + if (valbool)
> + sock_set_flag(sk, bit);
> + else
> + sock_reset_flag(sk, bit);
> +}
> +
> static inline bool sock_flag(const struct sock *sk, enum sock_flags flag)
> {
> return test_bit(flag, &sk->sk_flags);
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index b681338..ae6a495 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1465,6 +1465,24 @@ static inline u32 keepalive_time_elapsed(const struct tcp_sock *tp)
> tcp_jiffies32 - tp->rcv_tstamp);
> }
>
> +/* val must be validated at the top level function */
> +static inline void keepalive_time_set(struct tcp_sock *tp, int val)
> +{
> + struct sock *sk = (struct sock *)tp;
We prefer the other way to avoid a cast unless really needed :
static inline tcp_keepalive_time_set(struct sock *sk, int val)
{
stuct tcp_sock *tp = tcp_sk(sk);
> +
> + tp->keepalive_time = val * HZ;
> + if (sock_flag(sk, SOCK_KEEPOPEN) &&
> + !((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN))) {
> + u32 elapsed = keepalive_time_elapsed(tp);
> +
> + if (tp->keepalive_time > elapsed)
> + elapsed = tp->keepalive_time - elapsed;
> + else
> + elapsed = 0;
> + inet_csk_reset_keepalive_timer(sk, elapsed);
> + }
> +}
> +
> static inline int tcp_fin_time(const struct sock *sk)
> {
> int fin_timeout = tcp_sk(sk)->linger2 ? : sock_net(sk)->ipv4.sysctl_tcp_fin_timeout;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index a6fc234..1035e43 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4248,8 +4248,8 @@ static const struct bpf_func_proto bpf_get_socket_uid_proto = {
> static int _bpf_setsockopt(struct sock *sk, int level, int optname,
> char *optval, int optlen, u32 flags)
> {
> + int val, valbool;
> int ret = 0;
> - int val;
>
> if (!sk_fullsock(sk))
> return -EINVAL;
> @@ -4260,6 +4260,7 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
> if (optlen != sizeof(int))
> return -EINVAL;
> val = *((int *)optval);
> + valbool = val ? 1 : 0;
>
> /* Only some socketops are supported */
> switch (optname) {
> @@ -4298,6 +4299,11 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
> sk_dst_reset(sk);
> }
> break;
> + case SO_KEEPALIVE:
> + if (sk->sk_prot->keepalive)
> + sk->sk_prot->keepalive(sk, valbool);
> + sock_valbool_flag(sk, SOCK_KEEPOPEN, valbool);
> + break;
> default:
> ret = -EINVAL;
> }
> @@ -4358,6 +4364,7 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
> ret = tcp_set_congestion_control(sk, name, false,
> reinit, true);
> } else {
> + struct inet_connection_sock *icsk = inet_csk(sk);
> struct tcp_sock *tp = tcp_sk(sk);
>
> if (optlen != sizeof(int))
> @@ -4386,6 +4393,36 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
> else
> tp->save_syn = val;
> break;
> + case TCP_KEEPIDLE:
> + if (val < 1 || val > MAX_TCP_KEEPIDLE)
> + ret = -EINVAL;
> + else
> + keepalive_time_set(tp, val);
> + break;
> + case TCP_KEEPINTVL:
> + if (val < 1 || val > MAX_TCP_KEEPINTVL)
> + ret = -EINVAL;
> + else
> + tp->keepalive_intvl = val * HZ;
> + break;
> + case TCP_KEEPCNT:
> + if (val < 1 || val > MAX_TCP_KEEPCNT)
> + ret = -EINVAL;
> + else
> + tp->keepalive_probes = val;
> + break;
> + case TCP_SYNCNT:
> + if (val < 1 || val > MAX_TCP_SYNCNT)
> + ret = -EINVAL;
> + else
> + icsk->icsk_syn_retries = val;
> + break;
> + case TCP_USER_TIMEOUT:
> + if (val < 0)
> + ret = -EINVAL;
> + else
> + icsk->icsk_user_timeout = val;
> + break;
> default:
> ret = -EINVAL;
> }
> diff --git a/net/core/sock.c b/net/core/sock.c
> index fd85e65..9836b01 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -684,15 +684,6 @@ static int sock_getbindtodevice(struct sock *sk, char __user *optval,
> return ret;
> }
>
> -static inline void sock_valbool_flag(struct sock *sk, enum sock_flags bit,
> - int valbool)
> -{
> - if (valbool)
> - sock_set_flag(sk, bit);
> - else
> - sock_reset_flag(sk, bit);
> -}
> -
> bool sk_mc_loop(struct sock *sk)
> {
> if (dev_recursion_level())
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 9700649..7b239e8 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -3003,19 +3003,8 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
> case TCP_KEEPIDLE:
> if (val < 1 || val > MAX_TCP_KEEPIDLE)
> err = -EINVAL;
> - else {
> - tp->keepalive_time = val * HZ;
> - if (sock_flag(sk, SOCK_KEEPOPEN) &&
> - !((1 << sk->sk_state) &
> - (TCPF_CLOSE | TCPF_LISTEN))) {
> - u32 elapsed = keepalive_time_elapsed(tp);
> - if (tp->keepalive_time > elapsed)
> - elapsed = tp->keepalive_time - elapsed;
> - else
> - elapsed = 0;
> - inet_csk_reset_keepalive_timer(sk, elapsed);
> - }
> - }
> + else
> + keepalive_time_set(tp, val);
> break;
> case TCP_KEEPINTVL:
> if (val < 1 || val > MAX_TCP_KEEPINTVL)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: add SO_KEEPALIVE and related options to bpf_setsockopt
2020-05-27 15:05 [PATCH bpf-next] bpf: add SO_KEEPALIVE and related options to bpf_setsockopt Dmitry Yakunin
2020-05-27 16:42 ` Eric Dumazet
@ 2020-05-27 17:02 ` Martin KaFai Lau
2020-05-27 20:44 ` Dmitry Yakunin
1 sibling, 1 reply; 6+ messages in thread
From: Martin KaFai Lau @ 2020-05-27 17:02 UTC (permalink / raw)
To: Dmitry Yakunin; +Cc: davem, brakmo, bpf, netdev
On Wed, May 27, 2020 at 06:05:43PM +0300, Dmitry Yakunin wrote:
> This patch adds support of SO_KEEPALIVE flag and TCP related options
> to bpf_setsockopt() routine. This is helpful if we want to enable or tune
> TCP keepalive for applications which don't do it in the userspace code.
> In order to avoid copy-paste, common code from classic setsockopt was moved
> to auxiliary functions in the headers.
Thanks for refatoring some of the pieces. I suspect some more can be done.
In the long run, I don't think this copy-and-paste is scalable.
For most of the options (integer value and do not need ns_capable()),
do_tcp_setsockopt() and sock_setsockopt() can be directly called with
some refactoring.
The change looks good. For this patch,
Acked-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: add SO_KEEPALIVE and related options to bpf_setsockopt
2020-05-27 16:42 ` Eric Dumazet
@ 2020-05-27 20:17 ` Dmitry Yakunin
2020-05-29 12:41 ` David Laight
1 sibling, 0 replies; 6+ messages in thread
From: Dmitry Yakunin @ 2020-05-27 20:17 UTC (permalink / raw)
To: Eric Dumazet, davem, brakmo; +Cc: bpf, netdev
27.05.2020, 19:43, "Eric Dumazet" <eric.dumazet@gmail.com>:
> On 5/27/20 8:05 AM, Dmitry Yakunin wrote:
>> This patch adds support of SO_KEEPALIVE flag and TCP related options
>> to bpf_setsockopt() routine. This is helpful if we want to enable or tune
>> TCP keepalive for applications which don't do it in the userspace code.
>> In order to avoid copy-paste, common code from classic setsockopt was moved
>> to auxiliary functions in the headers.
>
> Please split this in two patches :
> - one adding the helpers, a pure TCP patch.
> - one for BPF additions.
Thanks for your comment.
I've split this into three patches (sock, tcp and bpf) and resent.
>> Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
>> ---
>> include/net/sock.h | 9 +++++++++
>> include/net/tcp.h | 18 ++++++++++++++++++
>> net/core/filter.c | 39 ++++++++++++++++++++++++++++++++++++++-
>> net/core/sock.c | 9 ---------
>> net/ipv4/tcp.c | 15 ++-------------
>> 5 files changed, 67 insertions(+), 23 deletions(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index 3e8c6d4..ee35dea 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -879,6 +879,15 @@ static inline void sock_reset_flag(struct sock *sk, enum sock_flags flag)
>> __clear_bit(flag, &sk->sk_flags);
>> }
>>
>> +static inline void sock_valbool_flag(struct sock *sk, enum sock_flags bit,
>> + int valbool)
>> +{
>> + if (valbool)
>> + sock_set_flag(sk, bit);
>> + else
>> + sock_reset_flag(sk, bit);
>> +}
>> +
>> static inline bool sock_flag(const struct sock *sk, enum sock_flags flag)
>> {
>> return test_bit(flag, &sk->sk_flags);
>> diff --git a/include/net/tcp.h b/include/net/tcp.h
>> index b681338..ae6a495 100644
>> --- a/include/net/tcp.h
>> +++ b/include/net/tcp.h
>> @@ -1465,6 +1465,24 @@ static inline u32 keepalive_time_elapsed(const struct tcp_sock *tp)
>> tcp_jiffies32 - tp->rcv_tstamp);
>> }
>>
>> +/* val must be validated at the top level function */
>> +static inline void keepalive_time_set(struct tcp_sock *tp, int val)
>> +{
>> + struct sock *sk = (struct sock *)tp;
>
> We prefer the other way to avoid a cast unless really needed :
>
> static inline tcp_keepalive_time_set(struct sock *sk, int val)
> {
> stuct tcp_sock *tp = tcp_sk(sk);
>
>> +
>> + tp->keepalive_time = val * HZ;
>> + if (sock_flag(sk, SOCK_KEEPOPEN) &&
>> + !((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN))) {
>> + u32 elapsed = keepalive_time_elapsed(tp);
>> +
>> + if (tp->keepalive_time > elapsed)
>> + elapsed = tp->keepalive_time - elapsed;
>> + else
>> + elapsed = 0;
>> + inet_csk_reset_keepalive_timer(sk, elapsed);
>> + }
>> +}
>> +
>> static inline int tcp_fin_time(const struct sock *sk)
>> {
>> int fin_timeout = tcp_sk(sk)->linger2 ? : sock_net(sk)->ipv4.sysctl_tcp_fin_timeout;
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index a6fc234..1035e43 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -4248,8 +4248,8 @@ static const struct bpf_func_proto bpf_get_socket_uid_proto = {
>> static int _bpf_setsockopt(struct sock *sk, int level, int optname,
>> char *optval, int optlen, u32 flags)
>> {
>> + int val, valbool;
>> int ret = 0;
>> - int val;
>>
>> if (!sk_fullsock(sk))
>> return -EINVAL;
>> @@ -4260,6 +4260,7 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
>> if (optlen != sizeof(int))
>> return -EINVAL;
>> val = *((int *)optval);
>> + valbool = val ? 1 : 0;
>>
>> /* Only some socketops are supported */
>> switch (optname) {
>> @@ -4298,6 +4299,11 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
>> sk_dst_reset(sk);
>> }
>> break;
>> + case SO_KEEPALIVE:
>> + if (sk->sk_prot->keepalive)
>> + sk->sk_prot->keepalive(sk, valbool);
>> + sock_valbool_flag(sk, SOCK_KEEPOPEN, valbool);
>> + break;
>> default:
>> ret = -EINVAL;
>> }
>> @@ -4358,6 +4364,7 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
>> ret = tcp_set_congestion_control(sk, name, false,
>> reinit, true);
>> } else {
>> + struct inet_connection_sock *icsk = inet_csk(sk);
>> struct tcp_sock *tp = tcp_sk(sk);
>>
>> if (optlen != sizeof(int))
>> @@ -4386,6 +4393,36 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
>> else
>> tp->save_syn = val;
>> break;
>> + case TCP_KEEPIDLE:
>> + if (val < 1 || val > MAX_TCP_KEEPIDLE)
>> + ret = -EINVAL;
>> + else
>> + keepalive_time_set(tp, val);
>> + break;
>> + case TCP_KEEPINTVL:
>> + if (val < 1 || val > MAX_TCP_KEEPINTVL)
>> + ret = -EINVAL;
>> + else
>> + tp->keepalive_intvl = val * HZ;
>> + break;
>> + case TCP_KEEPCNT:
>> + if (val < 1 || val > MAX_TCP_KEEPCNT)
>> + ret = -EINVAL;
>> + else
>> + tp->keepalive_probes = val;
>> + break;
>> + case TCP_SYNCNT:
>> + if (val < 1 || val > MAX_TCP_SYNCNT)
>> + ret = -EINVAL;
>> + else
>> + icsk->icsk_syn_retries = val;
>> + break;
>> + case TCP_USER_TIMEOUT:
>> + if (val < 0)
>> + ret = -EINVAL;
>> + else
>> + icsk->icsk_user_timeout = val;
>> + break;
>> default:
>> ret = -EINVAL;
>> }
>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index fd85e65..9836b01 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -684,15 +684,6 @@ static int sock_getbindtodevice(struct sock *sk, char __user *optval,
>> return ret;
>> }
>>
>> -static inline void sock_valbool_flag(struct sock *sk, enum sock_flags bit,
>> - int valbool)
>> -{
>> - if (valbool)
>> - sock_set_flag(sk, bit);
>> - else
>> - sock_reset_flag(sk, bit);
>> -}
>> -
>> bool sk_mc_loop(struct sock *sk)
>> {
>> if (dev_recursion_level())
>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>> index 9700649..7b239e8 100644
>> --- a/net/ipv4/tcp.c
>> +++ b/net/ipv4/tcp.c
>> @@ -3003,19 +3003,8 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
>> case TCP_KEEPIDLE:
>> if (val < 1 || val > MAX_TCP_KEEPIDLE)
>> err = -EINVAL;
>> - else {
>> - tp->keepalive_time = val * HZ;
>> - if (sock_flag(sk, SOCK_KEEPOPEN) &&
>> - !((1 << sk->sk_state) &
>> - (TCPF_CLOSE | TCPF_LISTEN))) {
>> - u32 elapsed = keepalive_time_elapsed(tp);
>> - if (tp->keepalive_time > elapsed)
>> - elapsed = tp->keepalive_time - elapsed;
>> - else
>> - elapsed = 0;
>> - inet_csk_reset_keepalive_timer(sk, elapsed);
>> - }
>> - }
>> + else
>> + keepalive_time_set(tp, val);
>> break;
>> case TCP_KEEPINTVL:
>> if (val < 1 || val > MAX_TCP_KEEPINTVL)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: add SO_KEEPALIVE and related options to bpf_setsockopt
2020-05-27 17:02 ` Martin KaFai Lau
@ 2020-05-27 20:44 ` Dmitry Yakunin
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Yakunin @ 2020-05-27 20:44 UTC (permalink / raw)
To: Martin KaFai Lau; +Cc: davem, brakmo, bpf, netdev
27.05.2020, 20:03, "Martin KaFai Lau" <kafai@fb.com>:
> On Wed, May 27, 2020 at 06:05:43PM +0300, Dmitry Yakunin wrote:
>> This patch adds support of SO_KEEPALIVE flag and TCP related options
>> to bpf_setsockopt() routine. This is helpful if we want to enable or tune
>> TCP keepalive for applications which don't do it in the userspace code.
>> In order to avoid copy-paste, common code from classic setsockopt was moved
>> to auxiliary functions in the headers.
>
> Thanks for refatoring some of the pieces. I suspect some more can be done.
> In the long run, I don't think this copy-and-paste is scalable.
> For most of the options (integer value and do not need ns_capable()),
> do_tcp_setsockopt() and sock_setsockopt() can be directly called with
> some refactoring.
>
> The change looks good. For this patch,
>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
Thanks for your comment, Martin.
I agree with you, bpf_setsockopt and older setsockopts should be refactored to use common code.
I'll keep this in mind if i want to add new options.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH bpf-next] bpf: add SO_KEEPALIVE and related options to bpf_setsockopt
2020-05-27 16:42 ` Eric Dumazet
2020-05-27 20:17 ` Dmitry Yakunin
@ 2020-05-29 12:41 ` David Laight
1 sibling, 0 replies; 6+ messages in thread
From: David Laight @ 2020-05-29 12:41 UTC (permalink / raw)
To: 'Eric Dumazet', Dmitry Yakunin, davem, brakmo; +Cc: bpf, netdev
From: Eric Dumazet
> Sent: 27 May 2020 17:43
>
> On 5/27/20 8:05 AM, Dmitry Yakunin wrote:
> > This patch adds support of SO_KEEPALIVE flag and TCP related options
> > to bpf_setsockopt() routine. This is helpful if we want to enable or tune
> > TCP keepalive for applications which don't do it in the userspace code.
> > In order to avoid copy-paste, common code from classic setsockopt was moved
> > to auxiliary functions in the headers.
>
>
> Please split this in two patches :
> - one adding the helpers, a pure TCP patch.
> - one for BPF additions.
>
...
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index a6fc234..1035e43 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
...
> > + case TCP_KEEPIDLE:
> > + if (val < 1 || val > MAX_TCP_KEEPIDLE)
> > + ret = -EINVAL;
> > + else
> > + keepalive_time_set(tp, val);
> > + break;
> > + case TCP_KEEPINTVL:
> > + if (val < 1 || val > MAX_TCP_KEEPINTVL)
> > + ret = -EINVAL;
> > + else
> > + tp->keepalive_intvl = val * HZ;
> > + break;
> > + case TCP_KEEPCNT:
> > + if (val < 1 || val > MAX_TCP_KEEPCNT)
> > + ret = -EINVAL;
> > + else
> > + tp->keepalive_probes = val;
> > + break;
> > + case TCP_SYNCNT:
> > + if (val < 1 || val > MAX_TCP_SYNCNT)
> > + ret = -EINVAL;
> > + else
> > + icsk->icsk_syn_retries = val;
> > + break;
> > + case TCP_USER_TIMEOUT:
> > + if (val < 0)
> > + ret = -EINVAL;
> > + else
> > + icsk->icsk_user_timeout = val;
> > + break;
It also cannot be right to be layer breaking like this
and directly accessing the protocol socket internals.
At least a kernel_setsockopt() function keeps this separate
and ensures that the socket type is correct.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-29 12:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 15:05 [PATCH bpf-next] bpf: add SO_KEEPALIVE and related options to bpf_setsockopt Dmitry Yakunin
2020-05-27 16:42 ` Eric Dumazet
2020-05-27 20:17 ` Dmitry Yakunin
2020-05-29 12:41 ` David Laight
2020-05-27 17:02 ` Martin KaFai Lau
2020-05-27 20:44 ` Dmitry Yakunin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).