Commit b8844aab authored by Qingfang Deng's avatar Qingfang Deng Committed by Jakub Kicinski
Browse files

ppp: remove rwlock usage 



In struct channel, the upl lock is implemented using rwlock_t,
protecting access to pch->ppp and pch->bridge.

As previously discussed on the list, using rwlock in the network fast
path is not recommended.
This patch replaces the rwlock with a spinlock for writers, and uses RCU
for readers.

- pch->ppp and pch->bridge are now declared as __rcu pointers.
- Readers use rcu_dereference_bh() under rcu_read_lock_bh().
- Writers use spin_lock() to update.

Signed-off-by: default avatarQingfang Deng <dqfext@gmail.com>
Link: https://patch.msgid.link/20250822012548.6232-1-dqfext@gmail.com


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parent 60c481d4

drivers/net/ppp/ppp_generic.c

+63 −57
Original line number Diff line number Diff line
@@ -179,11 +179,11 @@ struct channel { 
	struct ppp_channel *chan;	/* public channel data structure */

	struct rw_semaphore chan_sem;	/* protects `chan' during chan ioctl */

	spinlock_t	downl;		/* protects `chan', file.xq dequeue */

	struct ppp	*ppp;		/* ppp unit we're connected to */

	struct ppp __rcu *ppp;		/* ppp unit we're connected to */

	struct net	*chan_net;	/* the net channel belongs to */

	netns_tracker	ns_tracker;

	struct list_head clist;		/* link in list of channels per unit */

	rwlock_t	upl;		/* protects `ppp' and 'bridge' */

	spinlock_t	upl;		/* protects `ppp' and 'bridge' */

	struct channel __rcu *bridge;	/* "bridged" ppp channel */

#ifdef CONFIG_PPP_MULTILINK

	u8		avail;		/* flag used in multilink stuff */
@@ -645,34 +645,34 @@ static struct bpf_prog *compat_ppp_get_filter(struct sock_fprog32 __user *p) 
 */

static int ppp_bridge_channels(struct channel *pch, struct channel *pchb)

{

	write_lock_bh(&pch->upl);

	if (pch->ppp ||

	spin_lock(&pch->upl);

	if (rcu_dereference_protected(pch->ppp, lockdep_is_held(&pch->upl)) ||

	    rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl))) {

		write_unlock_bh(&pch->upl);

		spin_unlock(&pch->upl);

		return -EALREADY;

	}

	refcount_inc(&pchb->file.refcnt);

	rcu_assign_pointer(pch->bridge, pchb);

	write_unlock_bh(&pch->upl);

	spin_unlock(&pch->upl);



	write_lock_bh(&pchb->upl);

	if (pchb->ppp ||

	spin_lock(&pchb->upl);

	if (rcu_dereference_protected(pchb->ppp, lockdep_is_held(&pchb->upl)) ||

	    rcu_dereference_protected(pchb->bridge, lockdep_is_held(&pchb->upl))) {

		write_unlock_bh(&pchb->upl);

		spin_unlock(&pchb->upl);

		goto err_unset;

	}

	refcount_inc(&pch->file.refcnt);

	rcu_assign_pointer(pchb->bridge, pch);

	write_unlock_bh(&pchb->upl);

	spin_unlock(&pchb->upl);



	return 0;



err_unset:

	write_lock_bh(&pch->upl);

	spin_lock(&pch->upl);

	/* Re-read pch->bridge with upl held in case it was modified concurrently */

	pchb = rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl));

	RCU_INIT_POINTER(pch->bridge, NULL);

	write_unlock_bh(&pch->upl);

	spin_unlock(&pch->upl);

	synchronize_rcu();



	if (pchb)
@@ -686,25 +686,25 @@ static int ppp_unbridge_channels(struct channel *pch) 
{

	struct channel *pchb, *pchbb;



	write_lock_bh(&pch->upl);

	spin_lock(&pch->upl);

	pchb = rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl));

	if (!pchb) {

		write_unlock_bh(&pch->upl);

		spin_unlock(&pch->upl);

		return -EINVAL;

	}

	RCU_INIT_POINTER(pch->bridge, NULL);

	write_unlock_bh(&pch->upl);

	spin_unlock(&pch->upl);



	/* Only modify pchb if phcb->bridge points back to pch.

	 * If not, it implies that there has been a race unbridging (and possibly

	 * even rebridging) pchb.  We should leave pchb alone to avoid either a

	 * refcount underflow, or breaking another established bridge instance.

	 */

	write_lock_bh(&pchb->upl);

	spin_lock(&pchb->upl);

	pchbb = rcu_dereference_protected(pchb->bridge, lockdep_is_held(&pchb->upl));

	if (pchbb == pch)

		RCU_INIT_POINTER(pchb->bridge, NULL);

	write_unlock_bh(&pchb->upl);

	spin_unlock(&pchb->upl);



	synchronize_rcu();
