[PATCH net-next v2 0/1] ppp: Replace per-CPU recursion counter with lock-owner field

[PATCH net-next v2 0/1] ppp: Replace per-CPU recursion counter with lock-owner field

[Sebastian Andrzej Siewior]

This is another approach to avoid relying on local_bh_disable() for
locking of per-CPU in ppp.

I redid it with the per-CPU lock and local_lock_nested_bh() as discussed
in v1. The xmit_recursion counter has been removed since it served the
same purpose as the owner field. Both were updated and checked.

The xmit_recursion looks like a counter in ppp_channel_push() but at
this point, the counter should always be 0 so it always serves as a
boolean. Therefore I removed it.

I do admit that this looks easier to review. On the other hand v1 had a
negative diffstat :)

v1…v2 https://lore.kernel.org/all/20250627105013.Qtv54bEk@linutronix.de/
  - Instead of rewriting the sequence and adding two owner fields to
    the two variables that may recurse it now adds a per-CPU variable
    for locking and keeping mostly the old code flow.

Sebastian Andrzej Siewior (1):
  ppp: Replace per-CPU recursion counter with lock-owner field

 drivers/net/ppp/ppp_generic.c | 38 ++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

-- 
2.50.0

[PATCH net-next v2 1/1] ppp: Replace per-CPU recursion counter with lock-owner field

[Sebastian Andrzej Siewior]

The per-CPU variable ppp::xmit_recursion is protecting against recursion
due to wrong configuration of the ppp channels. The per-CPU variable
relies on disabled BH for its locking. Without per-CPU locking in
local_bh_disable() on PREEMPT_RT this data structure requires explicit
locking.

The ppp::xmit_recursion is used as a per-CPU boolean. The counter is
checked early in the send routing and the transmit path is only entered
if the counter is zero. Then the counter is incremented to avoid
recursion. It used to detect recursion on channel::downl and
ppp::wlock.

Create a struct ppp_xmit_recursion and move the counter into it.
Add local_lock_t to the struct and use local_lock_nested_bh() for
locking. Due to possible nesting, the lock cannot be acquired
unconditionally but it requires an owner field to identify recursion
before attempting to acquire the lock.

The counter is incremented and checked only after the lock is acquired.
Since it functions as a boolean rather than a count, and its role is now
superseded by the owner field, it can be safely removed.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ppp/ppp_generic.c | 38 ++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index def84e87e05b2..0edc916e0a411 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -119,6 +119,11 @@ struct ppp_link_stats {
 	u64 tx_bytes;
 };
 
+struct ppp_xmit_recursion {
+	struct task_struct *owner;
+	local_lock_t bh_lock;
+};
+
 /*
  * Data structure describing one ppp unit.
  * A ppp unit corresponds to a ppp network interface device
@@ -132,7 +137,7 @@ struct ppp {
 	int		n_channels;	/* how many channels are attached 54 */
 	spinlock_t	rlock;		/* lock for receive side 58 */
 	spinlock_t	wlock;		/* lock for transmit side 5c */
-	int __percpu	*xmit_recursion; /* xmit recursion detect */
+	struct ppp_xmit_recursion __percpu *xmit_recursion; /* xmit recursion detect */
 	int		mru;		/* max receive unit 60 */
 	unsigned int	flags;		/* control bits 64 */
 	unsigned int	xstate;		/* transmit state bits 68 */
