fbmem: reset file->private_data on failed fb_open()
RFC 1/1 fbmem: reset file->private_data on failed fb_open()
I wrote this when looking at NULL dereference bug
https://bugzilla.kernel.org/show_bug.cgi?id912
Will it help by clearing private_data? I have no idea at all, because for regular files, ->release won't be called on failed ->open. Just in case there are some exceptions in fbmem...
--- drivers/video/fbmem.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/video/fbmem.c +3 -1
@@ -1424,26 +1424,28 @@ __releases(&info->lock) file->private_data = info;if (info->fbops->fb_open) { res = info->fbops->fb_open (info,1 );if (res)module_put (info->fbops->owner); }#ifdef CONFIG_FB_DEFERRED_IO if (info->fbdefio)fb_deferred_io_open (info, inode, file);#endif out:mutex_unlock (&info->lock); -if (res) +if (res) { + file->private_data =NULL ;put_fb_info (info); + }return res; }static int fb_release (struct inode *inode,struct file *file) __acquires(&info->lock) __releases(&info->lock) {struct fb_info *const info = file->private_data;mutex_lock (&info->lock);if (info->fbops->fb_release)
Quoted 2 lines · On Thu, 9 Jun 2011 11:06:28 Wu Fengguang <[email protected]> wrote:
I wrote this when looking at NULL dereference bug
https://bugzilla.kernel.org/show_bug.cgi?id912
The trace over there rather looks like closing of a framebuffer that has been replaced (and destroyed) during boot sequence. This action happens when switching from VESA to KMS from initrd or early userspace boot sequence where plymouthd had already opened vesafb.
That should hopefully be fixed by refcounting FBs as done during 2.6.39-rc7.
Quoted 3 lines
Will it help by clearing
private_data? I have no idea at all, because for regular files, ->release won't be called on failed ->open. Just in case there are some exceptions in fbmem...
fb devices files should be just as regular as any other device files...
Quoted 36 lines
--- drivers/video/fbmem.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/video/fbmem.c +3
-1@@ -1424,26 +1424,28 @@ __releases(&info->lock) file->private_data = info;if (info->fbops->fb_open) { res = info->fbops->fb_open (info,1 );if (res)module_put (info->fbops->owner); }#ifdef CONFIG_FB_DEFERRED_IO if (info->fbdefio)fb_deferred_io_open (info, inode, file);#endif out:mutex_unlock (&info->lock); -if (res) +if (res) { + file->private_data =NULL ;put_fb_info (info); + }return res; }static int fb_release (struct inode *inode,struct file *file) __acquires(&info->lock) __releases(&info->lock) {struct fb_info *const info = file->private_data;mutex_lock (&info->lock);if (info->fbops->fb_release)
[Add CC to Thomas Creutz]
Quoted 4 lines · Bruno Prémont
Quoted 2 lines · On Thu, 9 Jun 2011 11:06:28 Wu Fengguang <[email protected]> wrote:
I wrote this when looking at NULL dereference bug
https://bugzilla.kernel.org/show_bug.cgi?id912
Quoted 8 lines
The trace over there rather looks like closing of a framebuffer that has been replaced (and destroyed) during boot sequence. This action happens when switching from VESA to KMS from initrd or early userspace boot sequence where plymouthd had already opened vesafb.
That should hopefully be fixed by refcounting FBs as done during 2.6.39-rc7.
That's great! Then hopefully the bug can be closed?
Quoted 5 lines
Quoted 3 lines
Will it help by clearing
private_data? I have no idea at all, because for regular files, ->release won't be called on failed ->open. Just in case there are some exceptions in fbmem...fb devices files should be just as regular as any other device files...
Good, thanks for the confirmation.
Thanks,
Fengguang
Quoted 36 lines
Quoted 36 lines
--- drivers/video/fbmem.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/video/fbmem.c +3
-1@@ -1424,26 +1424,28 @@ __releases(&info->lock) file->private_data = info;if (info->fbops->fb_open) { res = info->fbops->fb_open (info,1 );if (res)module_put (info->fbops->owner); }#ifdef CONFIG_FB_DEFERRED_IO if (info->fbdefio)fb_deferred_io_open (info, inode, file);#endif out:mutex_unlock (&info->lock); -if (res) +if (res) { + file->private_data =NULL ;put_fb_info (info); + }return res; }static int fb_release (struct inode *inode,struct file *file) __acquires(&info->lock) __releases(&info->lock) {struct fb_info *const info = file->private_data;mutex_lock (&info->lock);if (info->fbops->fb_release)