[PATCH 0/4] treewide: fix interrupted release

[PATCH 0/4] treewide: fix interrupted release

[Johan Hovold]

Two old USB drivers had a bug in them which could lead to memory leaks
if an interrupted process raced with a disconnect event.

Turns out we had a few more driver in other subsystems with the same
kind of bug in them.

Note that all but the s390 patch have only been compile tested, while
the s390 one has not even been built.

Johan


Johan Hovold (4):
  drm/msm: fix memleak on release
  media: bdisp: fix memleak on release
  media: radio: wl1273: fix interrupt masking on release
  s390/zcrypt: fix memleak at release

 drivers/gpu/drm/msm/msm_debugfs.c             | 6 +-----
 drivers/media/platform/sti/bdisp/bdisp-v4l2.c | 3 +--
 drivers/media/radio/radio-wl1273.c            | 3 +--
 drivers/s390/crypto/zcrypt_api.c              | 3 +--
 4 files changed, 4 insertions(+), 11 deletions(-)

-- 
2.23.0

[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!

Re: [PATCH 0/4] treewide: fix interrupted release

[Daniel Vetter]

On Thu, Oct 10, 2019 at 03:13:29PM +0200, Johan Hovold wrote:
> Two old USB drivers had a bug in them which could lead to memory leaks
> if an interrupted process raced with a disconnect event.
> 
> Turns out we had a few more driver in other subsystems with the same
> kind of bug in them.
> 
> Note that all but the s390 patch have only been compile tested, while
> the s390 one has not even been built.

Random funny idea: Could we do some debug annotations (akin to
might_sleep) that splats when you might_sleep_interruptible somewhere
where interruptible sleeps are generally a bad idea? Like in
fops->release?

Something like non_block_start/end that I've recently done, but for
interruptible sleeps only? Would need might_sleep_interruptibly()
annotations and non_interruptly_sleep_start/end annotations.
-Daniel

> 
> Johan
> 
> 
> Johan Hovold (4):
>   drm/msm: fix memleak on release
>   media: bdisp: fix memleak on release
>   media: radio: wl1273: fix interrupt masking on release
>   s390/zcrypt: fix memleak at release
> 
>  drivers/gpu/drm/msm/msm_debugfs.c             | 6 +-----
>  drivers/media/platform/sti/bdisp/bdisp-v4l2.c | 3 +--
>  drivers/media/radio/radio-wl1273.c            | 3 +--
>  drivers/s390/crypto/zcrypt_api.c              | 3 +--
>  4 files changed, 4 insertions(+), 11 deletions(-)
> 
> -- 
> 2.23.0
> 

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

Re: [PATCH 0/4] treewide: fix interrupted release

[Johan Hovold]

On Thu, Oct 10, 2019 at 03:50:43PM +0200, Daniel Vetter wrote:
> On Thu, Oct 10, 2019 at 03:13:29PM +0200, Johan Hovold wrote:
> > Two old USB drivers had a bug in them which could lead to memory leaks
> > if an interrupted process raced with a disconnect event.
> > 
> > Turns out we had a few more driver in other subsystems with the same
> > kind of bug in them.

> Random funny idea: Could we do some debug annotations (akin to
> might_sleep) that splats when you might_sleep_interruptible somewhere
> where interruptible sleeps are generally a bad idea? Like in
> fops->release?

There's nothing wrong with interruptible sleep in fops->release per se,
it's just that drivers cannot return -ERESTARTSYS and friends and expect
to be called again later.

The return value from release() is ignored by vfs, and adding a splat in
__fput() to catch these buggy drivers might be overkill.

Johan

Re: [PATCH 0/4] treewide: fix interrupted release

[Daniel Vetter]

On Fri, Oct 11, 2019 at 11:36:33AM +0200, Johan Hovold wrote:
> On Thu, Oct 10, 2019 at 03:50:43PM +0200, Daniel Vetter wrote:
> > On Thu, Oct 10, 2019 at 03:13:29PM +0200, Johan Hovold wrote:
> > > Two old USB drivers had a bug in them which could lead to memory leaks
> > > if an interrupted process raced with a disconnect event.
> > > 
> > > Turns out we had a few more driver in other subsystems with the same
> > > kind of bug in them.
> 
> > Random funny idea: Could we do some debug annotations (akin to
> > might_sleep) that splats when you might_sleep_interruptible somewhere
> > where interruptible sleeps are generally a bad idea? Like in
> > fops->release?
> 
> There's nothing wrong with interruptible sleep in fops->release per se,
> it's just that drivers cannot return -ERESTARTSYS and friends and expect
> to be called again later.

Do you have a legit usecase for interruptible sleeps in fops->release?

I'm not even sure killable is legit in there, since it's an fd, not a
process context ...

> The return value from release() is ignored by vfs, and adding a splat in
> __fput() to catch these buggy drivers might be overkill.

Ime once you have a handful of instances of a broken pattern, creating a
check for it (under a debug option only ofc) is very much justified.
Otherwise they just come back to life like the undead, all the time. And
there's a _lot_ of fops->release callbacks in the kernel.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/4] treewide: fix interrupted release

[Johan Hovold]

On Mon, Oct 14, 2019 at 10:48:47AM +0200, Daniel Vetter wrote:
> On Fri, Oct 11, 2019 at 11:36:33AM +0200, Johan Hovold wrote:
> > On Thu, Oct 10, 2019 at 03:50:43PM +0200, Daniel Vetter wrote:
> > > On Thu, Oct 10, 2019 at 03:13:29PM +0200, Johan Hovold wrote:
> > > > Two old USB drivers had a bug in them which could lead to memory leaks
> > > > if an interrupted process raced with a disconnect event.
> > > > 
> > > > Turns out we had a few more driver in other subsystems with the same
> > > > kind of bug in them.
> > 
> > > Random funny idea: Could we do some debug annotations (akin to
> > > might_sleep) that splats when you might_sleep_interruptible somewhere
> > > where interruptible sleeps are generally a bad idea? Like in
> > > fops->release?
> > 
> > There's nothing wrong with interruptible sleep in fops->release per se,
> > it's just that drivers cannot return -ERESTARTSYS and friends and expect
> > to be called again later.
> 
> Do you have a legit usecase for interruptible sleeps in fops->release?

The tty layer depends on this for example when waiting for buffered
writes to complete (something which may never happen when using flow
control).

> I'm not even sure killable is legit in there, since it's an fd, not a
> process context ...

It will be run in process context in many cases, and for ttys we're good
AFAICT.

> > The return value from release() is ignored by vfs, and adding a splat in
> > __fput() to catch these buggy drivers might be overkill.
> 
> Ime once you have a handful of instances of a broken pattern, creating a
> check for it (under a debug option only ofc) is very much justified.
> Otherwise they just come back to life like the undead, all the time. And
> there's a _lot_ of fops->release callbacks in the kernel.

Yeah, you have a point.

But take tty again as an example, the close tty operation called from
release() is declared void so there's no propagated return value for vfs
to check.

It may even be better to fix up the 100 or so callbacks potentially
returning non-zero and make fops->release void so that the compiler
would help us catch any future bugs and also serve as a hint for
developers that returning errnos from fops->release is probably not
what you want to do.

But that's a lot of churn of course.

Johan

Re: [PATCH 0/4] treewide: fix interrupted release

[Daniel Vetter]

On Mon, Oct 14, 2019 at 06:13:26PM +0200, Johan Hovold wrote:
> On Mon, Oct 14, 2019 at 10:48:47AM +0200, Daniel Vetter wrote:
> > On Fri, Oct 11, 2019 at 11:36:33AM +0200, Johan Hovold wrote:
> > > On Thu, Oct 10, 2019 at 03:50:43PM +0200, Daniel Vetter wrote:
> > > > On Thu, Oct 10, 2019 at 03:13:29PM +0200, Johan Hovold wrote:
> > > > > Two old USB drivers had a bug in them which could lead to memory leaks
> > > > > if an interrupted process raced with a disconnect event.
> > > > > 
> > > > > Turns out we had a few more driver in other subsystems with the same
> > > > > kind of bug in them.
> > > 
> > > > Random funny idea: Could we do some debug annotations (akin to
> > > > might_sleep) that splats when you might_sleep_interruptible somewhere
> > > > where interruptible sleeps are generally a bad idea? Like in
> > > > fops->release?
> > > 
> > > There's nothing wrong with interruptible sleep in fops->release per se,
> > > it's just that drivers cannot return -ERESTARTSYS and friends and expect
> > > to be called again later.
> > 
> > Do you have a legit usecase for interruptible sleeps in fops->release?
> 
> The tty layer depends on this for example when waiting for buffered
> writes to complete (something which may never happen when using flow
> control).
> 
> > I'm not even sure killable is legit in there, since it's an fd, not a
> > process context ...
> 
> It will be run in process context in many cases, and for ttys we're good
> AFAICT.

Huh, read it a bit, all the ->shutdown callbacks have void return type.
But there's indeed interruptible sleeps in there. Doesn't this break
userspace that expects that a close() actually flushes the tty?

Imo if you're ->release callbacks feels like it should do a wait to
guaranteed something userspace expects, then doing a
wait_interruptible/killable feels like a bug. Or alternatively, the wait
isn't really needed in the first place.

> > > The return value from release() is ignored by vfs, and adding a splat in
> > > __fput() to catch these buggy drivers might be overkill.
> > 
> > Ime once you have a handful of instances of a broken pattern, creating a
> > check for it (under a debug option only ofc) is very much justified.
> > Otherwise they just come back to life like the undead, all the time. And
> > there's a _lot_ of fops->release callbacks in the kernel.
> 
> Yeah, you have a point.
> 
> But take tty again as an example, the close tty operation called from
> release() is declared void so there's no propagated return value for vfs
> to check.
> 
> It may even be better to fix up the 100 or so callbacks potentially
> returning non-zero and make fops->release void so that the compiler
> would help us catch any future bugs and also serve as a hint for
> developers that returning errnos from fops->release is probably not
> what you want to do.
> 
> But that's a lot of churn of course.

Hm indeed ->release has int as return type. I guess that's needed for
file I/O errno and similar stuff ...

Still void return value doesn't catch funny stuff like doing interruptible
waits and occasionally failing if you have a process that likes to use
signals and also uses some library somewhere to do something. In graphics
we have that, with Xorg loving signals for various things.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/4] treewide: fix interrupted release

