Skip to contentSkip to conversation

vesafb memory size mismatch

PATCH In review Aurelien Jacobs 4 participants 7 messages Oct 1, 2004 – Oct 4, 2004 Framebuffer layer

lore
Share & tools
lore

Aurelien Jacobs

author
#0 lore raw

PATCH 1/1 vesafb memory size mismatch

Hi,

I found a bug in the vesafb driver. The video memory size which is reserved by vesafb do not correspond to the really needed memory. Currently the needed memory is calculated using screen_info.lfb_width * vesafb_defined.bits_per_pixel >> 3 This works with most cards because it's usually equal to vesafb_fix.line_length.
But with some cards (voodoo3 in 24 bits mode) line_length is greater than width * bytes_per_pixel (probably for alignment purpose, line_length = 4096 on voodoo3 800x600-24).
Su currently, with such cards, writting in the second buffer (as memory is allocated for double buffering) can go past the end of reserved memory, overwritting some other memory area and leading to a kernel crash.

So I wrote this tiny patch wich correct this bug. I tested it successfully with different video cards (Matrox, Nvidia, Sis, 3dfx) in different modes.

I don't know if it's the right place to send a patch for vesafb ? Could you please direct me to the right place if it's not ?

Aurel

drivers/video/vesafb.c +1 -1
diff -Naur linux-2.6.8.1.orig/drivers/video/vesafb.c linux-2.6.8.1/drivers/video/vesafb.c
@@ -234,7 +234,7 @@
 	vesafb_fix.line_length = screen_info.lfb_linelength;
 
 	/* Allocate enough memory for double buffering */
-	vesafb_fix.smem_len = screen_info.lfb_width * screen_info.lfb_height * vesafb_defined.bits_per_pixel >> 2;
+	vesafb_fix.smem_len = 2 * screen_info.lfb_linelength * screen_info.lfb_height;
 
 	/* check that we don't remap more memory than old cards have */
 	if (vesafb_fix.smem_len > (screen_info.lfb_size * 65536))
#1 lore raw
Quoted 4 lines · Aurelien Jacobs

Hi,

I found a bug in the vesafb driver. The video memory size which is reserved by vesafb do not correspond to the really needed memory.

Oh, while we are at it, I've hacked a patch as well some time ago. All the size calculation in vesafb is a bit tricky. I've tried to cleanup that a bit and fix some bugs along the way, but never managed to submit it. I think the boundary check cleanups should also fix the bug you've seen.

Ok, here we go ...

The patch does introduce some variables for the different sizes used and add some comments what they are good for, so it's easier to see what is actually going on:

- size_vmode: minimum memory required for the video mode.
- size_total: the total amount of memory we have.
- size_remap: the amount of memory vesafb is going to use (and allocate kernel address space for). By default this is size_vmode*2, so fbcon has some room to use.

For ressource allocation + mtrr entries size_total video memory is used through. The later is important for X11 performance, as the X-Server will try to create a mtrr entry for the whole video memory and fail in case vesafb creates a small one for size_remap only.

Gerd

