[PATCH 1/4] drm/msm: fix memleak on release

[PATCH 1/4] drm/msm: fix memleak on release

[Johan Hovold]

If a process is interrupted while accessing the "gpu" debugfs file and
the drm device struct_mutex is contended, release() could return early
and fail to free related resources.

Note that the return value from release() is ignored.

Fixes: 4f776f4511c7 ("drm/msm/gpu: Convert the GPU show function to use the GPU state")
Cc: stable <stable@vger.kernel.org>     # 4.18
Cc: Jordan Crouse <jcrouse@codeaurora.org>
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpu/drm/msm/msm_debugfs.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
index 6be879578140..1c74381a4fc9 100644
--- a/drivers/gpu/drm/msm/msm_debugfs.c
+++ b/drivers/gpu/drm/msm/msm_debugfs.c
@@ -47,12 +47,8 @@ static int msm_gpu_release(struct inode *inode, struct file *file)
 	struct msm_gpu_show_priv *show_priv = m->private;
 	struct msm_drm_private *priv = show_priv->dev->dev_private;
 	struct msm_gpu *gpu = priv->gpu;
-	int ret;
-
-	ret = mutex_lock_interruptible(&show_priv->dev->struct_mutex);
-	if (ret)
-		return ret;
 
+	mutex_lock(&show_priv->dev->struct_mutex);
 	gpu->funcs->gpu_state_put(show_priv->state);
 	mutex_unlock(&show_priv->dev->struct_mutex);
 
-- 
2.23.0

Re: [PATCH 1/4] drm/msm: fix memleak on release

[Johan Hovold]

On Thu, Oct 10, 2019 at 03:13:30PM +0200, Johan Hovold wrote:
> If a process is interrupted while accessing the "gpu" debugfs file and
> the drm device struct_mutex is contended, release() could return early
> and fail to free related resources.
> 
> Note that the return value from release() is ignored.
> 
> Fixes: 4f776f4511c7 ("drm/msm/gpu: Convert the GPU show function to use the GPU state")
> Cc: stable <stable@vger.kernel.org>     # 4.18
> Cc: Jordan Crouse <jcrouse@codeaurora.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---

Rob, Sean,

Sending a reminder about this one, which is not yet in linux-next.

Perhaps Daniel can pick it up otherwise?

Thanks,
Johan

>  drivers/gpu/drm/msm/msm_debugfs.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index 6be879578140..1c74381a4fc9 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -47,12 +47,8 @@ static int msm_gpu_release(struct inode *inode, struct file *file)
>  	struct msm_gpu_show_priv *show_priv = m->private;
>  	struct msm_drm_private *priv = show_priv->dev->dev_private;
>  	struct msm_gpu *gpu = priv->gpu;
> -	int ret;
> -
> -	ret = mutex_lock_interruptible(&show_priv->dev->struct_mutex);
> -	if (ret)
> -		return ret;
>  
> +	mutex_lock(&show_priv->dev->struct_mutex);
>  	gpu->funcs->gpu_state_put(show_priv->state);
>  	mutex_unlock(&show_priv->dev->struct_mutex);

Re: [PATCH 1/4] drm/msm: fix memleak on release

[Johan Hovold]

On Wed, Oct 30, 2019 at 11:01:46AM +0100, Johan Hovold wrote:
> On Thu, Oct 10, 2019 at 03:13:30PM +0200, Johan Hovold wrote:
> > If a process is interrupted while accessing the "gpu" debugfs file and
> > the drm device struct_mutex is contended, release() could return early
> > and fail to free related resources.
> > 
> > Note that the return value from release() is ignored.
> > 
> > Fixes: 4f776f4511c7 ("drm/msm/gpu: Convert the GPU show function to use the GPU state")
> > Cc: stable <stable@vger.kernel.org>     # 4.18
> > Cc: Jordan Crouse <jcrouse@codeaurora.org>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> 
> Rob, Sean,
> 
> Sending a reminder about this one, which is not yet in linux-next.
> 
> Perhaps Daniel can pick it up otherwise?

Another two weeks, another reminder. This one is still not in -next.

Johan

> >  drivers/gpu/drm/msm/msm_debugfs.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> > index 6be879578140..1c74381a4fc9 100644
> > --- a/drivers/gpu/drm/msm/msm_debugfs.c
> > +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> > @@ -47,12 +47,8 @@ static int msm_gpu_release(struct inode *inode, struct file *file)
> >  	struct msm_gpu_show_priv *show_priv = m->private;
> >  	struct msm_drm_private *priv = show_priv->dev->dev_private;
> >  	struct msm_gpu *gpu = priv->gpu;
> > -	int ret;
> > -
> > -	ret = mutex_lock_interruptible(&show_priv->dev->struct_mutex);
> > -	if (ret)
> > -		return ret;
> >  
> > +	mutex_lock(&show_priv->dev->struct_mutex);
> >  	gpu->funcs->gpu_state_put(show_priv->state);
> >  	mutex_unlock(&show_priv->dev->struct_mutex);