[Johan Hovold]

On Tue, Oct 15, 2019 at 04:07:26PM +0200, Daniel Vetter wrote:
> On Mon, Oct 14, 2019 at 06:13:26PM +0200, Johan Hovold wrote:
> > On Mon, Oct 14, 2019 at 10:48:47AM +0200, Daniel Vetter wrote:

> > > Do you have a legit usecase for interruptible sleeps in fops->release?
> > 
> > The tty layer depends on this for example when waiting for buffered
> > writes to complete (something which may never happen when using flow
> > control).
> > 
> > > I'm not even sure killable is legit in there, since it's an fd, not a
> > > process context ...
> > 
> > It will be run in process context in many cases, and for ttys we're good
> > AFAICT.
> 
> Huh, read it a bit, all the ->shutdown callbacks have void return type.
> But there's indeed interruptible sleeps in there. Doesn't this break
> userspace that expects that a close() actually flushes the tty?

This behaviour has been there since "forever" so the problem is rather
the other way round; changing it now might break user space.

> Imo if you're ->release callbacks feels like it should do a wait to
> guaranteed something userspace expects, then doing a
> wait_interruptible/killable feels like a bug. Or alternatively, the wait
> isn't really needed in the first place.

Posix says that the final tty close should cause any output to be sent.
And as mentioned before, due to flow control this may never finish. So
for usability reasons, you want to be able to interrupt that final
close, while removing the flush completely would break applications
currently expecting output to be flushed.

