{"id":805928,"url":"http://patchwork.ozlabs.org/api/patches/805928/?format=json","web_url":"http://patchwork.ozlabs.org/project/netdev/patch/11ba48ce2320c0e9fc70872c448e3e3b9276516d.1503671776.git.g.nault@alphalink.fr/","project":{"id":7,"url":"http://patchwork.ozlabs.org/api/projects/7/?format=json","name":"Linux network development","link_name":"netdev","list_id":"netdev.vger.kernel.org","list_email":"netdev@vger.kernel.org","web_url":null,"scm_url":null,"webscm_url":null,"list_archive_url":"","list_archive_url_format":"","commit_url_format":""},"msgid":"<11ba48ce2320c0e9fc70872c448e3e3b9276516d.1503671776.git.g.nault@alphalink.fr>","list_archive_url":null,"date":"2017-08-25T14:51:40","name":"[net,1/5] l2tp: hold tunnel while looking up sessions in l2tp_netlink","commit_ref":null,"pull_url":null,"state":"accepted","archived":true,"hash":"952e035491e8735723cf58d06e37f4238cbf5ac3","submitter":{"id":22975,"url":"http://patchwork.ozlabs.org/api/people/22975/?format=json","name":"Guillaume Nault","email":"g.nault@alphalink.fr"},"delegate":{"id":34,"url":"http://patchwork.ozlabs.org/api/users/34/?format=json","username":"davem","first_name":"David","last_name":"Miller","email":"davem@davemloft.net"},"mbox":"http://patchwork.ozlabs.org/project/netdev/patch/11ba48ce2320c0e9fc70872c448e3e3b9276516d.1503671776.git.g.nault@alphalink.fr/mbox/","series":[],"comments":"http://patchwork.ozlabs.org/api/patches/805928/comments/","check":"pending","checks":"http://patchwork.ozlabs.org/api/patches/805928/checks/","tags":{},"related":[],"headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xf3yw5HHsz9sN5\n\tfor ;\n\tSat, 26 Aug 2017 00:51:52 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1757288AbdHYOvu (ORCPT );\n\tFri, 25 Aug 2017 10:51:50 -0400","from zimbra.alphalink.fr ([217.15.80.77]:53407 \"EHLO\n\tzimbra.alphalink.fr\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1755006AbdHYOvr (ORCPT\n\t); Fri, 25 Aug 2017 10:51:47 -0400","from localhost (localhost [127.0.0.1])\n\tby mail-2-cbv2.admin.alphalink.fr (Postfix) with ESMTP id\n\t00F1C2B5206F; Fri, 25 Aug 2017 16:51:45 +0200 (CEST)","from zimbra.alphalink.fr ([127.0.0.1])\n\tby localhost (mail-2-cbv2.admin.alphalink.fr [127.0.0.1])\n\t(amavisd-new, port 10032)\n\twith ESMTP id LWRYC8gJ_BOq; Fri, 25 Aug 2017 16:51:41 +0200 (CEST)","from localhost (localhost [127.0.0.1])\n\tby mail-2-cbv2.admin.alphalink.fr (Postfix) with ESMTP id\n\t340DD2B52097; Fri, 25 Aug 2017 16:51:41 +0200 (CEST)","from zimbra.alphalink.fr ([127.0.0.1])\n\tby localhost (mail-2-cbv2.admin.alphalink.fr [127.0.0.1])\n\t(amavisd-new, port 10026)\n\twith ESMTP id AUBFXNXgVwDz; Fri, 25 Aug 2017 16:51:41 +0200 (CEST)","from c-dev-0.admin.alphalink.fr (94-84-15-217.reverse.alphalink.fr\n\t[217.15.84.94])\n\tby mail-2-cbv2.admin.alphalink.fr (Postfix) with ESMTP id\n\t0434C2B5206F; Fri, 25 Aug 2017 16:51:41 +0200 (CEST)","by c-dev-0.admin.alphalink.fr (Postfix, from userid 1000)\n\tid DC1D260176; Fri, 25 Aug 2017 16:51:40 +0200 (CEST)"],"X-Virus-Scanned":"amavisd-new at mail-2-cbv2.admin.alphalink.fr","Date":"Fri, 25 Aug 2017 16:51:40 +0200","From":"Guillaume Nault ","To":"netdev@vger.kernel.org","Cc":"James Chapman ","Subject":"[PATCH net 1/5] l2tp: hold tunnel while looking up sessions in\n\tl2tp_netlink","Message-ID":"<11ba48ce2320c0e9fc70872c448e3e3b9276516d.1503671776.git.g.nault@alphalink.fr>","References":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","X-Mutt-Fcc":"=Sent","User-Agent":"NeoMutt/20170609 (1.8.3)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"},"content":"l2tp_tunnel_find() doesn't take a reference on the returned tunnel.\nTherefore, it's unsafe to use it because the returned tunnel can go\naway on us anytime.\n\nFix this by defining l2tp_tunnel_get(), which works like\nl2tp_tunnel_find(), but takes a reference on the returned tunnel.\nCaller then has to drop this reference using l2tp_tunnel_dec_refcount().\n\nAs l2tp_tunnel_dec_refcount() needs to be moved to l2tp_core.h, let's\nsimplify the patch and not move the L2TP_REFCNT_DEBUG part. This code\nhas been broken (not even compiling) in May 2012 by\ncommit a4ca44fa578c (\"net: l2tp: Standardize logging styles\")\nand fixed more than two years later by\ncommit 29abe2fda54f (\"l2tp: fix missing line continuation\"). So it\ndoesn't appear to be used by anyone.\n\nSame thing for l2tp_tunnel_free(); instead of moving it to l2tp_core.h,\nlet's just simplify things and call kfree_rcu() directly in\nl2tp_tunnel_dec_refcount(). Extra assertions and debugging code\nprovided by l2tp_tunnel_free() didn't help catching any of the\nreference counting and socket handling issues found while working on\nthis series.\n\nFixes: 309795f4bec2 (\"l2tp: Add netlink control API for L2TP\")\nSigned-off-by: Guillaume Nault \n---\n net/l2tp/l2tp_core.c | 66 ++++++++++++++++---------------------------------\n net/l2tp/l2tp_core.h | 13 ++++++++++\n net/l2tp/l2tp_netlink.c | 6 +++--\n 3 files changed, 38 insertions(+), 47 deletions(-)","diff":"diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c\nindex b0c2d4ae781d..1adebfb9bde2 100644\n--- a/net/l2tp/l2tp_core.c\n+++ b/net/l2tp/l2tp_core.c\n@@ -113,7 +113,6 @@ struct l2tp_net {\n \tspinlock_t l2tp_session_hlist_lock;\n };\n \n-static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel);\n \n static inline struct l2tp_tunnel *l2tp_tunnel(struct sock *sk)\n {\n@@ -127,39 +126,6 @@ static inline struct l2tp_net *l2tp_pernet(const struct net *net)\n \treturn net_generic(net, l2tp_net_id);\n }\n \n-/* Tunnel reference counts. Incremented per session that is added to\n- * the tunnel.\n- */\n-static inline void l2tp_tunnel_inc_refcount_1(struct l2tp_tunnel *tunnel)\n-{\n-\trefcount_inc(&tunnel->ref_count);\n-}\n-\n-static inline void l2tp_tunnel_dec_refcount_1(struct l2tp_tunnel *tunnel)\n-{\n-\tif (refcount_dec_and_test(&tunnel->ref_count))\n-\t\tl2tp_tunnel_free(tunnel);\n-}\n-#ifdef L2TP_REFCNT_DEBUG\n-#define l2tp_tunnel_inc_refcount(_t)\t\t\t\t\t\\\n-do {\t\t\t\t\t\t\t\t\t\\\n-\tpr_debug(\"l2tp_tunnel_inc_refcount: %s:%d %s: cnt=%d\\n\",\t\\\n-\t\t __func__, __LINE__, (_t)->name,\t\t\t\\\n-\t\t refcount_read(&_t->ref_count));\t\t\t\\\n-\tl2tp_tunnel_inc_refcount_1(_t);\t\t\t\t\t\\\n-} while (0)\n-#define l2tp_tunnel_dec_refcount(_t)\t\t\t\t\t\\\n-do {\t\t\t\t\t\t\t\t\t\\\n-\tpr_debug(\"l2tp_tunnel_dec_refcount: %s:%d %s: cnt=%d\\n\",\t\\\n-\t\t __func__, __LINE__, (_t)->name,\t\t\t\\\n-\t\t refcount_read(&_t->ref_count));\t\t\t\\\n-\tl2tp_tunnel_dec_refcount_1(_t);\t\t\t\t\t\\\n-} while (0)\n-#else\n-#define l2tp_tunnel_inc_refcount(t) l2tp_tunnel_inc_refcount_1(t)\n-#define l2tp_tunnel_dec_refcount(t) l2tp_tunnel_dec_refcount_1(t)\n-#endif\n-\n /* Session hash global list for L2TPv3.\n * The session_id SHOULD be random according to RFC3931, but several\n * L2TP implementations use incrementing session_ids. So we do a real\n@@ -229,6 +195,27 @@ l2tp_session_id_hash(struct l2tp_tunnel *tunnel, u32 session_id)\n \treturn &tunnel->session_hlist[hash_32(session_id, L2TP_HASH_BITS)];\n }\n \n+/* Lookup a tunnel. A new reference is held on the returned tunnel. */\n+struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id)\n+{\n+\tconst struct l2tp_net *pn = l2tp_pernet(net);\n+\tstruct l2tp_tunnel *tunnel;\n+\n+\trcu_read_lock_bh();\n+\tlist_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) {\n+\t\tif (tunnel->tunnel_id == tunnel_id) {\n+\t\t\tl2tp_tunnel_inc_refcount(tunnel);\n+\t\t\trcu_read_unlock_bh();\n+\n+\t\t\treturn tunnel;\n+\t\t}\n+\t}\n+\trcu_read_unlock_bh();\n+\n+\treturn NULL;\n+}\n+EXPORT_SYMBOL_GPL(l2tp_tunnel_get);\n+\n /* Lookup a session. A new reference is held on the returned session.\n * Optionally calls session->ref() too if do_ref is true.\n */\n@@ -1348,17 +1335,6 @@ static void l2tp_udp_encap_destroy(struct sock *sk)\n \t}\n }\n \n-/* Really kill the tunnel.\n- * Come here only when all sessions have been cleared from the tunnel.\n- */\n-static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel)\n-{\n-\tBUG_ON(refcount_read(&tunnel->ref_count) != 0);\n-\tBUG_ON(tunnel->sock != NULL);\n-\tl2tp_info(tunnel, L2TP_MSG_CONTROL, \"%s: free...\\n\", tunnel->name);\n-\tkfree_rcu(tunnel, rcu);\n-}\n-\n /* Workqueue tunnel deletion function */\n static void l2tp_tunnel_del_work(struct work_struct *work)\n {\ndiff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h\nindex cdb6e3327f74..9101297f27ad 100644\n--- a/net/l2tp/l2tp_core.h\n+++ b/net/l2tp/l2tp_core.h\n@@ -231,6 +231,8 @@ static inline struct l2tp_tunnel *l2tp_sock_to_tunnel(struct sock *sk)\n \treturn tunnel;\n }\n \n+struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id);\n+\n struct l2tp_session *l2tp_session_get(const struct net *net,\n \t\t\t\t struct l2tp_tunnel *tunnel,\n \t\t\t\t u32 session_id, bool do_ref);\n@@ -269,6 +271,17 @@ int l2tp_nl_register_ops(enum l2tp_pwtype pw_type,\n void l2tp_nl_unregister_ops(enum l2tp_pwtype pw_type);\n int l2tp_ioctl(struct sock *sk, int cmd, unsigned long arg);\n \n+static inline void l2tp_tunnel_inc_refcount(struct l2tp_tunnel *tunnel)\n+{\n+\trefcount_inc(&tunnel->ref_count);\n+}\n+\n+static inline void l2tp_tunnel_dec_refcount(struct l2tp_tunnel *tunnel)\n+{\n+\tif (refcount_dec_and_test(&tunnel->ref_count))\n+\t\tkfree_rcu(tunnel, rcu);\n+}\n+\n /* Session reference counts. Incremented when code obtains a reference\n * to a session.\n */\ndiff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c\nindex 12cfcd0ca807..27ee94b5c189 100644\n--- a/net/l2tp/l2tp_netlink.c\n+++ b/net/l2tp/l2tp_netlink.c\n@@ -65,10 +65,12 @@ static struct l2tp_session *l2tp_nl_session_get(struct genl_info *info,\n \t\t (info->attrs[L2TP_ATTR_CONN_ID])) {\n \t\ttunnel_id = nla_get_u32(info->attrs[L2TP_ATTR_CONN_ID]);\n \t\tsession_id = nla_get_u32(info->attrs[L2TP_ATTR_SESSION_ID]);\n-\t\ttunnel = l2tp_tunnel_find(net, tunnel_id);\n-\t\tif (tunnel)\n+\t\ttunnel = l2tp_tunnel_get(net, tunnel_id);\n+\t\tif (tunnel) {\n \t\t\tsession = l2tp_session_get(net, tunnel, session_id,\n \t\t\t\t\t\t do_ref);\n+\t\t\tl2tp_tunnel_dec_refcount(tunnel);\n+\t\t}\n \t}\n \n \treturn session;\n","prefixes":["net","1/5"]}