ABBA deadlock occurs in the following scenario:

       CPU0                           CPU1
       ----                           ----
  n_vclocks_store()
    lock(&ptp->n_vclocks_mux) [1]
                                     pc_clock_adjtime()
                                       lock(&clk->rwsem) [2]
                                       ...
                                       ptp_clock_freerun()
                                         ptp_vclock_in_use()
                                           lock(&ptp->n_vclocks_mux) [3]
    ptp_clock_unregister()
      posix_clock_unregister()
        lock(&clk->rwsem) [4]

To solve this with minimal patches, we should change ptp_clock_freerun()
to briefly release the read lock before calling ptp_vclock_in_use() and
then re-lock it when we're done.

Reported-by: syzbot+7cfb66a237c4a5fb22ad@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7cfb66a237c4a5fb22ad
Fixes: 73f37068d540 ("ptp: support ptp physical/virtual clocks conversion")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
v2: Add CC Vladimir
- Link to v1: https://lore.kernel.org/all/20250705145031.140571-1-aha310510@gmail.com/
---
 drivers/ptp/ptp_private.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index a6aad743c282..e2c37e968c88 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -124,10 +124,16 @@ static inline bool ptp_vclock_in_use(struct ptp_clock *ptp)
 /* Check if ptp clock shall be free running */
 static inline bool ptp_clock_freerun(struct ptp_clock *ptp)
 {
+	bool ret = false;
+
 	if (ptp->has_cycles)
-		return false;
+		return ret;
+
+	up_read(&ptp->clock.rwsem);
+	ret = ptp_vclock_in_use(ptp);
+	down_read(&ptp->clock.rwsem);
 
-	return ptp_vclock_in_use(ptp);
+	return ret;
 }
 
 extern const struct class ptp_class;
--

On Fri, Jul 18, 2025 at 08:49:58PM +0900, Jeongjun Park wrote:
> ABBA deadlock occurs in the following scenario:
> 
>        CPU0                           CPU1
>        ----                           ----
>   n_vclocks_store()
>     lock(&ptp->n_vclocks_mux) [1]
>                                      pc_clock_adjtime()
>                                        lock(&clk->rwsem) [2]
>                                        ...
>                                        ptp_clock_freerun()
>                                          ptp_vclock_in_use()
>                                            lock(&ptp->n_vclocks_mux) [3]
>     ptp_clock_unregister()
>       posix_clock_unregister()
>         lock(&clk->rwsem) [4]
> 
> To solve this with minimal patches, we should change ptp_clock_freerun()
> to briefly release the read lock before calling ptp_vclock_in_use() and
> then re-lock it when we're done.

The most important part of solving a problem is understanding the problem
that there is to solve. It appears that you've jumped over that step.

The n_vclocks sysfs is exposed for a physical clock, and acquires the
physical clock's n_vclocks_mux, as shown in your diagram at step [1].

Another process calls pc_clock_adjtime(), acquires &clk->rwsem at step [2],
and calls ptp_clock_adjtime(). This further tests ptp_clock_freerun() ->
ptp_vclock_in_use(), and the fact that ptp_vclock_in_use() gets to acquire
n_vclocks_mux at step [3] means, as per its implementation modified in
commit 5ab73b010cad ("ptp: fix breakage after ptp_vclock_in_use() rework"),
that the PTP clock modified by pc_clock_adjtime() could have only been a
physical clock (ptp->is_virtual_clock == false). This is because we do
not acquire n_vclocks_mux on virtual clocks.

Back to the CPU0 code path, where we iterate over the physical PTP
clock's virtual clocks, and call device_for_each_child_reverse(...,
unregister_vclock) -> ptp_vclock_unregister() -> ptp_clock_unregister() ->
posix_clock_unregister() on them. During the unregister procedure,
posix_clock_unregister() acquires the virtual clock's &clk->rwsem as
shown in your final step [4].

It is clear that the clock which CPU0 unregisters cannot be the same as
the clock which CPU1 adjusts, because the unregistering CPU0 clock is
virtual, and the adjusted CPU1 clock is physical.

The crucial bit of information from lockdep's message "WARNING: possible
circular locking dependency detected" is the word "possible".

See Documentation/locking/lockdep-design.rst section "Lock-class" to
understand that lockdep does not operate on individual locks, but
instead on "classes". Therefore, simply put, it does not see, in lack of
any extra annotations, that the &clk->rwsem of the physical clock is
different than the &clk->rwsem of a child virtual clock. They have the
same class.

