* [PATCH] fbdev: put module after running driver callback
@ 2015-09-11 16:10 David Herrmann
2015-09-24 11:24 ` Tomi Valkeinen
0 siblings, 1 reply; 3+ messages in thread
From: David Herrmann @ 2015-09-11 16:10 UTC (permalink / raw
To: linux-fbdev; +Cc: Tomi Valkeinen, Jean-Christophe Plagniol-Villard, dri-devel
Currently, for each open() on an fbdev device, we pin the underlying
fbdev device and driver module. On close(), we release both. This
guarantees that the fbdev object stays around until the last FD is
released (even though it might be unregistered already).
However, currently we call module_put() *before* calling put_fb_info().
This has the side-effect that the driver module might be unloaded before
put_fb_info() calls into fbinfo->fbops->fb_destroy().
Fix this by keeping the module pinned until after we release our fbdev
reference. Note that register_framebuffer() and unregister_framebuffer()
are special as we require the driver to unregister device before
unloading. Hence, they don't need to pin the module. However, all open
handlers *have to*.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/video/fbdev/core/fbmem.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 0705d88..4e78731 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1482,13 +1482,16 @@ __acquires(&info->lock)
__releases(&info->lock)
{
struct fb_info * const info = file->private_data;
+ struct module *owner;
mutex_lock(&info->lock);
if (info->fbops->fb_release)
info->fbops->fb_release(info,1);
- module_put(info->fbops->owner);
+ owner = info->fbops->owner;
mutex_unlock(&info->lock);
+
put_fb_info(info);
+ module_put(owner);
return 0;
}
--
2.5.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] fbdev: put module after running driver callback
2015-09-11 16:10 [PATCH] fbdev: put module after running driver callback David Herrmann
@ 2015-09-24 11:24 ` Tomi Valkeinen
2015-09-25 17:58 ` David Herrmann
0 siblings, 1 reply; 3+ messages in thread
From: Tomi Valkeinen @ 2015-09-24 11:24 UTC (permalink / raw
To: David Herrmann, linux-fbdev; +Cc: Jean-Christophe Plagniol-Villard, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 2000 bytes --]
On 11/09/15 19:10, David Herrmann wrote:
> Currently, for each open() on an fbdev device, we pin the underlying
> fbdev device and driver module. On close(), we release both. This
> guarantees that the fbdev object stays around until the last FD is
> released (even though it might be unregistered already).
>
> However, currently we call module_put() *before* calling put_fb_info().
> This has the side-effect that the driver module might be unloaded before
> put_fb_info() calls into fbinfo->fbops->fb_destroy().
>
> Fix this by keeping the module pinned until after we release our fbdev
> reference. Note that register_framebuffer() and unregister_framebuffer()
> are special as we require the driver to unregister device before
> unloading. Hence, they don't need to pin the module. However, all open
> handlers *have to*.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> drivers/video/fbdev/core/fbmem.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 0705d88..4e78731 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1482,13 +1482,16 @@ __acquires(&info->lock)
> __releases(&info->lock)
> {
> struct fb_info * const info = file->private_data;
> + struct module *owner;
>
> mutex_lock(&info->lock);
> if (info->fbops->fb_release)
> info->fbops->fb_release(info,1);
> - module_put(info->fbops->owner);
> + owner = info->fbops->owner;
> mutex_unlock(&info->lock);
> +
> put_fb_info(info);
> + module_put(owner);
> return 0;
> }
Looking at fb_open(), in error case it calls module_put() followed by
put_fb_info(). Is that broken also?
Have you hit this bug, or did you just find it by looking at the code?
In other words, is this for 4.3 fixes, or 4.4. I guess the user needs to
unload the module just at the right time to trigger this bug.
Tomi
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] fbdev: put module after running driver callback
2015-09-24 11:24 ` Tomi Valkeinen
@ 2015-09-25 17:58 ` David Herrmann
0 siblings, 0 replies; 3+ messages in thread
From: David Herrmann @ 2015-09-25 17:58 UTC (permalink / raw
To: Tomi Valkeinen
Cc: linux-fbdev@vger.kernel.org, Jean-Christophe Plagniol-Villard,
dri-devel@lists.freedesktop.org
Hi
On Thu, Sep 24, 2015 at 1:24 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> On 11/09/15 19:10, David Herrmann wrote:
>> Currently, for each open() on an fbdev device, we pin the underlying
>> fbdev device and driver module. On close(), we release both. This
>> guarantees that the fbdev object stays around until the last FD is
>> released (even though it might be unregistered already).
>>
>> However, currently we call module_put() *before* calling put_fb_info().
>> This has the side-effect that the driver module might be unloaded before
>> put_fb_info() calls into fbinfo->fbops->fb_destroy().
>>
>> Fix this by keeping the module pinned until after we release our fbdev
>> reference. Note that register_framebuffer() and unregister_framebuffer()
>> are special as we require the driver to unregister device before
>> unloading. Hence, they don't need to pin the module. However, all open
>> handlers *have to*.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> drivers/video/fbdev/core/fbmem.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index 0705d88..4e78731 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -1482,13 +1482,16 @@ __acquires(&info->lock)
>> __releases(&info->lock)
>> {
>> struct fb_info * const info = file->private_data;
>> + struct module *owner;
>>
>> mutex_lock(&info->lock);
>> if (info->fbops->fb_release)
>> info->fbops->fb_release(info,1);
>> - module_put(info->fbops->owner);
>> + owner = info->fbops->owner;
>> mutex_unlock(&info->lock);
>> +
>> put_fb_info(info);
>> + module_put(owner);
>> return 0;
>> }
>
> Looking at fb_open(), in error case it calls module_put() followed by
> put_fb_info(). Is that broken also?
Indeed, same issue here. I will send v2 which fixes both.
> Have you hit this bug, or did you just find it by looking at the code?
> In other words, is this for 4.3 fixes, or 4.4. I guess the user needs to
> unload the module just at the right time to trigger this bug.
Theoretical issue. It's almost impossible to trigger, as module
unloading takes ages compared to the time required to release the
fbinfo.
Thanks
David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-09-25 17:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-11 16:10 [PATCH] fbdev: put module after running driver callback David Herrmann
2015-09-24 11:24 ` Tomi Valkeinen
2015-09-25 17:58 ` David Herrmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).