netdev - Re: [PATCH net-next] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250708094959.zByWzNMQ@linutronix.de>
Date: Tue, 8 Jul 2025 11:49:59 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Paolo Abeni <pabeni@...hat.com>
Cc: linux-ppp@...r.kernel.org, netdev@...r.kernel.org,
	linux-rt-devel@...ts.linux.dev,
	"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>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Gao Feng <gfree.wind@....163.com>,
	Guillaume Nault <g.nault@...halink.fr>
Subject: Re: [PATCH net-next] ppp: Replace per-CPU recursion counter with
 lock-owner field

On 2025-07-08 10:28:24 [+0200], Paolo Abeni wrote:
> Hi,
Hi,

> I'm sorry for the latency, OoO here in between.

All good. I appreciate someone looking at it.

> On 7/4/25 5:48 PM, Sebastian Andrzej Siewior wrote:
> > On 2025-07-03 09:55:21 [+0200], Paolo Abeni wrote:
> >> Is there any special reason to not use local_lock here? I find this
> >> patch quite hard to read and follow, as opposed to the local_lock usage
> >> pattern. Also the fact that the code change does not affect RT enabled
> >> build only is IMHO a negative thing.
> > 
> > Adding a local_lock_t to "protect" the counter isn't that simple. I
> > still have to check for the owner of the lock before the lock is
> > acquired to avoid recursion on that local_lock_t. I need to acquire the
> > lock before checking the counter because another task might have
> > incremented the counter (so acquiring the lock would not deadlock). This
> > is similar to the recursion detection in openvswitch. That means I would
> > need to add the local_lock_t and an owner field next to the recursion
> > counter.
> 
> IMHO using a similar approach to something already implemented is a
> plus, and the OVS code did not look that scaring. Also it had the IMHO
> significant advantage of keeping the changes constrained to the RT build.

I intended to improve the code and making it more understandable of what
happens here and why. Additionally it would also fit with RT and not
just make this change to fit with RT.

> > I've been looking at the counter and how it is used and it did not look
> > right. The recursion, it should detect, was described in commit
> > 55454a565836e ("ppp: avoid dealock on recursive xmit"). There are two
> > locks that can be acquired due to recursion and that one counter is
> > supposed to catch both cases based on current code flow.
> > 
> > It is also not obvious why ppp_channel_push() makes the difference
> > depending on pch->ppp while ->start_xmit callback is invoked based on
> > pch->chan.
> > It looked more natural to avoid the per-CPU usage and detect the
> > recursion based on the lock that might be acquired recursively. I hope
> > this makes it easier to understand what is going on here.
> 
> Actually I'm a bit lost. According to 55454a565836e a single recursion
> check in ppp_xmit_process() should be enough, and I think that keeping
> the complexity constraint there be better.

Okay. I didn't think that this complicated the code flow.

> > While looking through the code I wasn't sure if
> > ppp_channel_bridge_input() requires the same kind of check for recursion
> > but adding it based on the lock, that is about to be acquired, would be
> > easier.
> 
> (still lost in PPP, but) The xmit -> input path transition should have
> already break the recursion (via the backlog). Recursion check in tx
> should be sufficient.
> 
> All in all I think it would be safer the local lock based approach.

Okay. I disagree but let me do as you suggested.

Thank you.

> Thanks,
> 
> Paolo

Sebastian