[PATCH v2 17/19] fbcon: Maintain a private array of fb_info
From: Daniel Vetter
Date: Tue Feb 08 2022 - 17:39:28 EST
Accessing the one in fbmem.c without taking the right locks is a bad
idea. Instead maintain our own private copy, which is fully protected
by console_lock() (like everything else in fbcon.c). That copy is
serialized through fbcon_fb_registered/unregistered() calls.
Also this means we do not need to hold a full fb_info reference, which
is nice because doing so would mean a refcount loop between the
console and the fb_info. But it's also not nice since it means
console_lock() must be held absolutely everywhere. Well strictly
speaking we could still try to do some refcounting games again by
calling get_fb_info before we drop the console_lock. But things will
get tricky.
Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
Cc: Daniel Vetter <daniel@xxxxxxxx>
Cc: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
Cc: Claudio Suarez <cssk@xxxxxxxx>
Cc: Du Cheng <ducheng2@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
drivers/video/fbdev/core/fbcon.c | 82 +++++++++++++++++---------------
1 file changed, 43 insertions(+), 39 deletions(-)
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 944f514c77ec..6a7d470beec7 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -86,10 +86,6 @@
* - fbcon state itself is protected by the console_lock, and the code does a
* pretty good job at making sure that lock is held everywhere it's needed.
*
- * - access to the registered_fb array is entirely unprotected. This should use
- * proper object lifetime handling, i.e. get/put_fb_info. This also means
- * switching from indices to proper pointers for fb_info everywhere.
- *
* - fbcon doesn't bother with fb_lock/unlock at all. This is buggy, since it
* means concurrent access to the same fbdev from both fbcon and userspace
* will blow up. To fix this all fbcon calls from fbmem.c need to be moved out
@@ -107,6 +103,13 @@ enum {
static struct fbcon_display fb_display[MAX_NR_CONSOLES];
+struct fb_info *fbcon_registered_fb[FB_MAX];
+int fbcon_num_registered_fb;
+
+#define fbcon_for_each_registered_fb(i) \
+ for (i = 0; WARN_CONSOLE_UNLOCKED(), i < FB_MAX; i++) \
+ if (!fbcon_registered_fb[i]) {} else
+
static signed char con2fb_map[MAX_NR_CONSOLES];
static signed char con2fb_map_boot[MAX_NR_CONSOLES];
@@ -114,12 +117,7 @@ static struct fb_info *fbcon_info_from_console(int console)
{
WARN_CONSOLE_UNLOCKED();
- /*
- * Note that only con2fb_map is protected by the console lock,
- * registered_fb is protected by a separate mutex. This lookup can
- * therefore race.
- */
- return registered_fb[con2fb_map[console]];
+ return fbcon_registered_fb[con2fb_map[console]];
}
static int logo_lines;
@@ -518,7 +516,7 @@ static int do_fbcon_takeover(int show_logo)
{
int err, i;
- if (!num_registered_fb)
+ if (!fbcon_num_registered_fb)
return -ENODEV;
if (!show_logo)
@@ -821,7 +819,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
{
struct vc_data *vc = vc_cons[unit].d;
int oldidx = con2fb_map[unit];
- struct fb_info *info = registered_fb[newidx];
+ struct fb_info *info = fbcon_registered_fb[newidx];
struct fb_info *oldinfo = NULL;
int found, err = 0, show_logo;
@@ -839,7 +837,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
}
if (oldidx != -1)
- oldinfo = registered_fb[oldidx];
+ oldinfo = fbcon_registered_fb[oldidx];
found = search_fb_in_map(newidx);
@@ -931,13 +929,13 @@ static const char *fbcon_startup(void)
* If num_registered_fb is zero, this is a call for the dummy part.
* The frame buffer devices weren't initialized yet.
*/
- if (!num_registered_fb || info_idx == -1)
+ if (!fbcon_num_registered_fb || info_idx == -1)
return display_desc;
/*
* Instead of blindly using registered_fb[0], we use info_idx, set by
* fbcon_fb_registered();
*/
- info = registered_fb[info_idx];
+ info = fbcon_registered_fb[info_idx];
if (!info)
return NULL;
@@ -1150,9 +1148,9 @@ static void fbcon_release_all(void)
struct fb_info *info;
int i, j, mapped;
- for_each_registered_fb(i) {
+ fbcon_for_each_registered_fb(i) {
mapped = 0;
- info = registered_fb[i];
+ info = fbcon_registered_fb[i];
for (j = first_fb_vc; j <= last_fb_vc; j++) {
if (con2fb_map[j] == i) {
@@ -1179,7 +1177,7 @@ static void fbcon_deinit(struct vc_data *vc)
if (idx == -1)
goto finished;
- info = registered_fb[idx];
+ info = fbcon_registered_fb[idx];
if (!info)
goto finished;
@@ -2098,9 +2096,9 @@ static int fbcon_switch(struct vc_data *vc)
*
* info->currcon = vc->vc_num;
*/
- for_each_registered_fb(i) {
- if (registered_fb[i]->fbcon_par) {
- struct fbcon_ops *o = registered_fb[i]->fbcon_par;
+ fbcon_for_each_registered_fb(i) {
+ if (fbcon_registered_fb[i]->fbcon_par) {
+ struct fbcon_ops *o = fbcon_registered_fb[i]->fbcon_par;
o->currcon = vc->vc_num;
}
@@ -2745,7 +2743,7 @@ int fbcon_mode_deleted(struct fb_info *info,
j = con2fb_map[i];
if (j == -1)
continue;
- fb_info = registered_fb[j];
+ fb_info = fbcon_registered_fb[j];
if (fb_info != info)
continue;
p = &fb_display[i];
@@ -2801,7 +2799,7 @@ void fbcon_fb_unbind(struct fb_info *info)
set_con2fb_map(i, new_idx, 0);
}
} else {
- struct fb_info *info = registered_fb[idx];
+ struct fb_info *info = fbcon_registered_fb[idx];
/* This is sort of like set_con2fb_map, except it maps
* the consoles to no device and then releases the
@@ -2831,6 +2829,9 @@ void fbcon_fb_unregistered(struct fb_info *info)
console_lock();
+ fbcon_registered_fb[info->node] = NULL;
+ fbcon_num_registered_fb--;
+
if (deferred_takeover) {
console_unlock();
return;
@@ -2845,7 +2846,7 @@ void fbcon_fb_unregistered(struct fb_info *info)
if (idx == info_idx) {
info_idx = -1;
- for_each_registered_fb(i) {
+ fbcon_for_each_registered_fb(i) {
info_idx = i;
break;
}
@@ -2861,7 +2862,7 @@ void fbcon_fb_unregistered(struct fb_info *info)
if (primary_device == idx)
primary_device = -1;
- if (!num_registered_fb)
+ if (!fbcon_num_registered_fb)
do_unregister_con_driver(&fb_con);
console_unlock();
}
@@ -2936,6 +2937,9 @@ int fbcon_fb_registered(struct fb_info *info)
else
atomic_inc(&ignore_console_lock_warning);
+ fbcon_registered_fb[info->node] = info;
+ fbcon_num_registered_fb++;
+
idx = info->node;
fbcon_select_primary(info);
@@ -3055,9 +3059,9 @@ int fbcon_set_con2fb_map_ioctl(void __user *argp)
return -EINVAL;
if (con2fb.framebuffer >= FB_MAX)
return -EINVAL;
- if (!registered_fb[con2fb.framebuffer])
+ if (!fbcon_registered_fb[con2fb.framebuffer])
request_module("fb%d", con2fb.framebuffer);
- if (!registered_fb[con2fb.framebuffer]) {
+ if (!fbcon_registered_fb[con2fb.framebuffer]) {
return -EINVAL;
}
@@ -3124,10 +3128,10 @@ static ssize_t store_rotate(struct device *device,
console_lock();
idx = con2fb_map[fg_console];
- if (idx == -1 || registered_fb[idx] == NULL)
+ if (idx == -1 || fbcon_registered_fb[idx] == NULL)
goto err;
- info = registered_fb[idx];
+ info = fbcon_registered_fb[idx];
rotate = simple_strtoul(buf, last, 0);
fbcon_rotate(info, rotate);
err:
@@ -3146,10 +3150,10 @@ static ssize_t store_rotate_all(struct device *device,
console_lock();
idx = con2fb_map[fg_console];
- if (idx == -1 || registered_fb[idx] == NULL)
+ if (idx == -1 || fbcon_registered_fb[idx] == NULL)
goto err;
- info = registered_fb[idx];
+ info = fbcon_registered_fb[idx];
rotate = simple_strtoul(buf, last, 0);
fbcon_rotate_all(info, rotate);
err:
@@ -3166,10 +3170,10 @@ static ssize_t show_rotate(struct device *device,
console_lock();
idx = con2fb_map[fg_console];
- if (idx == -1 || registered_fb[idx] == NULL)
+ if (idx == -1 || fbcon_registered_fb[idx] == NULL)
goto err;
- info = registered_fb[idx];
+ info = fbcon_registered_fb[idx];
rotate = fbcon_get_rotate(info);
err:
console_unlock();
@@ -3186,10 +3190,10 @@ static ssize_t show_cursor_blink(struct device *device,
console_lock();
idx = con2fb_map[fg_console];
- if (idx == -1 || registered_fb[idx] == NULL)
+ if (idx == -1 || fbcon_registered_fb[idx] == NULL)
goto err;
- info = registered_fb[idx];
+ info = fbcon_registered_fb[idx];
ops = info->fbcon_par;
if (!ops)
@@ -3212,10 +3216,10 @@ static ssize_t store_cursor_blink(struct device *device,
console_lock();
idx = con2fb_map[fg_console];
- if (idx == -1 || registered_fb[idx] == NULL)
+ if (idx == -1 || fbcon_registered_fb[idx] == NULL)
goto err;
- info = registered_fb[idx];
+ info = fbcon_registered_fb[idx];
if (!info->fbcon_par)
goto err;
@@ -3275,8 +3279,8 @@ static void fbcon_register_existing_fbs(struct work_struct *work)
deferred_takeover = false;
logo_shown = FBCON_LOGO_DONTSHOW;
- for_each_registered_fb(i)
- fbcon_fb_registered(registered_fb[i]);
+ fbcon_for_each_registered_fb(i)
+ fbcon_fb_registered(fbcon_registered_fb[i]);
console_unlock();
}
--
2.34.1