| Message ID | 1510050988-17688-1-git-send-email-laoar.shao@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net/tcp: track all ipv4/tcp state transition in tcp_set_state | expand |
From: Yafang Shao <laoar.shao@gmail.com> Date: Tue, 7 Nov 2017 18:36:28 +0800 > When I hooked the function tcp_set_state with kprobe to track the ipv4/tcp > state transistion, I found state transition from TCP_LISTEN to TCP_SYN_RECV > is missed. > > I think it is better to use the helper to do state transition instead of > assigning the state to sk_state directly. > Then we can monitor the whole tcp lifespans with kprobe or ftrace easily. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> This is really heavy handed and excessive for these cases. They don't have to handle any of the issues dealt with in tcp_set_state(). I would prefer if you made a special helper to net/tcp.h which did: static inline void __tcp_set_state(struct sock *sk, int state) { trace_tcp_set_state(sk, sk->sk_state, state); sk->sk_state = state; }
2017-11-08 14:51 GMT+08:00 David Miller <davem@davemloft.net>: > From: Yafang Shao <laoar.shao@gmail.com> > Date: Tue, 7 Nov 2017 18:36:28 +0800 > >> When I hooked the function tcp_set_state with kprobe to track the ipv4/tcp >> state transistion, I found state transition from TCP_LISTEN to TCP_SYN_RECV >> is missed. >> >> I think it is better to use the helper to do state transition instead of >> assigning the state to sk_state directly. >> Then we can monitor the whole tcp lifespans with kprobe or ftrace easily. >> >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > This is really heavy handed and excessive for these cases. > > They don't have to handle any of the issues dealt with in > tcp_set_state(). > > I would prefer if you made a special helper to net/tcp.h which did: > > static inline void __tcp_set_state(struct sock *sk, int state) > { > trace_tcp_set_state(sk, sk->sk_state, state); > sk->sk_state = state; > } Good idea! I will try to implement it. Thanks Yafang
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index c039c93..7c987c5 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -786,7 +786,7 @@ struct sock *inet_csk_clone_lock(const struct sock *sk, if (newsk) { struct inet_connection_sock *newicsk = inet_csk(newsk); - newsk->sk_state = TCP_SYN_RECV; + tcp_set_state(newsk, TCP_SYN_RECV); newicsk->icsk_bind_hash = NULL; inet_sk(newsk)->inet_dport = inet_rsk(req)->ir_rmt_port; @@ -880,7 +880,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog) * It is OK, because this socket enters to hash table only * after validation is complete. */ - sk_state_store(sk, TCP_LISTEN); + tcp_set_state(sk, TCP_LISTEN); if (!sk->sk_prot->get_port(sk, inet->inet_num)) { inet->inet_sport = htons(inet->inet_num); @@ -891,7 +891,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog) return 0; } - sk->sk_state = TCP_CLOSE; + tcp_set_state(sk, TCP_CLOSE); return err; } EXPORT_SYMBOL_GPL(inet_csk_listen_start); diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c index 597bb4c..bea8318 100644 --- a/net/ipv4/inet_hashtables.c +++ b/net/ipv4/inet_hashtables.c @@ -430,7 +430,7 @@ bool inet_ehash_nolisten(struct sock *sk, struct sock *osk) sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1); } else { percpu_counter_inc(sk->sk_prot->orphan_count); - sk->sk_state = TCP_CLOSE; + tcp_set_state(sk, TCP_CLOSE); sock_set_flag(sk, SOCK_DEAD); inet_csk_destroy_sock(sk); }
When I hooked the function tcp_set_state with kprobe to track the ipv4/tcp state transistion, I found state transition from TCP_LISTEN to TCP_SYN_RECV is missed. I think it is better to use the helper to do state transition instead of assigning the state to sk_state directly. Then we can monitor the whole tcp lifespans with kprobe or ftrace easily. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- net/ipv4/inet_connection_sock.c | 6 +++--- net/ipv4/inet_hashtables.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)