2011-06-09 03:06:33

by Fengguang Wu

Subject: [RFC] 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?id=18912

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

--- linux-next.orig/drivers/video/fbmem.c 2011-06-09 10:36:06.000000000 +0800
+++ linux-next/drivers/video/fbmem.c 2011-06-09 10:39:30.000000000 +0800
@@ -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)

2011-06-09 06:10:25

by Bruno Prémont

Subject: Re: [RFC] fbmem: reset file->private_data on failed fb_open()

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?id=18912

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.

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

> ---
> drivers/video/fbmem.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> --- linux-next.orig/drivers/video/fbmem.c 2011-06-09 10:36:06.000000000 +0800
> +++ linux-next/drivers/video/fbmem.c 2011-06-09 10:39:30.000000000 +0800
> @@ -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)

2011-06-09 06:38:16

by Fengguang Wu

Subject: Re: [RFC] fbmem: reset file->private_data on failed fb_open()

[Add CC to Thomas Creutz]

On Thu, Jun 09, 2011 at 02:10:19PM +0800, Bruno Prémont wrote:
> 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?id=18912
>
> 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?

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

> > ---
> > drivers/video/fbmem.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > --- linux-next.orig/drivers/video/fbmem.c 2011-06-09 10:36:06.000000000 +0800
> > +++ linux-next/drivers/video/fbmem.c 2011-06-09 10:39:30.000000000 +0800
> > @@ -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)