generic_file_write code segment in 2.2.18

generic_file_write code segment in 2.2.18

[Asang K Dani]

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)?

Please post/CC your comments to me if this is not of general interest
to the list and is too silly to be answered there.

regards,
Asang Dani

__________________________________________________
Do You Yahoo!?
Yahoo! Photos - Share your holiday photos online!
http://photos.yahoo.com/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: generic_file_write code segment in 2.2.18

[Stephen C. Tweedie]

Hi,

On Thu, Jan 04, 2001 at 08:51:37AM +0100, Andi Kleen wrote:
> On Wed, Jan 03, 2001 at 09:29:48PM -0800, Asang K Dani wrote:
> 
> 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).  

But in this case, _any_ non-zero number of bytes copied is a success,
because it indicates that we have dirtied a portion of the page cache.

> Correct would be:
> 
> 	
> --- linux-work/mm/filemap.c-o	Wed Jan  3 17:37:27 2001
>  		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);

No, because then you'd be skipping the updatepage() call if we took a
fault mid-copy after copying some data.  That would imply you had
dirtied the page cache without an updatepage().

The current behaviour should just result in a short IO, which should
be fine.

--Stephen
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: generic_file_write code segment in 2.2.18

[Andi Kleen]

On Thu, Jan 04, 2001 at 10:42:34PM +0000, Stephen C. Tweedie wrote:
> No, because then you'd be skipping the updatepage() call if we took a
> fault mid-copy after copying some data.  That would imply you had
> dirtied the page cache without an updatepage().
> 
> The current behaviour should just result in a short IO, which should
> be fine.

The problem is that the short write is not reported to the caller, even when only
zero bytes are copied (the page is corrupted anyways though because cfu 
zeros the uncopied rest).  

-Andi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: generic_file_write code segment in 2.2.18

[Asang K Dani]

--- Andi Kleen <ak@suse.de> wrote:
> On Thu, Jan 04, 2001 at 10:42:34PM +0000, Stephen C. Tweedie wrote:
> > No, because then you'd be skipping the updatepage() call if we
> took a
> > fault mid-copy after copying some data.  That would imply you had
> > dirtied the page cache without an updatepage().
> > 
> > The current behaviour should just result in a short IO, which
> should
> > be fine.
> 
> The problem is that the short write is not reported to the caller,
> even when only zero bytes are copied (the page is corrupted anyways
> though because cfu  zeros the uncopied rest).  

I think it will be reported to caller, because when cfu copies 0
bytes,

   bytes -= copy_from_user(dest, buf, bytes);

will make 'bytes' zero. Since 'bytes' is 'zero' updatepage will not
be called and status retains value '-EFAULT' and it breaks out of
the while loop immediately.

> 
> -Andi

asang..

__________________________________________________
Do You Yahoo!?
Yahoo! Photos - Share your holiday photos online!
http://photos.yahoo.com/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: generic_file_write code segment in 2.2.18

[Andi Kleen]

On Thu, Jan 04, 2001 at 02:58:58PM -0800, Asang K Dani wrote:
> --- Andi Kleen <ak@suse.de> wrote:
> > On Thu, Jan 04, 2001 at 10:42:34PM +0000, Stephen C. Tweedie wrote:
> > > No, because then you'd be skipping the updatepage() call if we
> > took a
> > > fault mid-copy after copying some data.  That would imply you had
> > > dirtied the page cache without an updatepage().
> > > 
> > > The current behaviour should just result in a short IO, which
> > should
> > > be fine.
> > 
> > The problem is that the short write is not reported to the caller,
> > even when only zero bytes are copied (the page is corrupted anyways
> > though because cfu  zeros the uncopied rest).  
> 
> I think it will be reported to caller, because when cfu copies 0
> bytes,
> 
>    bytes -= copy_from_user(dest, buf, bytes);
> 
> will make 'bytes' zero. Since 'bytes' is 'zero' updatepage will not
> be called and status retains value '-EFAULT' and it breaks out of
> the while loop immediately.

Right for zero it is handled, but not for 1 byte copied but the rest zeroed
(which is a severe IO error) 


-Andi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: generic_file_write code segment in 2.2.18

[Asang K Dani]

--- Andi Kleen <ak@suse.de> wrote:
> On Thu, Jan 04, 2001 at 02:58:58PM -0800, Asang K Dani wrote:
> > --- Andi Kleen <ak@suse.de> wrote:
> > > On Thu, Jan 04, 2001 at 10:42:34PM +0000, Stephen C. Tweedie
> wrote:
> > > > No, because then you'd be skipping the updatepage() call if
> we
> > > took a
> > > > fault mid-copy after copying some data.  That would imply you
> had
> > > > dirtied the page cache without an updatepage().
> > > > 
> > > > The current behaviour should just result in a short IO, which
> > > should
> > > > be fine.
> > > 
> > > The problem is that the short write is not reported to the
> caller,
> > > even when only zero bytes are copied (the page is corrupted
> anyways
> > > though because cfu  zeros the uncopied rest).  
> > 
> > I think it will be reported to caller, because when cfu copies 0
> > bytes,
> > 
> >    bytes -= copy_from_user(dest, buf, bytes);
> > 
> > will make 'bytes' zero. Since 'bytes' is 'zero' updatepage will
> not
> > be called and status retains value '-EFAULT' and it breaks out of
> > the while loop immediately.
> 
> Right for zero it is handled, but not for 1 byte copied but the
> rest zeroed
> (which is a severe IO error) 
>

For all those cases (when bytes != 0 after cfu), 'status' will be
overwritten by the return value of 'updatepage'.
 
> -Andi

asang..


__________________________________________________
Do You Yahoo!?
Yahoo! Photos - Share your holiday photos online!
http://photos.yahoo.com/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/