Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit cf3752e2 authored by James Chapman's avatar James Chapman Committed by David S. Miller
Browse files

[PPPOL2TP]: Make locking calls softirq-safe 



Fix locking issues in the pppol2tp driver which can cause a kernel
crash on SMP boxes. There were two problems:-

1. The driver was violating read_lock() and write_lock() scheduling
   rules because it wasn't using softirq-safe locks in softirq
   contexts. So we now consistently use the _bh variants of the lock
   functions.

2. The driver was calling sk_dst_get() in pppol2tp_xmit() which was
   taking sk_dst_lock in softirq context. We now call __sk_dst_get().

Signed-off-by: default avatarJames Chapman <jchapman@katalix.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 5a346a10

drivers/net/pppol2tp.c

+29 −29
Original line number Original line Diff line number Diff line
@@ -302,14 +302,14 @@ pppol2tp_session_find(struct pppol2tp_tunnel *tunnel, u16 session_id) 
	struct pppol2tp_session *session;

	struct pppol2tp_session *session;

	struct hlist_node *walk;

	struct hlist_node *walk;





	read_lock(&tunnel->hlist_lock);

	read_lock_bh(&tunnel->hlist_lock);

	hlist_for_each_entry(session, walk, session_list, hlist) {

	hlist_for_each_entry(session, walk, session_list, hlist) {

		if (session->tunnel_addr.s_session == session_id) {

		if (session->tunnel_addr.s_session == session_id) {

			read_unlock(&tunnel->hlist_lock);

			read_unlock_bh(&tunnel->hlist_lock);

			return session;

			return session;

		}

		}

	}

	}

	read_unlock(&tunnel->hlist_lock);

	read_unlock_bh(&tunnel->hlist_lock);





	return NULL;

	return NULL;

}

}
@@ -320,14 +320,14 @@ static struct pppol2tp_tunnel *pppol2tp_tunnel_find(u16 tunnel_id) 
{

{

	struct pppol2tp_tunnel *tunnel = NULL;

	struct pppol2tp_tunnel *tunnel = NULL;





	read_lock(&pppol2tp_tunnel_list_lock);

	read_lock_bh(&pppol2tp_tunnel_list_lock);

	list_for_each_entry(tunnel, &pppol2tp_tunnel_list, list) {

	list_for_each_entry(tunnel, &pppol2tp_tunnel_list, list) {

		if (tunnel->stats.tunnel_id == tunnel_id) {

		if (tunnel->stats.tunnel_id == tunnel_id) {

			read_unlock(&pppol2tp_tunnel_list_lock);

			read_unlock_bh(&pppol2tp_tunnel_list_lock);

			return tunnel;

			return tunnel;

		}

		}

	}

	}

	read_unlock(&pppol2tp_tunnel_list_lock);

	read_unlock_bh(&pppol2tp_tunnel_list_lock);





	return NULL;

	return NULL;

}

}
@@ -344,7 +344,7 @@ static void pppol2tp_recv_queue_skb(struct pppol2tp_session *session, struct sk_ 
	struct sk_buff *skbp;

	struct sk_buff *skbp;

	u16 ns = PPPOL2TP_SKB_CB(skb)->ns;

	u16 ns = PPPOL2TP_SKB_CB(skb)->ns;





	spin_lock(&session->reorder_q.lock);

	spin_lock_bh(&session->reorder_q.lock);

	skb_queue_walk(&session->reorder_q, skbp) {

	skb_queue_walk(&session->reorder_q, skbp) {

		if (PPPOL2TP_SKB_CB(skbp)->ns > ns) {

		if (PPPOL2TP_SKB_CB(skbp)->ns > ns) {

			__skb_insert(skb, skbp->prev, skbp, &session->reorder_q);

			__skb_insert(skb, skbp->prev, skbp, &session->reorder_q);
@@ -360,7 +360,7 @@ static void pppol2tp_recv_queue_skb(struct pppol2tp_session *session, struct sk_ 
	__skb_queue_tail(&session->reorder_q, skb);

	__skb_queue_tail(&session->reorder_q, skb);





out:

out:

	spin_unlock(&session->reorder_q.lock);

	spin_unlock_bh(&session->reorder_q.lock);

}

}





/* Dequeue a single skb.

/* Dequeue a single skb.
@@ -442,7 +442,7 @@ static void pppol2tp_recv_dequeue(struct pppol2tp_session *session) 
	 * expect to send up next, dequeue it and any other

	 * expect to send up next, dequeue it and any other

	 * in-sequence packets behind it.

	 * in-sequence packets behind it.

	 */

	 */

	spin_lock(&session->reorder_q.lock);

	spin_lock_bh(&session->reorder_q.lock);

	skb_queue_walk_safe(&session->reorder_q, skb, tmp) {

	skb_queue_walk_safe(&session->reorder_q, skb, tmp) {

		if (time_after(jiffies, PPPOL2TP_SKB_CB(skb)->expires)) {

		if (time_after(jiffies, PPPOL2TP_SKB_CB(skb)->expires)) {

			session->stats.rx_seq_discards++;

			session->stats.rx_seq_discards++;
@@ -470,13 +470,13 @@ static void pppol2tp_recv_dequeue(struct pppol2tp_session *session) 
				goto out;

				goto out;

			}

			}

		}

		}

		spin_unlock(&session->reorder_q.lock);

		spin_unlock_bh(&session->reorder_q.lock);

		pppol2tp_recv_dequeue_skb(session, skb);

		pppol2tp_recv_dequeue_skb(session, skb);

		spin_lock(&session->reorder_q.lock);

		spin_lock_bh(&session->reorder_q.lock);

	}

	}





out:

out:

	spin_unlock(&session->reorder_q.lock);

	spin_unlock_bh(&session->reorder_q.lock);

}

}





