[PATCH v2] ptp: remove ptp->n_vclocks check logic in ptp_vclock_in_use()

[PATCH v2] ptp: remove ptp->n_vclocks check logic in ptp_vclock_in_use()

[Jeongjun Park]

There is no disagreement that we should check both ptp->is_virtual_clock
and ptp->n_vclocks to check if the ptp virtual clock is in use.

However, when we acquire ptp->n_vclocks_mux to read ptp->n_vclocks in
ptp_vclock_in_use(), we observe a recursive lock in the call trace
starting from n_vclocks_store().

============================================
WARNING: possible recursive locking detected
6.15.0-rc6 #1 Not tainted
--------------------------------------------
syz.0.1540/13807 is trying to acquire lock:
ffff888035a24868 (&ptp->n_vclocks_mux){+.+.}-{4:4}, at:
 ptp_vclock_in_use drivers/ptp/ptp_private.h:103 [inline]
ffff888035a24868 (&ptp->n_vclocks_mux){+.+.}-{4:4}, at:
 ptp_clock_unregister+0x21/0x250 drivers/ptp/ptp_clock.c:415

but task is already holding lock:
ffff888030704868 (&ptp->n_vclocks_mux){+.+.}-{4:4}, at:
 n_vclocks_store+0xf1/0x6d0 drivers/ptp/ptp_sysfs.c:215

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&ptp->n_vclocks_mux);
  lock(&ptp->n_vclocks_mux);

 *** DEADLOCK ***
....
============================================

The best way to solve this is to remove the logic that checks
ptp->n_vclocks in ptp_vclock_in_use().

The reason why this is appropriate is that any path that uses
ptp->n_vclocks must unconditionally check if ptp->n_vclocks is greater
than 0 before unregistering vclocks, and all functions are already
written this way. And in the function that uses ptp->n_vclocks, we
already get ptp->n_vclocks_mux before unregistering vclocks.

Therefore, we need to remove the redundant check for ptp->n_vclocks in
ptp_vclock_in_use() to prevent recursive locking.

Fixes: 73f37068d540 ("ptp: support ptp physical/virtual clocks conversion")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
v2: Remove changes unrelated to the patch subject
- Link to v1: https://lore.kernel.org/all/20250519153735.66940-1-aha310510@gmail.com/
---
 drivers/ptp/ptp_private.h | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 18934e28469e..528d86a33f37 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -98,17 +98,7 @@ static inline int queue_cnt(const struct timestamp_event_queue *q)
 /* Check if ptp virtual clock is in use */
 static inline bool ptp_vclock_in_use(struct ptp_clock *ptp)
 {
-	bool in_use = false;
-
-	if (mutex_lock_interruptible(&ptp->n_vclocks_mux))
-		return true;
-
-	if (!ptp->is_virtual_clock && ptp->n_vclocks)
-		in_use = true;
-
-	mutex_unlock(&ptp->n_vclocks_mux);
-
-	return in_use;
+	return !ptp->is_virtual_clock;
 }
 
 /* Check if ptp clock shall be free running */
--

Re: [PATCH v2] ptp: remove ptp->n_vclocks check logic in ptp_vclock_in_use()

[Richard Cochran]

On Wed, May 21, 2025 at 01:07:17AM +0900, Jeongjun Park wrote:
> There is no disagreement that we should check both ptp->is_virtual_clock
> and ptp->n_vclocks to check if the ptp virtual clock is in use.
> 
> However, when we acquire ptp->n_vclocks_mux to read ptp->n_vclocks in
> ptp_vclock_in_use(), we observe a recursive lock in the call trace
> starting from n_vclocks_store().

> The best way to solve this is to remove the logic that checks
> ptp->n_vclocks in ptp_vclock_in_use().

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH v2] ptp: remove ptp->n_vclocks check logic in ptp_vclock_in_use()

[Jakub Kicinski]

On Wed, 21 May 2025 01:07:17 +0900 Jeongjun Park wrote:
> The reason why this is appropriate is that any path that uses
> ptp->n_vclocks must unconditionally check if ptp->n_vclocks is greater
> than 0 before unregistering vclocks, and all functions are already
> written this way. And in the function that uses ptp->n_vclocks, we
> already get ptp->n_vclocks_mux before unregistering vclocks.

What about ptp_clock_freerun()? We seem to call it for clock ops
like settime and it does not check n_vclocks.

> Therefore, we need to remove the redundant check for ptp->n_vclocks in
> ptp_vclock_in_use() to prevent recursive locking.

IIUC lockdep is complaining that we are trying to lock the vclock's
n_vclocks_mux, while we already hold that lock for the real clock's
instance. It doesn't understand that the two are in a fixed hierarchy
so the deadlock is not possible.

If my understanding is correct could you please clearly state in the
commit message that this is a false positive? And if so isn't a better
fix to _move_ the !ptp->is_virtual_clock check before the lock in
ptp_vclock_in_use()? that way we preserve current behavior for real
clocks, but vclocks can return early and avoid confusing lockdep?
-- 
pw-bot: cr

Re: [PATCH v2] ptp: remove ptp->n_vclocks check logic in ptp_vclock_in_use()