@@ -2158,10 +2158,9 @@ static int ppp_mp_explode(struct ppp *ppp, struct sk_buff *skb) 
#endif /* CONFIG_PPP_MULTILINK */



/* Try to send data out on a channel */

static void __ppp_channel_push(struct channel *pch)

static void __ppp_channel_push(struct channel *pch, struct ppp *ppp)

{

	struct sk_buff *skb;

	struct ppp *ppp;



	spin_lock(&pch->downl);

	if (pch->chan) {
@@ -2180,7 +2179,6 @@ static void __ppp_channel_push(struct channel *pch) 
	spin_unlock(&pch->downl);

	/* see if there is anything from the attached unit to be sent */

	if (skb_queue_empty(&pch->file.xq)) {

		ppp = pch->ppp;

		if (ppp)

			__ppp_xmit_process(ppp, NULL);

	}
@@ -2189,19 +2187,21 @@ static void __ppp_channel_push(struct channel *pch) 
static void ppp_channel_push(struct channel *pch)

{

	struct ppp_xmit_recursion *xmit_recursion;

	struct ppp *ppp;



	read_lock_bh(&pch->upl);

	if (pch->ppp) {

		xmit_recursion = this_cpu_ptr(pch->ppp->xmit_recursion);

		local_lock_nested_bh(&pch->ppp->xmit_recursion->bh_lock);

	rcu_read_lock_bh();

	ppp = rcu_dereference_bh(pch->ppp);

	if (ppp) {

		xmit_recursion = this_cpu_ptr(ppp->xmit_recursion);

		local_lock_nested_bh(&ppp->xmit_recursion->bh_lock);

		xmit_recursion->owner = current;

		__ppp_channel_push(pch);

		__ppp_channel_push(pch, ppp);

		xmit_recursion->owner = NULL;

		local_unlock_nested_bh(&pch->ppp->xmit_recursion->bh_lock);

		local_unlock_nested_bh(&ppp->xmit_recursion->bh_lock);

	} else {

		__ppp_channel_push(pch);

		__ppp_channel_push(pch, NULL);

	}

	read_unlock_bh(&pch->upl);

	rcu_read_unlock_bh();

}



/*
@@ -2303,6 +2303,7 @@ void 
ppp_input(struct ppp_channel *chan, struct sk_buff *skb)

{

	struct channel *pch = chan->ppp;

	struct ppp *ppp;

	int proto;



	if (!pch) {
@@ -2314,18 +2315,19 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb) 
	if (ppp_channel_bridge_input(pch, skb))

		return;



	read_lock_bh(&pch->upl);

	rcu_read_lock_bh();

	ppp = rcu_dereference_bh(pch->ppp);

	if (!ppp_decompress_proto(skb)) {

		kfree_skb(skb);

		if (pch->ppp) {

			++pch->ppp->dev->stats.rx_length_errors;

			ppp_receive_error(pch->ppp);

		if (ppp) {

			++ppp->dev->stats.rx_length_errors;

			ppp_receive_error(ppp);

		}

		goto done;

	}



	proto = PPP_PROTO(skb);

	if (!pch->ppp || proto >= 0xc000 || proto == PPP_CCPFRAG) {

	if (!ppp || proto >= 0xc000 || proto == PPP_CCPFRAG) {

		/* put it on the channel queue */

		skb_queue_tail(&pch->file.rq, skb);

		/* drop old frames if queue too long */
@@ -2334,11 +2336,11 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb) 
			kfree_skb(skb);

		wake_up_interruptible(&pch->file.rwait);

	} else {

		ppp_do_recv(pch->ppp, skb, pch);

		ppp_do_recv(ppp, skb, pch);

	}



done:

	read_unlock_bh(&pch->upl);

	rcu_read_unlock_bh();

}



