diff mbox series

[net-next,07/16] l2tp: hide sessions if they are closing

Message ID 1518188432-9245-8-git-send-email-jchapman@katalix.com
State Superseded, archived
Delegated to: David Miller
Headers show
Series l2tp: fix API races discovered by syzbot | expand

Commit Message

James Chapman Feb. 9, 2018, 3 p.m. UTC 
Replace the dead flag in the session context with a closing flag and
spinlock. Check it in session lookup functions such that we don't try
to access session data while it is being destroyed.

Signed-off-by: James Chapman <jchapman@katalix.com>
---
 net/l2tp/l2tp_core.c | 34 +++++++++++++++++++++++++++++++++-
 net/l2tp/l2tp_core.h |  3 ++-
 2 files changed, 35 insertions(+), 2 deletions(-)

Comments

kernel test robot Feb. 12, 2018, 3:37 a.m. UTC | #1 
Hi James,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/James-Chapman/l2tp-fix-API-races-discovered-by-syzbot/20180212-102613
config: i386-randconfig-x079-201806 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

Note: the linux-review/James-Chapman/l2tp-fix-API-races-discovered-by-syzbot/20180212-102613 HEAD 0b6885922178c50183cc441ee2b0e2346292de81 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   net/l2tp/l2tp_core.c: In function 'l2tp_tunnel_closeall':
>> net/l2tp/l2tp_core.c:1266:36: error: 'struct l2tp_session' has no member named 'dead'
       if (test_and_set_bit(0, &session->dead))
                                       ^~

vim +1266 net/l2tp/l2tp_core.c

fd558d18 James Chapman   2010-04-02  1240  
fd558d18 James Chapman   2010-04-02  1241  /* When the tunnel is closed, all the attached sessions need to go too.
fd558d18 James Chapman   2010-04-02  1242   */
e34f4c70 Tom Parkin      2013-03-19  1243  void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
fd558d18 James Chapman   2010-04-02  1244  {
fd558d18 James Chapman   2010-04-02  1245  	int hash;
fd558d18 James Chapman   2010-04-02  1246  	struct hlist_node *walk;
fd558d18 James Chapman   2010-04-02  1247  	struct hlist_node *tmp;
fd558d18 James Chapman   2010-04-02  1248  	struct l2tp_session *session;
fd558d18 James Chapman   2010-04-02  1249  
fd558d18 James Chapman   2010-04-02  1250  	BUG_ON(tunnel == NULL);
fd558d18 James Chapman   2010-04-02  1251  
a4ca44fa Joe Perches     2012-05-16  1252  	l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: closing all sessions...\n",
a4ca44fa Joe Perches     2012-05-16  1253  		  tunnel->name);
fd558d18 James Chapman   2010-04-02  1254  
fd558d18 James Chapman   2010-04-02  1255  	write_lock_bh(&tunnel->hlist_lock);
fd558d18 James Chapman   2010-04-02  1256  	for (hash = 0; hash < L2TP_HASH_SIZE; hash++) {
fd558d18 James Chapman   2010-04-02  1257  again:
fd558d18 James Chapman   2010-04-02  1258  		hlist_for_each_safe(walk, tmp, &tunnel->session_hlist[hash]) {
fd558d18 James Chapman   2010-04-02  1259  			session = hlist_entry(walk, struct l2tp_session, hlist);
fd558d18 James Chapman   2010-04-02  1260  
a4ca44fa Joe Perches     2012-05-16  1261  			l2tp_info(session, L2TP_MSG_CONTROL,
fd558d18 James Chapman   2010-04-02  1262  				  "%s: closing session\n", session->name);
fd558d18 James Chapman   2010-04-02  1263  
fd558d18 James Chapman   2010-04-02  1264  			hlist_del_init(&session->hlist);
fd558d18 James Chapman   2010-04-02  1265  
b228a940 Guillaume Nault 2017-09-22 @1266  			if (test_and_set_bit(0, &session->dead))
b228a940 Guillaume Nault 2017-09-22  1267  				goto again;
b228a940 Guillaume Nault 2017-09-22  1268  
fd558d18 James Chapman   2010-04-02  1269  			write_unlock_bh(&tunnel->hlist_lock);
fd558d18 James Chapman   2010-04-02  1270  
f6e16b29 Tom Parkin      2013-03-19  1271  			__l2tp_session_unhash(session);
4c6e2fd3 Tom Parkin      2013-03-19  1272  			l2tp_session_queue_purge(session);
4c6e2fd3 Tom Parkin      2013-03-19  1273  
fd558d18 James Chapman   2010-04-02  1274  			if (session->session_close != NULL)
fd558d18 James Chapman   2010-04-02  1275  				(*session->session_close)(session);
fd558d18 James Chapman   2010-04-02  1276  
9980d001 Tom Parkin      2013-03-19  1277  			l2tp_session_dec_refcount(session);
9980d001 Tom Parkin      2013-03-19  1278  
fd558d18 James Chapman   2010-04-02  1279  			write_lock_bh(&tunnel->hlist_lock);
fd558d18 James Chapman   2010-04-02  1280  
fd558d18 James Chapman   2010-04-02  1281  			/* Now restart from the beginning of this hash
fd558d18 James Chapman   2010-04-02  1282  			 * chain.  We always remove a session from the
fd558d18 James Chapman   2010-04-02  1283  			 * list so we are guaranteed to make forward
fd558d18 James Chapman   2010-04-02  1284  			 * progress.
fd558d18 James Chapman   2010-04-02  1285  			 */
fd558d18 James Chapman   2010-04-02  1286  			goto again;
fd558d18 James Chapman   2010-04-02  1287  		}
fd558d18 James Chapman   2010-04-02  1288  	}
fd558d18 James Chapman   2010-04-02  1289  	write_unlock_bh(&tunnel->hlist_lock);
fd558d18 James Chapman   2010-04-02  1290  }
e34f4c70 Tom Parkin      2013-03-19  1291  EXPORT_SYMBOL_GPL(l2tp_tunnel_closeall);
fd558d18 James Chapman   2010-04-02  1292  

