Re: generic_file_write code segment in 2.2.18

[Andi Kleen]

On Wed, Jan 03, 2001 at 09:29:48PM -0800, Asang K Dani wrote:
> hi everyone,
>    I was trying to understand following piece of code in
> 'generic_file_read' (mm/filemap.c) for 2.2.18 kernel. The code
> segment
> is as follows:
> 
> ----------------------------------------------------------------
> 		dest = (char *) page_address(page) + offset;
> 		if (dest != buf) { /* See comment in
>                                       update_vm_cache_cond. */
> 			bytes -= copy_from_user(dest, buf, bytes);
> 			flush_dcache_page(page_address(page));
> 		}
> 		status = -EFAULT;
> 		if (bytes)
> 			status = inode->i_op->updatepage(file, page,                      
>                       offset, bytes, sync);
> 
>  unlock:
> 		/* Mark it unlocked again and drop the page.. */
> 		clear_bit(PG_locked, &page->flags);
> 		wake_up(&page->wait);
> 		page_cache_release(page);
> 
> 		if (status < 0)
> 			break;
> 
> 		written += status;
> 		count -= status;
> 		pos += status;
> 		buf += status;
> ----------------------------------------------------------------
> copy_from_user returns the number of bytes copied from userspace
> to 'dest'. In case it succeeds, 'bytes' will be set to 0 after the
> call. However, 'status' is set to -EFAULT. So wouldn't it break
> out of the while loop (prematurely)?

The code is buggy as far as I can see. copy_from_user doesn't return the 
number of bytes copied, but the number of bytes not copied when an error
occurs (or 0 on success).  

Correct would be:

	
--- linux-work/mm/filemap.c-o	Wed Jan  3 17:37:27 2001
+++ linux-work/mm/filemap.c	Thu Jan  4 08:48:42 2001
@@ -1685,11 +1685,11 @@
 		 */
 		dest = (char *) page_address(page) + offset;
 		if (dest != buf) { /* See comment in update_vm_cache_cond. */
-			bytes -= copy_from_user(dest, buf, bytes);
+			if (copy_from_user(dest, buf, bytes))
+				status = -EFAULT; 
 			flush_dcache_page(page_address(page));
 		}
-		status = -EFAULT;
-		if (bytes)
+		if (!status)
 			status = inode->i_op->updatepage(file, page, offset, bytes, sync);
 
  unlock:

-Andi