Re: [PATCH 1/4] drm/msm: fix memleak on release

[Daniel Vetter]

On Tue, Nov 12, 2019 at 11:40:01AM +0100, Johan Hovold wrote:
> On Wed, Oct 30, 2019 at 11:01:46AM +0100, Johan Hovold wrote:
> > On Thu, Oct 10, 2019 at 03:13:30PM +0200, Johan Hovold wrote:
> > > If a process is interrupted while accessing the "gpu" debugfs file and
> > > the drm device struct_mutex is contended, release() could return early
> > > and fail to free related resources.
> > > 
> > > Note that the return value from release() is ignored.
> > > 
> > > Fixes: 4f776f4511c7 ("drm/msm/gpu: Convert the GPU show function to use the GPU state")
> > > Cc: stable <stable@vger.kernel.org>     # 4.18
> > > Cc: Jordan Crouse <jcrouse@codeaurora.org>
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > ---
> > 
> > Rob, Sean,
> > 
> > Sending a reminder about this one, which is not yet in linux-next.
> > 
> > Perhaps Daniel can pick it up otherwise?
> 
> Another two weeks, another reminder. This one is still not in -next.

Well msm is maintained in a separate tree, so the usual group maintainer
fallback for when patches are stuck doesn't apply.

Rob, Sean, time to reconsider drm-misc for msm? I think there's some more
oddball patches that occasionally get stuck for msm ...

Also +Dave.
-Daniel

> 
> Johan
> 
> > >  drivers/gpu/drm/msm/msm_debugfs.c | 6 +-----
> > >  1 file changed, 1 insertion(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> > > index 6be879578140..1c74381a4fc9 100644
> > > --- a/drivers/gpu/drm/msm/msm_debugfs.c
> > > +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> > > @@ -47,12 +47,8 @@ static int msm_gpu_release(struct inode *inode, struct file *file)
> > >  	struct msm_gpu_show_priv *show_priv = m->private;
> > >  	struct msm_drm_private *priv = show_priv->dev->dev_private;
> > >  	struct msm_gpu *gpu = priv->gpu;
> > > -	int ret;
> > > -
> > > -	ret = mutex_lock_interruptible(&show_priv->dev->struct_mutex);
> > > -	if (ret)
> > > -		return ret;
> > >  
> > > +	mutex_lock(&show_priv->dev->struct_mutex);
> > >  	gpu->funcs->gpu_state_put(show_priv->state);
> > >  	mutex_unlock(&show_priv->dev->struct_mutex);

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/4] drm/msm: fix memleak on release

[Rob Clark]

On Tue, Nov 12, 2019 at 6:01 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Nov 12, 2019 at 11:40:01AM +0100, Johan Hovold wrote:
> > On Wed, Oct 30, 2019 at 11:01:46AM +0100, Johan Hovold wrote:
> > > On Thu, Oct 10, 2019 at 03:13:30PM +0200, Johan Hovold wrote:
> > > > If a process is interrupted while accessing the "gpu" debugfs file and
> > > > the drm device struct_mutex is contended, release() could return early
> > > > and fail to free related resources.
> > > >
> > > > Note that the return value from release() is ignored.
> > > >
> > > > Fixes: 4f776f4511c7 ("drm/msm/gpu: Convert the GPU show function to use the GPU state")
> > > > Cc: stable <stable@vger.kernel.org>     # 4.18
> > > > Cc: Jordan Crouse <jcrouse@codeaurora.org>
> > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > > ---
> > >
> > > Rob, Sean,
> > >
> > > Sending a reminder about this one, which is not yet in linux-next.
> > >
> > > Perhaps Daniel can pick it up otherwise?
> >
> > Another two weeks, another reminder. This one is still not in -next.
>
> Well msm is maintained in a separate tree, so the usual group maintainer
> fallback for when patches are stuck doesn't apply.

oh, sorry, this wasn't showing up in patchwork.. or rather it did but
the non-msm related series subject made me overlook it.

I've already sent a PR, but this shouldn't conflict with anything and
I think it can go in via drm-misc/fixes

Reviewed-by: Rob Clark <robdclark@gmail.com>

