[PATCH] drm: Fix writeback_job leak when state is check only or check failed

[PATCH] drm: Fix writeback_job leak when state is check only or check failed

[james qian wang (Arm Technology China)]

The writeback job will not be added to writeback queue if the state is
check only or check failed, to avoid leak, need to cleanup writeback job
in connector_destroy_state if the job existed.

Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c |  4 ++++
 drivers/gpu/drm/drm_writeback.c           | 21 ++++++++++++++++++---
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 4985384e51f6..6a8e414233de 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -28,6 +28,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_plane.h>
 #include <drm/drm_connector.h>
+#include <drm/drm_writeback.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_device.h>
 
@@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state);
 void
 __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
 {
+	if (state->writeback_job)
+		drm_writeback_cleanup_job(state->writeback_job);
+
 	if (state->crtc)
 		drm_connector_put(state->connector);
 
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index c20e6fe00cb3..486121150eaa 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
 }
 EXPORT_SYMBOL(drm_writeback_queue_job);
 
+/**
+ * drm_writeback_cleanup_job - cleanup a writeback job
+ * @job: The job to cleanup
+ */
+void drm_writeback_cleanup_job(struct drm_writeback_job *job)
+{
+	if (job->fb)
+		drm_framebuffer_put(job->fb);
+
+	if (job->out_fence)
+		dma_fence_put(job->out_fence);
+
+	kfree(job);
+}
+EXPORT_SYMBOL(drm_writeback_cleanup_job);
+
 /*
  * @cleanup_work: deferred cleanup of a writeback job
  *
@@ -280,11 +296,9 @@ static void cleanup_work(struct work_struct *work)
 	struct drm_writeback_job *job = container_of(work,
 						     struct drm_writeback_job,
 						     cleanup_work);
-	drm_framebuffer_put(job->fb);
-	kfree(job);
+	drm_writeback_cleanup_job(job);
 }
 
-
 /**
  * drm_writeback_signal_completion - Signal the completion of a writeback job
  * @wb_connector: The writeback connector whose job is complete
@@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector,
 				dma_fence_set_error(job->out_fence, status);
 			dma_fence_signal(job->out_fence);
 			dma_fence_put(job->out_fence);
+			job->out_fence = NULL;
 		}
 	}
 	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Fix writeback_job leak when state is check only or check failed

[Maarten Lankhorst]

Hey

Op 21-02-2019 om 12:14 schreef james qian wang (Arm Technology China):
> The writeback job will not be added to writeback queue if the state is
> check only or check failed, to avoid leak, need to cleanup writeback job
> in connector_destroy_state if the job existed.
>
> Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>

Is writeback_job in the conn_state set to null somewhere? I'm worried we might end up freeing writeback_job twice on success.

~Maarten

>  drivers/gpu/drm/drm_atomic_state_helper.c |  4 ++++
>  drivers/gpu/drm/drm_writeback.c           | 21 ++++++++++++++++++---
>  2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 4985384e51f6..6a8e414233de 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -28,6 +28,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_plane.h>
>  #include <drm/drm_connector.h>
> +#include <drm/drm_writeback.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_device.h>
>  
> @@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state);
>  void
>  __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
>  {
> +	if (state->writeback_job)
> +		drm_writeback_cleanup_job(state->writeback_job);
> +
>  	if (state->crtc)
>  		drm_connector_put(state->connector);
>  
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index c20e6fe00cb3..486121150eaa 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
>  }
>  EXPORT_SYMBOL(drm_writeback_queue_job);
>  
> +/**
> + * drm_writeback_cleanup_job - cleanup a writeback job
> + * @job: The job to cleanup
> + */
> +void drm_writeback_cleanup_job(struct drm_writeback_job *job)
> +{
> +	if (job->fb)
> +		drm_framebuffer_put(job->fb);
> +
> +	if (job->out_fence)
> +		dma_fence_put(job->out_fence);
> +
> +	kfree(job);
> +}
> +EXPORT_SYMBOL(drm_writeback_cleanup_job);
> +
>  /*
>   * @cleanup_work: deferred cleanup of a writeback job
>   *
> @@ -280,11 +296,9 @@ static void cleanup_work(struct work_struct *work)
>  	struct drm_writeback_job *job = container_of(work,
>  						     struct drm_writeback_job,
>  						     cleanup_work);
> -	drm_framebuffer_put(job->fb);
> -	kfree(job);
> +	drm_writeback_cleanup_job(job);
>  }
>  
> -
>  /**
>   * drm_writeback_signal_completion - Signal the completion of a writeback job
>   * @wb_connector: The writeback connector whose job is complete
> @@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector,
>  				dma_fence_set_error(job->out_fence, status);
>  			dma_fence_signal(job->out_fence);
>  			dma_fence_put(job->out_fence);
> +			job->out_fence = NULL;
>  		}
>  	}
>  	spin_unlock_irqrestore(&wb_connector->job_lock, flags);


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Fix writeback_job leak when state is check only or check failed

[james qian wang (Arm Technology China)]

On Thu, Feb 21, 2019 at 02:56:56PM +0100, Maarten Lankhorst wrote:
> Hey
> 
> Op 21-02-2019 om 12:14 schreef james qian wang (Arm Technology China):
> > The writeback job will not be added to writeback queue if the state is
> > check only or check failed, to avoid leak, need to cleanup writeback job
> > in connector_destroy_state if the job existed.
> >
> > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> 
> Is writeback_job in the conn_state set to null somewhere? I'm worried we might end up freeing writeback_job twice on success.
> 
> ~Maarten

Yes, the state->writeback_job need to be set to null once the job has been
queued.
Maybe we can refine the queue_job function and add this set NULL into
it.

void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
			     struct drm_writeback_job **new_job)
{
        struct drm_writeback_job *job = *new_job;
	unsigned long flags;

        *new_job = NULL;

	spin_lock_irqsave(&wb_connector->job_lock, flags);
	list_add_tail(&job->list_entry, &wb_connector->job_queue);
	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
}

Thanks
James

> 
> >  drivers/gpu/drm/drm_atomic_state_helper.c |  4 ++++
> >  drivers/gpu/drm/drm_writeback.c           | 21 ++++++++++++++++++---
> >  2 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 4985384e51f6..6a8e414233de 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -28,6 +28,7 @@
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_plane.h>
> >  #include <drm/drm_connector.h>
> > +#include <drm/drm_writeback.h>
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_device.h>
> >  
> > @@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state);
> >  void
> >  __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
> >  {
> > +	if (state->writeback_job)
> > +		drm_writeback_cleanup_job(state->writeback_job);
> > +
> >  	if (state->crtc)
> >  		drm_connector_put(state->connector);
> >  
> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > index c20e6fe00cb3..486121150eaa 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> >  }
> >  EXPORT_SYMBOL(drm_writeback_queue_job);
> >  
> > +/**
> > + * drm_writeback_cleanup_job - cleanup a writeback job
> > + * @job: The job to cleanup
> > + */
> > +void drm_writeback_cleanup_job(struct drm_writeback_job *job)
> > +{
> > +	if (job->fb)
> > +		drm_framebuffer_put(job->fb);
> > +
> > +	if (job->out_fence)
> > +		dma_fence_put(job->out_fence);
> > +
> > +	kfree(job);
> > +}
> > +EXPORT_SYMBOL(drm_writeback_cleanup_job);
> > +
> >  /*
> >   * @cleanup_work: deferred cleanup of a writeback job
> >   *
> > @@ -280,11 +296,9 @@ static void cleanup_work(struct work_struct *work)
> >  	struct drm_writeback_job *job = container_of(work,
> >  						     struct drm_writeback_job,
> >  						     cleanup_work);
> > -	drm_framebuffer_put(job->fb);
> > -	kfree(job);
> > +	drm_writeback_cleanup_job(job);
> >  }
> >  
> > -
> >  /**
> >   * drm_writeback_signal_completion - Signal the completion of a writeback job
> >   * @wb_connector: The writeback connector whose job is complete
> > @@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector,
> >  				dma_fence_set_error(job->out_fence, status);
> >  			dma_fence_signal(job->out_fence);
> >  			dma_fence_put(job->out_fence);
> > +			job->out_fence = NULL;
> >  		}
> >  	}
> >  	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Fix writeback_job leak when state is check only or check failed

[Brian Starkey]

Hi James,

On Fri, Feb 22, 2019 at 07:39:55AM +0000, james qian wang (Arm Technology China) wrote:
> On Thu, Feb 21, 2019 at 02:56:56PM +0100, Maarten Lankhorst wrote:
> > Hey
> > 
> > Op 21-02-2019 om 12:14 schreef james qian wang (Arm Technology China):
> > > The writeback job will not be added to writeback queue if the state is
> > > check only or check failed, to avoid leak, need to cleanup writeback job
> > > in connector_destroy_state if the job existed.
> > >
> > > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> > 
> > Is writeback_job in the conn_state set to null somewhere? I'm worried we might end up freeing writeback_job twice on success.
> > 
> > ~Maarten
> 
> Yes, the state->writeback_job need to be set to null once the job has been
> queued.
> Maybe we can refine the queue_job function and add this set NULL into
> it.
> 

Laurent has submitted patches for both of these issues, please check

[PATCH v5 12/19] drm: writeback: Cleanup job ownership handling when queuing job
[PATCH v5 13/19] drm: writeback: Fix leak of writeback job

Also note they will impact the komeda implementation

Thanks,
-Brian

> void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> 			     struct drm_writeback_job **new_job)
> {
>         struct drm_writeback_job *job = *new_job;
> 	unsigned long flags;
> 
>         *new_job = NULL;
> 
> 	spin_lock_irqsave(&wb_connector->job_lock, flags);
> 	list_add_tail(&job->list_entry, &wb_connector->job_queue);
> 	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
> }
> 
> Thanks
> James
> 
> > 
> > >  drivers/gpu/drm/drm_atomic_state_helper.c |  4 ++++
> > >  drivers/gpu/drm/drm_writeback.c           | 21 ++++++++++++++++++---
> > >  2 files changed, 22 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > index 4985384e51f6..6a8e414233de 100644
> > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > @@ -28,6 +28,7 @@
> > >  #include <drm/drm_crtc.h>
> > >  #include <drm/drm_plane.h>
> > >  #include <drm/drm_connector.h>
> > > +#include <drm/drm_writeback.h>
> > >  #include <drm/drm_atomic.h>
> > >  #include <drm/drm_device.h>
> > >  
> > > @@ -407,6 +408,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state);
> > >  void
> > >  __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
> > >  {
> > > +	if (state->writeback_job)
> > > +		drm_writeback_cleanup_job(state->writeback_job);
> > > +
> > >  	if (state->crtc)
> > >  		drm_connector_put(state->connector);
> > >  
> > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > > index c20e6fe00cb3..486121150eaa 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -268,6 +268,22 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> > >  }
> > >  EXPORT_SYMBOL(drm_writeback_queue_job);
> > >  
> > > +/**
> > > + * drm_writeback_cleanup_job - cleanup a writeback job
> > > + * @job: The job to cleanup
> > > + */
> > > +void drm_writeback_cleanup_job(struct drm_writeback_job *job)
> > > +{
> > > +	if (job->fb)
> > > +		drm_framebuffer_put(job->fb);
> > > +
> > > +	if (job->out_fence)
> > > +		dma_fence_put(job->out_fence);
> > > +
> > > +	kfree(job);
> > > +}
> > > +EXPORT_SYMBOL(drm_writeback_cleanup_job);
> > > +
> > >  /*
> > >   * @cleanup_work: deferred cleanup of a writeback job
> > >   *
> > > @@ -280,11 +296,9 @@ static void cleanup_work(struct work_struct *work)
> > >  	struct drm_writeback_job *job = container_of(work,
> > >  						     struct drm_writeback_job,
> > >  						     cleanup_work);
> > > -	drm_framebuffer_put(job->fb);
> > > -	kfree(job);
> > > +	drm_writeback_cleanup_job(job);
> > >  }
> > >  
> > > -
> > >  /**
> > >   * drm_writeback_signal_completion - Signal the completion of a writeback job
> > >   * @wb_connector: The writeback connector whose job is complete
> > > @@ -319,6 +333,7 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector,
> > >  				dma_fence_set_error(job->out_fence, status);
> > >  			dma_fence_signal(job->out_fence);
> > >  			dma_fence_put(job->out_fence);
> > > +			job->out_fence = NULL;
> > >  		}
> > >  	}
> > >  	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
> > 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel