2011-05-05 17:42:21

by Tim Gardner

Subject: [PATCH 0/1] fbcon -- fix race between open and removal of framebuffers

I'm sending this out for Andy since he is a bit busy this week and has been
gone for the past several weeks. Its been on our todo list to get this
upstreamed, but release pressures have intervened. Mea culpa. I've also had
a couple of requests recently which have served to remind me to get off my butt.

https://lists.ubuntu.com/archives/kernel-team/2011-May/015544.html

rtg

2011-05-05 17:42:22

by Tim Gardner

Subject: [PATCH] fbcon -- fix race between open and removal of framebuffers

From: Andy Whitcroft <[email protected]>

Currently there is no locking for updates to the registered_fb list.
This allows an open through /dev/fbN to pick up a registered framebuffer
pointer in parallel with it being released, as happens when a conflicting
framebuffer is ejected or on module unload. There is also no reference
counting on the framebuffer descriptor which is referenced from all open
files, leading to references to released or reused memory to persist on
these open files.

This patch adds a reference count to the framebuffer descriptor to prevent
it from being released until after all pending opens are closed. This
allows the pending opens to detect the closed status and unmap themselves.
It also adds locking to the framebuffer lookup path, locking it against
the removal path such that it is possible to atomically lookup and take a
reference to the descriptor. It also adds locking to the read and write
paths which currently could access the framebuffer descriptor after it
has been freed. Finally it moves the device to FBINFO_STATE_REMOVED to
indicate that all access should be errored for this device.

Signed-off-by: Andy Whitcroft <[email protected]>
Acked-by: Stefan Bader <[email protected]>
Signed-off-by: Leann Ogasawara <[email protected]>
Signed-off-by: Tim Gardner <[email protected]>
---
drivers/video/fbmem.c | 132 ++++++++++++++++++++++++++++++++++++++-----------
include/linux/fb.h | 2 +
2 files changed, 105 insertions(+), 29 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index e0c2284..c8562c1 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -42,6 +42,8 @@

#define FBPIXMAPSIZE (1024 * 8)

+/* Protects the registered framebuffer list and count. */
+static DEFINE_SPINLOCK(registered_lock);
struct fb_info *registered_fb[FB_MAX] __read_mostly;
int num_registered_fb __read_mostly;

@@ -694,9 +696,7 @@ static ssize_t
fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct inode *inode = file->f_path.dentry->d_inode;
- int fbidx = iminor(inode);
- struct fb_info *info = registered_fb[fbidx];
+ struct fb_info *info = file->private_data;
u8 *buffer, *dst;
u8 __iomem *src;
int c, cnt = 0, err = 0;
@@ -705,19 +705,28 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
if (!info || ! info->screen_base)
return -ENODEV;

- if (info->state != FBINFO_STATE_RUNNING)
- return -EPERM;
+ if (!lock_fb_info(info))
+ return -ENODEV;
+
+ if (info->state != FBINFO_STATE_RUNNING) {
+ err = -EPERM;
+ goto out_fb_info;
+ }

- if (info->fbops->fb_read)
- return info->fbops->fb_read(info, buf, count, ppos);
+ if (info->fbops->fb_read) {
+ err = info->fbops->fb_read(info, buf, count, ppos);
+ goto out_fb_info;
+ }

total_size = info->screen_size;

if (total_size == 0)
total_size = info->fix.smem_len;

- if (p >= total_size)
- return 0;
+ if (p >= total_size) {
+ err = 0;
+ goto out_fb_info;
+ }

if (count >= total_size)
count = total_size;
@@ -727,8 +736,10 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)

buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
GFP_KERNEL);
- if (!buffer)
- return -ENOMEM;
+ if (!buffer) {
+ err = -ENOMEM;
+ goto out_fb_info;
+ }

src = (u8 __iomem *) (info->screen_base + p);

@@ -751,19 +762,21 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
cnt += c;
count -= c;
}
+ if (!err)
+ err = cnt;

kfree(buffer);
+out_fb_info:
+ unlock_fb_info(info);

- return (err) ? err : cnt;
+ return err;
}

static ssize_t
fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct inode *inode = file->f_path.dentry->d_inode;
- int fbidx = iminor(inode);
- struct fb_info *info = registered_fb[fbidx];
+ struct fb_info *info = file->private_data;
u8 *buffer, *src;
u8 __iomem *dst;
int c, cnt = 0, err = 0;
@@ -772,8 +785,13 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
if (!info || !info->screen_base)
return -ENODEV;

- if (info->state != FBINFO_STATE_RUNNING)
- return -EPERM;
+ if (!lock_fb_info(info))
+ return -ENODEV;
+
+ if (info->state != FBINFO_STATE_RUNNING) {
+ err = -EPERM;
+ goto out_fb_info;
+ }

if (info->fbops->fb_write)
return info->fbops->fb_write(info, buf, count, ppos);
@@ -783,8 +801,10 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
if (total_size == 0)
total_size = info->fix.smem_len;

- if (p > total_size)
- return -EFBIG;
+ if (p > total_size) {
+ err = -EFBIG;
+ goto out_fb_info;
+ }

if (count > total_size) {
err = -EFBIG;
@@ -800,8 +820,10 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)

buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
GFP_KERNEL);
- if (!buffer)
- return -ENOMEM;
+ if (!buffer) {
+ err = -ENOMEM;
+ goto out_fb_info;
+ }

dst = (u8 __iomem *) (info->screen_base + p);

@@ -825,10 +847,14 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
cnt += c;
count -= c;
}
+ if (cnt)
+ err = cnt;

kfree(buffer);
+out_fb_info:
+ unlock_fb_info(info);

- return (cnt) ? cnt : err;
+ return err;
}

int
@@ -1303,8 +1329,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
static int
fb_mmap(struct file *file, struct vm_area_struct * vma)
{
- int fbidx = iminor(file->f_path.dentry->d_inode);
- struct fb_info *info = registered_fb[fbidx];
+ struct fb_info * const info = file->private_data;
struct fb_ops *fb = info->fbops;
unsigned long off;
unsigned long start;
@@ -1316,6 +1341,11 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
if (!fb)
return -ENODEV;
mutex_lock(&info->mm_lock);
+ if (info->state == FBINFO_STATE_REMOVED) {
+ mutex_unlock(&info->mm_lock);
+ return -ENODEV;
+ }
+
if (fb->fb_mmap) {
int res;
res = fb->fb_mmap(info, vma);
@@ -1352,6 +1382,34 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
return 0;
}

+static struct fb_info *get_framebuffer_info(int idx)
+__acquires(&registered_lock)
+__releases(&registered_lock)
+{
+ struct fb_info *fb_info;
+
+ spin_lock(&registered_lock);
+ fb_info = registered_fb[idx];
+ fb_info->ref_count++;
+ spin_unlock(&registered_lock);
+
+ return fb_info;
+}
+
+static void put_framebuffer_info(struct fb_info *fb_info)
+__acquires(&registered_lock)
+__releases(&registered_lock)
+{
+ int keep;
+
+ spin_lock(&registered_lock);
+ keep = --fb_info->ref_count;
+ spin_unlock(&registered_lock);
+
+ if (!keep && fb_info->fbops->fb_destroy)
+ fb_info->fbops->fb_destroy(fb_info);
+}
+
static int
fb_open(struct inode *inode, struct file *file)
__acquires(&info->lock)
@@ -1363,13 +1421,17 @@ __releases(&info->lock)

if (fbidx >= FB_MAX)
return -ENODEV;
- info = registered_fb[fbidx];
+ info = get_framebuffer_info(fbidx);
if (!info)
request_module("fb%d", fbidx);
- info = registered_fb[fbidx];
+ info = get_framebuffer_info(fbidx);
if (!info)
return -ENODEV;
mutex_lock(&info->lock);
+ if (info->state == FBINFO_STATE_REMOVED) {
+ res = -ENODEV;
+ goto out;
+ }
if (!try_module_get(info->fbops->owner)) {
res = -ENODEV;
goto out;
@@ -1386,6 +1448,8 @@ __releases(&info->lock)
#endif
out:
mutex_unlock(&info->lock);
+ if (res)
+ put_framebuffer_info(info);
return res;
}

@@ -1401,6 +1465,7 @@ __releases(&info->lock)
info->fbops->fb_release(info,1);
module_put(info->fbops->owner);
mutex_unlock(&info->lock);
+ put_framebuffer_info(info);
return 0;
}

