lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <4EDEAFDC.8000308@linux.vnet.ibm.com>
Date:	Tue, 06 Dec 2011 17:14:20 -0700
From:	Allison Henderson <achender@...ux.vnet.ibm.com>
To:	Andy Whitcroft <apw@...onical.com>
CC:	"Theodore Ts'o" <tytso@....edu>,
	Andreas Dilger <adilger.kernel@...ger.ca>,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] ext4: correct partial write discard size calculation

Hi Andy,

We are currently doing some debugging in this region of the code, and 
the code that you are modifying here may need to come back out, but I 
will try your patch in my sand box too.  Thx!

Allison Henderson

On 12/06/2011 03:19 PM, Andy Whitcroft wrote:
> When copying large numbers of files we are seeing occasional write failures
> with errno EINVAL.  These are being returned from ext4_da_write_end()
> when attempting to discard the end portion of a partial write.  The error
> is detected and reported by the page index check below:
>
>      int ext4_discard_partial_page_buffers_no_lock(handle_t *handle,
> 		    struct inode *inode, struct page *page, loff_t from,
> 		    loff_t length, int flags)
>      {
>      [...]
>          if (index != page->index)
>                  return -EINVAL;
>      [...]
>
> This code was introduced by the commit below:
>
>    commit 02fac1297eb3f471a27368271aadd285548297b0
>    Author: Allison Henderson<achender@...ux.vnet.ibm.com>
>    Date:   Tue Sep 6 21:53:01 2011 -0400
>
>      ext4: fix partial page writes
>
> This error is triggering when a write occurs at pos == 0 and results in
> 0 bytes being written (copied == 0):
>
>      page_len = PAGE_CACHE_SIZE -
>                      ((pos + copied - 1)&  (PAGE_CACHE_SIZE - 1));
>      if (page_len>  0) {
>              ret = ext4_discard_partial_page_buffers_no_lock(handle,
>                      inode, page, pos + copied - 1, page_len,
>      [...]
>
> In this case we will calculate that we need to clear out only one byte of
> the page.  As we are aligned at the page boundary and wrote 0 bytes we
> actually need to clear the entire page.  Also note that when we attempt
> to apply the discard we will apply it at offset -1 (0 + 0 - 1), which is
> the wrong place:
>
>      page_len = 4096 - ((0 + 0 - 1)&  4095)
>      page_len = 1
>
> Firstly fix up the offset calculation.  Once this is done the erroring
> case will correctly believe that the entire page needs to be discarded.
> However in this case we did not actually write to the page so the page
> is not instantiated and no discard is required.  So also only apply the
> discard where we are not discarding the entire page.
>
> Signed-off-by: Andy Whitcroft<apw@...onical.com>
> ---
>   fs/ext4/inode.c |    6 +++---
>   1 files changed, 3 insertions(+), 3 deletions(-)
>
> This issue is most easily reproducible within a VM on a fast, lightly
> loaded host.  In that configuration I can trigger a failure with about
> 1/2GB of medium sized files (.debs in this case).  Without the patch
> the copy will fail 'EINVAL' 99% of the time, always failing within two
> iterations.  With the patch I have run 100 iterations of the same copy
> without failure.
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 240f6e2..c137168 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2533,11 +2533,11 @@ static int ext4_da_write_end(struct file *file,
>   							page, fsdata);
>
>   	page_len = PAGE_CACHE_SIZE -
> -			((pos + copied - 1)&  (PAGE_CACHE_SIZE - 1));
> +			((pos + copied)&  (PAGE_CACHE_SIZE - 1));
>
> -	if (page_len>  0) {
> +	if (page_len>  0&&  page_len<  PAGE_CACHE_SIZE) {
>   		ret = ext4_discard_partial_page_buffers_no_lock(handle,
> -			inode, page, pos + copied - 1, page_len,
> +			inode, page, pos + copied, page_len,
>   			EXT4_DISCARD_PARTIAL_PG_ZERO_UNMAPPED);
>   	}
>

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