Therefore, there is no AB/BA ordering between locks themselves, because
the first "A" is the &clk->rwsem of a physical clock, and the second "A"
is the &clk->rwsem of a virtual clock. The "B" lock may be the same: the
&ptp->n_vclocks_mux of the physical clock.

Of course, having lockdep be able to validate locking using its
class-based algorithm is still important, and a patch is still needed.
The solution here is at your choice, but the problem space that needs to
be explored in order to fulfill that is extremely different from the
patch that you've proposed, to the point that I don't even think I need
to mention that a patch that makes an unsafe funtional change (drops a
lock and reacquires it), when alternatives having to do with annotating
lock subclasses are available and sufficient, is going to get a well
justified NACK from maintainers.

Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Fri, Jul 18, 2025 at 08:49:58PM +0900, Jeongjun Park wrote:
> > ABBA deadlock occurs in the following scenario:
> >
> >        CPU0                           CPU1
> >        ----                           ----
> >   n_vclocks_store()
> >     lock(&ptp->n_vclocks_mux) [1]
> >                                      pc_clock_adjtime()
> >                                        lock(&clk->rwsem) [2]
> >                                        ...
> >                                        ptp_clock_freerun()
> >                                          ptp_vclock_in_use()
> >                                            lock(&ptp->n_vclocks_mux) [3]
> >     ptp_clock_unregister()
> >       posix_clock_unregister()
> >         lock(&clk->rwsem) [4]
> >
> > To solve this with minimal patches, we should change ptp_clock_freerun()
> > to briefly release the read lock before calling ptp_vclock_in_use() and
> > then re-lock it when we're done.
>
> The most important part of solving a problem is understanding the problem
> that there is to solve. It appears that you've jumped over that step.
>
> The n_vclocks sysfs is exposed for a physical clock, and acquires the
> physical clock's n_vclocks_mux, as shown in your diagram at step [1].
>
> Another process calls pc_clock_adjtime(), acquires &clk->rwsem at step [2],
> and calls ptp_clock_adjtime(). This further tests ptp_clock_freerun() ->
> ptp_vclock_in_use(), and the fact that ptp_vclock_in_use() gets to acquire
> n_vclocks_mux at step [3] means, as per its implementation modified in
> commit 5ab73b010cad ("ptp: fix breakage after ptp_vclock_in_use() rework"),
> that the PTP clock modified by pc_clock_adjtime() could have only been a
> physical clock (ptp->is_virtual_clock == false). This is because we do
> not acquire n_vclocks_mux on virtual clocks.
>
> Back to the CPU0 code path, where we iterate over the physical PTP
> clock's virtual clocks, and call device_for_each_child_reverse(...,
> unregister_vclock) -> ptp_vclock_unregister() -> ptp_clock_unregister() ->
> posix_clock_unregister() on them. During the unregister procedure,
> posix_clock_unregister() acquires the virtual clock's &clk->rwsem as
> shown in your final step [4].
>
> It is clear that the clock which CPU0 unregisters cannot be the same as
> the clock which CPU1 adjusts, because the unregistering CPU0 clock is
> virtual, and the adjusted CPU1 clock is physical.
>
> The crucial bit of information from lockdep's message "WARNING: possible
> circular locking dependency detected" is the word "possible".
>
> See Documentation/locking/lockdep-design.rst section "Lock-class" to
> understand that lockdep does not operate on individual locks, but
> instead on "classes". Therefore, simply put, it does not see, in lack of
> any extra annotations, that the &clk->rwsem of the physical clock is
> different than the &clk->rwsem of a child virtual clock. They have the
> same class.
>
> Therefore, there is no AB/BA ordering between locks themselves, because
> the first "A" is the &clk->rwsem of a physical clock, and the second "A"
> is the &clk->rwsem of a virtual clock. The "B" lock may be the same: the
> &ptp->n_vclocks_mux of the physical clock.
>
> Of course, having lockdep be able to validate locking using its
> class-based algorithm is still important, and a patch is still needed.
> The solution here is at your choice, but the problem space that needs to
> be explored in order to fulfill that is extremely different from the
> patch that you've proposed, to the point that I don't even think I need
> to mention that a patch that makes an unsafe funtional change (drops a
> lock and reacquires it), when alternatives having to do with annotating
> lock subclasses are available and sufficient, is going to get a well
> justified NACK from maintainers.

Thanks for the detailed explanation!

Thanks to you, I figured out that I need to add a annotating lockdep
subclass to some of the locks in vclock. I'll write a v3 patch and send
it to you right away.

Regards,

Jeongjun Park