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: Qingfang Deng <dqfext@gmail.com>
Link: https://patch.msgid.link/20250822012548.6232-1-dqfext@gmail.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 824c8dc4120b33d0d637abc50f48ab0dc2cf1a86..65795d0991661b05bbe30e56de9ed8f3386e30d4 100644 (file)
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -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);