@@ -1262,13 +1267,18 @@ static int ppp_dev_configure(struct net *src_net, struct net_device *dev,
 	spin_lock_init(&ppp->rlock);
 	spin_lock_init(&ppp->wlock);
 
-	ppp->xmit_recursion = alloc_percpu(int);
+	ppp->xmit_recursion = alloc_percpu(struct ppp_xmit_recursion);
 	if (!ppp->xmit_recursion) {
 		err = -ENOMEM;
 		goto err1;
 	}
-	for_each_possible_cpu(cpu)
-		(*per_cpu_ptr(ppp->xmit_recursion, cpu)) = 0;
+	for_each_possible_cpu(cpu) {
+		struct ppp_xmit_recursion *xmit_recursion;
+
+		xmit_recursion = per_cpu_ptr(ppp->xmit_recursion, cpu);
+		xmit_recursion->owner = NULL;
+		local_lock_init(&xmit_recursion->bh_lock);
+	}
 
 #ifdef CONFIG_PPP_MULTILINK
 	ppp->minseq = -1;
@@ -1683,15 +1693,20 @@ static void __ppp_xmit_process(struct ppp *ppp, struct sk_buff *skb)
 
 static void ppp_xmit_process(struct ppp *ppp, struct sk_buff *skb)
 {
+	struct ppp_xmit_recursion *xmit_recursion;
+
 	local_bh_disable();
 
-	if (unlikely(*this_cpu_ptr(ppp->xmit_recursion)))
+	xmit_recursion = this_cpu_ptr(ppp->xmit_recursion);
+	if (xmit_recursion->owner == current)
 		goto err;
+	local_lock_nested_bh(&ppp->xmit_recursion->bh_lock);
+	xmit_recursion->owner = current;
 
-	(*this_cpu_ptr(ppp->xmit_recursion))++;
 	__ppp_xmit_process(ppp, skb);
-	(*this_cpu_ptr(ppp->xmit_recursion))--;
 
+	xmit_recursion->owner = NULL;
+	local_unlock_nested_bh(&ppp->xmit_recursion->bh_lock);
 	local_bh_enable();
 
 	return;
@@ -2193,11 +2208,16 @@ static void __ppp_channel_push(struct channel *pch)
 
 static void ppp_channel_push(struct channel *pch)
 {
+	struct ppp_xmit_recursion *xmit_recursion;
+
 	read_lock_bh(&pch->upl);
 	if (pch->ppp) {
-		(*this_cpu_ptr(pch->ppp->xmit_recursion))++;
+		xmit_recursion = this_cpu_ptr(pch->ppp->xmit_recursion);
+		local_lock_nested_bh(&pch->ppp->xmit_recursion->bh_lock);
+		xmit_recursion->owner = current;
 		__ppp_channel_push(pch);
-		(*this_cpu_ptr(pch->ppp->xmit_recursion))--;
+		xmit_recursion->owner = NULL;
+		local_unlock_nested_bh(&pch->ppp->xmit_recursion->bh_lock);
 	} else {
 		__ppp_channel_push(pch);
 	}
-- 
2.50.0

Re: [PATCH net-next v2 1/1] ppp: Replace per-CPU recursion counter with lock-owner field

[Guillaume Nault]

On Thu, Jul 10, 2025 at 06:24:03PM +0200, Sebastian Andrzej Siewior wrote:
> The per-CPU variable ppp::xmit_recursion is protecting against recursion
> due to wrong configuration of the ppp channels. The per-CPU variable

I'd rather say that it's the ppp unit that is badly configured: it's
the ppp unit that can creates the loop (as it creates a networking
interface).

> relies on disabled BH for its locking. Without per-CPU locking in
> local_bh_disable() on PREEMPT_RT this data structure requires explicit
> locking.
> 
> The ppp::xmit_recursion is used as a per-CPU boolean. The counter is
> checked early in the send routing and the transmit path is only entered
> if the counter is zero. Then the counter is incremented to avoid
> recursion. It used to detect recursion on channel::downl and
> ppp::wlock.
> 
> Create a struct ppp_xmit_recursion and move the counter into it.
> Add local_lock_t to the struct and use local_lock_nested_bh() for
> locking. Due to possible nesting, the lock cannot be acquired
> unconditionally but it requires an owner field to identify recursion
> before attempting to acquire the lock.
> 
> The counter is incremented and checked only after the lock is acquired.
> Since it functions as a boolean rather than a count, and its role is now
> superseded by the owner field, it can be safely removed.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/net/ppp/ppp_generic.c | 38 ++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index def84e87e05b2..0edc916e0a411 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -119,6 +119,11 @@ struct ppp_link_stats {
>  	u64 tx_bytes;
>  };
>  
> +struct ppp_xmit_recursion {
> +	struct task_struct *owner;
> +	local_lock_t bh_lock;
> +};
> +

This hunk conflicts with latest changes in net-next.

Apart from the two minor comments above, the patch looks good to me.
Thanks!

Re: [PATCH net-next v2 1/1] ppp: Replace per-CPU recursion counter with lock-owner field

[Sebastian Andrzej Siewior]

On 2025-07-14 18:10:47 [+0200], Guillaume Nault wrote:
> On Thu, Jul 10, 2025 at 06:24:03PM +0200, Sebastian Andrzej Siewior wrote:
> > The per-CPU variable ppp::xmit_recursion is protecting against recursion
> > due to wrong configuration of the ppp channels. The per-CPU variable
> 
> I'd rather say that it's the ppp unit that is badly configured: it's
> the ppp unit that can creates the loop (as it creates a networking
> interface).

I can reword this.

> > index def84e87e05b2..0edc916e0a411 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
> > @@ -119,6 +119,11 @@ struct ppp_link_stats {
> >  	u64 tx_bytes;
> >  };
> >  
> > +struct ppp_xmit_recursion {
> > +	struct task_struct *owner;
> > +	local_lock_t bh_lock;
> > +};
> > +
> 
> This hunk conflicts with latest changes in net-next.

Thank you.

> Apart from the two minor comments above, the patch looks good to me.
> Thanks!

Okay. As of the people involved while this detection was added and
polished, do you have an opinion on v1?
	https://lore.kernel.org/all/20250627105013.Qtv54bEk@linutronix.de/

Sebastian

Re: [PATCH net-next v2 1/1] ppp: Replace per-CPU recursion counter with lock-owner field

[Guillaume Nault]

On Mon, Jul 14, 2025 at 10:01:39PM +0200, Sebastian Andrzej Siewior wrote:
> On 2025-07-14 18:10:47 [+0200], Guillaume Nault wrote:
> > On Thu, Jul 10, 2025 at 06:24:03PM +0200, Sebastian Andrzej Siewior wrote:
> > > The per-CPU variable ppp::xmit_recursion is protecting against recursion
> > > due to wrong configuration of the ppp channels. The per-CPU variable
> > 
> > I'd rather say that it's the ppp unit that is badly configured: it's
> > the ppp unit that can creates the loop (as it creates a networking
> > interface).
> 
> I can reword this.
> 
> > > index def84e87e05b2..0edc916e0a411 100644
> > > --- a/drivers/net/ppp/ppp_generic.c
> > > +++ b/drivers/net/ppp/ppp_generic.c
> > > @@ -119,6 +119,11 @@ struct ppp_link_stats {
> > >  	u64 tx_bytes;
> > >  };
> > >  
> > > +struct ppp_xmit_recursion {
> > > +	struct task_struct *owner;
> > > +	local_lock_t bh_lock;
> > > +};
> > > +
> > 
> > This hunk conflicts with latest changes in net-next.
> 
> Thank you.
> 
> > Apart from the two minor comments above, the patch looks good to me.
> > Thanks!
> 
> Okay. As of the people involved while this detection was added and
> polished, do you have an opinion on v1?
> 	https://lore.kernel.org/all/20250627105013.Qtv54bEk@linutronix.de/

I like the idea of having an owner for each of the locks involved in
the recursion. That looks cleaner than the current approach of selecting
strategic places where to handle the possible recursion.

However, as a reviewer, I agree with Paolo that the diff is difficult
to reason about. Reviewing the v1 patch actually requires reviewing the
complete PPP channel and PPP unit transmit paths, with all their funny
features and lock interactions.

So I'd prefer that we merge your v2 (or v3). Then, if you really want
to push for the v1 approach, maybe consider proposing it as a follow up
during the next development cycle. Note that if you do so, I'd like
that you also write a selftest that could reliably trigger the
recursion when sending a packet through the channel and when sending
one through the unit.

In the end, I'm honestly not sure if the small cleanup benefice of the
lock owners approach is worth it, considering the general difficulty of
maintaining the kernel PPP implementation (brittle code, questionable
architecture, almost no reviewer).

> Sebastian
>