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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aHaQ/DZ3U/Sxd7Rs@debian>
Date: Tue, 15 Jul 2025 19:33:48 +0200
From: Guillaume Nault <gnault@...hat.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: netdev@...r.kernel.org, linux-rt-devel@...ts.linux.dev,
	linux-ppp@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Clark Williams <clrkwllms@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH net-next v2 1/1] ppp: Replace per-CPU recursion counter
 with lock-owner field

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
>