| Message ID | 20191010131333.23635-1-johan@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | treewide: fix interrupted release | expand |
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 >
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
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
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
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
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