Re: [PATCH] fbcon -- fix race between open and removal of
From: Bruno Prémont <bonbons@linux-vserver.org>
Date: 2011-05-05 18:30:28
Also in:
lkml
On Thu, 05 May 2011 tim.gardner@canonical.com wrote:
From: Andy Whitcroft <apw@canonical.com> 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 <apw@canonical.com> Acked-by: Stefan Bader <redacted> Signed-off-by: Leann Ogasawara <redacted> Signed-off-by: Tim Gardner <redacted> --- drivers/video/fbmem.c | 132 ++++++++++++++++++++++++++++++++++++++----------- include/linux/fb.h | 2 + 2 files changed, 105 insertions(+), 29 deletions(-)
...
quoted hunk ↗ jump to hunk
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 */