linux-ext4 - Re: [RFC PATCH 2/2] ext4: fix up ext4_try_to_write_inline

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] [day] [month] [year] [list]

Message-ID: <20170607035442.GB594@zzz>
Date:   Tue, 6 Jun 2017 20:54:42 -0700
From:   Eric Biggers <ebiggers3@...il.com>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        gnehzuil.liu@...il.com
Subject: Re: [RFC PATCH 2/2] ext4: fix up ext4_try_to_write_inline_data()

On Mon, Jun 05, 2017 at 08:03:59PM -0400, Theodore Ts'o wrote:
> There were a number of bugs in ext4_try_to_write_inline_data() and the
> ext4_convert_inline_data_to_extent() function (which was only used by
> ext4_try_to_write_inline_data).
> 
> For ext4_convert_inline_data_to_extent():
> 
> * It didn't handle the dioread_nolock case correctly
>   * It didn't convert the extent tree entry from unwritten to written.
>   * It didn't correctly handle racing DIO reads
> * It didn't handle data=journal case correctly -- it doesn't follow
>   the block modification correctly by failing to call
>   ext4_handle_dirty_metadata() on the data block.
> 
> We fix this by eliminating ext4_convert_inline_data_to_extent()
> completely, and use reg_convert_inline_data_nolock() since it has been
> fixed to be Completely Correct (tm).  :-)
> 

Is ext4_da_convert_inline_data_to_extent() broken too?

>  /*
>   * Try to write data in the inode.
>   * If the inode has inline data, check whether the new write can be
> @@ -662,13 +553,19 @@ int ext4_try_to_write_inline_data(struct address_space *mapping,
>  	struct page *page;
>  	struct ext4_iloc iloc;
>  
> -	if (pos + len > ext4_get_max_inline_size(inode))
> -		goto convert;
> -
>  	ret = ext4_get_inode_loc(inode, &iloc);
>  	if (ret)
>  		return ret;
>  
> +	page = grab_cache_page_write_begin(mapping, 0, flags);
> +	if (!page) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +

Likewise, doesn't the page lock rank below transaction start?  Also this jumps
to 'out' which looks at 'handle' before it's been initialized.

Eric