Also note that we have an interface for controlling how long to wait for
data to be sent (typically 30 s by default, but can be set to wait
forever).

> > > > The return value from release() is ignored by vfs, and adding a splat in
> > > > __fput() to catch these buggy drivers might be overkill.
> > > 
> > > Ime once you have a handful of instances of a broken pattern, creating a
> > > check for it (under a debug option only ofc) is very much justified.
> > > Otherwise they just come back to life like the undead, all the time. And
> > > there's a _lot_ of fops->release callbacks in the kernel.
> > 
> > Yeah, you have a point.
> > 
> > But take tty again as an example, the close tty operation called from
> > release() is declared void so there's no propagated return value for vfs
> > to check.
> > 
> > It may even be better to fix up the 100 or so callbacks potentially
> > returning non-zero and make fops->release void so that the compiler
> > would help us catch any future bugs and also serve as a hint for
> > developers that returning errnos from fops->release is probably not
> > what you want to do.
> > 
> > But that's a lot of churn of course.
> 
> Hm indeed ->release has int as return type. I guess that's needed for
> file I/O errno and similar stuff ...
> 
> Still void return value doesn't catch funny stuff like doing interruptible
> waits and occasionally failing if you have a process that likes to use
> signals and also uses some library somewhere to do something. In graphics
> we have that, with Xorg loving signals for various things.

Right, but since there arguable are legitimate uses for interruptible
sleep at release(), I don't see how we can catch that at runtime.

Johan