> Rob, Sean, time to reconsider drm-misc for msm? I think there's some more
> oddball patches that occasionally get stuck for msm ...
>
> Also +Dave.
> -Daniel
>
> >
> > Johan
> >
> > > >  drivers/gpu/drm/msm/msm_debugfs.c | 6 +-----
> > > >  1 file changed, 1 insertion(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> > > > index 6be879578140..1c74381a4fc9 100644
> > > > --- a/drivers/gpu/drm/msm/msm_debugfs.c
> > > > +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> > > > @@ -47,12 +47,8 @@ static int msm_gpu_release(struct inode *inode, struct file *file)
> > > >   struct msm_gpu_show_priv *show_priv = m->private;
> > > >   struct msm_drm_private *priv = show_priv->dev->dev_private;
> > > >   struct msm_gpu *gpu = priv->gpu;
> > > > - int ret;
> > > > -
> > > > - ret = mutex_lock_interruptible(&show_priv->dev->struct_mutex);
> > > > - if (ret)
> > > > -         return ret;
> > > >
> > > > + mutex_lock(&show_priv->dev->struct_mutex);
> > > >   gpu->funcs->gpu_state_put(show_priv->state);
> > > >   mutex_unlock(&show_priv->dev->struct_mutex);
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 1/4] drm/msm: fix memleak on release

[Sean Paul]

On Tue, Nov 12, 2019 at 08:32:07AM -0800, Rob Clark wrote:
> On Tue, Nov 12, 2019 at 6:01 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Tue, Nov 12, 2019 at 11:40:01AM +0100, Johan Hovold wrote:
> > > On Wed, Oct 30, 2019 at 11:01:46AM +0100, Johan Hovold wrote:
> > > > On Thu, Oct 10, 2019 at 03:13:30PM +0200, Johan Hovold wrote:
> > > > > If a process is interrupted while accessing the "gpu" debugfs file and
> > > > > the drm device struct_mutex is contended, release() could return early
> > > > > and fail to free related resources.
> > > > >
> > > > > Note that the return value from release() is ignored.
> > > > >
> > > > > Fixes: 4f776f4511c7 ("drm/msm/gpu: Convert the GPU show function to use the GPU state")
> > > > > Cc: stable <stable@vger.kernel.org>     # 4.18
> > > > > Cc: Jordan Crouse <jcrouse@codeaurora.org>
> > > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > > > ---
> > > >
> > > > Rob, Sean,
> > > >
> > > > Sending a reminder about this one, which is not yet in linux-next.
> > > >
> > > > Perhaps Daniel can pick it up otherwise?
> > >
> > > Another two weeks, another reminder. This one is still not in -next.
> >
> > Well msm is maintained in a separate tree, so the usual group maintainer
> > fallback for when patches are stuck doesn't apply.
> 
> oh, sorry, this wasn't showing up in patchwork.. or rather it did but
> the non-msm related series subject made me overlook it.
> 
> I've already sent a PR, but this shouldn't conflict with anything and
> I think it can go in via drm-misc/fixes
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>

Thanks for the patch, pushed to drm-misc-next-fixes

Sean

