espintcp: remove encap socket caching to avoid reference leak

[ Upstream commit 028363685bd0b7a19b4a820f82dd905b1dc83999 ]

The current scheme for caching the encap socket can lead to reference
leaks when we try to delete the netns.

The reference chain is: xfrm_state -> enacp_sk -> netns

Since the encap socket is a userspace socket, it holds a reference on
the netns. If we delete the espintcp state (through flush or
individual delete) before removing the netns, the reference on the
socket is dropped and the netns is correctly deleted. Otherwise, the
netns may not be reachable anymore (if all processes within the ns
have terminated), so we cannot delete the xfrm state to drop its
reference on the socket.

This patch results in a small (~2% in my tests) performance
regression.

A GC-type mechanism could be added for the socket cache, to clear
references if the state hasn't been used "recently", but it's a lot
more complex than just not caching the socket.

Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index bf670929622dc9195b1da51c68a5cb30d43a0bc7..64911162ab5f44957424180d824f9bcd91e420f4 100644 (file)
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -212,7 +212,6 @@ struct xfrm_state {
 
        /* Data for encapsulator */
        struct xfrm_encap_tmpl  *encap;
-       struct sock __rcu       *encap_sk;
 
        /* Data for care-of address */
        xfrm_address_t  *coaddr;

diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index 419969b2682259a7af053556bb4a6db0004add3d..8f5417ff355d71cae247ad6a437e483193f5b1d4 100644 (file)
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -118,47 +118,16 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp)
 }
 
 #ifdef CONFIG_INET_ESPINTCP
-struct esp_tcp_sk {
-       struct sock *sk;
-       struct rcu_head rcu;
-};
-
-static void esp_free_tcp_sk(struct rcu_head *head)
-{
-       struct esp_tcp_sk *esk = container_of(head, struct esp_tcp_sk, rcu);
-
-       sock_put(esk->sk);
-       kfree(esk);
-}
-
 static struct sock *esp_find_tcp_sk(struct xfrm_state *x)
 {
        struct xfrm_encap_tmpl *encap = x->encap;
        struct net *net = xs_net(x);
-       struct esp_tcp_sk *esk;
        __be16 sport, dport;
-       struct sock *nsk;
        struct sock *sk;
 
-       sk = rcu_dereference(x->encap_sk);
-       if (sk && sk->sk_state == TCP_ESTABLISHED)
-               return sk;
-
        spin_lock_bh(&x->lock);
        sport = encap->encap_sport;
        dport = encap->encap_dport;
-       nsk = rcu_dereference_protected(x->encap_sk,
-                                       lockdep_is_held(&x->lock));
-       if (sk && sk == nsk) {
-               esk = kmalloc(sizeof(*esk), GFP_ATOMIC);
-               if (!esk) {
-                       spin_unlock_bh(&x->lock);
-                       return ERR_PTR(-ENOMEM);
-               }
-               RCU_INIT_POINTER(x->encap_sk, NULL);
-               esk->sk = sk;
-               call_rcu(&esk->rcu, esp_free_tcp_sk);
-       }
        spin_unlock_bh(&x->lock);
 
        sk = inet_lookup_established(net, net->ipv4.tcp_death_row.hashinfo, x->id.daddr.a4,
@@ -171,20 +140,6 @@ static struct sock *esp_find_tcp_sk(struct xfrm_state *x)
                return ERR_PTR(-EINVAL);
        }
 
-       spin_lock_bh(&x->lock);
-       nsk = rcu_dereference_protected(x->encap_sk,
-                                       lockdep_is_held(&x->lock));
-       if (encap->encap_sport != sport ||
-           encap->encap_dport != dport) {
-               sock_put(sk);
-               sk = nsk ?: ERR_PTR(-EREMCHG);
-       } else if (sk == nsk) {
-               sock_put(sk);
-       } else {
-               rcu_assign_pointer(x->encap_sk, sk);
-       }
-       spin_unlock_bh(&x->lock);
-
        return sk;
 }
 
@@ -207,6 +162,8 @@ static int esp_output_tcp_finish(struct xfrm_state *x, struct sk_buff *skb)
                err = espintcp_push_skb(sk, skb);
        bh_unlock_sock(sk);
 
+       sock_put(sk);
+
 out:
        rcu_read_unlock();
        return err;
