Re: [PATCH net-next] net: bail on sock_wfree, sock_rfree when we have a TCP_TIMEWAIT sk
flat view
From: Eric Dumazet <hidden>
Date: 2015-07-01 23:54:00
Subsystem:
ipvs, netfilter, networking [general], networking [sockets], the rest · Maintainers:
Simon Horman, Julian Anastasov, Pablo Neira Ayuso, Florian Westphal, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kuniyuki Iwashima, Willem de Bruijn, Linus Torvalds
Possibly related (same subject, not in this thread)
- 2015-07-01 · Re: [PATCH net-next] net: bail on sock_wfree, sock_rfree when we have a TCP_TIMEWAIT sk · Eric Dumazet <hidden>
- 2015-07-01 · Re: [PATCH net-next] net: bail on sock_wfree, sock_rfree when we have a TCP_TIMEWAIT sk · David Miller <davem@davemloft.net>
- 2015-07-01 · [PATCH net-next] net: bail on sock_wfree, sock_rfree when we have a TCP_TIMEWAIT sk · Alex Gartrell <hidden>
On Thu, 2015-07-02 at 01:26 +0200, Eric Dumazet wrote:
On Thu, Jul 2, 2015 at 1:18 AM, Alex Gartrell [off-list ref] wrote:quoted
On Wednesday, July 1, 2015, Eric Dumazet [off-list ref] wrote:quoted
On Wed, Jul 1, 2015 at 11:14 PM, David Miller [off-list ref] wrote:quoted
From: Alex Gartrell <redacted> Date: Wed, 1 Jul 2015 13:13:09 -0700quoted
If we early-demux bind a TCP_TIMEWAIT socket to an skb and then orphan it (as we need to do in the ipvs forwarding case), sock_wfree and sock_rfree are going to reach into the inet_timewait_sock and mess with fields that don't exist. Signed-off-by: Alex Gartrell <redacted>If we're forwarding, we should not find a local socket, period.A socket cannot change state to TCP_TIMEWAIT. A new object is allocated and old one is removed from ehash, then freed (rcu rules being applied) Also sock_wfree() has nothing to do with early demux. It is for output path skbs only.Alright I kind of cheated and didn't include full context here. The problem is that within ipvs we are getting packets that have been early demuxed and associated with time wait sockets which we then wish to forward immediately (ip_vs_xmit.c). Under normal circumstances it would never be associated with any sk at all, but it is because of early demux, so we want to drop the relationship by calling skb_orphan. This invokes the destructor which lands us there. So that is how we reach this illegal "treating a twsk like an sk" state. If there is a better way to drop the association than skb_orphan I will use it.I think you are mistaken Alex. socket early demux cannot possibly set skb->destructor to sock_rfree() If skb->destructor is set by early demux, it correctly points to sock_edemux() And this one correctly handles all socket variants.
If ipvs is the problem, could you try instead following patch ? Shoot in the dark, as you do not give a lot of details :(
diff --git a/include/net/sock.h b/include/net/sock.h
index 05a8c1aea251..f77fe9acc7a4 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h@@ -1932,6 +1932,14 @@ static inline void skb_set_hash_from_sk(struct sk_buff *skb, struct sock *sk) } } +/* This helper checks if a socket is a full socket, + * ie _not_ a timewait or request socket. + */ +static inline bool sk_fullsock(const struct sock *sk) +{ + return (1 << sk->sk_state) & ~(TCPF_TIME_WAIT | TCPF_NEW_SYN_RECV); +} + /* * Queue a received datagram if it will fit. Stream and sequenced * protocols can't normally use this as they need to fit buffers in
@@ -1944,6 +1952,9 @@ static inline void skb_set_hash_from_sk(struct sk_buff *skb, struct sock *sk) static inline void skb_set_owner_w(struct sk_buff *skb, struct sock *sk) { skb_orphan(skb); + if (unlikely(!sk_fullsock(sk)) + return; + skb->sk = sk; skb->destructor = sock_wfree; skb_set_hash_from_sk(skb, sk);
@@ -2204,14 +2215,6 @@ static inline struct sock *skb_steal_sock(struct sk_buff *skb) return NULL; } -/* This helper checks if a socket is a full socket, - * ie _not_ a timewait or request socket. - */ -static inline bool sk_fullsock(const struct sock *sk) -{ - return (1 << sk->sk_state) & ~(TCPF_TIME_WAIT | TCPF_NEW_SYN_RECV); -} - void sock_enable_timestamp(struct sock *sk, int flag); int sock_get_timestamp(struct sock *, struct timeval __user *); int sock_get_timestampns(struct sock *, struct timespec __user *);
diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 5d2b806a862e..ff05ec5a9016 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c@@ -1161,9 +1161,10 @@ ip_vs_out(unsigned int hooknum, struct sk_buff *skb, int af) if (unlikely(skb->sk != NULL && hooknum == NF_INET_LOCAL_OUT && af == AF_INET)) { struct sock *sk = skb->sk; - struct inet_sock *inet = inet_sk(skb->sk); - if (inet && sk->sk_family == PF_INET && inet->nodefrag) + if (sk_fullsock(sk) && + sk->sk_family == PF_INET && + inet_sk(sk)->nodefrag) return NF_ACCEPT; }
@@ -1640,9 +1641,10 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af) if (unlikely(skb->sk != NULL && hooknum == NF_INET_LOCAL_OUT && af == AF_INET)) { struct sock *sk = skb->sk; - struct inet_sock *inet = inet_sk(skb->sk); - if (inet && sk->sk_family == PF_INET && inet->nodefrag) + if (sk_fullsock(sk) && + sk->sk_family == PF_INET && + inet_sk(sk)->nodefrag) return NF_ACCEPT; }