Message-ID: <CAO9qdTE0jt5U_dN09kPEzu-NUCds2VY1Ch2up9RoLazsc1j49w@mail.gmail.com>
Date: Mon, 16 Jun 2025 00:34:59 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, Richard Cochran <richardcochran@...il.com>, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Yangbo Lu <yangbo.lu@....com>
Subject: Re: [PATCH net 1/2] ptp: fix breakage after ptp_vclock_in_use() rework

Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> What is broken
> --------------
>
> ptp4l, and any other application which calls clock_adjtime() on a
> physical clock, is greeted with error -EBUSY after commit 87f7ce260a3c
> ("ptp: remove ptp->n_vclocks check logic in ptp_vclock_in_use()").
>
> Explanation for the breakage
> ----------------------------
>
> The blamed commit was based on the false assumption that
> ptp_vclock_in_use() callers already test for n_vclocks prior to calling
> this function.
>
> This is notably incorrect for the code path below, in which there is, in
> fact, no n_vclocks test:
>
> ptp_clock_adjtime()
> -> ptp_clock_freerun()
>    -> ptp_vclock_in_use()
>
> The result is that any clock adjustment on any physical clock is now
> impossible. This is _despite_ there not being any vclock over this
> physical clock.
>
> $ ptp4l -i eno0 -2 -P -m
> ptp4l[58.425]: selected /dev/ptp0 as PTP clock
> [   58.429749] ptp: physical clock is free running
> ptp4l[58.431]: Failed to open /dev/ptp0: Device or resource busy
> failed to create a clock
> $ cat /sys/class/ptp/ptp0/n_vclocks
> 0
>
> The patch makes the ptp_vclock_in_use() function say "if it's not a
> virtual clock, then this physical clock does have virtual clocks on
> top".
>
> Then ptp_clock_freerun() uses this information to say "this physical
> clock has virtual clocks on top, so it must stay free-running".
>
> Then ptp_clock_adjtime() uses this information to say "well, if this
> physical clock has to be free-running, I can't do it, return -EBUSY".
>
> Simply put, ptp_vclock_in_use() cannot be simplified so as to remove the
> test whether vclocks are in use.
>
> What did the blamed commit intend to fix
> ----------------------------------------
>
> The blamed commit presents a lockdep warning stating "possible recursive
> locking detected", with the n_vclocks_store() and ptp_clock_unregister()
> functions involved.
>
> The recursive locking seems this:
> n_vclocks_store()
> -> mutex_lock_interruptible(&ptp->n_vclocks_mux) // 1
> -> device_for_each_child_reverse(..., unregister_vclock)
>    -> unregister_vclock()
>       -> ptp_vclock_unregister()
>          -> ptp_clock_unregister()
>             -> ptp_vclock_in_use()
>                -> mutex_lock_interruptible(&ptp->n_vclocks_mux) // 2
>
> The issue can be triggered by creating and then deleting vclocks:
> $ echo 2 > /sys/class/ptp/ptp0/n_vclocks
> $ echo 0 > /sys/class/ptp/ptp0/n_vclocks
>
> But note that in the original stack trace, the address of the first lock
> is different from the address of the second lock. This is because at
> step 1 marked above, &ptp->n_vclocks_mux is the lock of the parent
> (physical) PTP clock, and at step 2, the lock is of the child (virtual)
> PTP clock. They are different locks of different devices.
>
> In this situation there is no real deadlock, the lockdep warning is
> caused by the fact that the mutexes have the same lock class on both the
> parent and the child. Functionally it is fine.
>
> Proposed alternative solution
> -----------------------------
>
> We must reintroduce the body of ptp_vclock_in_use() mostly as it was
> structured prior to the blamed commit, but avoid the lockdep warning.
>
> Based on the fact that vclocks cannot be nested on top of one another
> (ptp_is_attribute_visible() hides n_vclocks for virtual clocks), we
> already know that ptp->n_vclocks is zero for a virtual clock. And
> ptp->is_virtual_clock is a runtime invariant, established at
> ptp_clock_register() time and never changed. There is no need to
> serialize on any mutex in order to read ptp->is_virtual_clock, and we
> take advantage of that by moving it outside the lock.
>
> Thus, virtual clocks do not need to acquire &ptp->n_vclocks_mux at
> all, and step 2 in the code walkthrough above can simply go away.
> We can simply return false to the question "ptp_vclock_in_use(a virtual
> clock)".
>
> Other notes
> -----------
>
> Releasing &ptp->n_vclocks_mux before ptp_vclock_in_use() returns
> execution seems racy, because the returned value can become stale as
> soon as the function returns and before the return value is used (i.e.
> n_vclocks_store() can run any time). The locking requirement should
> somehow be transferred to the caller, to ensure a longer life time for
> the returned value, but this seems out of scope for this severe bug fix.
>
> Because we are also fixing up the logic from the original commit, there
> is another Fixes: tag for that.
>