/* Internal receive frame. Do the real work of receiving an L2TP data frame

/* Internal receive frame. Do the real work of receiving an L2TP data frame
@@ -1059,7 +1059,7 @@ static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb) 




	/* Get routing info from the tunnel socket */

	/* Get routing info from the tunnel socket */

	dst_release(skb->dst);

	dst_release(skb->dst);

	skb->dst = sk_dst_get(sk_tun);

	skb->dst = dst_clone(__sk_dst_get(sk_tun));

	skb_orphan(skb);

	skb_orphan(skb);

	skb->sk = sk_tun;

	skb->sk = sk_tun;
@@ -1107,7 +1107,7 @@ static void pppol2tp_tunnel_closeall(struct pppol2tp_tunnel *tunnel) 
	PRINTK(tunnel->debug, PPPOL2TP_MSG_CONTROL, KERN_INFO,

	PRINTK(tunnel->debug, PPPOL2TP_MSG_CONTROL, KERN_INFO,

	       "%s: closing all sessions...\n", tunnel->name);

	       "%s: closing all sessions...\n", tunnel->name);





	write_lock(&tunnel->hlist_lock);

	write_lock_bh(&tunnel->hlist_lock);

	for (hash = 0; hash < PPPOL2TP_HASH_SIZE; hash++) {

	for (hash = 0; hash < PPPOL2TP_HASH_SIZE; hash++) {

again:

again:

		hlist_for_each_safe(walk, tmp, &tunnel->session_hlist[hash]) {

		hlist_for_each_safe(walk, tmp, &tunnel->session_hlist[hash]) {
@@ -1129,7 +1129,7 @@ static void pppol2tp_tunnel_closeall(struct pppol2tp_tunnel *tunnel) 
			 * disappear as we're jumping between locks.

			 * disappear as we're jumping between locks.

			 */

			 */

			sock_hold(sk);

			sock_hold(sk);

			write_unlock(&tunnel->hlist_lock);

			write_unlock_bh(&tunnel->hlist_lock);

			lock_sock(sk);

			lock_sock(sk);





			if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND)) {

			if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND)) {
@@ -1154,11 +1154,11 @@ static void pppol2tp_tunnel_closeall(struct pppol2tp_tunnel *tunnel) 
			 * list so we are guaranteed to make forward

			 * list so we are guaranteed to make forward

			 * progress.

			 * progress.

			 */

			 */

			write_lock(&tunnel->hlist_lock);

			write_lock_bh(&tunnel->hlist_lock);

			goto again;

			goto again;

		}

		}

	}

	}

	write_unlock(&tunnel->hlist_lock);

	write_unlock_bh(&tunnel->hlist_lock);

}

}





/* Really kill the tunnel.

/* Really kill the tunnel.
@@ -1167,9 +1167,9 @@ static void pppol2tp_tunnel_closeall(struct pppol2tp_tunnel *tunnel) 
static void pppol2tp_tunnel_free(struct pppol2tp_tunnel *tunnel)

static void pppol2tp_tunnel_free(struct pppol2tp_tunnel *tunnel)

{

{

	/* Remove from socket list */

	/* Remove from socket list */

	write_lock(&pppol2tp_tunnel_list_lock);

	write_lock_bh(&pppol2tp_tunnel_list_lock);

	list_del_init(&tunnel->list);

	list_del_init(&tunnel->list);

	write_unlock(&pppol2tp_tunnel_list_lock);

	write_unlock_bh(&pppol2tp_tunnel_list_lock);





	atomic_dec(&pppol2tp_tunnel_count);

	atomic_dec(&pppol2tp_tunnel_count);

	kfree(tunnel);

	kfree(tunnel);
