Re: [PATCH 3.2 009/152] ext4: check for extents that wrap around

[Vegard Nossum]

On 11/14/2016 01:14 AM, Ben Hutchings wrote:
> 3.2.84-rc1 review patch.  If anyone has any objections, please let me know.

Just a general comment on stable review workflow, really:

It might be more useful to send the diff-of-diffs with the upstream
commit so I can easily see if you had any conflicts when cherry-picking
this and how they were resolved.

That's generally much more interesting than just the plain patch, where
I can't really tell if there were any changes at all (or conversely,
much more boring in case there were no changes, and thus easier to
review).

If you could push this commit to git before sending the review, you
could also include a command that I can use to quickly do the
diff-of-diffs myself without having to download and apply the patch (or
look for it), e.g. something like (using the 3.12 stable commit vs
upstream):

"""
diff -yw \
  <(echo upstream; git log -p -W f70749c^..f70749c) \
  <(echo 3.2; git log -p -W 33234c6^..33234c6)
"""

At least that would make it a lot easier for me (and I suspect other
casual stable contributors) to glance at a stable review email and tell
if the backport is correct or not. It should be pretty easy to script on
your end(s) for the benefit of everybody.

Just my 2 cents. Thanks,


Vegard

> ------------------
>
> From: Vegard Nossum <vegard.nossum@xxxxxxxxxx>
>
> commit f70749ca42943faa4d4dcce46dfdcaadb1d0c4b6 upstream.
>
> An extent with lblock = 4294967295 and len = 1 will pass the
> ext4_valid_extent() test:
>
> 	ext4_lblk_t last = lblock + len - 1;
>
> 	if (len == 0 || lblock > last)
> 		return 0;
>
> since last = 4294967295 + 1 - 1 = 4294967295. This would later trigger
> the BUG_ON(es->es_lblk + es->es_len < es->es_lblk) in ext4_es_end().
>
> We can simplify it by removing the - 1 altogether and changing the test
> to use lblock + len <= lblock, since now if len = 0, then lblock + 0 ==
> lblock and it fails, and if len > 0 then lblock + len > lblock in order
> to pass (i.e. it doesn't overflow).
>
> Fixes: 5946d0893 ("ext4: check for overlapping extents in ext4_valid_extent_entries()")
> Fixes: 2f974865f ("ext4: check for zero length extent explicitly")
> Cc: Eryu Guan <guaneryu@xxxxxxxxx>
> Signed-off-by: Phil Turnbull <phil.turnbull@xxxxxxxxxx>
> Signed-off-by: Vegard Nossum <vegard.nossum@xxxxxxxxxx>
> Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>
> Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> ---
>  fs/ext4/extents.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -319,9 +319,13 @@ static int ext4_valid_extent(struct inod
>  	ext4_fsblk_t block = ext4_ext_pblock(ext);
>  	int len = ext4_ext_get_actual_len(ext);
>  	ext4_lblk_t lblock = le32_to_cpu(ext->ee_block);
> -	ext4_lblk_t last = lblock + len - 1;
>
> -	if (len == 0 || lblock > last)
> +	/*
> +	 * We allow neither:
> +	 *  - zero length
> +	 *  - overflow/wrap-around
> +	 */
> +	if (lblock + len <= lblock)
>  		return 0;
>  	return ext4_data_block_valid(EXT4_SB(inode->i_sb), block, len);
>  }