Commit efc3acbc authored by Daniel Vetter's avatar Daniel Vetter
Browse files

fbcon: Maintain a private array of fb_info 



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: default avatarSam Ravnborg <sam@ravnborg.org>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20220405210335.3434130-18-daniel.vetter@ffwll.ch
parent c75300b5

drivers/video/fbdev/core/fbcon.c

+43 −39
Original line number Original line Diff line number Diff line
@@ -86,10 +86,6 @@ 
 * - fbcon state itself is protected by the console_lock, and the code does a

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

 *   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

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

 *   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

 *   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];

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[MAX_NR_CONSOLES];

static signed char con2fb_map_boot[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();

	WARN_CONSOLE_UNLOCKED();





	/*

	return fbcon_registered_fb[con2fb_map[console]];

	 * 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]];

}

}





static int logo_lines;

static int logo_lines;
@@ -518,7 +516,7 @@ static int do_fbcon_takeover(int show_logo) 
{

{

	int err, i;

	int err, i;





	if (!num_registered_fb)

	if (!fbcon_num_registered_fb)

		return -ENODEV;

		return -ENODEV;





	if (!show_logo)

	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;

	struct vc_data *vc = vc_cons[unit].d;

	int oldidx = con2fb_map[unit];

	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;

	struct fb_info *oldinfo = NULL;

	int found, err = 0, show_logo;

	int found, err = 0, show_logo;
@@ -839,7 +837,7 @@ static int set_con2fb_map(int unit, int newidx, int user) 
	}

	}





	if (oldidx != -1)

	if (oldidx != -1)

		oldinfo = registered_fb[oldidx];

		oldinfo = fbcon_registered_fb[oldidx];





	found = search_fb_in_map(newidx);

	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.

	 *  If num_registered_fb is zero, this is a call for the dummy part.

	 *  The frame buffer devices weren't initialized yet.

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

		return display_desc;

	/*

	/*

	 * Instead of blindly using registered_fb[0], we use info_idx, set by

	 * Instead of blindly using registered_fb[0], we use info_idx, set by

	 * fbcon_fb_registered();

	 * fbcon_fb_registered();

	 */

	 */

	info = registered_fb[info_idx];

	info = fbcon_registered_fb[info_idx];

	if (!info)

	if (!info)

		return NULL;

		return NULL;
@@ -1150,9 +1148,9 @@ static void fbcon_release_all(void) 
	struct fb_info *info;

	struct fb_info *info;

	int i, j, mapped;

	int i, j, mapped;





	for_each_registered_fb(i) {

	fbcon_for_each_registered_fb(i) {

		mapped = 0;

		mapped = 0;

		info = registered_fb[i];

		info = fbcon_registered_fb[i];





		for (j = first_fb_vc; j <= last_fb_vc; j++) {

		for (j = first_fb_vc; j <= last_fb_vc; j++) {

			if (con2fb_map[j] == i) {

			if (con2fb_map[j] == i) {
@@ -1179,7 +1177,7 @@ static void fbcon_deinit(struct vc_data *vc) 
	if (idx == -1)

	if (idx == -1)

		goto finished;

		goto finished;





	info = registered_fb[idx];

	info = fbcon_registered_fb[idx];





	if (!info)

	if (!info)

		goto finished;

		goto finished;
@@ -2098,9 +2096,9 @@ static int fbcon_switch(struct vc_data *vc) 
	 *

	 *

	 * info->currcon = vc->vc_num;

	 * info->currcon = vc->vc_num;

	 */

	 */

	for_each_registered_fb(i) {

	fbcon_for_each_registered_fb(i) {

		if (registered_fb[i]->fbcon_par) {

		if (fbcon_registered_fb[i]->fbcon_par) {

			struct fbcon_ops *o = registered_fb[i]->fbcon_par;

			struct fbcon_ops *o = fbcon_registered_fb[i]->fbcon_par;





			o->currcon = vc->vc_num;

			o->currcon = vc->vc_num;

		}

		}
@@ -2745,7 +2743,7 @@ int fbcon_mode_deleted(struct fb_info *info, 
		j = con2fb_map[i];

		j = con2fb_map[i];

		if (j == -1)

		if (j == -1)

			continue;

			continue;

		fb_info = registered_fb[j];

		fb_info = fbcon_registered_fb[j];

		if (fb_info != info)

		if (fb_info != info)

			continue;

			continue;

		p = &fb_display[i];

		p = &fb_display[i];
@@ -2801,7 +2799,7 @@ void fbcon_fb_unbind(struct fb_info *info) 
				set_con2fb_map(i, new_idx, 0);

				set_con2fb_map(i, new_idx, 0);

		}

		}

	} else {

	} 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

		/* This is sort of like set_con2fb_map, except it maps

		 * the consoles to no device and then releases the

		 * the consoles to no device and then releases the
@@ -2831,6 +2829,9 @@ void fbcon_fb_unregistered(struct fb_info *info) 




	console_lock();

	console_lock();





	fbcon_registered_fb[info->node] = NULL;

	fbcon_num_registered_fb--;



	if (deferred_takeover) {

	if (deferred_takeover) {

		console_unlock();

		console_unlock();

		return;

		return;
@@ -2845,7 +2846,7 @@ void fbcon_fb_unregistered(struct fb_info *info) 
	if (idx == info_idx) {

	if (idx == info_idx) {

		info_idx = -1;

		info_idx = -1;





		for_each_registered_fb(i) {

		fbcon_for_each_registered_fb(i) {

			info_idx = i;

			info_idx = i;

			break;

			break;

		}

		}
@@ -2861,7 +2862,7 @@ void fbcon_fb_unregistered(struct fb_info *info) 
	if (primary_device == idx)

	if (primary_device == idx)

		primary_device = -1;

		primary_device = -1;





	if (!num_registered_fb)

	if (!fbcon_num_registered_fb)

		do_unregister_con_driver(&fb_con);

		do_unregister_con_driver(&fb_con);

	console_unlock();

	console_unlock();

}

}
@@ -2936,6 +2937,9 @@ int fbcon_fb_registered(struct fb_info *info) 
	else

	else

		atomic_inc(&ignore_console_lock_warning);

		atomic_inc(&ignore_console_lock_warning);





	fbcon_registered_fb[info->node] = info;

	fbcon_num_registered_fb++;



	idx = info->node;

	idx = info->node;

	fbcon_select_primary(info);

	fbcon_select_primary(info);
@@ -3055,9 +3059,9 @@ int fbcon_set_con2fb_map_ioctl(void __user *argp) 
		return -EINVAL;

		return -EINVAL;

	if (con2fb.framebuffer >= FB_MAX)

	if (con2fb.framebuffer >= FB_MAX)

		return -EINVAL;

		return -EINVAL;

	if (!registered_fb[con2fb.framebuffer])

	if (!fbcon_registered_fb[con2fb.framebuffer])

		request_module("fb%d", con2fb.framebuffer);

		request_module("fb%d", con2fb.framebuffer);

	if (!registered_fb[con2fb.framebuffer]) {

	if (!fbcon_registered_fb[con2fb.framebuffer]) {

		return -EINVAL;

		return -EINVAL;

	}

	}
@@ -3124,10 +3128,10 @@ static ssize_t store_rotate(struct device *device, 
	console_lock();

	console_lock();

	idx = con2fb_map[fg_console];

	idx = con2fb_map[fg_console];





	if (idx == -1 || registered_fb[idx] == NULL)

	if (idx == -1 || fbcon_registered_fb[idx] == NULL)

		goto err;

		goto err;





	info = registered_fb[idx];

	info = fbcon_registered_fb[idx];

	rotate = simple_strtoul(buf, last, 0);

	rotate = simple_strtoul(buf, last, 0);

	fbcon_rotate(info, rotate);

	fbcon_rotate(info, rotate);

err:

err:
@@ -3146,10 +3150,10 @@ static ssize_t store_rotate_all(struct device *device, 
	console_lock();

	console_lock();

	idx = con2fb_map[fg_console];

	idx = con2fb_map[fg_console];





	if (idx == -1 || registered_fb[idx] == NULL)

	if (idx == -1 || fbcon_registered_fb[idx] == NULL)

		goto err;

		goto err;





	info = registered_fb[idx];

	info = fbcon_registered_fb[idx];

	rotate = simple_strtoul(buf, last, 0);

	rotate = simple_strtoul(buf, last, 0);

	fbcon_rotate_all(info, rotate);

	fbcon_rotate_all(info, rotate);

err:

err:
@@ -3166,10 +3170,10 @@ static ssize_t show_rotate(struct device *device, 
	console_lock();

	console_lock();

	idx = con2fb_map[fg_console];

	idx = con2fb_map[fg_console];





	if (idx == -1 || registered_fb[idx] == NULL)

	if (idx == -1 || fbcon_registered_fb[idx] == NULL)

		goto err;

		goto err;





	info = registered_fb[idx];

	info = fbcon_registered_fb[idx];

	rotate = fbcon_get_rotate(info);

	rotate = fbcon_get_rotate(info);

err:

err:

	console_unlock();

	console_unlock();
@@ -3186,10 +3190,10 @@ static ssize_t show_cursor_blink(struct device *device, 
	console_lock();

	console_lock();

	idx = con2fb_map[fg_console];

	idx = con2fb_map[fg_console];





	if (idx == -1 || registered_fb[idx] == NULL)

	if (idx == -1 || fbcon_registered_fb[idx] == NULL)

		goto err;

		goto err;





	info = registered_fb[idx];

	info = fbcon_registered_fb[idx];

	ops = info->fbcon_par;

	ops = info->fbcon_par;





	if (!ops)

	if (!ops)
@@ -3212,10 +3216,10 @@ static ssize_t store_cursor_blink(struct device *device, 
	console_lock();

	console_lock();

	idx = con2fb_map[fg_console];

	idx = con2fb_map[fg_console];





	if (idx == -1 || registered_fb[idx] == NULL)

	if (idx == -1 || fbcon_registered_fb[idx] == NULL)

		goto err;

		goto err;





	info = registered_fb[idx];

	info = fbcon_registered_fb[idx];





	if (!info->fbcon_par)

	if (!info->fbcon_par)

		goto err;

		goto err;
@@ -3275,8 +3279,8 @@ static void fbcon_register_existing_fbs(struct work_struct *work) 
	deferred_takeover = false;

	deferred_takeover = false;

	logo_shown = FBCON_LOGO_DONTSHOW;

	logo_shown = FBCON_LOGO_DONTSHOW;





	for_each_registered_fb(i)

	fbcon_for_each_registered_fb(i)

		fbcon_fb_registered(registered_fb[i]);

		fbcon_fb_registered(fbcon_registered_fb[i]);





	console_unlock();

	console_unlock();

}

}