@@ -1245,9 +1245,9 @@ static void pppol2tp_session_destruct(struct sock *sk) 
				/* Delete the session socket from the

				/* Delete the session socket from the

				 * hash

				 * hash

				 */

				 */

				write_lock(&tunnel->hlist_lock);

				write_lock_bh(&tunnel->hlist_lock);

				hlist_del_init(&session->hlist);

				hlist_del_init(&session->hlist);

				write_unlock(&tunnel->hlist_lock);

				write_unlock_bh(&tunnel->hlist_lock);





				atomic_dec(&pppol2tp_session_count);

				atomic_dec(&pppol2tp_session_count);

			}

			}
@@ -1392,9 +1392,9 @@ static struct sock *pppol2tp_prepare_tunnel_socket(int fd, u16 tunnel_id, 




	/* Add tunnel to our list */

	/* Add tunnel to our list */

	INIT_LIST_HEAD(&tunnel->list);

	INIT_LIST_HEAD(&tunnel->list);

	write_lock(&pppol2tp_tunnel_list_lock);

	write_lock_bh(&pppol2tp_tunnel_list_lock);

	list_add(&tunnel->list, &pppol2tp_tunnel_list);

	list_add(&tunnel->list, &pppol2tp_tunnel_list);

	write_unlock(&pppol2tp_tunnel_list_lock);

	write_unlock_bh(&pppol2tp_tunnel_list_lock);

	atomic_inc(&pppol2tp_tunnel_count);

	atomic_inc(&pppol2tp_tunnel_count);





	/* Bump the reference count. The tunnel context is deleted

	/* Bump the reference count. The tunnel context is deleted
@@ -1599,11 +1599,11 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr, 
	sk->sk_user_data = session;

	sk->sk_user_data = session;





	/* Add session to the tunnel's hash list */

	/* Add session to the tunnel's hash list */

	write_lock(&tunnel->hlist_lock);

	write_lock_bh(&tunnel->hlist_lock);

	hlist_add_head(&session->hlist,

	hlist_add_head(&session->hlist,

		       pppol2tp_session_id_hash(tunnel,

		       pppol2tp_session_id_hash(tunnel,

						session->tunnel_addr.s_session));

						session->tunnel_addr.s_session));

	write_unlock(&tunnel->hlist_lock);

	write_unlock_bh(&tunnel->hlist_lock);





	atomic_inc(&pppol2tp_session_count);

	atomic_inc(&pppol2tp_session_count);
@@ -2205,7 +2205,7 @@ static struct pppol2tp_session *next_session(struct pppol2tp_tunnel *tunnel, str 
	int next = 0;

	int next = 0;

	int i;

	int i;





	read_lock(&tunnel->hlist_lock);

	read_lock_bh(&tunnel->hlist_lock);

	for (i = 0; i < PPPOL2TP_HASH_SIZE; i++) {

	for (i = 0; i < PPPOL2TP_HASH_SIZE; i++) {

		hlist_for_each_entry(session, walk, &tunnel->session_hlist[i], hlist) {

		hlist_for_each_entry(session, walk, &tunnel->session_hlist[i], hlist) {

			if (curr == NULL) {

			if (curr == NULL) {
@@ -2223,7 +2223,7 @@ static struct pppol2tp_session *next_session(struct pppol2tp_tunnel *tunnel, str 
		}

		}

	}

	}

out:

out:

	read_unlock(&tunnel->hlist_lock);

	read_unlock_bh(&tunnel->hlist_lock);

	if (!found)

	if (!found)

		session = NULL;

		session = NULL;
@@ -2234,13 +2234,13 @@ static struct pppol2tp_tunnel *next_tunnel(struct pppol2tp_tunnel *curr) 
{

{

	struct pppol2tp_tunnel *tunnel = NULL;

	struct pppol2tp_tunnel *tunnel = NULL;





	read_lock(&pppol2tp_tunnel_list_lock);

	read_lock_bh(&pppol2tp_tunnel_list_lock);

	if (list_is_last(&curr->list, &pppol2tp_tunnel_list)) {

	if (list_is_last(&curr->list, &pppol2tp_tunnel_list)) {

		goto out;

		goto out;

	}

	}

	tunnel = list_entry(curr->list.next, struct pppol2tp_tunnel, list);

	tunnel = list_entry(curr->list.next, struct pppol2tp_tunnel, list);

out:

out:

	read_unlock(&pppol2tp_tunnel_list_lock);

	read_unlock_bh(&pppol2tp_tunnel_list_lock);





	return tunnel;

	return tunnel;

}

}