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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250704154806.twigjkbU@linutronix.de>
Date: Fri, 4 Jul 2025 17:48:06 +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-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.

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.
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.

> /P
Sebastian