drivers/video/vesafb.c +32 -22
Index: linux-2.6.8/drivers/video/vesafb.c
===================================================================
@@ -218,6 +218,9 @@ static int __init vesafb_probe(struct de
 	struct platform_device *dev = to_platform_device(device);
 	struct fb_info *info;
 	int i, err;
+	unsigned int size_vmode;
+	unsigned int size_remap;
+	unsigned int size_total;
 
 	if (screen_info.orig_video_isVGA != VIDEO_TYPE_VLFB)
 		return -ENXIO;

@@ -229,32 +232,37 @@ static int __init vesafb_probe(struct de
 	vesafb_defined.xres = screen_info.lfb_width;
 	vesafb_defined.yres = screen_info.lfb_height;
 	vesafb_fix.line_length = screen_info.lfb_linelength;
-
-	/* Allocate enough memory for double buffering */
-	vesafb_fix.smem_len = screen_info.lfb_width * screen_info.lfb_height * vesafb_defined.bits_per_pixel >> 2;
-
-	/* check that we don't remap more memory than old cards have */
-	if (vesafb_fix.smem_len > (screen_info.lfb_size * 65536))
-		vesafb_fix.smem_len = screen_info.lfb_size * 65536;
-
-	/* Set video size according to vram boot option */
-	if (vram)
-		vesafb_fix.smem_len = vram * 1024 * 1024;
-
 	vesafb_fix.visual   = (vesafb_defined.bits_per_pixel == 8) ?
 		FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_TRUECOLOR;
 
-	/* limit framebuffer size to 16 MB.  Otherwise we'll eat tons of
-	 * kernel address space for nothing if the gfx card has alot of
-	 * memory (>= 128 MB isn't uncommon these days ...) */
-	if (vesafb_fix.smem_len > 16 * 1024 * 1024)
-		vesafb_fix.smem_len = 16 * 1024 * 1024;
+	/*   size_vmode -- that is the amount of memory needed for the
+	 *                 used video mode, i.e. the minimum amount of
+	 *                 memory we need. */
+	size_vmode = (vesafb_defined.xres * vesafb_defined.yres *
+		vesafb_defined.bits_per_pixel) >> 3;
+
+	/*   size_total -- all video memory we have. Used for mtrr
+	 *                 entries and bounds checking. */
+	size_total = screen_info.lfb_size * 65536;
+	if (size_total < size_vmode)
+		size_total = size_vmode;
+	if (vram)
+		size_total = vram * 1024 * 1024;
+
+	/*   size_remap -- the amount of video memory we are going to
+	 *                 use for vesafb.  With modern cards it is no
+	 *                 option to simply use size_total as that
+	 *                 wastes plenty of kernel address space. */
+	size_remap  = size_vmode * 2;
+	if (size_remap > size_total)
+		size_remap = size_total;
+	vesafb_fix.smem_len = size_remap;
 
 #ifndef __i386__
 	screen_info.vesapm_seg = 0;
 #endif
 
-	if (!request_mem_region(vesafb_fix.smem_start, vesafb_fix.smem_len, "vesafb")) {
+	if (!request_mem_region(vesafb_fix.smem_start, size_total, "vesafb")) {
 		printk(KERN_WARNING
 		       "vesafb: abort, cannot reserve video memory at 0x%lx\n",
 			vesafb_fix.smem_start);

@@ -279,8 +287,10 @@ static int __init vesafb_probe(struct de
 		goto err;
 	}
 
-	printk(KERN_INFO "vesafb: framebuffer at 0x%lx, mapped to 0x%p, size %dk\n",
-	       vesafb_fix.smem_start, info->screen_base, vesafb_fix.smem_len/1024);
+	printk(KERN_INFO "vesafb: framebuffer at 0x%lx, mapped to 0x%p, "
+	       "using %dk, total %dk\n",
+	       vesafb_fix.smem_start, info->screen_base,
+	       size_remap/1024, size_total/1024);
 	printk(KERN_INFO "vesafb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
 	       vesafb_defined.xres, vesafb_defined.yres, vesafb_defined.bits_per_pixel, vesafb_fix.line_length, screen_info.pages);
 

@@ -364,7 +374,7 @@ static int __init vesafb_probe(struct de
 	request_region(0x3c0, 32, "vesafb");
 
 	if (mtrr) {
-		int temp_size = vesafb_fix.smem_len;
+		int temp_size = size_total;
 		/* Find the largest power-of-two */
 		while (temp_size & (temp_size - 1))
                 	temp_size &= (temp_size - 1);

@@ -395,7 +405,7 @@ static int __init vesafb_probe(struct de
 	return 0;
 err:
 	framebuffer_release(info);
-	release_mem_region(vesafb_fix.smem_start, vesafb_fix.smem_len);
+	release_mem_region(vesafb_fix.smem_start, size_total);
 	return err;
 }
#2 lore raw
Quoted 12 lines · Gerd Knorr
drivers/video/vesafb.c +12 -0
+	/*   size_vmode -- that is the amount of memory needed for the
+	 *                 used video mode, i.e. the minimum amount of
+	 *                 memory we need. */
+	size_vmode = (vesafb_defined.xres * vesafb_defined.yres *
+		vesafb_defined.bits_per_pixel) >> 3;
+
+	/*   size_total -- all video memory we have. Used for mtrr
+	 *                 entries and bounds checking. */
+	size_total = screen_info.lfb_size * 65536;
+	if (size_total < size_vmode)
+		size_total = size_vmode;
+	if (vram)

drivers/video/vesafb.c static int __init vesafb_probe(struct de · line 259

Quoted 10 lines
drivers/video/vesafb.c +10 -0
+		size_total = vram * 1024 * 1024;
+
+	/*   size_remap -- the amount of video memory we are going to
+	 *                 use for vesafb.  With modern cards it is no
+	 *                 option to simply use size_total as that
+	 *                 wastes plenty of kernel address space. */
+	size_remap  = size_vmode * 2;
+	if (size_remap > size_total)
+		size_remap = size_total;
+	vesafb_fix.smem_len = size_remap;

Probably a typo, but shouldn't it be...

size_remap = size_vmode * 2;
if (vram)

	size_remap = vram * 1024 * 1024;

if (size_remap > size_total)
size_remap = size_total

... so vram doesn't mess up with mtrr and user and fbcon can multibuffer

Quoted 1 line

size_vmode?

Tony

#3 lore raw
Quoted 10 lines

drivers/video/vesafb.c static int __init vesafb_probe(struct de · line 250

Quoted 7 lines
drivers/video/vesafb.c +7 -0
+	/*   size_total -- all video memory we have. Used for mtrr
+	 *                 entries and bounds checking. */
+	size_total = screen_info.lfb_size * 65536;
+	if (size_total < size_vmode)
+		size_total = size_vmode;
+	if (vram)
+		size_total = vram * 1024 * 1024;

Probably a typo, but shouldn't it be...

Quoted 5 lines

size_remap = size_vmode * 2;
if (vram)

	size_remap = vram * 1024 * 1024;

if (size_remap > size_total)
size_remap = size_total

No, it's intentional. Some bioses seem to report bogous values for the total amount of memory, so you can use vram to fixup that. The "size_total < size_vmode" check which is kida silly is there for the very same reason: on a bug-free bios it should never ever trigger, but looks like it is needed neverless.

We might want to add another parameter to allow the user to adjust size_remap through. I'll prepare an updated patch one later today, with other issue (line_length != width * depth) fixed as well.

Gerd

Signature · 1 line
return -ENOSIG;
author
#4 lore raw
In reply to Gerd Knorr
Quoted 4 lines · On 01 Oct 2004 17:48:21 +0200 Gerd Knorr <[email protected]> wrote:
Quoted 2 lines · Aurelien Jacobs

Hi,

Quoted 8 lines
Quoted 2 lines

I found a bug in the vesafb driver. The video memory size which is reserved by vesafb do not correspond to the really needed memory.

Oh, while we are at it, I've hacked a patch as well some time ago. All the size calculation in vesafb is a bit tricky. I've tried to cleanup that a bit and fix some bugs along the way, but never managed to submit it. I think the boundary check cleanups should also fix the bug you've seen.

Unfortunately not :-(
Your patch still keep the same way to calculate minimum memory required for the video mode (based on xres*bytes_per_pixel). It's wrong ! With video cards such as 3dfx in 24 bits mode, line_length is bigger than xres*bytes_per_pixel (probably for alignment pupose). So not enough memory is allocated, leading to a crash.

Here is an updated version of your patch with the right way to calculate size_vmode.

Aurel

drivers/video/vesafb.c +31 -22
diff -aur linux-2.6.8.1.orig/drivers/video/vesafb.c linux-2.6.8.1/drivers/video/vesafb.c
@@ -218,6 +218,9 @@
 	struct platform_device *dev = to_platform_device(device);
 	struct fb_info *info;
 	int i, err;
+	unsigned int size_vmode;
+	unsigned int size_remap;
+	unsigned int size_total;
 
 	if (screen_info.orig_video_isVGA != VIDEO_TYPE_VLFB)
 		return -ENXIO;

@@ -229,32 +232,36 @@
 	vesafb_defined.xres = screen_info.lfb_width;
 	vesafb_defined.yres = screen_info.lfb_height;
 	vesafb_fix.line_length = screen_info.lfb_linelength;
-
-	/* Allocate enough memory for double buffering */
-	vesafb_fix.smem_len = screen_info.lfb_width * screen_info.lfb_height * vesafb_defined.bits_per_pixel >> 2;
-
-	/* check that we don't remap more memory than old cards have */
-	if (vesafb_fix.smem_len > (screen_info.lfb_size * 65536))
-		vesafb_fix.smem_len = screen_info.lfb_size * 65536;
-
-	/* Set video size according to vram boot option */
-	if (vram)
-		vesafb_fix.smem_len = vram * 1024 * 1024;
-
 	vesafb_fix.visual   = (vesafb_defined.bits_per_pixel == 8) ?
 		FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_TRUECOLOR;
 
-	/* limit framebuffer size to 16 MB.  Otherwise we'll eat tons of
-	 * kernel address space for nothing if the gfx card has alot of
-	 * memory (>= 128 MB isn't uncommon these days ...) */
-	if (vesafb_fix.smem_len > 16 * 1024 * 1024)
-		vesafb_fix.smem_len = 16 * 1024 * 1024;
+	/*   size_vmode -- that is the amount of memory needed for the
+	 *                 used video mode, i.e. the minimum amount of
+	 *                 memory we need. */
+	size_vmode = vesafb_fix.line_length * vesafb_defined.yres;
+
+	/*   size_total -- all video memory we have. Used for mtrr
+	 *                 entries and bounds checking. */
+	size_total = screen_info.lfb_size * 65536;
+	if (size_total < size_vmode)
+		size_total = size_vmode;
+	if (vram)
+		size_total = vram * 1024 * 1024;
+
+	/*   size_remap -- the amount of video memory we are going to
+	 *                 use for vesafb.  With modern cards it is no
+	 *                 option to simply use size_total as that
+	 *                 wastes plenty of kernel address space. */
+	size_remap  = size_vmode * 2;
+	if (size_remap > size_total)
+		size_remap = size_total;
+	vesafb_fix.smem_len = size_remap;
 
 #ifndef __i386__
 	screen_info.vesapm_seg = 0;
 #endif
 
-	if (!request_mem_region(vesafb_fix.smem_start, vesafb_fix.smem_len, "vesafb")) {
+	if (!request_mem_region(vesafb_fix.smem_start, size_total, "vesafb")) {
 		printk(KERN_WARNING
 		       "vesafb: abort, cannot reserve video memory at 0x%lx\n",
 			vesafb_fix.smem_start);

@@ -279,8 +286,10 @@
 		goto err;
 	}
 
-	printk(KERN_INFO "vesafb: framebuffer at 0x%lx, mapped to 0x%p, size %dk\n",
-	       vesafb_fix.smem_start, info->screen_base, vesafb_fix.smem_len/1024);
+	printk(KERN_INFO "vesafb: framebuffer at 0x%lx, mapped to 0x%p, "
+	       "using %dk, total %dk\n",
+	       vesafb_fix.smem_start, info->screen_base,
+	       size_remap/1024, size_total/1024);
 	printk(KERN_INFO "vesafb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
 	       vesafb_defined.xres, vesafb_defined.yres, vesafb_defined.bits_per_pixel, vesafb_fix.line_length, screen_info.pages);
 

@@ -364,7 +373,7 @@
 	request_region(0x3c0, 32, "vesafb");
 
 	if (mtrr) {
-		int temp_size = vesafb_fix.smem_len;
+		int temp_size = size_total;
 		/* Find the largest power-of-two */
 		while (temp_size & (temp_size - 1))
                 	temp_size &= (temp_size - 1);

@@ -395,7 +404,7 @@
 	return 0;
 err:
 	framebuffer_release(info);
-	release_mem_region(vesafb_fix.smem_start, vesafb_fix.smem_len);
+	release_mem_region(vesafb_fix.smem_start, size_total);
 	return err;
 }
#5 lore raw

(Gerd re-added to Cc)

Quoted 24 lines · Aurelien Jacobs
Quoted 4 lines · On 01 Oct 2004 17:48:21 +0200 Gerd Knorr <[email protected]> wrote:
Quoted 2 lines · Aurelien Jacobs

Hi,

Quoted 8 lines
Quoted 2 lines

I found a bug in the vesafb driver. The video memory size which is reserved by vesafb do not correspond to the really needed memory.

Oh, while we are at it, I've hacked a patch as well some time ago. All the size calculation in vesafb is a bit tricky. I've tried to cleanup that a bit and fix some bugs along the way, but never managed to submit it. I think the boundary check cleanups should also fix the bug you've seen.

Unfortunately not :-(
Your patch still keep the same way to calculate minimum memory required for the video mode (based on xres*bytes_per_pixel). It's wrong ! With video cards such as 3dfx in 24 bits mode, line_length is bigger than xres*bytes_per_pixel (probably for alignment pupose). So not enough memory is allocated, leading to a crash.

Quoted 3 lines

Here is an updated version of your patch with the right way to calculate size_vmode.

OK, I'll drop the patch which I added to -mm. Could someone please resend the final version when it's all sorted out?

Thanks.

-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal Use IT products in your business? Tell us what you think of them. Give us Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

#6 lore raw
Quoted 25 lines · Andrew Morton

(Gerd re-added to Cc)

Quoted 22 lines · Aurelien Jacobs

On 01 Oct 2004 17:48:21 +0200

Quoted 11 lines · Gerd Knorr
Quoted 4 lines · Aurelien Jacobs

Hi,

I found a bug in the vesafb driver. The video memory size which is reserved by vesafb do not correspond to the really needed memory.

Oh, while we are at it, I've hacked a patch as well some time ago. All the size calculation in vesafb is a bit tricky. I've tried to cleanup that a bit and fix some bugs along the way, but never managed to submit it. I think the boundary check cleanups should also fix the bug you've seen.

Unfortunately not :-(
Your patch still keep the same way to calculate minimum memory required for the video mode (based on xres*bytes_per_pixel). It's wrong ! With video cards such as 3dfx in 24 bits mode, line_length is bigger than xres*bytes_per_pixel (probably for alignment pupose). So not enough memory is allocated, leading to a crash.

Quoted 6 lines
Quoted 2 lines

Here is an updated version of your patch with the right way to calculate size_vmode.

OK, I'll drop the patch which I added to -mm. Could someone please resend the final version when it's all sorted out?

Yes, Aurelien is correct, calculating the memory needed for a mode is more accurate if line_length * yres is used.

I'll submit a patch combining Gerd's, Aurelien's and a few of my changes later today.

Tony

-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal Use IT products in your business? Tell us what you think of them. Give us Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl