Btrfs devel - Apr 2013 - [PATCH v2] Btrfs: return error when we specify wrong start to defrag

We need such a sanity check for wrong start when we defrag a file, otherwise,
even with a wrong start that''s larger than file size, we can end up
changing
not only inode''s force compress flag but also FS''s incompat
flags.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
v2: make changelog more clearly.

 fs/btrfs/ioctl.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 69cd80d..262d9db 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1152,8 +1152,11 @@ int btrfs_defrag_file(struct inode *inode, struct file
*file,
 	u64 new_align = ~((u64)128 * 1024 - 1);
 	struct page **pages = NULL;
 
-	if (extent_thresh == 0)
-		extent_thresh = 256 * 1024;
+	if (isize == 0)
+		return 0;
+
+	if (range->start >= isize)
+		return -EINVAL;
 
 	if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS) {
 		if (range->compress_type > BTRFS_COMPRESS_TYPES)
@@ -1162,8 +1165,8 @@ int btrfs_defrag_file(struct inode *inode, struct file
*file,
 			compress_type = range->compress_type;
 	}
 
-	if (isize == 0)
-		return 0;
+	if (extent_thresh == 0)
+		extent_thresh = 256 * 1024;
 
 	/*
 	 * if we were not given a file, allocate a readahead
-- 
1.7.7

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Apr 16, 2013 at 05:20:28PM +0800, Liu Bo wrote:
> We need such a sanity check for wrong start when we defrag a file,
otherwise,
> even with a wrong start that''s larger than file size, we can end
up changing
> not only inode''s force compress flag but also FS''s
incompat flags.
The range->start check is good, but why are you worried about the
incompat flag? LZO support has been around for more than 2 years.
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1152,8 +1152,11 @@ int btrfs_defrag_file(struct inode *inode, struct
file *file,
>  	u64 new_align = ~((u64)128 * 1024 - 1);
>  	struct page **pages = NULL;
>  
> -	if (extent_thresh == 0)
> -		extent_thresh = 256 * 1024;
> +	if (isize == 0)
> +		return 0;
> +
> +	if (range->start >= isize)
> +		return -EINVAL;
...
> -	if (isize == 0)
> -		return 0;
> +	if (extent_thresh == 0)
> +		extent_thresh = 256 * 1024;
That''s a more logical order of the checks, good.

Reviewed-by: David Sterba <dsterba@suse.cz>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Apr 16, 2013 at 03:38:28PM +0200, David Sterba
wrote:
> On Tue, Apr 16, 2013 at 05:20:28PM +0800, Liu Bo wrote:
> > We need such a sanity check for wrong start when we defrag a file,
otherwise,
> > even with a wrong start that''s larger than file size, we can
end up changing
> > not only inode''s force compress flag but also FS''s
incompat flags.
> 
> The range->start check is good, but why are you worried about the
> incompat flag? LZO support has been around for more than 2 years.
As the code of setting LZO incompat flags is just there, so I take it as a side
effect.

Well, I''m not worried now :)
> 
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -1152,8 +1152,11 @@ int btrfs_defrag_file(struct inode *inode,
struct file *file,
> >  	u64 new_align = ~((u64)128 * 1024 - 1);
> >  	struct page **pages = NULL;
> >  
> > -	if (extent_thresh == 0)
> > -		extent_thresh = 256 * 1024;
> > +	if (isize == 0)
> > +		return 0;
> > +
> > +	if (range->start >= isize)
> > +		return -EINVAL;
> ...
> > -	if (isize == 0)
> > -		return 0;
> > +	if (extent_thresh == 0)
> > +		extent_thresh = 256 * 1024;
> 
> That''s a more logical order of the checks, good.
> 
> Reviewed-by: David Sterba <dsterba@suse.cz>
Thanks for the quick review!

thanks,
liubo
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html