:::::: The code at line 1266 was first introduced by commit
:::::: b228a94066406b6c456321d69643b0d7ce11cfa6 l2tp: fix race between l2tp_session_delete() and l2tp_tunnel_closeall()

:::::: TO: Guillaume Nault <g.nault@alphalink.fr>
:::::: CC: David S. Miller <davem@davemloft.net>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox series

Patch

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 477b96cf8ab3..869dec89ff0f 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -198,7 +198,14 @@  struct l2tp_session *l2tp_session_get(const struct net *net,
 		rcu_read_lock_bh();
 		hlist_for_each_entry_rcu(session, session_list, global_hlist) {
 			if (session->session_id == session_id) {
+				spin_lock_bh(&session->lock);
+				if (session->closing) {
+					spin_unlock_bh(&session->lock);
+					rcu_read_unlock_bh();
+					return NULL;
+				}
 				l2tp_session_inc_refcount(session);
+				spin_unlock_bh(&session->lock);
 				rcu_read_unlock_bh();
 
 				return session;
@@ -213,7 +220,14 @@  struct l2tp_session *l2tp_session_get(const struct net *net,
 	read_lock_bh(&tunnel->hlist_lock);
 	hlist_for_each_entry(session, session_list, hlist) {
 		if (session->session_id == session_id) {
+			spin_lock_bh(&session->lock);
+			if (session->closing) {
+				spin_unlock_bh(&session->lock);
+				read_unlock_bh(&tunnel->hlist_lock);
+				return NULL;
+			}
 			l2tp_session_inc_refcount(session);
+			spin_unlock_bh(&session->lock);
 			read_unlock_bh(&tunnel->hlist_lock);
 
 			return session;
@@ -234,6 +248,12 @@  struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth)
 	read_lock_bh(&tunnel->hlist_lock);
 	for (hash = 0; hash < L2TP_HASH_SIZE; hash++) {
 		hlist_for_each_entry(session, &tunnel->session_hlist[hash], hlist) {
+			spin_lock_bh(&session->lock);
+			if (session->closing) {
+				spin_unlock_bh(&session->lock);
+				continue;
+			}
+			spin_unlock_bh(&session->lock);
 			if (++count > nth) {
 				l2tp_session_inc_refcount(session);
 				read_unlock_bh(&tunnel->hlist_lock);
@@ -261,6 +281,12 @@  struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
 	rcu_read_lock_bh();
 	for (hash = 0; hash < L2TP_HASH_SIZE_2; hash++) {
 		hlist_for_each_entry_rcu(session, &pn->l2tp_session_hlist[hash], global_hlist) {
+			spin_lock_bh(&session->lock);
+			if (session->closing) {
+				spin_unlock_bh(&session->lock);
+				continue;
+			}
+			spin_unlock_bh(&session->lock);
 			if (!strcmp(session->ifname, ifname)) {
 				l2tp_session_inc_refcount(session);
 				rcu_read_unlock_bh();
@@ -1678,8 +1704,13 @@  void __l2tp_session_unhash(struct l2tp_session *session)
  */
 int l2tp_session_delete(struct l2tp_session *session)
 {
-	if (test_and_set_bit(0, &session->dead))
+	spin_lock_bh(&session->lock);
+	if (session->closing) {
+		spin_unlock_bh(&session->lock);
 		return 0;
+	}
+	session->closing = true;
+	spin_unlock_bh(&session->lock);
 
 	__l2tp_session_unhash(session);
 	l2tp_session_queue_purge(session);
@@ -1747,6 +1778,7 @@  struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 
 		INIT_HLIST_NODE(&session->hlist);
 		INIT_HLIST_NODE(&session->global_hlist);
+		spin_lock_init(&session->lock);
 
 		/* Inherit debug options from tunnel */
 		session->debug = tunnel->debug;
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 4e098c822cd1..98709086fe84 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -74,7 +74,8 @@  struct l2tp_session_cfg {
 struct l2tp_session {
 	int			magic;		/* should be
 						 * L2TP_SESSION_MAGIC */
-	long			dead;
+	bool                    closing;
+	spinlock_t              lock;		/* protect closing */
 
 	struct l2tp_tunnel	*tunnel;	/* back pointer to tunnel
 						 * context */