treewide: fix interrupted release

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 <[email protected]> # 4.18

Cc Jordan Crouse <[email protected]>

Cc Rob Clark <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

 drivers/gpu/drm/msm/msm_debugfs.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/gpu/drm/msm/msm_debugfs.c +0 -0

index 6be879578140..1c74381a4fc9 100644
@@ -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;

Quoted 4 lines · 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.

Quoted 8 lines

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 <[email protected]> # 4.18

Cc Jordan Crouse <[email protected]>

Cc Rob Clark <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Rob, Sean,

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

Perhaps Daniel can pick it up otherwise?

Quoted 20 lines

 drivers/gpu/drm/msm/msm_debugfs.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/gpu/drm/msm/msm_debugfs.c +1 -5

index 6be879578140..1c74381a4fc9 100644
@@ -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);

Quoted 14 lines · Johan Hovold

Quoted 12 lines · 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 <[email protected]> # 4.18

Cc Jordan Crouse <[email protected]>

Cc Rob Clark <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Quoted 5 lines

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

Quoted 20 lines

Quoted 20 lines

 drivers/gpu/drm/msm/msm_debugfs.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/gpu/drm/msm/msm_debugfs.c +1 -5

index 6be879578140..1c74381a4fc9 100644
@@ -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);

Quoted 17 lines · Johan Hovold

Quoted 16 lines · Johan Hovold

Quoted 4 lines · 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.

Quoted 8 lines

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 <[email protected]> # 4.18

Cc Jordan Crouse <[email protected]>

Cc Rob Clark <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Rob, Sean,

Quoted 5 lines

Quoted 3 lines

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

Quoted 20 lines

Johan

Quoted 17 lines

Quoted 17 lines

 drivers/gpu/drm/msm/msm_debugfs.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/gpu/drm/msm/msm_debugfs.c +0 -5

index 6be879578140..1c74381a4fc9 100644
@@ -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;
 

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/msm/msm_debugfs.c static int msm_gpu_release(struct inode *inode, struct file *file) · line 53

Quoted 3 lines

drivers/gpu/drm/msm/msm_debugfs.c +1 -0

+ 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

Quoted 19 lines · Daniel Vetter

Quoted 17 lines · Johan Hovold

Quoted 16 lines · Johan Hovold

Quoted 4 lines · 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.

Quoted 8 lines

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 <[email protected]> # 4.18

Cc Jordan Crouse <[email protected]>

Cc Rob Clark <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Rob, Sean,

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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 <[email protected]>

Quoted 35 lines

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

Quoted 20 lines

Johan

Quoted 17 lines

Quoted 17 lines

 drivers/gpu/drm/msm/msm_debugfs.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/gpu/drm/msm/msm_debugfs.c +0 -5

index 6be879578140..1c74381a4fc9 100644
@@ -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;
 

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/msm/msm_debugfs.c static int msm_gpu_release(struct inode *inode, struct file *file) · line 53

Quoted 3 lines

drivers/gpu/drm/msm/msm_debugfs.c +1 -0

+ 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

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 <[email protected]> # 4.2

Cc Fabien Dessenne <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
  drivers/media/platform/sti/bdisp/bdisp-v4l2.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

drivers/media/platform/sti/bdisp/bdisp-v4l2.c +1 -2

index e90f1ba30574..675b5f2b4c2e 100644
@@ -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);
 

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 <[email protected]> # 4.19

Cc Harald Freudenberger <[email protected]>

Cc Martin Schwidefsky <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/s390/crypto/zcrypt_api.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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(-)

Signature · 1 line

2.23.0

Quoted 5 lines · 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.

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?

Quoted 6 lines · Daniel Vetter

Quoted 5 lines · 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.

Quoted 4 lines

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.

Quoted 13 lines · Johan Hovold

Quoted 11 lines · Daniel Vetter

Quoted 5 lines · 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.

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?

Quoted 3 lines

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 ...

Quoted 2 lines

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.

Quoted 14 lines · Daniel Vetter

Quoted 13 lines · Johan Hovold

Quoted 11 lines · Daniel Vetter

Quoted 5 lines · 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.

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?

Quoted 5 lines

Quoted 3 lines

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).

Quoted 2 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 11 lines · Johan Hovold

Quoted 1 line · Daniel Vetter

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).

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 14 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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.