[PATCH] fbcon: Clean up fbcon data in fb_info on FB_EVENT_FB_UNBIND with 0 fbs

[PATCH] fbcon: Clean up fbcon data in fb_info on FB_EVENT_FB_UNBIND with 0 fbs

[Keith Packard]

When FB_EVENT_FB_UNBIND is sent, fbcon has two paths, one path taken
when there is another frame buffer to switch any affected vcs to and
another path when there isn't.

In the case where there is another frame buffer to use,
fbcon_fb_unbind calls set_con2fb_map to remap all of the affected vcs
to the replacement frame buffer. set_con2fb_map will eventually call
con2fb_release_oldinfo when the last vcs gets unmapped from the old
frame buffer.

con2fb_release_oldinfo frees the fbcon data that is hooked off of the
fb_info structure, including the cursor timer.

In the case where there isn't another frame buffer to use,
fbcon_fb_unbind simply calls fbcon_unbind, which doesn't clear the
con2fb_map or free the fbcon data hooked from the fb_info
structure. In particular, it doesn't stop the cursor blink timer. When
the fb_info structure is then freed, we end up with a timer queue
pointing into freed memory and "bad things" start happening.

This patch first changes con2fb_release_oldinfo so that it can take a
NULL pointer for the new frame buffer, but still does all of the
deallocation and cursor timer cleanup.

Finally, the patch tries to replicate some of what set_con2fb_map does
by clearing the con2fb_map for the affected vcs and calling the
modified con2fb_release_info function to clean up the fb_info structure.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/video/console/fbcon.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index a92783e..ba9e149 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -787,7 +787,7 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
 		  newinfo in an undefined state. Thus, a call to
 		  fb_set_par() may be needed for the newinfo.
 		*/
-		if (newinfo->fbops->fb_set_par) {
+		if (newinfo && newinfo->fbops->fb_set_par) {
 			ret = newinfo->fbops->fb_set_par(newinfo);
 
 			if (ret)
@@ -3056,8 +3056,31 @@ static int fbcon_fb_unbind(int idx)
 			if (con2fb_map[i] = idx)
 				set_con2fb_map(i, new_idx, 0);
 		}
-	} else
+	} else {
+		struct fb_info *info = registered_fb[idx];
+
+		/* This is sort of like set_con2fb_map, except it maps
+		 * the consoles to no device and then releases the
+		 * oldinfo to free memory and cancel the cursor blink
+		 * timer. I can imagine this just becoming part of
+		 * set_con2fb_map where new_idx is -1
+		 */
+		for (i = first_fb_vc; i <= last_fb_vc; i++) {
+			if (con2fb_map[i] = idx) {
+				con2fb_map[i] = -1;
+				if (!search_fb_in_map(idx)) {
+					ret = con2fb_release_oldinfo(vc_cons[i].d,
+								     info, NULL, i,
+								     idx, 0);
+					if (ret) {
+						con2fb_map[i] = idx;
+						return ret;
+					}
+				}
+			}
+		}
 		ret = fbcon_unbind();
+	}
 
 	return ret;
 }
-- 
1.8.5.1

Re: [PATCH] fbcon: Clean up fbcon data in fb_info on FB_EVENT_FB_UNBIND with 0 fbs

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

Keith Packard <keithp@keithp.com> writes:

> When FB_EVENT_FB_UNBIND is sent, fbcon has two paths, one path taken
> when there is another frame buffer to switch any affected vcs to and
> another path when there isn't.

What I meant to attach to this bug report but clearly failed was that I think
this case essentially *always* occurs if you have a generic frame buffer
driver loaded before a 'real' driver is loaded. In my case, this is
efifb. So, we have a reference to freed memory hanging out on a timer
list.

I'm really unsure why this hasn't caused more grief, but I can say that
it was greatly aggravated by adding 'fbcon=vc:2-6' to the kernel command
line, which limits fbcon to supporting consoles only on vt 2-6.

Definitely one of those 'how could this ever have worked, and why hasn't
my machine been crashing every day' moments when I read through the
code.

As the patch indicates, I'm not sure this patch is actually what we
want, but I'd love to know if I've isolated the root cause correctly and
then figure out what patch we actually want.

For my own part, I've got a happy customer with the patch, which is
definitely a nice way to end the day.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

[PATCH] fbcon: Clean up fbcon data in fb_info on FB_EVENT_FB_UNBIND with 0 fbs

[Keith Packard]

