[PATCH][FBDEV] Fix copy_to/from_user in fbmem.c:fb_read/write

[PATCH][FBDEV] Fix copy_to/from_user in fbmem.c:fb_read/write

[Antonino A. Daplas]

Hi,

This patch fixes a prblem reported by David S. Miller <davem@redhat.com>

"I just noticed that fb_{read,write}() uses copy_*_user() with
the kernel buffer being the frame buffer.  It needs to use
the proper device address accessor functions."

Tony

Signed-off-by: Antonino Daplas <adaplas@pol.net>
---

diff -uprN linux-2.6.9-rc1-mm1-orig/drivers/video/fbmem.c linux-2.6.9-rc1-mm1/drivers/video/fbmem.c
--- linux-2.6.9-rc1-mm1-orig/drivers/video/fbmem.c	2004-08-27 05:24:32.000000000 +0800
+++ linux-2.6.9-rc1-mm1/drivers/video/fbmem.c	2004-08-27 05:25:02.200341392 +0800
@@ -878,6 +878,8 @@ fb_read(struct file *file, char __user *
 	struct inode *inode = file->f_dentry->d_inode;
 	int fbidx = iminor(inode);
 	struct fb_info *info = registered_fb[fbidx];
+	u32 *buffer, *dst, *src;
+	int c, i, cnt = 0, err = 0;
 
 	if (!info || ! info->screen_base)
 		return -ENODEV;
@@ -894,18 +896,40 @@ fb_read(struct file *file, char __user *
 	    count = info->fix.smem_len;
 	if (count + p > info->fix.smem_len)
 		count = info->fix.smem_len - p;
+
+	cnt = 0;
+	buffer = kmalloc((count > 64 * 1024) ? 64 * 1024 : count,
+			 GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	src = (u32 *) (info->screen_base + p);
+
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
-	if (count) {
-	    char *base_addr;
-
-	    base_addr = info->screen_base;
-	    count -= copy_to_user(buf, base_addr+p, count);
-	    if (!count)
-		return -EFAULT;
-	    *ppos += count;
+	
+	while (count) {
+		c  = (count > 64 * 1024) ? 64 * 1024 : count;
+		dst = buffer;
+		for (i = c >> 2; i--; ) 
+			*dst++ = fb_readl(src++);
+		if (c & 3) {
+			for (i = c & 3; i--;) 
+				*((u8 *)dst++) = fb_readb((u8 *) src++);
+		}
+		
+		if (copy_to_user(buf, buffer, c)) {
+			err = -EFAULT;
+			break;
+		}
+		*ppos += c;
+		buf += c;
+		cnt += c;
+		count -= c;
 	}
-	return count;
+
+	kfree(buffer);
+	return (err) ? err : cnt;
 }
 
 static ssize_t
@@ -915,7 +939,8 @@ fb_write(struct file *file, const char _
 	struct inode *inode = file->f_dentry->d_inode;
 	int fbidx = iminor(inode);
 	struct fb_info *info = registered_fb[fbidx];
-	int err;
+	u32 *buffer, *dst, *src;
+	int c, i, cnt = 0, err;
 
 	if (!info || !info->screen_base)
 		return -ENODEV;
@@ -935,19 +960,38 @@ fb_write(struct file *file, const char _
 	    count = info->fix.smem_len - p;
 	    err = -ENOSPC;
 	}
+	cnt = 0;
+	buffer = kmalloc((count > 64 * 1024) ? 64 * 1024 : count,
+			 GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+	
+	dst = (u32 *) (info->screen_base + p);
+
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
-	if (count) {
-	    char *base_addr;
 
-	    base_addr = info->screen_base;
-	    count -= copy_from_user(base_addr+p, buf, count);
-	    *ppos += count;
-	    err = -EFAULT;
+	while (count) {
+		c = (count > 64 * 1024) ? 64 * 1024 : count;
+		src = buffer;
+		if (copy_from_user(src, buf, c)) {
+			err = -EFAULT;
+			break;
+		}
+		for (i = c >> 2; i--; )
+			fb_writel(*src++, dst++);
+		if (c & 3) {
+			for (i = c & 3; i--; )
+				fb_writeb(*(u8 *)src++, (u8 *)dst++);
+		}
+		*ppos += c;
+		buf += c;
+		cnt += c;
+		count -= c;
 	}
-	if (count)
-		return count;
-	return err;
+	kfree(buffer);
+
+	return (err) ? err : cnt;
 }
 
 #ifdef CONFIG_KMOD






-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click

Re: [PATCH][FBDEV] Fix copy_to/from_user in fbmem.c:fb_read/write

[David S. Miller]

On Mon, 30 Aug 2004 10:47:28 +0800
"Antonino A. Daplas" <adaplas@hotpop.com> wrote:

> This patch fixes a prblem reported by David S. Miller <davem@redhat.com>
> 
> "I just noticed that fb_{read,write}() uses copy_*_user() with
> the kernel buffer being the frame buffer.  It needs to use
> the proper device address accessor functions."

Looks great Tony, thanks for fixing this.


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click

Re: [PATCH][FBDEV] Fix copy_to/from_user in fbmem.c:fb_read/write

[Geert Uytterhoeven]

On Mon, 30 Aug 2004, Antonino A. Daplas wrote:
> This patch fixes a prblem reported by David S. Miller <davem@redhat.com>
>
> "I just noticed that fb_{read,write}() uses copy_*_user() with
> the kernel buffer being the frame buffer.  It needs to use
> the proper device address accessor functions."
>
> Tony
>
> Signed-off-by: Antonino Daplas <adaplas@pol.net>
> ---
>
> diff -uprN linux-2.6.9-rc1-mm1-orig/drivers/video/fbmem.c linux-2.6.9-rc1-mm1/drivers/video/fbmem.c
> --- linux-2.6.9-rc1-mm1-orig/drivers/video/fbmem.c	2004-08-27 05:24:32.000000000 +0800
> +++ linux-2.6.9-rc1-mm1/drivers/video/fbmem.c	2004-08-27 05:25:02.200341392 +0800
> @@ -878,6 +878,8 @@ fb_read(struct file *file, char __user *
>  	struct inode *inode = file->f_dentry->d_inode;
>  	int fbidx = iminor(inode);
>  	struct fb_info *info = registered_fb[fbidx];
> +	u32 *buffer, *dst, *src;
> +	int c, i, cnt = 0, err = 0;
>
>  	if (!info || ! info->screen_base)
>  		return -ENODEV;
> @@ -894,18 +896,40 @@ fb_read(struct file *file, char __user *
>  	    count = info->fix.smem_len;
>  	if (count + p > info->fix.smem_len)
>  		count = info->fix.smem_len - p;
> +
> +	cnt = 0;
> +	buffer = kmalloc((count > 64 * 1024) ? 64 * 1024 : count,
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +			 GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;

Woops, what's the probability of a 64 kiB kmalloc() to fail?

    [...]

> +	while (count) {
> +		c  = (count > 64 * 1024) ? 64 * 1024 : count;
> +		dst = buffer;
> +		for (i = c >> 2; i--; )
> +			*dst++ = fb_readl(src++);
> +		if (c & 3) {
> +			for (i = c & 3; i--;)
> +				*((u8 *)dst++) = fb_readb((u8 *) src++);
> +		}
> +
> +		if (copy_to_user(buf, buffer, c)) {
> +			err = -EFAULT;
> +			break;
> +		}
> +		*ppos += c;
> +		buf += c;
> +		cnt += c;
> +		count -= c;
>  	}

I don't expect the penalty for always using a buffer of PAGE_SIZE to be very
large. Or am I missing something?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click

Re: [PATCH][FBDEV] Fix copy_to/from_user in fbmem.c:fb_read/write

[Andrew Morton]

Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> > +
>  > +	cnt = 0;
>  > +	buffer = kmalloc((count > 64 * 1024) ? 64 * 1024 : count,
>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  > +			 GFP_KERNEL);
>  > +	if (!buffer)
>  > +		return -ENOMEM;
> 
>  Woops, what's the probability of a 64 kiB kmalloc() to fail?

Relatively high.

>      [...]
> 
>  > +	while (count) {
>  > +		c  = (count > 64 * 1024) ? 64 * 1024 : count;
>  > +		dst = buffer;
>  > +		for (i = c >> 2; i--; )
>  > +			*dst++ = fb_readl(src++);
>  > +		if (c & 3) {
>  > +			for (i = c & 3; i--;)
>  > +				*((u8 *)dst++) = fb_readb((u8 *) src++);
>  > +		}
>  > +
>  > +		if (copy_to_user(buf, buffer, c)) {
>  > +			err = -EFAULT;
>  > +			break;
>  > +		}
>  > +		*ppos += c;
>  > +		buf += c;
>  > +		cnt += c;
>  > +		count -= c;
>  >  	}
> 
>  I don't expect the penalty for always using a buffer of PAGE_SIZE to be very
>  large. Or am I missing something?

The 64k copy buffer will most likely be slower than using an 8k one.  it
depends, of course, upon the vintage of the CPU.

4k or 8k should be fine here.


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click

Re: [PATCH][FBDEV] Fix copy_to/from_user in fbmem.c:fb_read/write

[Antonino A. Daplas]

On Monday 30 August 2004 17:10, Geert Uytterhoeven wrote:
>
> I don't expect the penalty for always using a buffer of PAGE_SIZE to be
> very large. Or am I missing something?
>

You're absolutely correct, there is no significant performance penalty whether
using a small or large-sized buffer.

Will resubmit a patch.

Tony




-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click