[PATCH 2/2] drm/vc4: Stop the active perfmon before being destroyed

[Juan A. Suárez]

Worth to mention we got this issue happened also for v3d (a fix was
already submitted).


Reviewed-by: Juan A. Suarez <jasuarez at igalia.com>


On Fri, 2024-10-04 at 09:36 -0300, Maíra Canal wrote:
> Upon closing the file descriptor, the active performance monitor is
> not
> stopped. Although all perfmons are destroyed in
> `vc4_perfmon_close_file()`,
> the active performance monitor's pointer (`vc4->active_perfmon`) is
> still
> retained.
> 
> If we open a new file descriptor and submit a few jobs with
> performance
> monitors, the driver will attempt to stop the active performance
> monitor
> using the stale pointer in `vc4->active_perfmon`. However, this
> pointer
> is no longer valid because the previous process has already
> terminated,
> and all performance monitors associated with it have been destroyed
> and
> freed.
> 
> To fix this, when the active performance monitor belongs to a given
> process, explicitly stop it before destroying and freeing it.
> 
> Cc: <stable at vger.kernel.org> # v4.17+
> Cc: Boris Brezillon <bbrezillon at kernel.org>
> Cc: Juan A. Suarez Romero <jasuarez at igalia.com>
> Fixes: 65101d8c9108 ("drm/vc4: Expose performance counters to
> userspace")
> Signed-off-by: Maíra Canal <mcanal at igalia.com>
> ---
>  drivers/gpu/drm/vc4/vc4_perfmon.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_perfmon.c
> b/drivers/gpu/drm/vc4/vc4_perfmon.c
> index f2e56d0f6298..f1342f917cf7 100644
> --- a/drivers/gpu/drm/vc4/vc4_perfmon.c
> +++ b/drivers/gpu/drm/vc4/vc4_perfmon.c
> @@ -116,6 +116,11 @@ void vc4_perfmon_open_file(struct vc4_file
> *vc4file)
>  static int vc4_perfmon_idr_del(int id, void *elem, void *data)
>  {
>  	struct vc4_perfmon *perfmon = elem;
> +	struct vc4_dev *vc4 = (struct vc4_dev *)data;
> +
> +	/* If the active perfmon is being destroyed, stop it first
> */
> +	if (perfmon == vc4->active_perfmon)
> +		vc4_perfmon_stop(vc4, perfmon, false);
>  
>  	vc4_perfmon_put(perfmon);
>  
> @@ -130,7 +135,7 @@ void vc4_perfmon_close_file(struct vc4_file
> *vc4file)
>  		return;
>  
>  	mutex_lock(&vc4file->perfmon.lock);
> -	idr_for_each(&vc4file->perfmon.idr, vc4_perfmon_idr_del,
> NULL);
> +	idr_for_each(&vc4file->perfmon.idr, vc4_perfmon_idr_del,
> vc4);
>  	idr_destroy(&vc4file->perfmon.idr);
>  	mutex_unlock(&vc4file->perfmon.lock);
>  	mutex_destroy(&vc4file->perfmon.lock);