[PATCH] e2fsprogs: Don't report uninit extents past EOF invalid

[PATCH] e2fsprogs: Don't report uninit extents past EOF invalid

[Eric Whitney]

Commit d3f32c2db8 caused e2fsck misbehavior during xfstests runs.
It reported that uninitialized extents created by fallocate() at
the end of file with the FALLOC_FL_KEEP_SIZE flag were invalid.
Because FALLOC_FL_KEEP_SIZE does not increase the file size when
an extent is fallocated, an uninitialized extent can legally contain
blocks past the end of file.

The information reported by ext2fs_extent_get() and used by the commit
to determine legal extent ranges is limited by the value of i_size
(determines end_blk in the root extent index), so block values greater
than that containing i_size were reported as invalid.

To fix this, filter out possible invalid extent candidates if they are
uninitialized and extend past the block containing the end of file.

Signed-off-by: Eric Whitney <enwlinux@gmail.com>
---
 e2fsck/pass1.c      |    4 +++-
 lib/ext2fs/ext2fs.h |    1 +
 lib/ext2fs/extent.c |    1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index ba6025b..b84b0d0 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -1892,7 +1892,9 @@ static void scan_extent_node(e2fsck_t ctx, struct problem_context *pctx,
 			problem = PR_1_EXTENT_BAD_START_BLK;
 		else if (extent.e_lblk < start_block)
 			problem = PR_1_OUT_OF_ORDER_EXTENTS;
-		else if (end_block && last_lblk > end_block)
+		else if ((end_block && last_lblk > end_block) &&
+			 (!(extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT &&
+			    last_lblk > info.eof_blk - 1)))
 			problem = PR_1_EXTENT_END_OUT_OF_BOUNDS;
 		else if (is_leaf && extent.e_len == 0)
 			problem = PR_1_EXTENT_LENGTH_ZERO;
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index 311ceda..85f2ac8 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -409,6 +409,7 @@ struct ext2_extent_info {
 	int		bytes_avail;
 	blk64_t		max_lblk;
 	blk64_t		max_pblk;
+	blk64_t         eof_blk;
 	__u32		max_len;
 	__u32		max_uninit_len;
 };
diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index 65bb099..de54319 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -1572,6 +1572,7 @@ errcode_t ext2fs_extent_get_info(ext2_extent_handle_t handle,
 	info->max_depth = handle->max_depth;
 	info->max_lblk = ((__u64) 1 << 32) - 1;
 	info->max_pblk = ((__u64) 1 << 48) - 1;
+	info->eof_blk = handle->path[0].end_blk;
 	info->max_len = (1UL << 15);
 	info->max_uninit_len = (1UL << 15) - 1;
 
-- 
1.7.10.4

Re: [PATCH] e2fsprogs: Don't report uninit extents past EOF invalid

[Eric Sandeen]

On 7/21/13 3:28 PM, Eric Whitney wrote:
> Commit d3f32c2db8 caused e2fsck misbehavior during xfstests runs.
> It reported that uninitialized extents created by fallocate() at
> the end of file with the FALLOC_FL_KEEP_SIZE flag were invalid.
> Because FALLOC_FL_KEEP_SIZE does not increase the file size when
> an extent is fallocated, an uninitialized extent can legally contain
> blocks past the end of file.
> 
> The information reported by ext2fs_extent_get() and used by the commit
> to determine legal extent ranges is limited by the value of i_size
> (determines end_blk in the root extent index), so block values greater
> than that containing i_size were reported as invalid.
> 
> To fix this, filter out possible invalid extent candidates if they are
> uninitialized and extend past the block containing the end of file.

The bummer about this approach is that it won't check for overlap
of any extents past EOF which are unwritten (of which there could be many).

It's probably worth merging now just to take care of the false positives,
but I think it's leaving some potential corruptions undetected.

(corruptions which I'm not yet sure how to detect ...)

-Eric

> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> ---
>  e2fsck/pass1.c      |    4 +++-
>  lib/ext2fs/ext2fs.h |    1 +
>  lib/ext2fs/extent.c |    1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index ba6025b..b84b0d0 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -1892,7 +1892,9 @@ static void scan_extent_node(e2fsck_t ctx, struct problem_context *pctx,
>  			problem = PR_1_EXTENT_BAD_START_BLK;
>  		else if (extent.e_lblk < start_block)
>  			problem = PR_1_OUT_OF_ORDER_EXTENTS;
> -		else if (end_block && last_lblk > end_block)
> +		else if ((end_block && last_lblk > end_block) &&
> +			 (!(extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT &&
> +			    last_lblk > info.eof_blk - 1)))
>  			problem = PR_1_EXTENT_END_OUT_OF_BOUNDS;
>  		else if (is_leaf && extent.e_len == 0)
>  			problem = PR_1_EXTENT_LENGTH_ZERO;
> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> index 311ceda..85f2ac8 100644
> --- a/lib/ext2fs/ext2fs.h
> +++ b/lib/ext2fs/ext2fs.h
> @@ -409,6 +409,7 @@ struct ext2_extent_info {
>  	int		bytes_avail;
>  	blk64_t		max_lblk;
>  	blk64_t		max_pblk;
> +	blk64_t         eof_blk;
>  	__u32		max_len;
>  	__u32		max_uninit_len;
>  };
> diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
> index 65bb099..de54319 100644
> --- a/lib/ext2fs/extent.c
> +++ b/lib/ext2fs/extent.c
> @@ -1572,6 +1572,7 @@ errcode_t ext2fs_extent_get_info(ext2_extent_handle_t handle,
>  	info->max_depth = handle->max_depth;
>  	info->max_lblk = ((__u64) 1 << 32) - 1;
>  	info->max_pblk = ((__u64) 1 << 48) - 1;
> +	info->eof_blk = handle->path[0].end_blk;
>  	info->max_len = (1UL << 15);
>  	info->max_uninit_len = (1UL << 15) - 1;
>  
> 

Re: [PATCH] e2fsprogs: Don't report uninit extents past EOF invalid

[Eric Sandeen]

On 7/21/13 3:28 PM, Eric Whitney wrote:
> Commit d3f32c2db8 caused e2fsck misbehavior during xfstests runs.
> It reported that uninitialized extents created by fallocate() at
> the end of file with the FALLOC_FL_KEEP_SIZE flag were invalid.
> Because FALLOC_FL_KEEP_SIZE does not increase the file size when
> an extent is fallocated, an uninitialized extent can legally contain
> blocks past the end of file.
> 
> The information reported by ext2fs_extent_get() and used by the commit
> to determine legal extent ranges is limited by the value of i_size
> (determines end_blk in the root extent index), so block values greater
> than that containing i_size were reported as invalid.
> 
> To fix this, filter out possible invalid extent candidates if they are
> uninitialized and extend past the block containing the end of file.
> 
> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> ---
>  e2fsck/pass1.c      |    4 +++-
>  lib/ext2fs/ext2fs.h |    1 +
>  lib/ext2fs/extent.c |    1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index ba6025b..b84b0d0 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -1892,7 +1892,9 @@ static void scan_extent_node(e2fsck_t ctx, struct problem_context *pctx,
>  			problem = PR_1_EXTENT_BAD_START_BLK;
>  		else if (extent.e_lblk < start_block)
>  			problem = PR_1_OUT_OF_ORDER_EXTENTS;
> -		else if (end_block && last_lblk > end_block)
> +		else if ((end_block && last_lblk > end_block) &&
> +			 (!(extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT &&
> +			    last_lblk > info.eof_blk - 1)))
>  			problem = PR_1_EXTENT_END_OUT_OF_BOUNDS;
>  		else if (is_leaf && extent.e_len == 0)
>  			problem = PR_1_EXTENT_LENGTH_ZERO;
> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> index 311ceda..85f2ac8 100644
> --- a/lib/ext2fs/ext2fs.h
> +++ b/lib/ext2fs/ext2fs.h
> @@ -409,6 +409,7 @@ struct ext2_extent_info {
>  	int		bytes_avail;
>  	blk64_t		max_lblk;
>  	blk64_t		max_pblk;
> +	blk64_t         eof_blk;
>  	__u32		max_len;
>  	__u32		max_uninit_len;
>  };

I just realized, this affects the ABI, doesn't it?  Hm.

As a hack-around, can probably just use ehandle->path[0].end_blk directly
in scan_extent_node and stash eof_blk locally?

-Eric

> diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
> index 65bb099..de54319 100644
> --- a/lib/ext2fs/extent.c
> +++ b/lib/ext2fs/extent.c
> @@ -1572,6 +1572,7 @@ errcode_t ext2fs_extent_get_info(ext2_extent_handle_t handle,
>  	info->max_depth = handle->max_depth;
>  	info->max_lblk = ((__u64) 1 << 32) - 1;
>  	info->max_pblk = ((__u64) 1 << 48) - 1;
> +	info->eof_blk = handle->path[0].end_blk;
>  	info->max_len = (1UL << 15);
>  	info->max_uninit_len = (1UL << 15) - 1;
>  
> 

Re: [PATCH] e2fsprogs: Don't report uninit extents past EOF invalid

[Eric Sandeen]

On 8/12/13 6:21 PM, Eric Sandeen wrote:
> On 7/21/13 3:28 PM, Eric Whitney wrote:
>> Commit d3f32c2db8 caused e2fsck misbehavior during xfstests runs.
>> It reported that uninitialized extents created by fallocate() at
>> the end of file with the FALLOC_FL_KEEP_SIZE flag were invalid.
>> Because FALLOC_FL_KEEP_SIZE does not increase the file size when
>> an extent is fallocated, an uninitialized extent can legally contain
>> blocks past the end of file.
>>
>> The information reported by ext2fs_extent_get() and used by the commit
>> to determine legal extent ranges is limited by the value of i_size
>> (determines end_blk in the root extent index), so block values greater
>> than that containing i_size were reported as invalid.
>>
>> To fix this, filter out possible invalid extent candidates if they are
>> uninitialized and extend past the block containing the end of file.
>>
>> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
>> ---
>>  e2fsck/pass1.c      |    4 +++-
>>  lib/ext2fs/ext2fs.h |    1 +
>>  lib/ext2fs/extent.c |    1 +
>>  3 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
>> index ba6025b..b84b0d0 100644
>> --- a/e2fsck/pass1.c
>> +++ b/e2fsck/pass1.c
>> @@ -1892,7 +1892,9 @@ static void scan_extent_node(e2fsck_t ctx, struct problem_context *pctx,
>>  			problem = PR_1_EXTENT_BAD_START_BLK;
>>  		else if (extent.e_lblk < start_block)
>>  			problem = PR_1_OUT_OF_ORDER_EXTENTS;
>> -		else if (end_block && last_lblk > end_block)
>> +		else if ((end_block && last_lblk > end_block) &&
>> +			 (!(extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT &&
>> +			    last_lblk > info.eof_blk - 1)))
>>  			problem = PR_1_EXTENT_END_OUT_OF_BOUNDS;
>>  		else if (is_leaf && extent.e_len == 0)
>>  			problem = PR_1_EXTENT_LENGTH_ZERO;
>> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
>> index 311ceda..85f2ac8 100644
>> --- a/lib/ext2fs/ext2fs.h
>> +++ b/lib/ext2fs/ext2fs.h
>> @@ -409,6 +409,7 @@ struct ext2_extent_info {
>>  	int		bytes_avail;
>>  	blk64_t		max_lblk;
>>  	blk64_t		max_pblk;
>> +	blk64_t         eof_blk;
>>  	__u32		max_len;
>>  	__u32		max_uninit_len;
>>  };
> 
> I just realized, this affects the ABI, doesn't it?  Hm.
> 
> As a hack-around, can probably just use ehandle->path[0].end_blk directly
> in scan_extent_node and stash eof_blk locally?

Nope, we can't crack an extent handle, it's an opaque type.

Ned some V2 interfaces now?  :(

> -Eric
> 
>> diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
>> index 65bb099..de54319 100644
>> --- a/lib/ext2fs/extent.c
>> +++ b/lib/ext2fs/extent.c
>> @@ -1572,6 +1572,7 @@ errcode_t ext2fs_extent_get_info(ext2_extent_handle_t handle,
>>  	info->max_depth = handle->max_depth;
>>  	info->max_lblk = ((__u64) 1 << 32) - 1;
>>  	info->max_pblk = ((__u64) 1 << 48) - 1;
>> +	info->eof_blk = handle->path[0].end_blk;
>>  	info->max_len = (1UL << 15);
>>  	info->max_uninit_len = (1UL << 15) - 1;
>>  
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] e2fsprogs: Don't report uninit extents past EOF invalid

[Eric Sandeen]

On 8/12/13 6:28 PM, Eric Sandeen wrote:
> On 8/12/13 6:21 PM, Eric Sandeen wrote:
>> On 7/21/13 3:28 PM, Eric Whitney wrote:
>>> Commit d3f32c2db8 caused e2fsck misbehavior during xfstests runs.
>>> It reported that uninitialized extents created by fallocate() at
>>> the end of file with the FALLOC_FL_KEEP_SIZE flag were invalid.
>>> Because FALLOC_FL_KEEP_SIZE does not increase the file size when
>>> an extent is fallocated, an uninitialized extent can legally contain
>>> blocks past the end of file.
>>>
>>> The information reported by ext2fs_extent_get() and used by the commit
>>> to determine legal extent ranges is limited by the value of i_size
>>> (determines end_blk in the root extent index), so block values greater
>>> than that containing i_size were reported as invalid.
>>>
>>> To fix this, filter out possible invalid extent candidates if they are
>>> uninitialized and extend past the block containing the end of file.
>>>
>>> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
>>> ---
>>>  e2fsck/pass1.c      |    4 +++-
>>>  lib/ext2fs/ext2fs.h |    1 +
>>>  lib/ext2fs/extent.c |    1 +
>>>  3 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
>>> index ba6025b..b84b0d0 100644
>>> --- a/e2fsck/pass1.c
>>> +++ b/e2fsck/pass1.c
>>> @@ -1892,7 +1892,9 @@ static void scan_extent_node(e2fsck_t ctx, struct problem_context *pctx,
>>>  			problem = PR_1_EXTENT_BAD_START_BLK;
>>>  		else if (extent.e_lblk < start_block)
>>>  			problem = PR_1_OUT_OF_ORDER_EXTENTS;
>>> -		else if (end_block && last_lblk > end_block)
>>> +		else if ((end_block && last_lblk > end_block) &&
>>> +			 (!(extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT &&
>>> +			    last_lblk > info.eof_blk - 1)))
>>>  			problem = PR_1_EXTENT_END_OUT_OF_BOUNDS;
>>>  		else if (is_leaf && extent.e_len == 0)
>>>  			problem = PR_1_EXTENT_LENGTH_ZERO;
>>> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
>>> index 311ceda..85f2ac8 100644
>>> --- a/lib/ext2fs/ext2fs.h
>>> +++ b/lib/ext2fs/ext2fs.h
>>> @@ -409,6 +409,7 @@ struct ext2_extent_info {
>>>  	int		bytes_avail;
>>>  	blk64_t		max_lblk;
>>>  	blk64_t		max_pblk;
>>> +	blk64_t         eof_blk;
>>>  	__u32		max_len;
>>>  	__u32		max_uninit_len;
>>>  };
>>
>> I just realized, this affects the ABI, doesn't it?  Hm.
>>
>> As a hack-around, can probably just use ehandle->path[0].end_blk directly
>> in scan_extent_node and stash eof_blk locally?
> 
> Nope, we can't crack an extent handle, it's an opaque type.
> 
> Ned some V2 interfaces now?  :(
> 

or maybe just:

+       eof_blk = (EXT2_I_SIZE(pctx->inode) + ctx->fs->blocksize - 1) >>
+                  EXT2_BLOCK_SIZE_BITS(ctx->fs->super);

unless that's too ugly.

-Eric (done replying to himself for now)

Re: [PATCH] e2fsprogs: Don't report uninit extents past EOF invalid

[Eric Whitney]

* Eric Sandeen <sandeen@redhat.com>:
> On 8/12/13 6:28 PM, Eric Sandeen wrote:
> > On 8/12/13 6:21 PM, Eric Sandeen wrote:
> >> On 7/21/13 3:28 PM, Eric Whitney wrote:
> >>> Commit d3f32c2db8 caused e2fsck misbehavior during xfstests runs.
> >>> It reported that uninitialized extents created by fallocate() at
> >>> the end of file with the FALLOC_FL_KEEP_SIZE flag were invalid.
> >>> Because FALLOC_FL_KEEP_SIZE does not increase the file size when
> >>> an extent is fallocated, an uninitialized extent can legally contain
> >>> blocks past the end of file.
> >>>
> >>> The information reported by ext2fs_extent_get() and used by the commit
> >>> to determine legal extent ranges is limited by the value of i_size
> >>> (determines end_blk in the root extent index), so block values greater
> >>> than that containing i_size were reported as invalid.
> >>>
> >>> To fix this, filter out possible invalid extent candidates if they are
> >>> uninitialized and extend past the block containing the end of file.
> >>>
> >>> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> >>> ---
> >>>  e2fsck/pass1.c      |    4 +++-
> >>>  lib/ext2fs/ext2fs.h |    1 +
> >>>  lib/ext2fs/extent.c |    1 +
> >>>  3 files changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> >>> index ba6025b..b84b0d0 100644
> >>> --- a/e2fsck/pass1.c
> >>> +++ b/e2fsck/pass1.c
> >>> @@ -1892,7 +1892,9 @@ static void scan_extent_node(e2fsck_t ctx, struct problem_context *pctx,
> >>>  			problem = PR_1_EXTENT_BAD_START_BLK;
> >>>  		else if (extent.e_lblk < start_block)
> >>>  			problem = PR_1_OUT_OF_ORDER_EXTENTS;
> >>> -		else if (end_block && last_lblk > end_block)
> >>> +		else if ((end_block && last_lblk > end_block) &&
> >>> +			 (!(extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT &&
> >>> +			    last_lblk > info.eof_blk - 1)))
> >>>  			problem = PR_1_EXTENT_END_OUT_OF_BOUNDS;
> >>>  		else if (is_leaf && extent.e_len == 0)
> >>>  			problem = PR_1_EXTENT_LENGTH_ZERO;
> >>> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> >>> index 311ceda..85f2ac8 100644
> >>> --- a/lib/ext2fs/ext2fs.h
> >>> +++ b/lib/ext2fs/ext2fs.h
> >>> @@ -409,6 +409,7 @@ struct ext2_extent_info {
> >>>  	int		bytes_avail;
> >>>  	blk64_t		max_lblk;
> >>>  	blk64_t		max_pblk;
> >>> +	blk64_t         eof_blk;
> >>>  	__u32		max_len;
> >>>  	__u32		max_uninit_len;
> >>>  };
> >>
> >> I just realized, this affects the ABI, doesn't it?  Hm.
> >>
> >> As a hack-around, can probably just use ehandle->path[0].end_blk directly
> >> in scan_extent_node and stash eof_blk locally?
> > 
> > Nope, we can't crack an extent handle, it's an opaque type.
> > 
> > Ned some V2 interfaces now?  :(
> > 
> 
> or maybe just:
> 
> +       eof_blk = (EXT2_I_SIZE(pctx->inode) + ctx->fs->blocksize - 1) >>
> +                  EXT2_BLOCK_SIZE_BITS(ctx->fs->super);
> 
> unless that's too ugly.
> 

Clearly, I wasn't thinking about the ABI at all - thanks for pointing out
that misstep.

So, I'd like to withdraw that patch, please, and will post a V2 in a bit.
Computing the eof_blk in that manner is better than an initial patch I had
that worked but which was pretty ugly.

Thanks,
Eric

Re: [PATCH] e2fsprogs: Don't report uninit extents past EOF invalid

[Theodore Ts'o]

On Tue, Aug 13, 2013 at 12:31:12PM -0400, Eric Whitney wrote:
> Clearly, I wasn't thinking about the ABI at all - thanks for pointing out
> that misstep.
> 
> So, I'd like to withdraw that patch, please, and will post a V2 in a bit.
> Computing the eof_blk in that manner is better than an initial patch I had
> that worked but which was pretty ugly.

It didn't really make sense to put the eof_blk in the extent_info
structure, anyway, since it's not information about the that specific
extent.  It's an inode-specific value which is set via:

	handle->path[0].end_blk =
		(EXT2_I_SIZE(handle->inode) + fs->blocksize - 1) >>
		 EXT2_BLOCK_SIZE_BITS(fs->super);

Right?  So this is something you can calculate without making any
changes in lib/ext2fs/extent.c, unless I'm missing something.

							- Ted