[PATCH net] ptp: fc3: unregister PTP clock on probe failure

[PATCH net] ptp: fc3: unregister PTP clock on probe failure

[Myeonghun Pak]

ptp_clock_register() leaves a registered clock referring to the clock
information embedded in idtfc3. If either post-registration operation
fails, probe returns and devres frees idtfc3 without unregistering the
clock.

Unregister the clock on both post-registration failure paths.

Fixes: 1ddfecafabf7 ("ptp: add FemtoClock3 Wireless as ptp hardware clock")
Cc: stable@vger.kernel.org
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/ptp/ptp_fc3.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_fc3.c b/drivers/ptp/ptp_fc3.c
--- a/drivers/ptp/ptp_fc3.c
+++ b/drivers/ptp/ptp_fc3.c
@@ -835,11 +835,11 @@ static int idtfc3_enable_ptp(struct idtfc3 *idtfc3)
 
 	err = idtfc3_set_overhead(idtfc3);
 	if (err)
-		return err;
+		goto err_unregister_clock;
 
 	err = idtfc3_init_timecounter(idtfc3);
 	if (err)
-		return err;
+		goto err_unregister_clock;
 
 	dev_info(idtfc3->dev, "TIME_SYNC_CHANNEL registered as ptp%d",
 		 idtfc3->ptp_clock->index);
@@ -847,4 +847,8 @@ static int idtfc3_enable_ptp(struct idtfc3 *idtfc3)
 	return 0;
+
+err_unregister_clock:
+	ptp_clock_unregister(idtfc3->ptp_clock);
+	return err;
 }
 
 static int idtfc3_load_firmware(struct idtfc3 *idtfc3)
-- 
2.50.1

Re: [PATCH net] ptp: fc3: unregister PTP clock on probe failure

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [1]: [PATCH net] ptp: fc3: unregister PTP clock on probe failure ---
commit b2adefd142cc562a99e7438a6d4b4f057c1bebfa
Author: Myeonghun Pak <mhun512@gmail.com>

ptp: fc3: unregister PTP clock on probe failure

This patch unregisters the PTP clock in idtfc3_enable_ptp() on failure
paths. It ensures that the registered clock doesn't refer to freed memory
if post-registration operations fail during probe.

> diff --git a/drivers/ptp/ptp_fc3.c b/drivers/ptp/ptp_fc3.c
> index f0e000428a3f4..32b61bb09b892 100644
> --- a/drivers/ptp/ptp_fc3.c
> +++ b/drivers/ptp/ptp_fc3.c
> @@ -835,16 +835,20 @@ static int idtfc3_enable_ptp(struct idtfc3 *idtfc3)
>  
>  	err = idtfc3_set_overhead(idtfc3);
>  	if (err)
> -		return err;
> +		goto err_unregister_clock;
>  
>  	err = idtfc3_init_timecounter(idtfc3);
>  	if (err)
> -		return err;
> +		goto err_unregister_clock;
>  
>  	dev_info(idtfc3->dev, "TIME_SYNC_CHANNEL registered as ptp%d",
>  		 idtfc3->ptp_clock->index);
>  
>  	return 0;
> +
> +err_unregister_clock:
> +	ptp_clock_unregister(idtfc3->ptp_clock);

[Severity: High]
Can calling ptp_clock_unregister() here introduce an AB-BA deadlock?

Looking at idtfc3_probe(), it acquires idtfc3->lock and calls
idtfc3_enable_ptp():

drivers/ptp/ptp_fc3.c:idtfc3_probe() {
    ...
    mutex_lock(idtfc3->lock);
    err = idtfc3_check_device_compatibility(idtfc3);
    ...
    err = idtfc3_enable_ptp(idtfc3);
    ...
}

Once ptp_clock_register() completes earlier in idtfc3_enable_ptp(), the
device is exposed to userspace. If a service like udev immediately tries
to read the clock, the POSIX clock core acquires a read lock on the
clock's rwsem and calls into the driver callback, which waits for
idtfc3->lock:

kernel/time/posix-clock.c:pc_clock_gettime() {
    down_read(&clk->rwsem);
    ...
    err = clk->ops.clock_gettime(clk, ts);
} ->
drivers/ptp/ptp_fc3.c:idtfc3_gettime() {
    ...
    mutex_lock(idtfc3->lock);
    ...
}

If the probe thread then fails in idtfc3_set_overhead() or
idtfc3_init_timecounter(), it reaches this new error path and calls
ptp_clock_unregister() while still holding idtfc3->lock.

ptp_clock_unregister() attempts to acquire the write lock on the clock's
rwsem:

kernel/time/posix-clock.c:posix_clock_unregister() {
    ...
    down_write(&clk->rwsem);
    ...
}

Would this result in an indefinite deadlock where the probe thread waits
for the rwsem and the userspace thread waits for the driver mutex?

> +	return err;
>  }
-- 
pw-bot: cr