linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: linux-kernel@vger.kernel.org, greg@kroah.com
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	stable@vger.kernel.org, Guillaume Nault <g.nault@alphalink.fr>,
	"David S. Miller" <davem@davemloft.net>,
	Giuliano Procida <gprocida@google.com>
Subject: [PATCH 4.9 37/64] l2tp: hold tunnel while looking up sessions in l2tp_netlink
Date: Tue, 26 May 2020 20:53:06 +0200	[thread overview]
Message-ID: <20200526183925.482465386@linuxfoundation.org> (raw)
In-Reply-To: <20200526183913.064413230@linuxfoundation.org>

From: Guillaume Nault <g.nault@alphalink.fr>

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.

Backporting Notes

l2tp_core.c: This patch deletes some code / moves some code to
l2tp_core.h and follows the patch (not including in this series) that
switched from atomic to refcount_t. Moved code changed back to atomic.

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>
Signed-off-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 net/l2tp/l2tp_core.c    |   66 +++++++++++++++---------------------------------
 net/l2tp/l2tp_core.h    |   13 +++++++++
 net/l2tp/l2tp_netlink.c |    6 ++--
 3 files changed, 38 insertions(+), 47 deletions(-)

--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -112,7 +112,6 @@ struct l2tp_net {
 	spinlock_t l2tp_session_hlist_lock;
 };
 
-static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel);
 
 static inline struct l2tp_tunnel *l2tp_tunnel(struct sock *sk)
 {
@@ -126,39 +125,6 @@ static inline struct l2tp_net *l2tp_pern
 	return net_generic(net, l2tp_net_id);
 }
 
-/* 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 {									\
-	pr_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 {									\
-	pr_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
@@ -228,6 +194,27 @@ l2tp_session_id_hash(struct l2tp_tunnel
 	return &tunnel->session_hlist[hash_32(session_id, L2TP_HASH_BITS)];
 }
 
+/* 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);
+
 /* Lookup a session. A new reference is held on the returned session.
  * Optionally calls session->ref() too if do_ref is true.
  */
@@ -1346,17 +1333,6 @@ static void l2tp_udp_encap_destroy(struc
 	}
 }
 
-/* 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)
-{
-	BUG_ON(atomic_read(&tunnel->ref_count) != 0);
-	BUG_ON(tunnel->sock != NULL);
-	l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: free...\n", tunnel->name);
-	kfree_rcu(tunnel, rcu);
-}
-
 /* Workqueue tunnel deletion function */
 static void l2tp_tunnel_del_work(struct work_struct *work)
 {
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -231,6 +231,8 @@ out:
 	return tunnel;
 }
 
+struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id);
+
 struct l2tp_session *l2tp_session_get(const struct net *net,
 				      struct l2tp_tunnel *tunnel,
 				      u32 session_id, bool do_ref);
@@ -269,6 +271,17 @@ int l2tp_nl_register_ops(enum l2tp_pwtyp
 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))