[Jeongjun Park]

Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 21 May 2025 01:07:17 +0900 Jeongjun Park wrote:
> > The reason why this is appropriate is that any path that uses
> > ptp->n_vclocks must unconditionally check if ptp->n_vclocks is greater
> > than 0 before unregistering vclocks, and all functions are already
> > written this way. And in the function that uses ptp->n_vclocks, we
> > already get ptp->n_vclocks_mux before unregistering vclocks.
>
> What about ptp_clock_freerun()? We seem to call it for clock ops
> like settime and it does not check n_vclocks.

ptp_clock_freerun() calls ptp_vclock_in_use() to check n_vclocks.

>
> > Therefore, we need to remove the redundant check for ptp->n_vclocks in
> > ptp_vclock_in_use() to prevent recursive locking.
>
> IIUC lockdep is complaining that we are trying to lock the vclock's
> n_vclocks_mux, while we already hold that lock for the real clock's
> instance. It doesn't understand that the two are in a fixed hierarchy
> so the deadlock is not possible.
>
> If my understanding is correct could you please clearly state in the
> commit message that this is a false positive? And if so isn't a better
> fix to _move_ the !ptp->is_virtual_clock check before the lock in
> ptp_vclock_in_use()? that way we preserve current behavior for real
> clocks, but vclocks can return early and avoid confusing lockdep?
> --
> pw-bot: cr

Your right! This deadlock report seems to be a false positive. It seems
appropriate to add a description of this false positive to the commit
message.

However, it is not appropriate to move the code that checks
ptp->is_virtual_clock. If you need to check n_vclocks when checking
whether ptp virtual clock is in use, it means that caller function has
already performed work related to n_vclocks, and in this case, it is
appropriate to perform n_vclocks check and n_vclocks_mux lock in caller
function.

Therefore, considering the overall structure, it is more appropriate to
remove unnecessary locks and n_vclocks checks in ptp_vclock_in_use().

Re: [PATCH v2] ptp: remove ptp->n_vclocks check logic in ptp_vclock_in_use()

[Jakub Kicinski]

On Mon, 26 May 2025 20:00:53 +0900 Jeongjun Park wrote:
> If you need to check n_vclocks when checking
> whether ptp virtual clock is in use, it means that caller function has
> already performed work related to n_vclocks, and in this case, it is
> appropriate to perform n_vclocks check and n_vclocks_mux lock in caller
> function.

Can you be a little less abstract in this explanation, given that
ptp_vclock_in_use() only has a handful of callers?
For ptp_clock_freerun() do you mean the ->has_cycles check?

Re: [PATCH v2] ptp: remove ptp->n_vclocks check logic in ptp_vclock_in_use()

[Jeongjun Park]

Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 26 May 2025 20:00:53 +0900 Jeongjun Park wrote:
> > If you need to check n_vclocks when checking
> > whether ptp virtual clock is in use, it means that caller function has
> > already performed work related to n_vclocks, and in this case, it is
> > appropriate to perform n_vclocks check and n_vclocks_mux lock in caller
> > function.
>
> Can you be a little less abstract in this explanation, given that
> ptp_vclock_in_use() only has a handful of callers?
> For ptp_clock_freerun() do you mean the ->has_cycles check?

There are two main cases where we need to check if ptp vclock is in use:

1. Check if ptp clock shall be free running (ptp_clock_freerun)
2. Check if ptp clock can be unregistered (ptp_clock_unregister)

In the first case, If I'm not misreading the code, ptp_clock_freerun()
determines whether it can run based on the result of ptp_vclock_in_use()
if ptp->has_cycles is false.

However, ptp_clock_{set,adj}time() , which calls ptp_clock_freerun(),
does not use n_vclocks . Therefore, it would be more appropriate to
remove unnecessary lock and n_vclocks checks when calling these
functions, both to prevent false positives in lockdep and to reduce
performance overhead.

And in the second case as well, there is no need to check n_vclocks in
every caller function. Because n_vclocks itself is a concept added for
physical/virtual clocks conversion in ptp physical clock sysfs, functions
other than sysfs actually do not need to check n_vclocks. Moreover,
since ptp_clock_unregister() is called by many functions, locking
n_vclocks_mux and checking n_vclocks when called by a function that
does not use n_vclocks causes performance overhead.

Therefore, after reviewing from various aspects, I think that the
unnecessary n_vclocks check and n_vclocks_mux lock in ptp_vclock_in_use()
should be removed to prevent false lockdep detection and performance
overhead, and it is more appropriate to change the n_vclocks check to be
performed first in the function that actually uses n_vclocks.

Re: [PATCH v2] ptp: remove ptp->n_vclocks check logic in ptp_vclock_in_use()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 21 May 2025 01:07:17 +0900 you wrote:
> There is no disagreement that we should check both ptp->is_virtual_clock
> and ptp->n_vclocks to check if the ptp virtual clock is in use.
> 
> However, when we acquire ptp->n_vclocks_mux to read ptp->n_vclocks in
> ptp_vclock_in_use(), we observe a recursive lock in the call trace
> starting from n_vclocks_store().
> 
> [...]

Here is the summary with links:
  - [v2] ptp: remove ptp->n_vclocks check logic in ptp_vclock_in_use()
    https://git.kernel.org/netdev/net/c/87f7ce260a3c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html