> 
> > Rob, Sean, time to reconsider drm-misc for msm? I think there's some more
> > oddball patches that occasionally get stuck for msm ...
> >
> > Also +Dave.
> > -Daniel
> >
> > >
> > > Johan
> > >
> > > > >  drivers/gpu/drm/msm/msm_debugfs.c | 6 +-----
> > > > >  1 file changed, 1 insertion(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> > > > > index 6be879578140..1c74381a4fc9 100644
> > > > > --- a/drivers/gpu/drm/msm/msm_debugfs.c
> > > > > +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> > > > > @@ -47,12 +47,8 @@ static int msm_gpu_release(struct inode *inode, struct file *file)
> > > > >   struct msm_gpu_show_priv *show_priv = m->private;
> > > > >   struct msm_drm_private *priv = show_priv->dev->dev_private;
> > > > >   struct msm_gpu *gpu = priv->gpu;
> > > > > - int ret;
> > > > > -
> > > > > - ret = mutex_lock_interruptible(&show_priv->dev->struct_mutex);
> > > > > - if (ret)
> > > > > -         return ret;
> > > > >
> > > > > + mutex_lock(&show_priv->dev->struct_mutex);
> > > > >   gpu->funcs->gpu_state_put(show_priv->state);
> > > > >   mutex_unlock(&show_priv->dev->struct_mutex);
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[PATCH 2/4] media: bdisp: fix memleak on release

[Johan Hovold]

If a process is interrupted while accessing the video device and the
device lock is contended, release() could return early and fail to free
related resources.

Note that the return value of the v4l2 release file operation is
ignored.

Fixes: 28ffeebbb7bd ("[media] bdisp: 2D blitter driver using v4l2 mem2mem framework")
Cc: stable <stable@vger.kernel.org>     # 4.2
Cc: Fabien Dessenne <fabien.dessenne@st.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/media/platform/sti/bdisp/bdisp-v4l2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
index e90f1ba30574..675b5f2b4c2e 100644
--- a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
+++ b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
@@ -651,8 +651,7 @@ static int bdisp_release(struct file *file)
 
 	dev_dbg(bdisp->dev, "%s\n", __func__);
 
-	if (mutex_lock_interruptible(&bdisp->lock))
-		return -ERESTARTSYS;
+	mutex_lock(&bdisp->lock);
 
 	v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
 
-- 
2.23.0

Re: [PATCH 2/4] media: bdisp: fix memleak on release

[Fabien DESSENNE]

Hi Johan

Thank you for the patch

BR

Fabien


On 10/10/2019 3:13 PM, Johan Hovold wrote:
> If a process is interrupted while accessing the video device and the
> device lock is contended, release() could return early and fail to free
> related resources.
>
> Note that the return value of the v4l2 release file operation is
> ignored.
>
> Fixes: 28ffeebbb7bd ("[media] bdisp: 2D blitter driver using v4l2 mem2mem framework")
> Cc: stable <stable@vger.kernel.org>     # 4.2
> Cc: Fabien Dessenne <fabien.dessenne@st.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
Reviewed-by: Fabien Dessenne <fabien.dessenne@st.com>
> ---
>   drivers/media/platform/sti/bdisp/bdisp-v4l2.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
> index e90f1ba30574..675b5f2b4c2e 100644
> --- a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
> +++ b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c
> @@ -651,8 +651,7 @@ static int bdisp_release(struct file *file)
>   
>   	dev_dbg(bdisp->dev, "%s\n", __func__);
>   
> -	if (mutex_lock_interruptible(&bdisp->lock))
> -		return -ERESTARTSYS;
> +	mutex_lock(&bdisp->lock);
>   
>   	v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
>   

[PATCH 3/4] media: radio: wl1273: fix interrupt masking on release

[Johan Hovold]

If a process is interrupted while accessing the radio device and the
core lock is contended, release() could return early and fail to update
the interrupt mask.

Note that the return value of the v4l2 release file operation is
ignored.

Fixes: 87d1a50ce451 ("[media] V4L2: WL1273 FM Radio: TI WL1273 FM radio driver")
Cc: stable <stable@vger.kernel.org>     # 2.6.38
Cc: Matti Aaltonen <matti.j.aaltonen@nokia.com>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/media/radio/radio-wl1273.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/radio/radio-wl1273.c b/drivers/media/radio/radio-wl1273.c
index 104ac41c6f96..112376873167 100644
--- a/drivers/media/radio/radio-wl1273.c
+++ b/drivers/media/radio/radio-wl1273.c
@@ -1148,8 +1148,7 @@ static int wl1273_fm_fops_release(struct file *file)
 	if (radio->rds_users > 0) {
 		radio->rds_users--;
 		if (radio->rds_users == 0) {
-			if (mutex_lock_interruptible(&core->lock))
-				return -EINTR;
+			mutex_lock(&core->lock);
 
 			radio->irq_flags &= ~WL1273_RDS_EVENT;
 
-- 
2.23.0

[PATCH 4/4] s390/zcrypt: fix memleak at release

[Johan Hovold]

If a process is interrupted while accessing the crypto device and the
global ap_perms_mutex is contented, release() could return early and
fail to free related resources.

Fixes: 00fab2350e6b ("s390/zcrypt: multiple zcrypt device nodes support")
Cc: stable <stable@vger.kernel.org>     # 4.19
Cc: Harald Freudenberger <freude@linux.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/s390/crypto/zcrypt_api.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
index 45bdb47f84c1..9157e728a362 100644
--- a/drivers/s390/crypto/zcrypt_api.c
+++ b/drivers/s390/crypto/zcrypt_api.c
@@ -522,8 +522,7 @@ static int zcrypt_release(struct inode *inode, struct file *filp)
 	if (filp->f_inode->i_cdev == &zcrypt_cdev) {
 		struct zcdn_device *zcdndev;
 
-		if (mutex_lock_interruptible(&ap_perms_mutex))
-			return -ERESTARTSYS;
+		mutex_lock(&ap_perms_mutex);
 		zcdndev = find_zcdndev_by_devt(filp->f_inode->i_rdev);
 		mutex_unlock(&ap_perms_mutex);
 		if (zcdndev) {
-- 
2.23.0

Re: [PATCH 4/4] s390/zcrypt: fix memleak at release

[Heiko Carstens]

On Thu, Oct 10, 2019 at 03:13:33PM +0200, Johan Hovold wrote:
> If a process is interrupted while accessing the crypto device and the
> global ap_perms_mutex is contented, release() could return early and
> fail to free related resources.
> 
> Fixes: 00fab2350e6b ("s390/zcrypt: multiple zcrypt device nodes support")
> Cc: stable <stable@vger.kernel.org>     # 4.19
> Cc: Harald Freudenberger <freude@linux.ibm.com>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/s390/crypto/zcrypt_api.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Applied, thanks!