When FB_EVENT_FB_UNBIND is sent, fbcon has two paths, one path taken
when there is another frame buffer to switch any affected vcs to and
another path when there isn't.

In the case where there is another frame buffer to use,
fbcon_fb_unbind calls set_con2fb_map to remap all of the affected vcs
to the replacement frame buffer. set_con2fb_map will eventually call
con2fb_release_oldinfo when the last vcs gets unmapped from the old
frame buffer.

con2fb_release_oldinfo frees the fbcon data that is hooked off of the
fb_info structure, including the cursor timer.

In the case where there isn't another frame buffer to use,
fbcon_fb_unbind simply calls fbcon_unbind, which doesn't clear the
con2fb_map or free the fbcon data hooked from the fb_info
structure. In particular, it doesn't stop the cursor blink timer. When
the fb_info structure is then freed, we end up with a timer queue
pointing into freed memory and "bad things" start happening.

This patch first changes con2fb_release_oldinfo so that it can take a
NULL pointer for the new frame buffer, but still does all of the
deallocation and cursor timer cleanup.

Finally, the patch tries to replicate some of what set_con2fb_map does
by clearing the con2fb_map for the affected vcs and calling the
modified con2fb_release_info function to clean up the fb_info structure.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/video/console/fbcon.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index cd8a802..9297a9b 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -759,7 +759,7 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
 		  newinfo in an undefined state. Thus, a call to
 		  fb_set_par() may be needed for the newinfo.
 		*/
-		if (newinfo->fbops->fb_set_par) {
+		if (newinfo && newinfo->fbops->fb_set_par) {
 			ret = newinfo->fbops->fb_set_par(newinfo);
 
 			if (ret)
@@ -3028,8 +3028,31 @@ static int fbcon_fb_unbind(int idx)
 			if (con2fb_map[i] = idx)
 				set_con2fb_map(i, new_idx, 0);
 		}
-	} else
+	} else {
+		struct fb_info *info = registered_fb[idx];
+
+		/* This is sort of like set_con2fb_map, except it maps
+		 * the consoles to no device and then releases the
+		 * oldinfo to free memory and cancel the cursor blink
+		 * timer. I can imagine this just becoming part of
+		 * set_con2fb_map where new_idx is -1
+		 */
+		for (i = first_fb_vc; i <= last_fb_vc; i++) {
+			if (con2fb_map[i] = idx) {
+				con2fb_map[i] = -1;
+				if (!search_fb_in_map(idx)) {
+					ret = con2fb_release_oldinfo(vc_cons[i].d,
+								     info, NULL, i,
+								     idx, 0);
+					if (ret) {
+						con2fb_map[i] = idx;
+						return ret;
+					}
+				}
+			}
+		}
 		ret = fbcon_unbind();
+	}
 
 	return ret;
 }
-- 
1.8.5.2

Re: [PATCH] fbcon: Clean up fbcon data in fb_info on FB_EVENT_FB_UNBIND with 0 fbs

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

Hi,

On 20/01/14 23:31, Keith Packard wrote:
> When FB_EVENT_FB_UNBIND is sent, fbcon has two paths, one path taken
> when there is another frame buffer to switch any affected vcs to and
> another path when there isn't.
> 
> In the case where there is another frame buffer to use,
> fbcon_fb_unbind calls set_con2fb_map to remap all of the affected vcs
> to the replacement frame buffer. set_con2fb_map will eventually call
> con2fb_release_oldinfo when the last vcs gets unmapped from the old
> frame buffer.
> 
> con2fb_release_oldinfo frees the fbcon data that is hooked off of the
> fb_info structure, including the cursor timer.
> 
> In the case where there isn't another frame buffer to use,
> fbcon_fb_unbind simply calls fbcon_unbind, which doesn't clear the
> con2fb_map or free the fbcon data hooked from the fb_info
> structure. In particular, it doesn't stop the cursor blink timer. When
> the fb_info structure is then freed, we end up with a timer queue
> pointing into freed memory and "bad things" start happening.
> 
> This patch first changes con2fb_release_oldinfo so that it can take a
> NULL pointer for the new frame buffer, but still does all of the
> deallocation and cursor timer cleanup.
> 
> Finally, the patch tries to replicate some of what set_con2fb_map does
> by clearing the con2fb_map for the affected vcs and calling the
> modified con2fb_release_info function to clean up the fb_info structure.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/video/console/fbcon.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)

Thanks, queued for 3.15.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]