stable.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.4 49/65] l2tp: hold tunnel while looking up sessions in l2tp_netlink
Date: Tue, 26 May 2020 20:53:08 +0200	[thread overview]
Message-ID: <20200526183922.958551997@linuxfoundation.org> (raw)
In-Reply-To: <20200526183905.988782958@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.
  */
@@ -1351,17 +1338,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
@@ -234,6 +234,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);
@@ -272,6 +274,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 19:32 UTC|newest]

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-26 18:52 [PATCH 4.4 00/65] 4.4.225-rc1 review Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 01/65] igb: use igb_adapter->io_addr instead of e1000_hw->hw_addr Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 02/65] padata: Remove unused but set variables Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 03/65] padata: get_next is never NULL Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 04/65] padata: ensure the reorder timer callback runs on the correct CPU Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 05/65] padata: ensure padata_do_serial() " Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 06/65] evm: Check also if *tfm is an error pointer in init_desc() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 07/65] fix multiplication overflow in copy_fdtable() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 08/65] HID: multitouch: add eGalaxTouch P80H84 support Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 09/65] ceph: fix double unlock in handle_cap_export() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 10/65] USB: core: Fix misleading driver bug report Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 11/65] platform/x86: asus-nb-wmi: Do not load on Asus T100TA and T200TA Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 12/65] ARM: futex: Address build warning Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 13/65] media: Fix media_open() to clear filp->private_data in error leg Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 14/65] drivers/media/media-devnode: clear private_data before put_device() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 15/65] media-devnode: add missing mutex lock in error handler Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 16/65] media-devnode: fix namespace mess Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 17/65] media-device: dynamically allocate struct media_devnode Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 18/65] media: fix use-after-free in cdev_put() when app exits after driver unbind Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 19/65] media: fix media devnode ioctl/syscall and unregister race Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 20/65] i2c: dev: switch from register_chrdev to cdev API Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 21/65] i2c: dev: dont start function name with return Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 22/65] i2c: dev: use after free in detach Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 23/65] i2c-dev: dont get i2c adapter via i2c_dev Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 24/65] i2c: dev: Fix the race between the release of i2c_dev and cdev Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 25/65] padata: set cpu_index of unused CPUs to -1 Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 26/65] sched/fair, cpumask: Export for_each_cpu_wrap() Greg Kroah-Hartman
2020-05-27  7:50   ` nobuhiro1.iwamatsu
2020-05-27  8:09     ` Greg KH
2020-05-27 14:03       ` Daniel Jordan
2020-05-26 18:52 ` [PATCH 4.4 27/65] padata: Replace delayed timer with immediate workqueue in padata_reorder Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 28/65] padata: initialize pd->cpu with effective cpumask Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 29/65] padata: purge get_cpu and reorder_via_wq from padata_do_serial Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 30/65] ALSA: pcm: fix incorrect hw_base increase Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 31/65] ext4: lock the xattr block before checksuming it Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 32/65] platform/x86: alienware-wmi: fix kfree on potentially uninitialized pointer Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 33/65] libnvdimm/btt: Remove unnecessary code in btt_freelist_init Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 34/65] l2tp: lock socket before checking flags in connect() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 35/65] l2tp: fix racy socket lookup in l2tp_ip and l2tp_ip6 bind() Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 36/65] l2tp: hold session while sending creation notifications Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 37/65] l2tp: take a reference on sessions used in genetlink handlers Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 38/65] l2tp: dont use l2tp_tunnel_find() in l2tp_ip and l2tp_ip6 Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 39/65] net: l2tp: export debug flags to UAPI Greg Kroah-Hartman
2020-05-26 18:52 ` [PATCH 4.4 40/65] net: l2tp: deprecate PPPOL2TP_MSG_* in favour of L2TP_MSG_* Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 41/65] net: l2tp: ppp: change PPPOL2TP_MSG_* => L2TP_MSG_* Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 42/65] New kernel function to get IP overhead on a socket Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 43/65] L2TP:Adjust intf MTU, add underlay L3, L2 hdrs Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 44/65] l2tp: remove useless duplicate session detection in l2tp_netlink Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 45/65] l2tp: remove l2tp_session_find() Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 46/65] l2tp: define parameters of l2tp_session_get*() as "const" Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 47/65] l2tp: define parameters of l2tp_tunnel_find*() " Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 48/65] 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.4 50/65] l2tp: hold tunnel while processing genl delete command Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 51/65] l2tp: hold tunnel while handling genl tunnel updates Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 52/65] l2tp: hold tunnel while handling genl TUNNEL_GET commands Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 53/65] l2tp: hold tunnel used while creating sessions with netlink Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 54/65] l2tp: prevent creation of sessions on terminated tunnels Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 55/65] l2tp: pass tunnel pointer to ->session_create() Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 56/65] l2tp: fix l2tp_eth module loading Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 57/65] l2tp: dont register sessions in l2tp_session_create() Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 58/65] l2tp: initialise l2tp_eth sessions before registering them Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 59/65] l2tp: protect sock pointer of struct pppol2tp_session with RCU Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 60/65] l2tp: initialise PPP sessions before registering them Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 61/65] Revert "gfs2: Dont demote a glock until its revokes are written" Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 62/65] staging: iio: ad2s1210: Fix SPI reading Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 63/65] mei: release me_cl object reference Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 64/65] iio: sca3000: Remove an erroneous get_device() Greg Kroah-Hartman
2020-05-26 18:53 ` [PATCH 4.4 65/65] l2tp: device MTU setup, tunnel socket needs a lock Greg Kroah-Hartman
2020-05-27  8:32 ` [PATCH 4.4 00/65] 4.4.225-rc1 review Jon Hunter
2020-05-27  8:52 ` Naresh Kamboju
2020-05-27 10:30 ` Chris Paterson
2020-05-27 13:50 ` Guenter Roeck
2020-05-27 17:16 ` 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=20200526183922.958551997@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).