[PATCH net v3] ptp: ocp: fix resource freeing order

[PATCH net v3] ptp: ocp: fix resource freeing order

[Vadim Fedorenko]

Commit a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable
events") added a call to ptp_disable_all_events() which changes the
configuration of pins if they support EXTTS events. In ptp_ocp_detach()
pins resources are freed before ptp_clock_unregister() and it leads to
use-after-free during driver removal. Fix it by changing the order of
free/unregister calls.

Fixes: a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable events")
Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
---
v2 -> v3:
* fix code issue in v2
v1 -> v2:
* on v1 sashiko mentioned that some IRQs may fire after bp->ptp freed.
  it looks like a false positive, because ptp_clock_unregister() will
  explicitly disable all EXTTS channels before destroying ptp_clock
  structure. But there is a possibility of timestampers being active
  without PTP_PF_EXTTS function set. It can be done through sysfs files
  and it will change pins configuration directly, but can only be done
  for TS1..TS4. Disable them explicitly after detaching sysfs files.
---

 drivers/ptp/ptp_ocp.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index beacc2ffb166..b3a9480b9bcb 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -4867,6 +4867,18 @@ ptp_ocp_detach(struct ptp_ocp *bp)
 	ptp_ocp_detach_sysfs(bp);
 	ptp_ocp_attr_group_del(bp);
 	timer_delete_sync(&bp->watchdog);
+	/* Disable interrupts on all timestampers */
+	if (bp->ts1)
+		ptp_ocp_ts_enable(bp->ts1, 0, false);
+	if (bp->ts2)
+		ptp_ocp_ts_enable(bp->ts2, 0, false);
+	if (bp->ts3)
+		ptp_ocp_ts_enable(bp->ts3, 0, false);
+	if (bp->ts4)
+		ptp_ocp_ts_enable(bp->ts4, 0, false);
+	if (bp->ptp)
+		ptp_clock_unregister(bp->ptp);
+	kfree(bp->ptp_info.pin_config);
 	ptp_ocp_unregister_ext(bp->ts0);
 	ptp_ocp_unregister_ext(bp->ts1);
 	ptp_ocp_unregister_ext(bp->ts2);
@@ -4884,9 +4896,6 @@ ptp_ocp_detach(struct ptp_ocp *bp)
 		clk_hw_unregister_fixed_rate(bp->i2c_clk);
 	if (bp->n_irqs)
 		pci_free_irq_vectors(bp->pdev);
-	if (bp->ptp)
-		ptp_clock_unregister(bp->ptp);
-	kfree(bp->ptp_info.pin_config);
 	device_unregister(&bp->dev);
 }
 
-- 
2.47.3

Re: [PATCH net v3] ptp: ocp: fix resource freeing order

[Vadim Fedorenko]

On 14/05/2026 11:00, Vadim Fedorenko wrote:
> Commit a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable
> events") added a call to ptp_disable_all_events() which changes the
> configuration of pins if they support EXTTS events. In ptp_ocp_detach()
> pins resources are freed before ptp_clock_unregister() and it leads to
> use-after-free during driver removal. Fix it by changing the order of
> free/unregister calls.
> 
> Fixes: a60fc3294a37 ("ptp: rework ptp_clock_unregister() to disable events")
> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> ---
> v2 -> v3:
> * fix code issue in v2
> v1 -> v2:
> * on v1 sashiko mentioned that some IRQs may fire after bp->ptp freed.
>    it looks like a false positive, because ptp_clock_unregister() will
>    explicitly disable all EXTTS channels before destroying ptp_clock
>    structure. But there is a possibility of timestampers being active
>    without PTP_PF_EXTTS function set. It can be done through sysfs files
>    and it will change pins configuration directly, but can only be done
>    for TS1..TS4. Disable them explicitly after detaching sysfs files.
> ---
> 
>   drivers/ptp/ptp_ocp.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index beacc2ffb166..b3a9480b9bcb 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -4867,6 +4867,18 @@ ptp_ocp_detach(struct ptp_ocp *bp)
>   	ptp_ocp_detach_sysfs(bp);
>   	ptp_ocp_attr_group_del(bp);
>   	timer_delete_sync(&bp->watchdog);
> +	/* Disable interrupts on all timestampers */
> +	if (bp->ts1)
> +		ptp_ocp_ts_enable(bp->ts1, 0, false);
> +	if (bp->ts2)
> +		ptp_ocp_ts_enable(bp->ts2, 0, false);
> +	if (bp->ts3)
> +		ptp_ocp_ts_enable(bp->ts3, 0, false);
> +	if (bp->ts4)
> +		ptp_ocp_ts_enable(bp->ts4, 0, false);
> +	if (bp->ptp)
> +		ptp_clock_unregister(bp->ptp);
> +	kfree(bp->ptp_info.pin_config);
>   	ptp_ocp_unregister_ext(bp->ts0);
>   	ptp_ocp_unregister_ext(bp->ts1);
>   	ptp_ocp_unregister_ext(bp->ts2);
> @@ -4884,9 +4896,6 @@ ptp_ocp_detach(struct ptp_ocp *bp)
>   		clk_hw_unregister_fixed_rate(bp->i2c_clk);
>   	if (bp->n_irqs)
>   		pci_free_irq_vectors(bp->pdev);
> -	if (bp->ptp)
> -		ptp_clock_unregister(bp->ptp);
> -	kfree(bp->ptp_info.pin_config);
>   	device_unregister(&bp->dev);
>   }
>   

Looks like Sashiko still has a point of in-flight irq handler.
Going to send v4 with synchronize_irq() added to the disable path
of ptp_ocp_ts_enable()

---
pw-bot: cr