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@xxxxxxxxx>

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