[v2,1/1,F] UBUNTU: SAUCE: ptp: free ptp clock properly

BugLink: https://bugs.launchpad.net/bugs/1864754

There is a bug in ptp_clock_unregister() where pps_unregister_source()
can free up resources needed by posix_clock_unregister() to properly
destroy a related sysfs device.

Fix this by calling pps_unregister_source() in ptp_clock_release().

See also:
commit 75718584cb3c ("ptp: free ptp device pin descriptors properly").

Fixes: a33121e5487b ("ptp: fix the race between the release of ptp_clock and cdev")
Tested-by: Piotr Morgwai Kotarbiński <foss@morgwai.pl>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
---

v2: move pps_unregister_source() to ptp_clock_release(), instead of
    posix_clock_unregister(), that would just introduce a resource leak

 drivers/ptp/ptp_clock.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

On 27/03/2020 10:07, Andrea Righi wrote:
> BugLink: https://bugs.launchpad.net/bugs/1864754
> 
> There is a bug in ptp_clock_unregister() where pps_unregister_source()
> can free up resources needed by posix_clock_unregister() to properly
> destroy a related sysfs device.
> 
> Fix this by calling pps_unregister_source() in ptp_clock_release().
> 
> See also:
> commit 75718584cb3c ("ptp: free ptp device pin descriptors properly").
> 
> Fixes: a33121e5487b ("ptp: fix the race between the release of ptp_clock and cdev")
> Tested-by: Piotr Morgwai Kotarbiński <foss@morgwai.pl>
> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> ---
> 
> v2: move pps_unregister_source() to ptp_clock_release(), instead of
>     posix_clock_unregister(), that would just introduce a resource leak
> 
>  drivers/ptp/ptp_clock.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index b84f16bbd6f2..d45c76566195 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -170,6 +170,9 @@ static void ptp_clock_release(struct device *dev)
>  {
>  	struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev);
>  
> +	/* Release the clock's resources. */
> +	if (ptp->pps_source)
> +		pps_unregister_source(ptp->pps_source);
>  	ptp_cleanup_pin_groups(ptp);
>  	mutex_destroy(&ptp->tsevq_mux);
>  	mutex_destroy(&ptp->pincfg_mux);
> @@ -298,11 +301,6 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
>  		kthread_cancel_delayed_work_sync(&ptp->aux_work);
>  		kthread_destroy_worker(ptp->kworker);
>  	}
> -
> -	/* Release the clock's resources. */
> -	if (ptp->pps_source)
> -		pps_unregister_source(ptp->pps_source);
> -
>  	posix_clock_unregister(&ptp->clock);
>  
>  	return 0;
> 

Is this upstream material?

Positive test results, looks sane to me. Thanks Andrea.

Acked-by: Colin Ian King <colin.king@canonical.com>

On Fri, Mar 27, 2020 at 11:30:08AM +0000, Colin Ian King wrote:
> On 27/03/2020 10:07, Andrea Righi wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1864754
> > 
> > There is a bug in ptp_clock_unregister() where pps_unregister_source()
> > can free up resources needed by posix_clock_unregister() to properly
> > destroy a related sysfs device.
> > 
> > Fix this by calling pps_unregister_source() in ptp_clock_release().
> > 
> > See also:
> > commit 75718584cb3c ("ptp: free ptp device pin descriptors properly").
> > 
> > Fixes: a33121e5487b ("ptp: fix the race between the release of ptp_clock and cdev")
> > Tested-by: Piotr Morgwai Kotarbiński <foss@morgwai.pl>
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> > ---
> > 
> > v2: move pps_unregister_source() to ptp_clock_release(), instead of
> >     posix_clock_unregister(), that would just introduce a resource leak
> > 
> >  drivers/ptp/ptp_clock.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> > index b84f16bbd6f2..d45c76566195 100644
> > --- a/drivers/ptp/ptp_clock.c
> > +++ b/drivers/ptp/ptp_clock.c
> > @@ -170,6 +170,9 @@ static void ptp_clock_release(struct device *dev)
> >  {
> >  	struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev);
> >  
> > +	/* Release the clock's resources. */
> > +	if (ptp->pps_source)
> > +		pps_unregister_source(ptp->pps_source);
> >  	ptp_cleanup_pin_groups(ptp);
> >  	mutex_destroy(&ptp->tsevq_mux);
> >  	mutex_destroy(&ptp->pincfg_mux);
> > @@ -298,11 +301,6 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
> >  		kthread_cancel_delayed_work_sync(&ptp->aux_work);
> >  		kthread_destroy_worker(ptp->kworker);
> >  	}
> > -
> > -	/* Release the clock's resources. */
> > -	if (ptp->pps_source)
> > -		pps_unregister_source(ptp->pps_source);
> > -
> >  	posix_clock_unregister(&ptp->clock);
> >  
> >  	return 0;
> > 
> 
> Is this upstream material?

Yes [1], but I need to provide more details on how exactly this problem
is happening and how this patch is fixing it.

Fortunately Morgwai (the bug reporter) can easily reproduce the problem,
so we should be able to collect more useful information to properly
justify the inclusion upstream.

For now I think it still makes sense to apply the patch to our kernel,
since it has been tested with positive results on the affected platform
and the fix is only restricted to the ptp clock unregistering code path.

Thanks,
-Andrea

[1] https://lkml.org/lkml/2020/3/9/708

> 
> Positive test results, looks sane to me. Thanks Andrea.
> 
> Acked-by: Colin Ian King <colin.king@canonical.com>