@@ -1549,6 +1614,7 @@ register_framebuffer(struct fb_info *fb_info)
fb_info->node = i;
mutex_init(&fb_info->lock);
mutex_init(&fb_info->mm_lock);
+ fb_info->ref_count = 1;

fb_info->dev = device_create(fb_class, fb_info->device,
MKDEV(FB_MAJOR, i), NULL, "fb%d", i);
@@ -1592,7 +1658,6 @@ register_framebuffer(struct fb_info *fb_info)
return 0;
}

-
/**
* unregister_framebuffer - releases a frame buffer device
* @fb_info: frame buffer info structure
@@ -1627,6 +1692,16 @@ unregister_framebuffer(struct fb_info *fb_info)
return -ENODEV;
event.info = fb_info;
ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
+ if (!ret) {
+ mutex_lock(&fb_info->mm_lock);
+ /*
+ * We must prevent any operations for this transition, we
+ * already have info->lock so grab the info->mm_lock to hold
+ * the remainder.
+ */
+ fb_info->state = FBINFO_STATE_REMOVED;
+ mutex_unlock(&fb_info->mm_lock);
+ }
unlock_fb_info(fb_info);

if (ret) {
@@ -1646,8 +1721,7 @@ unregister_framebuffer(struct fb_info *fb_info)
fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);

