linux-ext4 - Re: [PATCH] ext4: Make ext4_writepages() resilient to i

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20130802142324.GA20484@quack.suse.cz>
Date:	Fri, 2 Aug 2013 16:23:24 +0200
From:	Jan Kara <jack@...e.cz>
To:	Ted Tso <tytso@....edu>
Cc:	linux-ext4@...r.kernel.org, Dave Jones <davej@...hat.com>,
	Zheng Liu <gnehzuil.liu@...il.com>, Jan Kara <jack@...e.cz>
Subject: Re: [PATCH] ext4: Make ext4_writepages() resilient to i_size changes

On Thu 01-08-13 00:42:12, Jan Kara wrote:
> Inode size can arbitrarily change while writeback is in progress. This
> can have various strange effects when we use one value of i_size for one
> decision during writeback and another value of i_size for a different
> decision during writeback. In particular a check for lblk < blocks in
> mpage_map_and_submit_buffers() causes problems when i_size is reduced
> while writeback is running because we can end up not using all blocks
> we've allocated. Thus these blocks are leaked and also delalloc
> accounting gets wrong manifesting as a warning like:
> 
> ext4_da_release_space:1333: ext4_da_release_space: ino 12, to_free 1
> with only 0 reserved data blocks
> 
> The problem can happen only when blocksize < pagesize because the check
> for size is performed only after the first iteration of the mapping
> loop.
> 
> Fix the problem by removing the size check from the mapping loop. We
> have an extent allocated so we have to use it all before checking for
> i_size. We may call add_page_bufs_to_extent() unnecessarily but that
> function won't do anything if passed block number is beyond file size.
> 
> Also to avoid future surprises like this sample inode size when
> starting writeback in ext4_writepages() and then use this sampled size
> throughout the writeback call stack.
  Ted, please disregard this patch. It is buggy. I'll send a better fix
soon.

								Honza

> 
> Reported-by: Dave Jones <davej@...hat.com>
> Reported-by: Zheng Liu <gnehzuil.liu@...il.com>
> Signed-off-by: Jan Kara <jack@...e.cz>
> ---
>  fs/ext4/inode.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ba33c67..8bd0240 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1413,6 +1413,8 @@ static void ext4_da_page_release_reservation(struct page *page,
>  struct mpage_da_data {
>  	struct inode *inode;
>  	struct writeback_control *wbc;
> +	loff_t i_size;		/* Inode size can change while we do writeback.
> +				 * Use one fixed value to make things simpler */
>  
>  	pgoff_t first_page;	/* The first page to write */
>  	pgoff_t next_page;	/* Current page to examine */
> @@ -1943,7 +1945,7 @@ static bool add_page_bufs_to_extent(struct mpage_da_data *mpd,
>  				    ext4_lblk_t lblk)
>  {
>  	struct inode *inode = mpd->inode;
> -	ext4_lblk_t blocks = (i_size_read(inode) + (1 << inode->i_blkbits) - 1)
> +	ext4_lblk_t blocks = (mpd->i_size + (1 << inode->i_blkbits) - 1)
>  							>> inode->i_blkbits;
>  
>  	do {
> @@ -1968,12 +1970,11 @@ static bool add_page_bufs_to_extent(struct mpage_da_data *mpd,
>  static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
>  {
>  	int len;
> -	loff_t size = i_size_read(mpd->inode);
>  	int err;
>  
>  	BUG_ON(page->index != mpd->first_page);
> -	if (page->index == size >> PAGE_CACHE_SHIFT)
> -		len = size & ~PAGE_CACHE_MASK;
> +	if (page->index == mpd->i_size >> PAGE_CACHE_SHIFT)
> +		len = mpd->i_size & ~PAGE_CACHE_MASK;
>  	else
>  		len = PAGE_CACHE_SIZE;
>  	clear_page_dirty_for_io(page);
> @@ -2006,8 +2007,6 @@ static int mpage_map_and_submit_buffers(struct mpage_da_data *mpd)
>  	struct inode *inode = mpd->inode;
>  	struct buffer_head *head, *bh;
>  	int bpp_bits = PAGE_CACHE_SHIFT - inode->i_blkbits;
> -	ext4_lblk_t blocks = (i_size_read(inode) + (1 << inode->i_blkbits) - 1)
> -							>> inode->i_blkbits;
>  	pgoff_t start, end;
>  	ext4_lblk_t lblk;
>  	sector_t pblock;
> @@ -2052,8 +2051,7 @@ static int mpage_map_and_submit_buffers(struct mpage_da_data *mpd)
>  					bh->b_blocknr = pblock++;
>  				}
>  				clear_buffer_unwritten(bh);
> -			} while (++lblk < blocks &&
> -				 (bh = bh->b_this_page) != head);
> +			} while (lblk++, (bh = bh->b_this_page) != head);
>  
>  			/*
>  			 * FIXME: This is going to break if dioread_nolock
> @@ -2200,7 +2198,11 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>  			return err;
>  	} while (map->m_len);
>  
> -	/* Update on-disk size after IO is submitted */
> +	/*
> +	 * Update on-disk size after IO is submitted. Here we cannot use
> +	 * mpd->i_size as we must avoid increasing i_disksize when racing
> +	 * truncate already set it to a small value.
> +	 */
>  	disksize = ((loff_t)mpd->first_page) << PAGE_CACHE_SHIFT;
>  	if (disksize > i_size_read(inode))
>  		disksize = i_size_read(inode);
> @@ -2453,6 +2455,7 @@ static int ext4_writepages(struct address_space *mapping,
>  
>  	mpd.inode = inode;
>  	mpd.wbc = wbc;
> +	mpd.i_size = i_size_read(inode);
>  	ext4_io_submit_init(&mpd.io_submit, wbc);
>  retry:
>  	if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages)
> -- 
> 1.8.1.4
> 
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html