@@ -391,6 +348,8 @@ static struct ip_esp_hdr *esp_output_tcp_encap(struct xfrm_state *x,
        if (IS_ERR(sk))
                return ERR_CAST(sk);
 
+       sock_put(sk);
+
        *lenp = htons(len);
        esph = (struct ip_esp_hdr *)(lenp + 1);
 

diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index a021c88d3d9b81315f51c282326056cff61c713e..085a83b807afd02849b2a81c6ce82598ec7f25f4 100644 (file)
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -135,47 +135,16 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp)
 }
 
 #ifdef CONFIG_INET6_ESPINTCP
-struct esp_tcp_sk {
-       struct sock *sk;
-       struct rcu_head rcu;
-};
-
-static void esp_free_tcp_sk(struct rcu_head *head)
-{
-       struct esp_tcp_sk *esk = container_of(head, struct esp_tcp_sk, rcu);
-
-       sock_put(esk->sk);
-       kfree(esk);
-}
-
 static struct sock *esp6_find_tcp_sk(struct xfrm_state *x)
 {
        struct xfrm_encap_tmpl *encap = x->encap;
        struct net *net = xs_net(x);
-       struct esp_tcp_sk *esk;
        __be16 sport, dport;
-       struct sock *nsk;
        struct sock *sk;
 
-       sk = rcu_dereference(x->encap_sk);
-       if (sk && sk->sk_state == TCP_ESTABLISHED)
-               return sk;
-
        spin_lock_bh(&x->lock);
        sport = encap->encap_sport;
        dport = encap->encap_dport;
-       nsk = rcu_dereference_protected(x->encap_sk,
-                                       lockdep_is_held(&x->lock));
-       if (sk && sk == nsk) {
-               esk = kmalloc(sizeof(*esk), GFP_ATOMIC);
-               if (!esk) {
-                       spin_unlock_bh(&x->lock);
-                       return ERR_PTR(-ENOMEM);
-               }
-               RCU_INIT_POINTER(x->encap_sk, NULL);
-               esk->sk = sk;
-               call_rcu(&esk->rcu, esp_free_tcp_sk);
-       }
        spin_unlock_bh(&x->lock);
 
        sk = __inet6_lookup_established(net, net->ipv4.tcp_death_row.hashinfo, &x->id.daddr.in6,
@@ -188,20 +157,6 @@ static struct sock *esp6_find_tcp_sk(struct xfrm_state *x)
                return ERR_PTR(-EINVAL);
        }
 
-       spin_lock_bh(&x->lock);
-       nsk = rcu_dereference_protected(x->encap_sk,
-                                       lockdep_is_held(&x->lock));
-       if (encap->encap_sport != sport ||
-           encap->encap_dport != dport) {
-               sock_put(sk);
-               sk = nsk ?: ERR_PTR(-EREMCHG);
-       } else if (sk == nsk) {
-               sock_put(sk);
-       } else {
-               rcu_assign_pointer(x->encap_sk, sk);
-       }
-       spin_unlock_bh(&x->lock);
-
        return sk;
 }
 
@@ -224,6 +179,8 @@ static int esp_output_tcp_finish(struct xfrm_state *x, struct sk_buff *skb)
                err = espintcp_push_skb(sk, skb);
        bh_unlock_sock(sk);
 
+       sock_put(sk);
+
 out:
        rcu_read_unlock();
        return err;
@@ -427,6 +384,8 @@ static struct ip_esp_hdr *esp6_output_tcp_encap(struct xfrm_state *x,
        if (IS_ERR(sk))
                return ERR_CAST(sk);
 
+       sock_put(sk);
+
        *lenp = htons(len);
        esph = (struct ip_esp_hdr *)(lenp + 1);
 

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 2f4cf976b59a38eb5011c44ebb04f0fbd428e1e8..b5047a94c7d0147ecb4617f4a5afa07bc0c07918 100644 (file)
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -694,9 +694,6 @@ int __xfrm_state_delete(struct xfrm_state *x)
                net->xfrm.state_num--;
                spin_unlock(&net->xfrm.xfrm_state_lock);
 
-               if (x->encap_sk)
-                       sock_put(rcu_dereference_raw(x->encap_sk));
-
                xfrm_dev_state_delete(x);
 
                /* All xfrm_state objects are created by xfrm_state_alloc.