/* Put a 0-length skb in the receive queue as an error indication */
@@ -2347,20 +2349,22 @@ ppp_input_error(struct ppp_channel *chan, int code) 
{

	struct channel *pch = chan->ppp;

	struct sk_buff *skb;

	struct ppp *ppp;



	if (!pch)

		return;



	read_lock_bh(&pch->upl);

	if (pch->ppp) {

	rcu_read_lock_bh();

	ppp = rcu_dereference_bh(pch->ppp);

	if (ppp) {

		skb = alloc_skb(0, GFP_ATOMIC);

		if (skb) {

			skb->len = 0;		/* probably unnecessary */

			skb->cb[0] = code;

			ppp_do_recv(pch->ppp, skb, pch);

			ppp_do_recv(ppp, skb, pch);

		}

	}

	read_unlock_bh(&pch->upl);

	rcu_read_unlock_bh();

}



/*
@@ -2908,7 +2912,6 @@ int ppp_register_net_channel(struct net *net, struct ppp_channel *chan) 


	pn = ppp_pernet(net);



	pch->ppp = NULL;

	pch->chan = chan;

	pch->chan_net = get_net_track(net, &pch->ns_tracker, GFP_KERNEL);

	chan->ppp = pch;
@@ -2919,7 +2922,7 @@ int ppp_register_net_channel(struct net *net, struct ppp_channel *chan) 
#endif /* CONFIG_PPP_MULTILINK */

	init_rwsem(&pch->chan_sem);

	spin_lock_init(&pch->downl);

	rwlock_init(&pch->upl);

	spin_lock_init(&pch->upl);



	spin_lock_bh(&pn->all_channels_lock);

	pch->file.index = ++pn->last_channel_index;
@@ -2948,13 +2951,15 @@ int ppp_channel_index(struct ppp_channel *chan) 
int ppp_unit_number(struct ppp_channel *chan)

{

	struct channel *pch = chan->ppp;

	struct ppp *ppp;

	int unit = -1;



	if (pch) {

		read_lock_bh(&pch->upl);

		if (pch->ppp)

			unit = pch->ppp->file.index;

		read_unlock_bh(&pch->upl);

		rcu_read_lock();

		ppp = rcu_dereference(pch->ppp);

		if (ppp)

			unit = ppp->file.index;

		rcu_read_unlock();

	}

	return unit;

}
@@ -2966,12 +2971,14 @@ char *ppp_dev_name(struct ppp_channel *chan) 
{

	struct channel *pch = chan->ppp;

	char *name = NULL;

	struct ppp *ppp;



	if (pch) {

		read_lock_bh(&pch->upl);

		if (pch->ppp && pch->ppp->dev)

			name = pch->ppp->dev->name;

		read_unlock_bh(&pch->upl);

		rcu_read_lock();

		ppp = rcu_dereference(pch->ppp);

		if (ppp && ppp->dev)

			name = ppp->dev->name;

		rcu_read_unlock();

	}

	return name;

}
@@ -3494,9 +3501,9 @@ ppp_connect_channel(struct channel *pch, int unit) 
	ppp = ppp_find_unit(pn, unit);

	if (!ppp)

		goto out;

	write_lock_bh(&pch->upl);

	spin_lock(&pch->upl);

	ret = -EINVAL;

	if (pch->ppp ||

	if (rcu_dereference_protected(pch->ppp, lockdep_is_held(&pch->upl)) ||

	    rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl)))

		goto outl;
@@ -3521,13 +3528,13 @@ ppp_connect_channel(struct channel *pch, int unit) 
		ppp->dev->hard_header_len = hdrlen;

	list_add_tail_rcu(&pch->clist, &ppp->channels);

	++ppp->n_channels;

	pch->ppp = ppp;

	rcu_assign_pointer(pch->ppp, ppp);

	refcount_inc(&ppp->file.refcnt);

	ppp_unlock(ppp);

	ret = 0;



 outl:

	write_unlock_bh(&pch->upl);

	spin_unlock(&pch->upl);

 out:

	mutex_unlock(&pn->all_ppp_mutex);

	return ret;
@@ -3542,10 +3549,9 @@ ppp_disconnect_channel(struct channel *pch) 
	struct ppp *ppp;

	int err = -EINVAL;



	write_lock_bh(&pch->upl);

	ppp = pch->ppp;

	pch->ppp = NULL;

	write_unlock_bh(&pch->upl);

	spin_lock(&pch->upl);

	ppp = rcu_replace_pointer(pch->ppp, NULL, lockdep_is_held(&pch->upl));

	spin_unlock(&pch->upl);

	if (ppp) {

		/* remove it from the ppp unit's list */

		ppp_lock(ppp);