l2tp: hold tunnel while looking up sessions in l2tp_netlink

commit 54652eb12c1b72e9602d09cb2821d5760939190f upstream.

l2tp_tunnel_find() doesn't take a reference on the returned tunnel.
Therefore, it's unsafe to use it because the returned tunnel can go
away on us anytime.

Fix this by defining l2tp_tunnel_get(), which works like
l2tp_tunnel_find(), but takes a reference on the returned tunnel.
Caller then has to drop this reference using l2tp_tunnel_dec_refcount().

As l2tp_tunnel_dec_refcount() needs to be moved to l2tp_core.h, let's
simplify the patch and not move the L2TP_REFCNT_DEBUG part. This code
has been broken (not even compiling) in May 2012 by
commit a4ca44fa578c ("net: l2tp: Standardize logging styles")
and fixed more than two years later by
commit 29abe2fda54f ("l2tp: fix missing line continuation"). So it
doesn't appear to be used by anyone.

Same thing for l2tp_tunnel_free(); instead of moving it to l2tp_core.h,
let's just simplify things and call kfree_rcu() directly in
l2tp_tunnel_dec_refcount(). Extra assertions and debugging code
provided by l2tp_tunnel_free() didn't help catching any of the
reference counting and socket handling issues found while working on
this series.

Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
[bwh: Backported to 3.2: l2tp_tunnel_free() does more than just kfree_rcu(), so
 don't remove it]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 31c946f..b539c78 100644 (file)
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -112,7 +112,6 @@ struct l2tp_net {
 };
 
 static void l2tp_session_set_header_len(struct l2tp_session *session, int version);
-static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel);
 static void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel);
 
 static inline struct l2tp_net *l2tp_pernet(const struct net *net)
@@ -123,33 +122,6 @@ static inline struct l2tp_net *l2tp_pernet(const struct net *net)
 }
 
 
-/* Tunnel reference counts. Incremented per session that is added to
- * the tunnel.
- */
-static inline void l2tp_tunnel_inc_refcount_1(struct l2tp_tunnel *tunnel)
-{
-       atomic_inc(&tunnel->ref_count);
-}
-
-static inline void l2tp_tunnel_dec_refcount_1(struct l2tp_tunnel *tunnel)
-{
-       if (atomic_dec_and_test(&tunnel->ref_count))
-               l2tp_tunnel_free(tunnel);
-}
-#ifdef L2TP_REFCNT_DEBUG
-#define l2tp_tunnel_inc_refcount(_t) do { \
-               printk(KERN_DEBUG "l2tp_tunnel_inc_refcount: %s:%d %s: cnt=%d\n", __func__, __LINE__, (_t)->name, atomic_read(&_t->ref_count)); \
-               l2tp_tunnel_inc_refcount_1(_t);                         \
-       } while (0)
-#define l2tp_tunnel_dec_refcount(_t) do { \
-               printk(KERN_DEBUG "l2tp_tunnel_dec_refcount: %s:%d %s: cnt=%d\n", __func__, __LINE__, (_t)->name, atomic_read(&_t->ref_count)); \
-               l2tp_tunnel_dec_refcount_1(_t);                         \
-       } while (0)
-#else
-#define l2tp_tunnel_inc_refcount(t) l2tp_tunnel_inc_refcount_1(t)
-#define l2tp_tunnel_dec_refcount(t) l2tp_tunnel_dec_refcount_1(t)
-#endif
-
 /* Session hash global list for L2TPv3.
  * The session_id SHOULD be random according to RFC3931, but several
  * L2TP implementations use incrementing session_ids.  So we do a real
@@ -225,6 +197,27 @@ struct l2tp_session *l2tp_session_find(struct net *net, struct l2tp_tunnel *tunn
 }
 EXPORT_SYMBOL_GPL(l2tp_session_find);
 
+/* Lookup a tunnel. A new reference is held on the returned tunnel. */
+struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id)
+{
+       const struct l2tp_net *pn = l2tp_pernet(net);
+       struct l2tp_tunnel *tunnel;
+
+       rcu_read_lock_bh();
+       list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) {
+               if (tunnel->tunnel_id == tunnel_id) {
+                       l2tp_tunnel_inc_refcount(tunnel);
+                       rcu_read_unlock_bh();
+
+                       return tunnel;
+               }
+       }
+       rcu_read_unlock_bh();
+
+       return NULL;
+}
+EXPORT_SYMBOL_GPL(l2tp_tunnel_get);
+
 /* Like l2tp_session_find() but takes a reference on the returned session.
  * Optionally calls session->ref() too if do_ref is true.
  */
@@ -1341,7 +1334,7 @@ again:
 /* Really kill the tunnel.
  * Come here only when all sessions have been cleared from the tunnel.
  */
-static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel)
+void l2tp_tunnel_free(struct l2tp_tunnel *tunnel)
 {
        struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
 
@@ -1359,6 +1352,7 @@ static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel)
 
        atomic_dec(&l2tp_tunnel_count);
 }
+EXPORT_SYMBOL_GPL(l2tp_tunnel_free);
 
 /* Create a socket for the tunnel, if one isn't set up by
  * userspace. This is used for static tunnels where there is no

diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index ec3ac0e..2066d23 100644 (file)
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -222,6 +222,9 @@ out:
        return tunnel;
 }
 
+struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id);
+void l2tp_tunnel_free(struct l2tp_tunnel *tunnel);
+
 struct l2tp_session *l2tp_session_get(const struct net *net,
                                      struct l2tp_tunnel *tunnel,
                                      u32 session_id, bool do_ref);
@@ -248,6 +251,17 @@ extern int l2tp_nl_register_ops(enum l2tp_pwtype pw_type, const struct l2tp_nl_c
 extern void l2tp_nl_unregister_ops(enum l2tp_pwtype pw_type);
 int l2tp_ioctl(struct sock *sk, int cmd, unsigned long arg);
 
+static inline void l2tp_tunnel_inc_refcount(struct l2tp_tunnel *tunnel)
+{
+       atomic_inc(&tunnel->ref_count);
+}
+
+static inline void l2tp_tunnel_dec_refcount(struct l2tp_tunnel *tunnel)
+{
+       if (atomic_dec_and_test(&tunnel->ref_count))
+               l2tp_tunnel_free(tunnel);
+}
+
 /* Session reference counts. Incremented when code obtains a reference
  * to a session.
  */

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 6fd6322..b097d18 100644 (file)
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -57,10 +57,12 @@ static struct l2tp_session *l2tp_nl_session_get(struct genl_info *info,
                   (info->attrs[L2TP_ATTR_CONN_ID])) {
                tunnel_id = nla_get_u32(info->attrs[L2TP_ATTR_CONN_ID]);
                session_id = nla_get_u32(info->attrs[L2TP_ATTR_SESSION_ID]);
-               tunnel = l2tp_tunnel_find(net, tunnel_id);
-               if (tunnel)
+               tunnel = l2tp_tunnel_get(net, tunnel_id);
+               if (tunnel) {
                        session = l2tp_session_get(net, tunnel, session_id,
                                                   do_ref);
+                       l2tp_tunnel_dec_refcount(tunnel);
+               }
        }
 
        return session;