+		kfree_rcu(tunnel, rcu);
+}
+
 /* Session reference counts. Incremented when code obtains a reference
  * to a session.
  */
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -72,10 +72,12 @@ static struct l2tp_session *l2tp_nl_sess
 		   (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;



  parent reply	other threads:[~2020-05-26 18:59 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-26 18:52 [PATCH 4.9 00/64] 4.9.225-rc1 review Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 01/64] igb: use igb_adapter->io_addr instead of e1000_hw->hw_addr Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 02/64] padata: Remove unused but set variables Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 03/64] padata: get_next is never NULL Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 04/64] padata: ensure the reorder timer callback runs on the correct CPU Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 05/64] padata: ensure padata_do_serial() " Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 06/64] evm: Check also if *tfm is an error pointer in init_desc() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 07/64] ima: Fix return value of ima_write_policy() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 08/64] fix multiplication overflow in copy_fdtable() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 09/64] iommu/amd: Fix over-read of ACPI UID from IVRS table Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 10/64] i2c: mux: demux-pinctrl: Fix an error handling path in i2c_demux_pinctrl_probe() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 11/64] gcc-common.h: Update for GCC 10 Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 12/64] HID: multitouch: add eGalaxTouch P80H84 support Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 13/64] configfs: fix config_item refcnt leak in configfs_rmdir() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 14/64] component: Silence bind error on -EPROBE_DEFER Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 15/64] gtp: set NLM_F_MULTI flag in gtp_genl_dump_pdp() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 16/64] ceph: fix double unlock in handle_cap_export() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 17/64] USB: core: Fix misleading driver bug report Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 18/64] platform/x86: asus-nb-wmi: Do not load on Asus T100TA and T200TA Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 19/64] ARM: futex: Address build warning Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 20/64] i2c: dev: Fix the race between the release of i2c_dev and cdev Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 21/64] padata: set cpu_index of unused CPUs to -1 Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 22/64] padata: Replace delayed timer with immediate workqueue in padata_reorder Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 23/64] padata: initialize pd->cpu with effective cpumask Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 24/64] padata: purge get_cpu and reorder_via_wq from padata_do_serial Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 25/64] arm64: fix the flush_icache_range arguments in machine_kexec Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 26/64] watchdog: Fix the race between the release of watchdog_core_data and cdev Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 27/64] net: l2tp: export debug flags to UAPI Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 28/64] net: l2tp: deprecate PPPOL2TP_MSG_* in favour of L2TP_MSG_* Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 29/64] net: l2tp: ppp: change PPPOL2TP_MSG_* => L2TP_MSG_* Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.9 30/64] New kernel function to get IP overhead on a socket Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 31/64] L2TP:Adjust intf MTU, add underlay L3, L2 hdrs Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 32/64] l2tp: remove useless duplicate session detection in l2tp_netlink Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 33/64] l2tp: remove l2tp_session_find() Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 34/64] l2tp: define parameters of l2tp_session_get*() as "const" Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 35/64] l2tp: define parameters of l2tp_tunnel_find*() " Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 36/64] l2tp: initialise sessions refcount before making it reachable Greg Kroah-Hartman
2020-05-26 18:53 ` Greg Kroah-Hartman [this message]
2020-05-26 18:53 ` [PATCH 4.9 38/64] l2tp: hold tunnel while processing genl delete command Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 39/64] l2tp: hold tunnel while handling genl tunnel updates Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 40/64] l2tp: hold tunnel while handling genl TUNNEL_GET commands Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 41/64] l2tp: hold tunnel used while creating sessions with netlink Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 42/64] l2tp: prevent creation of sessions on terminated tunnels Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 43/64] l2tp: pass tunnel pointer to ->session_create() Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 44/64] l2tp: fix l2tp_eth module loading Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 45/64] l2tp: dont register sessions in l2tp_session_create() Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 46/64] l2tp: initialise l2tp_eth sessions before registering them Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 47/64] l2tp: protect sock pointer of struct pppol2tp_session with RCU Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 48/64] l2tp: initialise PPP sessions before registering them Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 49/64] ALSA: pcm: fix incorrect hw_base increase Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 50/64] dmaengine: tegra210-adma: Fix an error handling path in tegra_adma_probe() Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 51/64] l2tp: device MTU setup, tunnel socket needs a lock Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 52/64] x86/uaccess, ubsan: Fix UBSAN vs. SMAP Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 53/64] ubsan: build ubsan.c more conservatively Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 54/64] platform/x86: alienware-wmi: fix kfree on potentially uninitialized pointer Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 55/64] libnvdimm/btt: Remove unnecessary code in btt_freelist_init Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 56/64] cxgb4: free mac_hlist properly Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 57/64] cxgb4/cxgb4vf: Fix mac_hlist initialization and free Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 58/64] Revert "gfs2: Dont demote a glock until its revokes are written" Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 59/64] staging: iio: ad2s1210: Fix SPI reading Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 60/64] staging: greybus: Fix uninitialized scalar variable Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 61/64] iio: dac: vf610: Fix an error handling path in vf610_dac_probe() Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 62/64] mei: release me_cl object reference Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 63/64] rapidio: fix an error in get_user_pages_fast() error handling Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.9 64/64] iio: sca3000: Remove an erroneous get_device() Greg Kroah-Hartman
2020-05-27  8:32 ` [PATCH 4.9 00/64] 4.9.225-rc1 review Jon Hunter
2020-05-27  8:49 ` Naresh Kamboju
2020-05-27 13:51 ` Guenter Roeck
2020-05-27 16:53 ` shuah

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200526183925.482465386@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=davem@davemloft.net \
    --cc=g.nault@alphalink.fr \
    --cc=gprocida@google.com \
    --cc=greg@kroah.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).