/* this may free fb info */
- if (fb_info->fbops->fb_destroy)
- fb_info->fbops->fb_destroy(fb_info);
+ put_framebuffer_info(fb_info);
done:
return ret;
}
diff --git a/include/linux/fb.h b/include/linux/fb.h
index df728c1..60de3fa 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -834,6 +834,7 @@ struct fb_tile_ops {
struct fb_info {
int node;
int flags;
+ int ref_count;
struct mutex lock; /* Lock for open/release/ioctl funcs */
struct mutex mm_lock; /* Lock for fb_mmap and smem_* fields */
struct fb_var_screeninfo var; /* Current var */
@@ -873,6 +874,7 @@ struct fb_info {
void *pseudo_palette; /* Fake palette of 16 colors */
#define FBINFO_STATE_RUNNING 0
#define FBINFO_STATE_SUSPENDED 1
+#define FBINFO_STATE_REMOVED 2
u32 state; /* Hardware state i.e suspend */
void *fbcon_par; /* fbcon use-only private area */
/* From here on everything is device dependent */
--
1.7.0.4

2011-05-05 18:30:28

by Bruno Prémont

Subject: Re: [PATCH] fbcon -- fix race between open and removal of framebuffers

On Thu, 05 May 2011 [email protected] wrote:
> From: Andy Whitcroft <[email protected]>
>
> Currently there is no locking for updates to the registered_fb list.
> This allows an open through /dev/fbN to pick up a registered framebuffer
> pointer in parallel with it being released, as happens when a conflicting
> framebuffer is ejected or on module unload. There is also no reference
> counting on the framebuffer descriptor which is referenced from all open
> files, leading to references to released or reused memory to persist on
> these open files.
>
> This patch adds a reference count to the framebuffer descriptor to prevent
> it from being released until after all pending opens are closed. This
> allows the pending opens to detect the closed status and unmap themselves.
> It also adds locking to the framebuffer lookup path, locking it against
> the removal path such that it is possible to atomically lookup and take a
> reference to the descriptor. It also adds locking to the read and write
> paths which currently could access the framebuffer descriptor after it
> has been freed. Finally it moves the device to FBINFO_STATE_REMOVED to
> indicate that all access should be errored for this device.

Is there a good reason to not use kref for the refcounting? Except for
(un)registering framebuffers this would avoid the need for taking
registered_lock.

Unfortunately fbcon also accesses registered_fb (quite a lot!) but it
probably is save enough through use of the notifiers.

> Signed-off-by: Andy Whitcroft <[email protected]>
> Acked-by: Stefan Bader <[email protected]>
> Signed-off-by: Leann Ogasawara <[email protected]>
> Signed-off-by: Tim Gardner <[email protected]>
> ---
> drivers/video/fbmem.c | 132 ++++++++++++++++++++++++++++++++++++++-----------
> include/linux/fb.h | 2 +
> 2 files changed, 105 insertions(+), 29 deletions(-)
>

...

> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index df728c1..60de3fa 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -834,6 +834,7 @@ struct fb_tile_ops {
> struct fb_info {
> int node;
> int flags;
> + int ref_count;
> struct mutex lock; /* Lock for open/release/ioctl funcs */
> struct mutex mm_lock; /* Lock for fb_mmap and smem_* fields */
> struct fb_var_screeninfo var; /* Current var */
> @@ -873,6 +874,7 @@ struct fb_info {
> void *pseudo_palette; /* Fake palette of 16 colors */
> #define FBINFO_STATE_RUNNING 0
> #define FBINFO_STATE_SUSPENDED 1
> +#define FBINFO_STATE_REMOVED 2
> u32 state; /* Hardware state i.e suspend */
> void *fbcon_par; /* fbcon use-only private area */
> /* From here on everything is device dependent */

2011-05-05 21:00:47

by Jack Stone

Subject: Re: [PATCH] fbcon -- fix race between open and removal of framebuffers

On 05/05/2011 18:41, [email protected] wrote:
> +static struct fb_info *get_framebuffer_info(int idx)
> +__acquires(&registered_lock)
> +__releases(&registered_lock)
> +{
> + struct fb_info *fb_info;
> +
> + spin_lock(&registered_lock);
> + fb_info = registered_fb[idx];
> + fb_info->ref_count++;
> + spin_unlock(&registered_lock);
> +
> + return fb_info;
> +}
> +
> static int
> fb_open(struct inode *inode, struct file *file)
> __acquires(&info->lock)
> @@ -1363,13 +1421,17 @@ __releases(&info->lock)
>
> if (fbidx >= FB_MAX)
> return -ENODEV;
> - info = registered_fb[fbidx];
> + info = get_framebuffer_info(fbidx);
> if (!info)
> request_module("fb%d", fbidx);
> - info = registered_fb[fbidx];
> + info = get_framebuffer_info(fbidx);
> if (!info)
> return -ENODEV;

If the first get_framebuffer_info succeeds don't you up the ref count
twice? Shouldn't this be:

info = get_framebuffer_info(fbidx);
if (!info) {
request_module("fb%d", fbidx);
info = get_framebuffer_info(fbidx);
}
if (!info)
return -ENODEV;

Thanks,

Jack

2011-05-06 00:21:31

by Anca Emanuel

Subject: Re: [PATCH] fbcon -- fix race between open and removal of framebuffers

On Thu, May 5, 2011 at 8:41 PM, <[email protected]> wrote:
> From: Andy Whitcroft <[email protected]>
>
> Currently there is no locking for updates to the registered_fb list.
> This allows an open through /dev/fbN to pick up a registered framebuffer
> pointer in parallel with it being released, as happens when a conflicting
> framebuffer is ejected or on module unload. ?There is also no reference
> counting on the framebuffer descriptor which is referenced from all open
> files, leading to references to released or reused memory to persist on
> these open files.
>
> This patch adds a reference count to the framebuffer descriptor to prevent
> it from being released until after all pending opens are closed. ?This
> allows the pending opens to detect the closed status and unmap themselves.
> It also adds locking to the framebuffer lookup path, locking it against
> the removal path such that it is possible to atomically lookup and take a
> reference to the descriptor. ?It also adds locking to the read and write
> paths which currently could access the framebuffer descriptor after it
> has been freed. ?Finally it moves the device to FBINFO_STATE_REMOVED to
> indicate that all access should be errored for this device.
>
> Signed-off-by: Andy Whitcroft <[email protected]>
> Acked-by: Stefan Bader <[email protected]>
> Signed-off-by: Leann Ogasawara <[email protected]>
> Signed-off-by: Tim Gardner <[email protected]>
> ---
> ?drivers/video/fbmem.c | ?132 ++++++++++++++++++++++++++++++++++++++-----------
> ?include/linux/fb.h ? ?| ? ?2 +
> ?2 files changed, 105 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index e0c2284..c8562c1 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -42,6 +42,8 @@
>
> ?#define FBPIXMAPSIZE ? (1024 * 8)
>
> +/* Protects the registered framebuffer list and count. */
> +static DEFINE_SPINLOCK(registered_lock);
> ?struct fb_info *registered_fb[FB_MAX] __read_mostly;
> ?int num_registered_fb __read_mostly;
>
> @@ -694,9 +696,7 @@ static ssize_t
> ?fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> ?{
> ? ? ? ?unsigned long p = *ppos;
> - ? ? ? struct inode *inode = file->f_path.dentry->d_inode;
> - ? ? ? int fbidx = iminor(inode);
> - ? ? ? struct fb_info *info = registered_fb[fbidx];
> + ? ? ? struct fb_info *info = file->private_data;
> ? ? ? ?u8 *buffer, *dst;
> ? ? ? ?u8 __iomem *src;
> ? ? ? ?int c, cnt = 0, err = 0;
> @@ -705,19 +705,28 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> ? ? ? ?if (!info || ! info->screen_base)
> ? ? ? ? ? ? ? ?return -ENODEV;
>
> - ? ? ? if (info->state != FBINFO_STATE_RUNNING)
> - ? ? ? ? ? ? ? return -EPERM;
> + ? ? ? if (!lock_fb_info(info))
> + ? ? ? ? ? ? ? return -ENODEV;
> +
> + ? ? ? if (info->state != FBINFO_STATE_RUNNING) {
> + ? ? ? ? ? ? ? err = -EPERM;
> + ? ? ? ? ? ? ? goto out_fb_info;
> + ? ? ? }
>
> - ? ? ? if (info->fbops->fb_read)
> - ? ? ? ? ? ? ? return info->fbops->fb_read(info, buf, count, ppos);
> + ? ? ? if (info->fbops->fb_read) {
> + ? ? ? ? ? ? ? err = info->fbops->fb_read(info, buf, count, ppos);
> + ? ? ? ? ? ? ? goto out_fb_info;
> + ? ? ? }
>
> ? ? ? ?total_size = info->screen_size;
>
> ? ? ? ?if (total_size == 0)
> ? ? ? ? ? ? ? ?total_size = info->fix.smem_len;
>
> - ? ? ? if (p >= total_size)
> - ? ? ? ? ? ? ? return 0;
> + ? ? ? if (p >= total_size) {
> + ? ? ? ? ? ? ? err = 0;
> + ? ? ? ? ? ? ? goto out_fb_info;
> + ? ? ? }
>
> ? ? ? ?if (count >= total_size)
> ? ? ? ? ? ? ? ?count = total_size;
> @@ -727,8 +736,10 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>
> ? ? ? ?buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
> ? ? ? ? ? ? ? ? ? ? ? ? GFP_KERNEL);
> - ? ? ? if (!buffer)
> - ? ? ? ? ? ? ? return -ENOMEM;
> + ? ? ? if (!buffer) {
> + ? ? ? ? ? ? ? err = -ENOMEM;
> + ? ? ? ? ? ? ? goto out_fb_info;
> + ? ? ? }
>
> ? ? ? ?src = (u8 __iomem *) (info->screen_base + p);
>
> @@ -751,19 +762,21 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> ? ? ? ? ? ? ? ?cnt += c;
> ? ? ? ? ? ? ? ?count -= c;
> ? ? ? ?}
> + ? ? ? if (!err)
> + ? ? ? ? ? ? ? err = cnt;
>
> ? ? ? ?kfree(buffer);
> +out_fb_info:
> + ? ? ? unlock_fb_info(info);
>
> - ? ? ? return (err) ? err : cnt;
> + ? ? ? return err;
> ?}
>
> ?static ssize_t
> ?fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> ?{
> ? ? ? ?unsigned long p = *ppos;
> - ? ? ? struct inode *inode = file->f_path.dentry->d_inode;
> - ? ? ? int fbidx = iminor(inode);
> - ? ? ? struct fb_info *info = registered_fb[fbidx];
> + ? ? ? struct fb_info *info = file->private_data;
> ? ? ? ?u8 *buffer, *src;
> ? ? ? ?u8 __iomem *dst;
> ? ? ? ?int c, cnt = 0, err = 0;
> @@ -772,8 +785,13 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> ? ? ? ?if (!info || !info->screen_base)
> ? ? ? ? ? ? ? ?return -ENODEV;
>
> - ? ? ? if (info->state != FBINFO_STATE_RUNNING)
> - ? ? ? ? ? ? ? return -EPERM;
> + ? ? ? if (!lock_fb_info(info))
> + ? ? ? ? ? ? ? return -ENODEV;
> +
> + ? ? ? if (info->state != FBINFO_STATE_RUNNING) {
> + ? ? ? ? ? ? ? err = -EPERM;
> + ? ? ? ? ? ? ? goto out_fb_info;
> + ? ? ? }
>
> ? ? ? ?if (info->fbops->fb_write)
> ? ? ? ? ? ? ? ?return info->fbops->fb_write(info, buf, count, ppos);
> @@ -783,8 +801,10 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> ? ? ? ?if (total_size == 0)
> ? ? ? ? ? ? ? ?total_size = info->fix.smem_len;
>
> - ? ? ? if (p > total_size)
> - ? ? ? ? ? ? ? return -EFBIG;
> + ? ? ? if (p > total_size) {
> + ? ? ? ? ? ? ? err = -EFBIG;
> + ? ? ? ? ? ? ? goto out_fb_info;
> + ? ? ? }
>
> ? ? ? ?if (count > total_size) {
> ? ? ? ? ? ? ? ?err = -EFBIG;
> @@ -800,8 +820,10 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>
> ? ? ? ?buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
> ? ? ? ? ? ? ? ? ? ? ? ? GFP_KERNEL);
> - ? ? ? if (!buffer)
> - ? ? ? ? ? ? ? return -ENOMEM;
> + ? ? ? if (!buffer) {
> + ? ? ? ? ? ? ? err = -ENOMEM;
> + ? ? ? ? ? ? ? goto out_fb_info;
> + ? ? ? }
>
> ? ? ? ?dst = (u8 __iomem *) (info->screen_base + p);
>
> @@ -825,10 +847,14 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> ? ? ? ? ? ? ? ?cnt += c;
> ? ? ? ? ? ? ? ?count -= c;
> ? ? ? ?}
> + ? ? ? if (cnt)
> + ? ? ? ? ? ? ? err = cnt;
>
> ? ? ? ?kfree(buffer);
> +out_fb_info:
> + ? ? ? unlock_fb_info(info);
>
> - ? ? ? return (cnt) ? cnt : err;
> + ? ? ? return err;
> ?}
>
> ?int
> @@ -1303,8 +1329,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
> ?static int
> ?fb_mmap(struct file *file, struct vm_area_struct * vma)
> ?{
> - ? ? ? int fbidx = iminor(file->f_path.dentry->d_inode);
> - ? ? ? struct fb_info *info = registered_fb[fbidx];
> + ? ? ? struct fb_info * const info = file->private_data;
> ? ? ? ?struct fb_ops *fb = info->fbops;
> ? ? ? ?unsigned long off;
> ? ? ? ?unsigned long start;
> @@ -1316,6 +1341,11 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> ? ? ? ?if (!fb)
> ? ? ? ? ? ? ? ?return -ENODEV;
> ? ? ? ?mutex_lock(&info->mm_lock);
> + ? ? ? if (info->state == FBINFO_STATE_REMOVED) {
> + ? ? ? ? ? ? ? mutex_unlock(&info->mm_lock);
> + ? ? ? ? ? ? ? return -ENODEV;
> + ? ? ? }
> +
> ? ? ? ?if (fb->fb_mmap) {
> ? ? ? ? ? ? ? ?int res;
> ? ? ? ? ? ? ? ?res = fb->fb_mmap(info, vma);
> @@ -1352,6 +1382,34 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> ? ? ? ?return 0;
> ?}
>
> +static struct fb_info *get_framebuffer_info(int idx)
> +__acquires(&registered_lock)
> +__releases(&registered_lock)
> +{
> + ? ? ? struct fb_info *fb_info;
> +
> + ? ? ? spin_lock(&registered_lock);
> + ? ? ? fb_info = registered_fb[idx];
> + ? ? ? fb_info->ref_count++;
> + ? ? ? spin_unlock(&registered_lock);
> +
> + ? ? ? return fb_info;
> +}
> +
> +static void put_framebuffer_info(struct fb_info *fb_info)
> +__acquires(&registered_lock)
> +__releases(&registered_lock)
> +{
> + ? ? ? int keep;
> +
> + ? ? ? spin_lock(&registered_lock);
> + ? ? ? keep = --fb_info->ref_count;
> + ? ? ? spin_unlock(&registered_lock);
> +
> + ? ? ? if (!keep && fb_info->fbops->fb_destroy)
> + ? ? ? ? ? ? ? fb_info->fbops->fb_destroy(fb_info);
> +}
> +
> ?static int
> ?fb_open(struct inode *inode, struct file *file)
> ?__acquires(&info->lock)
> @@ -1363,13 +1421,17 @@ __releases(&info->lock)
>
> ? ? ? ?if (fbidx >= FB_MAX)
> ? ? ? ? ? ? ? ?return -ENODEV;
> - ? ? ? info = registered_fb[fbidx];
> + ? ? ? info = get_framebuffer_info(fbidx);
> ? ? ? ?if (!info)
> ? ? ? ? ? ? ? ?request_module("fb%d", fbidx);
> - ? ? ? info = registered_fb[fbidx];
> + ? ? ? info = get_framebuffer_info(fbidx);
> ? ? ? ?if (!info)
> ? ? ? ? ? ? ? ?return -ENODEV;
> ? ? ? ?mutex_lock(&info->lock);
> + ? ? ? if (info->state == FBINFO_STATE_REMOVED) {
> + ? ? ? ? ? ? ? res = -ENODEV;
> + ? ? ? ? ? ? ? goto out;
> + ? ? ? }
> ? ? ? ?if (!try_module_get(info->fbops->owner)) {
> ? ? ? ? ? ? ? ?res = -ENODEV;
> ? ? ? ? ? ? ? ?goto out;
> @@ -1386,6 +1448,8 @@ __releases(&info->lock)
> ?#endif
> ?out:
> ? ? ? ?mutex_unlock(&info->lock);
> + ? ? ? if (res)
> + ? ? ? ? ? ? ? put_framebuffer_info(info);
> ? ? ? ?return res;
> ?}
>
> @@ -1401,6 +1465,7 @@ __releases(&info->lock)
> ? ? ? ? ? ? ? ?info->fbops->fb_release(info,1);
> ? ? ? ?module_put(info->fbops->owner);
> ? ? ? ?mutex_unlock(&info->lock);
> + ? ? ? put_framebuffer_info(info);
> ? ? ? ?return 0;
> ?}
>
> @@ -1549,6 +1614,7 @@ register_framebuffer(struct fb_info *fb_info)
> ? ? ? ?fb_info->node = i;
> ? ? ? ?mutex_init(&fb_info->lock);
> ? ? ? ?mutex_init(&fb_info->mm_lock);
> + ? ? ? fb_info->ref_count = 1;
>
> ? ? ? ?fb_info->dev = device_create(fb_class, fb_info->device,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? MKDEV(FB_MAJOR, i), NULL, "fb%d", i);
> @@ -1592,7 +1658,6 @@ register_framebuffer(struct fb_info *fb_info)
> ? ? ? ?return 0;
> ?}
>
> -
> ?/**
> ?* ? ? unregister_framebuffer - releases a frame buffer device
> ?* ? ? @fb_info: frame buffer info structure
> @@ -1627,6 +1692,16 @@ unregister_framebuffer(struct fb_info *fb_info)
> ? ? ? ? ? ? ? ?return -ENODEV;
> ? ? ? ?event.info = fb_info;
> ? ? ? ?ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
> + ? ? ? if (!ret) {
> + ? ? ? ? ? ? ? mutex_lock(&fb_info->mm_lock);
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* We must prevent any operations for this transition, we
> + ? ? ? ? ? ? ? ?* already have info->lock so grab the info->mm_lock to hold
> + ? ? ? ? ? ? ? ?* the remainder.
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? fb_info->state = FBINFO_STATE_REMOVED;
> + ? ? ? ? ? ? ? mutex_unlock(&fb_info->mm_lock);
> + ? ? ? }
> ? ? ? ?unlock_fb_info(fb_info);
>
> ? ? ? ?if (ret) {
> @@ -1646,8 +1721,7 @@ unregister_framebuffer(struct fb_info *fb_info)
> ? ? ? ?fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
>
> ? ? ? ?/* this may free fb info */
> - ? ? ? if (fb_info->fbops->fb_destroy)
> - ? ? ? ? ? ? ? fb_info->fbops->fb_destroy(fb_info);
> + ? ? ? put_framebuffer_info(fb_info);
> ?done:
> ? ? ? ?return ret;
> ?}
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index df728c1..60de3fa 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -834,6 +834,7 @@ struct fb_tile_ops {
> ?struct fb_info {
> ? ? ? ?int node;
> ? ? ? ?int flags;
> + ? ? ? int ref_count;
> ? ? ? ?struct mutex lock; ? ? ? ? ? ? ?/* Lock for open/release/ioctl funcs */
> ? ? ? ?struct mutex mm_lock; ? ? ? ? ? /* Lock for fb_mmap and smem_* fields */
> ? ? ? ?struct fb_var_screeninfo var; ? /* Current var */
> @@ -873,6 +874,7 @@ struct fb_info {
> ? ? ? ?void *pseudo_palette; ? ? ? ? ? /* Fake palette of 16 colors */
> ?#define FBINFO_STATE_RUNNING ? 0
> ?#define FBINFO_STATE_SUSPENDED 1
> +#define FBINFO_STATE_REMOVED ? 2
> ? ? ? ?u32 state; ? ? ? ? ? ? ? ? ? ? ?/* Hardware state i.e suspend */
> ? ? ? ?void *fbcon_par; ? ? ? ? ? ? ? ?/* fbcon use-only private area */
> ? ? ? ?/* From here on everything is device dependent */
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>

Tested-by: Anca Emanuel <anca.emanuel.gmail.com>

I can not use S3 resume without this.
[ 21.964367] BUG: unable to handle kernel paging request at 0000010a00000010
[ 21.964396] IP: [<ffffffff8130abe0>] fb_release+0x30/0x70
[ 21.964410] PGD 0
[ 21.964416] Oops: 0000 [#1] SMP
[ 21.964424] last sysfs file: /sys/devices/virtual/vtconsole/vtcon1/uevent
[ 21.964434] CPU 1
[ 21.964438] Modules linked in: parport_pc ppdev
snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm
adt7475 hwmon_vid snd_seq_midi snd_rawmidi snd_seq_midi_event nouveau
snd_seq snd_timer snd_seq_device ttm drm_kms_helper snd intel_agp
psmouse soundcore serio_raw intel_gtt snd_page_alloc drm i2c_algo_bit
video lp parport pata_marvell ahci libahci r8169 mii
[ 21.964528]
[ 21.964533] Pid: 221, comm: plymouthd Not tainted 2.6.39-rc6 #7
MICRO-STAR INTERNATIONAL CO.,LTD MS-7360/MS-7360
[ 21.964548] RIP: 0010:[<ffffffff8130abe0>] [<ffffffff8130abe0>]
fb_release+0x30/0x70
[ 21.964560] RSP: 0018:ffff880037211eb8 EFLAGS: 00010286
[ 21.964566] RAX: ffff880037210000 RBX: ffff88007f817000 RCX: 0000000000000001
[ 21.964573] RDX: 0000010a00000000 RSI: ffff8800370f5540 RDI: ffff88007f817008
[ 21.964580] RBP: ffff880037211ec8 R08: 0000000000000000 R09: 0000000000000000
[ 21.964588] R10: ffff8800370f5550 R11: 0000000000000246 R12: ffff88007f817008
[ 21.964595] R13: ffff88007d3db540 R14: ffff88007be34d90 R15: ffff88007be34d90
[ 21.964604] FS: 00007fb335025720(0000) GS:ffff88007fc80000(0000)
knlGS:0000000000000000
[ 21.964739] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 21.964746] CR2: 0000010a00000010 CR3: 000000007b41a000 CR4: 00000000000006e0
[ 21.964754] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 21.964762] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 21.964770] Process plymouthd (pid: 221, threadinfo
ffff880037210000, task ffff880036cd16c0)
[ 21.964778] Stack:
[ 21.964782] ffff8800370f5540 0000000000000008 ffff880037211f18
ffffffff8115cfaa
[ 21.964797] ffff8800370f5550 ffff8800793c7b00 ffff88006744fcd0
ffff8800370f5540
[ 21.964811] ffff88007c3b9080 0000000000000000 000000000000000b
0000000000000000
[ 21.964825] Call Trace:
[ 21.964834] [<ffffffff8115cfaa>] fput+0xea/0x220
[ 21.964842] [<ffffffff811591f6>] filp_close+0x66/0x90
[ 21.964849] [<ffffffff811597c7>] sys_close+0xb7/0x120
[ 21.964858] [<ffffffff815b3002>] system_call_fastpath+0x16/0x1b
[ 21.964865] Code: 83 ec 10 48 89 1c 24 4c 89 64 24 08 0f 1f 44 00
00 48 8b 9e a0 00 00 00 4c 8d 63 08 4c 89 e7 e8 d7 ea 29 00 48 8b 93
b8 03 00 00
[ 21.964944] 8b 42 10 48 85 c0 74 11 be 01 00 00 00 48 89 df ff d0 48 8b
[ 21.964983] RIP [<ffffffff8130abe0>] fb_release+0x30/0x70
[ 21.964992] RSP <ffff880037211eb8>
[ 21.964997] CR2: 0000010a00000010

2011-05-06 01:09:47

by Anca Emanuel

Subject: Re: [PATCH] fbcon -- fix race between open and removal of framebuffers

@Greg: This is stable material for 2.6.38
link to the patch: http://is.gd/otIfGc

2011-05-06 01:55:40

by Greg KH

Subject: Re: [PATCH] fbcon -- fix race between open and removal of framebuffers

On Fri, May 06, 2011 at 04:09:44AM +0300, Anca Emanuel wrote:
> @Greg: This is stable material for 2.6.38
> link to the patch: http://is.gd/otIfGc

<form_letter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

thanks,

greg k-h

</form_letter>

2011-05-10 12:47:59

by Tim Gardner

Subject: [PATCH V2] fbcon -- fix race between open and removal of framebuffers

On 05/05/2011 11:00 PM, Jack Stone wrote:
> On 05/05/2011 18:41, [email protected] wrote:
>> +static struct fb_info *get_framebuffer_info(int idx)
>> +__acquires(&registered_lock)
>> +__releases(&registered_lock)
>> +{
>> + struct fb_info *fb_info;
>> +
>> + spin_lock(&registered_lock);
>> + fb_info = registered_fb[idx];
>> + fb_info->ref_count++;
>> + spin_unlock(&registered_lock);
>> +
>> + return fb_info;
>> +}
>> +
>> static int
>> fb_open(struct inode *inode, struct file *file)
>> __acquires(&info->lock)
>> @@ -1363,13 +1421,17 @@ __releases(&info->lock)
>>
>> if (fbidx>= FB_MAX)
>> return -ENODEV;
>> - info = registered_fb[fbidx];
>> + info = get_framebuffer_info(fbidx);
>> if (!info)
>> request_module("fb%d", fbidx);
>> - info = registered_fb[fbidx];
>> + info = get_framebuffer_info(fbidx);
>> if (!info)
>> return -ENODEV;
>
> If the first get_framebuffer_info succeeds don't you up the ref count
> twice? Shouldn't this be:
>
> info = get_framebuffer_info(fbidx);
> if (!info) {
> request_module("fb%d", fbidx);
> info = get_framebuffer_info(fbidx);
> }
> if (!info)
> return -ENODEV;
>
> Thanks,
>
> Jack

Good catch. See attached.

rtg
--
Tim Gardner [email protected]

Attachments:

2011-05-10 13:52:31

by Tim Gardner

Subject: [PATCH V3] fbcon -- fix race between open and removal of framebuffers

There was a second patch that I missed that fixed an original
regression, so I've just squashed that one into this V3 patch along with
the reference count issue that Jack Stone pointed out.

These patches can be found in their original form in
git://kernel.ubuntu.git/ubuntu/ubuntu-oneiric.git

UBUNTU: SAUCE: fbcon -- fix race between open and removal of framebuffers
UBUNTU: SAUCE: fbcon -- fix OOPs triggered by race prevention fixes

rtg
--
Tim Gardner [email protected]

Attachments:

2011-05-10 21:06:16

by Jack Stone

Subject: Re: [PATCH V2] fbcon -- fix race between open and removal of framebuffers

Hi Tim,

One more quick question:

On 10/05/2011 13:47, Tim Gardner wrote:
+static struct fb_info *get_framebuffer_info(int idx)
+__acquires(&registered_lock)
+__releases(&registered_lock)
+{
+ struct fb_info *fb_info;
+
+ spin_lock(&registered_lock);
+ fb_info = registered_fb[idx];
+ fb_info->ref_count++;
+ spin_unlock(&registered_lock);
+
+ return fb_info;
+}

[snip]

static int
fb_open(struct inode *inode, struct file *file)
__acquires(&info->lock)
@@ -1363,13 +1421,18 @@ __releases(&info->lock)

if (fbidx >= FB_MAX)
return -ENODEV;
- info = registered_fb[fbidx];
- if (!info)
+ info = get_framebuffer_info(fbidx);
+ if (!info) {
request_module("fb%d", fbidx);
- info = registered_fb[fbidx];
+ info = get_framebuffer_info(fbidx);
+ }
if (!info)
return -ENODEV;

This section of code implies that get_framebuffer_info can return NULL
but in that case wouldn't the fb_info->ref_count++ have oopsed?

You could add the simple case of

if(fb_info)
fb_info->ref_count++

to get_framebuffer_info. That should cover it.

Thanks,

Jack

2011-05-10 21:08:11

by Jack Stone

Subject: Re: [PATCH V2] fbcon -- fix race between open and removal of framebuffers

On 10/05/2011 22:06, Jack Stone wrote:
> Hi Tim,
>
> One more quick question:
>
> On 10/05/2011 13:47, Tim Gardner wrote:
> +static struct fb_info *get_framebuffer_info(int idx)
> +__acquires(&registered_lock)
> +__releases(&registered_lock)
> +{
> + struct fb_info *fb_info;
> +
> + spin_lock(&registered_lock);
> + fb_info = registered_fb[idx];
> + fb_info->ref_count++;
> + spin_unlock(&registered_lock);
> +
> + return fb_info;
> +}
>
> [snip]
>
> static int
> fb_open(struct inode *inode, struct file *file)
> __acquires(&info->lock)
> @@ -1363,13 +1421,18 @@ __releases(&info->lock)
>
> if (fbidx >= FB_MAX)
> return -ENODEV;
> - info = registered_fb[fbidx];
> - if (!info)
> + info = get_framebuffer_info(fbidx);
> + if (!info) {
> request_module("fb%d", fbidx);
> - info = registered_fb[fbidx];
> + info = get_framebuffer_info(fbidx);
> + }
> if (!info)
> return -ENODEV;
>
> This section of code implies that get_framebuffer_info can return NULL
> but in that case wouldn't the fb_info->ref_count++ have oopsed?
>
> You could add the simple case of
>
> if(fb_info)
> fb_info->ref_count++
>
> to get_framebuffer_info. That should cover it.
>

Just read your later patch. Sorry for the extra email.

Thanks,

Jack

2011-05-10 21:44:43

by Bruno Prémont

Subject: Re: [PATCH V3] fbcon -- fix race between open and removal of framebuffers

On Tue, 10 May 2011 Tim Gardner <[email protected]> wrote:
> From ca3ef33e2235c88856a6257c0be63192ab56c678 Mon Sep 17 00:00:00 2001
> From: Andy Whitcroft <[email protected]>
> Date: Thu, 29 Jul 2010 16:48:20 +0100
> Subject: [PATCH] fbcon -- fix race between open and removal of framebuffers
>
> Currently there is no locking for updates to the registered_fb list.
> This allows an open through /dev/fbN to pick up a registered framebuffer
> pointer in parallel with it being released, as happens when a conflicting
> framebuffer is ejected or on module unload. There is also no reference
> counting on the framebuffer descriptor which is referenced from all open
> files, leading to references to released or reused memory to persist on
> these open files.
>
> This patch adds a reference count to the framebuffer descriptor to prevent
> it from being released until after all pending opens are closed. This
> allows the pending opens to detect the closed status and unmap themselves.
> It also adds locking to the framebuffer lookup path, locking it against
> the removal path such that it is possible to atomically lookup and take a
> reference to the descriptor. It also adds locking to the read and write
> paths which currently could access the framebuffer descriptor after it
> has been freed. Finally it moves the device to FBINFO_STATE_REMOVED to
> indicate that all access should be errored for this device.

What framebuffer drivers was this patch tested with? Just x86 with
mainstream GPU (intel/amd/nvidia KMS) in compination with vgafb/vesafb or
did it see some testing with other framebuffers like those from embedded
world?

Otherwise a much smaller (memory leaking) patch would be to just change
vesafb/vgafb to not free their fb_info after unregistration as was suggested
by Alan Cox.

> Signed-off-by: Andy Whitcroft <[email protected]>
> Acked-by: Stefan Bader <[email protected]>
> Signed-off-by: Leann Ogasawara <[email protected]>
> Signed-off-by: Tim Gardner <[email protected]>
> ---
> drivers/video/fbmem.c | 136 ++++++++++++++++++++++++++++++++++++++-----------
> include/linux/fb.h | 2 +
> 2 files changed, 108 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index e0c2284..1afe435 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -42,6 +42,8 @@
>
> #define FBPIXMAPSIZE (1024 * 8)
>
> +/* Protects the registered framebuffer list and count. */
> +static DEFINE_SPINLOCK(registered_lock);

This only partially protects the list and count as two concurrent
framebuffer registrations do still race against each other.
For the issue addressed by this patch I don't think it makes sense to
have this spinlock at all as it's only used in get_framebuffer_info()
and in put_framebuffer_info() and put_framebuffer_info() doesn't even
look at registered_fb or num_registered_fb.
Such a spinlock makes sense in a separate patch that really protects
all access to registered_fb or num_registered_fb, be it during framebuffer
(un)registration or during access from fbcon.

I'm working on a more complete set of patches to get proper locking,
refcount (using kref) and release for struct fb_info but auditing the
various framebuffer drivers will take some time (some of the
open/release counting of drivers can probably get removed once fb_info
is ref-counted). Hopefully I can get some of it out for review on
Thursday evening so others can also look at their respective drivers.

> struct fb_info *registered_fb[FB_MAX] __read_mostly;
> int num_registered_fb __read_mostly;
>
> @@ -694,9 +696,7 @@ static ssize_t
> fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> {
> unsigned long p = *ppos;
> - struct inode *inode = file->f_path.dentry->d_inode;
> - int fbidx = iminor(inode);
> - struct fb_info *info = registered_fb[fbidx];
> + struct fb_info *info = file->private_data;
> u8 *buffer, *dst;
> u8 __iomem *src;
> int c, cnt = 0, err = 0;
> @@ -705,19 +705,28 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> if (!info || ! info->screen_base)
> return -ENODEV;
>
> - if (info->state != FBINFO_STATE_RUNNING)
> - return -EPERM;
> + if (!lock_fb_info(info))
> + return -ENODEV;
> +
> + if (info->state != FBINFO_STATE_RUNNING) {
> + err = -EPERM;
> + goto out_fb_info;
> + }
>
> - if (info->fbops->fb_read)
> - return info->fbops->fb_read(info, buf, count, ppos);
> + if (info->fbops->fb_read) {
> + err = info->fbops->fb_read(info, buf, count, ppos);
> + goto out_fb_info;
> + }
>
> total_size = info->screen_size;
>
> if (total_size == 0)
> total_size = info->fix.smem_len;
>
> - if (p >= total_size)
> - return 0;
> + if (p >= total_size) {
> + err = 0;
> + goto out_fb_info;
> + }
>
> if (count >= total_size)
> count = total_size;
> @@ -727,8 +736,10 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>
> buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
> GFP_KERNEL);
> - if (!buffer)
> - return -ENOMEM;
> + if (!buffer) {
> + err = -ENOMEM;
> + goto out_fb_info;
> + }
>
> src = (u8 __iomem *) (info->screen_base + p);
>
> @@ -751,19 +762,21 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> cnt += c;
> count -= c;
> }
> + if (!err)
> + err = cnt;
>
> kfree(buffer);
> +out_fb_info:
> + unlock_fb_info(info);
>
> - return (err) ? err : cnt;
> + return err;
> }
>
> static ssize_t
> fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> {
> unsigned long p = *ppos;
> - struct inode *inode = file->f_path.dentry->d_inode;
> - int fbidx = iminor(inode);
> - struct fb_info *info = registered_fb[fbidx];
> + struct fb_info *info = file->private_data;
> u8 *buffer, *src;
> u8 __iomem *dst;
> int c, cnt = 0, err = 0;
> @@ -772,8 +785,13 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> if (!info || !info->screen_base)
> return -ENODEV;
>
> - if (info->state != FBINFO_STATE_RUNNING)
> - return -EPERM;
> + if (!lock_fb_info(info))
> + return -ENODEV;
> +
> + if (info->state != FBINFO_STATE_RUNNING) {
> + err = -EPERM;
> + goto out_fb_info;
> + }
>
> if (info->fbops->fb_write)
> return info->fbops->fb_write(info, buf, count, ppos);
> @@ -783,8 +801,10 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> if (total_size == 0)
> total_size = info->fix.smem_len;
>
> - if (p > total_size)
> - return -EFBIG;
> + if (p > total_size) {
> + err = -EFBIG;
> + goto out_fb_info;
> + }
>
> if (count > total_size) {
> err = -EFBIG;
> @@ -800,8 +820,10 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>
> buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
> GFP_KERNEL);
> - if (!buffer)
> - return -ENOMEM;
> + if (!buffer) {
> + err = -ENOMEM;
> + goto out_fb_info;
> + }
>
> dst = (u8 __iomem *) (info->screen_base + p);
>
> @@ -825,10 +847,14 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> cnt += c;
> count -= c;
> }
> + if (cnt)
> + err = cnt;
>
> kfree(buffer);
> +out_fb_info:
> + unlock_fb_info(info);
>
> - return (cnt) ? cnt : err;
> + return err;
> }
>
> int
> @@ -1303,8 +1329,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
> static int
> fb_mmap(struct file *file, struct vm_area_struct * vma)
> {
> - int fbidx = iminor(file->f_path.dentry->d_inode);
> - struct fb_info *info = registered_fb[fbidx];
> + struct fb_info * const info = file->private_data;
> struct fb_ops *fb = info->fbops;
> unsigned long off;
> unsigned long start;
> @@ -1316,6 +1341,11 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> if (!fb)
> return -ENODEV;
> mutex_lock(&info->mm_lock);
> + if (info->state == FBINFO_STATE_REMOVED) {
> + mutex_unlock(&info->mm_lock);
> + return -ENODEV;
> + }
> +
> if (fb->fb_mmap) {
> int res;
> res = fb->fb_mmap(info, vma);
> @@ -1352,6 +1382,35 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> return 0;
> }
>
> +static struct fb_info *get_framebuffer_info(int idx)
> +__acquires(&registered_lock)
> +__releases(&registered_lock)
> +{
> + struct fb_info *fb_info;
> +
> + spin_lock(&registered_lock);
> + fb_info = registered_fb[idx];
> + if (fb_info)
> + fb_info->ref_count++;
> + spin_unlock(&registered_lock);
> +
> + return fb_info;
> +}
> +
> +static void put_framebuffer_info(struct fb_info *fb_info)
> +__acquires(&registered_lock)
> +__releases(&registered_lock)
> +{
> + int keep;
> +
> + spin_lock(&registered_lock);
> + keep = --fb_info->ref_count;
> + spin_unlock(&registered_lock);
> +
> + if (!keep && fb_info->fbops->fb_destroy)
> + fb_info->fbops->fb_destroy(fb_info);

What happens in case fbops->fb_destroy is NULL, we just leak
the framebuffer struct? (I didn't audit frambuffer drivers yet)
Maybe calling framebuffer_release(fb_info) would be a first step.

> +}
> +
> static int
> fb_open(struct inode *inode, struct file *file)
> __acquires(&info->lock)
> @@ -1363,13 +1422,18 @@ __releases(&info->lock)
>
> if (fbidx >= FB_MAX)
> return -ENODEV;
> - info = registered_fb[fbidx];
> - if (!info)
> + info = get_framebuffer_info(fbidx);
> + if (!info) {
> request_module("fb%d", fbidx);
> - info = registered_fb[fbidx];
> + info = get_framebuffer_info(fbidx);
> + }
> if (!info)
> return -ENODEV;
> mutex_lock(&info->lock);
> + if (info->state == FBINFO_STATE_REMOVED) {
> + res = -ENODEV;
> + goto out;
> + }
> if (!try_module_get(info->fbops->owner)) {
> res = -ENODEV;
> goto out;
> @@ -1386,6 +1450,8 @@ __releases(&info->lock)
> #endif
> out:
> mutex_unlock(&info->lock);
> + if (res)
> + put_framebuffer_info(info);
> return res;
> }
>
> @@ -1401,6 +1467,7 @@ __releases(&info->lock)
> info->fbops->fb_release(info,1);
> module_put(info->fbops->owner);
> mutex_unlock(&info->lock);
> + put_framebuffer_info(info);
> return 0;
> }
>
> @@ -1549,6 +1616,7 @@ register_framebuffer(struct fb_info *fb_info)
> fb_info->node = i;
> mutex_init(&fb_info->lock);
> mutex_init(&fb_info->mm_lock);
> + fb_info->ref_count = 1;
>
> fb_info->dev = device_create(fb_class, fb_info->device,
> MKDEV(FB_MAJOR, i), NULL, "fb%d", i);
> @@ -1592,7 +1660,6 @@ register_framebuffer(struct fb_info *fb_info)
> return 0;
> }
>
> -
> /**
> * unregister_framebuffer - releases a frame buffer device
> * @fb_info: frame buffer info structure
> @@ -1627,6 +1694,16 @@ unregister_framebuffer(struct fb_info *fb_info)
> return -ENODEV;
> event.info = fb_info;
> ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
> + if (!ret) {
> + mutex_lock(&fb_info->mm_lock);
> + /*
> + * We must prevent any operations for this transition, we
> + * already have info->lock so grab the info->mm_lock to hold
> + * the remainder.
> + */
> + fb_info->state = FBINFO_STATE_REMOVED;
> + mutex_unlock(&fb_info->mm_lock);
> + }
> unlock_fb_info(fb_info);
>
> if (ret) {
> @@ -1646,8 +1723,7 @@ unregister_framebuffer(struct fb_info *fb_info)
> fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
>
> /* this may free fb info */
> - if (fb_info->fbops->fb_destroy)
> - fb_info->fbops->fb_destroy(fb_info);
> + put_framebuffer_info(fb_info);
> done:
> return ret;
> }
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index df728c1..60de3fa 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -834,6 +834,7 @@ struct fb_tile_ops {
> struct fb_info {
> int node;
> int flags;
> + int ref_count;
> struct mutex lock; /* Lock for open/release/ioctl funcs */
> struct mutex mm_lock; /* Lock for fb_mmap and smem_* fields */
> struct fb_var_screeninfo var; /* Current var */
> @@ -873,6 +874,7 @@ struct fb_info {
> void *pseudo_palette; /* Fake palette of 16 colors */
> #define FBINFO_STATE_RUNNING 0
> #define FBINFO_STATE_SUSPENDED 1
> +#define FBINFO_STATE_REMOVED 2
> u32 state; /* Hardware state i.e suspend */
> void *fbcon_par; /* fbcon use-only private area */
> /* From here on everything is device dependent */

2011-05-11 16:14:51

by Tim Gardner

Subject: Re: [PATCH V3] fbcon -- fix race between open and removal of framebuffers

On 05/10/2011 11:44 PM, Bruno Pr?mont wrote:
> On Tue, 10 May 2011 Tim Gardner<[email protected]> wrote:
>> From ca3ef33e2235c88856a6257c0be63192ab56c678 Mon Sep 17 00:00:00 2001
>> From: Andy Whitcroft<[email protected]>
>> Date: Thu, 29 Jul 2010 16:48:20 +0100
>> Subject: [PATCH] fbcon -- fix race between open and removal of framebuffers
>>
>> Currently there is no locking for updates to the registered_fb list.
>> This allows an open through /dev/fbN to pick up a registered framebuffer
>> pointer in parallel with it being released, as happens when a conflicting
>> framebuffer is ejected or on module unload. There is also no reference
>> counting on the framebuffer descriptor which is referenced from all open
>> files, leading to references to released or reused memory to persist on
>> these open files.
>>
>> This patch adds a reference count to the framebuffer descriptor to prevent
>> it from being released until after all pending opens are closed. This
>> allows the pending opens to detect the closed status and unmap themselves.
>> It also adds locking to the framebuffer lookup path, locking it against
>> the removal path such that it is possible to atomically lookup and take a
>> reference to the descriptor. It also adds locking to the read and write
>> paths which currently could access the framebuffer descriptor after it
>> has been freed. Finally it moves the device to FBINFO_STATE_REMOVED to
>> indicate that all access should be errored for this device.
>
> What framebuffer drivers was this patch tested with? Just x86 with
> mainstream GPU (intel/amd/nvidia KMS) in compination with vgafb/vesafb or
> did it see some testing with other framebuffers like those from embedded
> world?
>

This patch is also in all of the armel (OMAP3/OMAP4) kernels.

> Otherwise a much smaller (memory leaking) patch would be to just change
> vesafb/vgafb to not free their fb_info after unregistration as was suggested
> by Alan Cox.
>

Sure, I suppose thats possible, but this is the patch that I have working.

<snip>

>
> This only partially protects the list and count as two concurrent
> framebuffer registrations do still race against each other.
> For the issue addressed by this patch I don't think it makes sense to
> have this spinlock at all as it's only used in get_framebuffer_info()
> and in put_framebuffer_info() and put_framebuffer_info() doesn't even
> look at registered_fb or num_registered_fb.
> Such a spinlock makes sense in a separate patch that really protects
> all access to registered_fb or num_registered_fb, be it during framebuffer
> (un)registration or during access from fbcon.
>

Our goal was merely to stop the user space open/close races. I agree
that the framebuffer registration list needs more orthogonal protection,
but that is going to be a much larger patch.

rtg
--
Tim Gardner [email protected]

2011-05-11 16:24:00

by Bruno Prémont

Subject: Re: [PATCH V3] fbcon -- fix race between open and removal of framebuffers

On Wed, 11 May 2011 16:09:29 Tim Gardner wrote:
> On 05/10/2011 11:44 PM, Bruno Prémont wrote:
> > On Tue, 10 May 2011 Tim Gardner<[email protected]> wrote:
> > This only partially protects the list and count as two concurrent
> > framebuffer registrations do still race against each other.
> > For the issue addressed by this patch I don't think it makes sense to
> > have this spinlock at all as it's only used in get_framebuffer_info()
> > and in put_framebuffer_info() and put_framebuffer_info() doesn't even
> > look at registered_fb or num_registered_fb.
> > Such a spinlock makes sense in a separate patch that really protects
> > all access to registered_fb or num_registered_fb, be it during framebuffer
> > (un)registration or during access from fbcon.
> >
>
> Our goal was merely to stop the user space open/close races. I agree
> that the framebuffer registration list needs more orthogonal protection,
> but that is going to be a much larger patch.

I know that such a protection needs a much larger patch. (that would be
for 2.6.40 or 2.6.41, I have preparing patches for that cooking)

My main issue for tis patch is that the comment reads as if spinlock was
protecting registered_fb[] and num_registered_fb. So changing the
comment would be a good thing (say it protects fb_info->ref_count).
Later patch can then protect registered_fb against concurrent
framebuffer registrations.

Bruno