Thanks for quickly finding the part I missed!

As you said, I confirmed that adjtime and settime functions do not work
properly due to this commit.

However, I don't think it is appropriate to fix ptp_vclock_in_use().
I agree that ptp->n_vclocks should be checked in the path where
ptp_clock_freerun() is called, but there are many drivers that do not
have any contact with ptp->n_vclocks in the path where
ptp_clock_unregister() is called.

The reason I removed the ptp->n_vclocks check logic from the
ptp_vclock_in_use() function is to prevent false positives from lockdep,
but also to prevent the performance overhead caused by locking
ptp->n_vclocks_mux and checking ptp->n_vclocks when calling
ptp_vclock_in_use() from a driver that has nothing to do with
ptp->n_vclocks.

Therefore, I think it would be appropriate to modify ptp_clock_freerun()
like this instead of ptp_vclock_in_use():
---
 drivers/ptp/ptp_private.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 528d86a33f37..abd99087f0ca 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -104,10 +104,20 @@ 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;
+
+   if (mutex_lock_interruptible(&ptp->n_vclocks_mux))
+       return true;
+
+   if (ptp_vclock_in_use(ptp) && ptp->n_vclocks)
+       ret = true;
+
+   mutex_unlock(&ptp->n_vclocks_mux);

-   return ptp_vclock_in_use(ptp);
+   return ret;
 }

 extern const struct class ptp_class;
-- 

I tested this patch with test.c and confirmed that
ptp_clock_{adj,set}time() works correctly again after applying this patch.

test.c:
```c
// gcc -o test test.c -lrt

#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <inttypes.h>
#include <math.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <sys/time.h>
#include <sys/timex.h>
#include <sys/types.h>
#include <time.h>
#include <unistd.h>

#include <linux/ptp_clock.h>

static clockid_t get_clockid(int fd)
{
#define CLOCKFD 3
#define FD_TO_CLOCKID(fd)   ((~(clockid_t) (fd) << 3) | CLOCKFD)
    return FD_TO_CLOCKID(fd);
}

int main() {
    int fd = open("/dev/ptp0", O_RDWR);
    clockid_t clockid = get_clockid(fd);
    struct timex tx;
    struct timespec ts;

    memset(&tx, 0, sizeof(tx));
    tx.modes = ADJ_OFFSET;
    tx.time.tv_sec = 0;
    tx.time.tv_usec = 0;

    clock_adjtime(clockid, &tx);

    ts.tv_sec = 0;
    ts.tv_nsec = 0;

    clock_settime(clockid, &ts);

    return 0;
}
```

> Fixes: 87f7ce260a3c ("ptp: remove ptp->n_vclocks check logic in ptp_vclock_in_use()")
> Fixes: 73f37068d540 ("ptp: support ptp physical/virtual clocks conversion")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>  drivers/ptp/ptp_private.h | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
> index 528d86a33f37..a6aad743c282 100644
> --- a/drivers/ptp/ptp_private.h
> +++ b/drivers/ptp/ptp_private.h
> @@ -98,7 +98,27 @@ 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)
>  {
> -       return !ptp->is_virtual_clock;
> +       bool in_use = false;
> +
> +       /* Virtual clocks can't be stacked on top of virtual clocks.
> +        * Avoid acquiring the n_vclocks_mux on virtual clocks, to allow this
> +        * function to be called from code paths where the n_vclocks_mux of the
> +        * parent physical clock is already held. Functionally that's not an
> +        * issue, but lockdep would complain, because they have the same lock
> +        * class.
> +        */
> +       if (ptp->is_virtual_clock)
> +               return false;
> +
> +       if (mutex_lock_interruptible(&ptp->n_vclocks_mux))
> +               return true;
> +
> +       if (ptp->n_vclocks)
> +               in_use = true;
> +
> +       mutex_unlock(&ptp->n_vclocks_mux);
> +
> +       return in_use;
>  }
>
>  /* Check if ptp clock shall be free running */
> --
> 2.43.0
>

Regards,

Jeongjun Park

Powered by blists - more mailing lists