[PATCH 1/2] Btrfs: fix file extent corruption

[PATCH 1/2] Btrfs: fix file extent corruption

[Josef Bacik]

In order to do hole punching we have a block reserve to hold the reservation we
need to drop the extents in our range.  Since we could end up dropping a lot of
extents we set rsv->failfast so we can just loop around again and drop the
remaining of the range.  Unfortunately we unconditionally fill the hole extents
in and start from the last extent we encountered, which we may or may not have
dropped.  So this can result in overlapping file extent entries, which can be
tripped over in a variety of ways, either by hitting BUG_ON(!ret) in
fill_holes() after the search, or in btrfs_set_item_key_safe() in
btrfs_drop_extent() at a later time by an unrelated task.  Fix this by only
setting drop_end to the last extent we did actually drop.  This way our holes
are filled in properly for the range that we did drop, and the rest of the range
that remains to be dropped is actually dropped.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/file.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index cbefdc8..1c15a98 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -706,6 +706,7 @@ int __btrfs_drop_extents(struct btrfs_trans_handle *trans,
 	u64 num_bytes = 0;
 	u64 extent_offset = 0;
 	u64 extent_end = 0;
+	u64 last_end = 0;
 	int del_nr = 0;
 	int del_slot = 0;
 	int extent_type;
@@ -797,8 +798,10 @@ next_slot:
 		 * extent item in the call to setup_items_for_insert() later
 		 * in this function.
 		 */
-		if (extent_end == key.offset && extent_end >= search_start)
+		if (extent_end == key.offset && extent_end >= search_start) {
+			last_end = extent_end;
 			goto delete_extent_item;
+		}
 
 		if (extent_end <= search_start) {
 			path->slots[0]++;
@@ -861,6 +864,12 @@ next_slot:
 			key.offset = start;
 		}
 		/*
+		 * From here on out we will have actually dropped something, so
+		 * last_end can be updated.
+		 */
+		last_end = extent_end;
+
+		/*
 		 *  | ---- range to drop ----- |
 		 *      | -------- extent -------- |
 		 */
@@ -1010,7 +1019,7 @@ delete_extent_item:
 	if (!replace_extent || !(*key_inserted))
 		btrfs_release_path(path);
 	if (drop_end)
-		*drop_end = found ? min(end, extent_end) : end;
+		*drop_end = found ? min(end, last_end) : end;
 	return ret;
 }
 
-- 
2.7.4

[PATCH 2/2] Btrfs: abort transaction if fill_holes() fails

[Josef Bacik]

At this point we will have dropped extent entries from the file, so if we fail
to insert the new hole entries then we are leaving the fs in a corrupt state
(albeit an easily fixed one).  Abort the transaciton if this happens so we can
avoid corrupting the fs.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/file.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 1c15a98..d6fc719 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -2234,9 +2234,14 @@ static int fill_holes(struct btrfs_trans_handle *trans, struct inode *inode,
 	key.offset = offset;
 
 	ret = btrfs_search_slot(trans, root, &key, path, 0, 1);
-	if (ret < 0)
+	if (ret <= 0) {
+		/* We should have dropped this offset, so if we find it then
+		 * something has gone horribly wrong.
+		 */
+		if (ret == 0)
+			ret = -EINVAL;
 		return ret;
-	BUG_ON(!ret);
+	}
 
 	leaf = path->nodes[0];
 	if (hole_mergeable(inode, leaf, path->slots[0]-1, offset, end)) {
@@ -2539,6 +2544,12 @@ static int btrfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 			ret = fill_holes(trans, inode, path, cur_offset,
 					 drop_end);
 			if (ret) {
+				/* If we failed then we didn't insert our hole
+				 * entries for the area we dropped, so now the
+				 * fs is corrupted, so we must abort the
+				 * transaction.
+				 */
+				btrfs_abort_transaction(trans, ret);
 				err = ret;
 				break;
 			}
@@ -2603,6 +2614,8 @@ static int btrfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 	if (cur_offset < ino_size && cur_offset < drop_end) {
 		ret = fill_holes(trans, inode, path, cur_offset, drop_end);
 		if (ret) {
+			/* Same comment as above. */
+			btrfs_abort_transaction(trans, ret);
 			err = ret;
 			goto out_trans;
 		}
-- 
2.7.4

Re: [PATCH 1/2] Btrfs: fix file extent corruption

[Chris Mason]

On 11/14/2016 02:06 PM, Josef Bacik wrote:
> In order to do hole punching we have a block reserve to hold the reservation we
> need to drop the extents in our range.  Since we could end up dropping a lot of
> extents we set rsv->failfast so we can just loop around again and drop the
> remaining of the range.  Unfortunately we unconditionally fill the hole extents
> in and start from the last extent we encountered, which we may or may not have
> dropped.  So this can result in overlapping file extent entries, which can be
> tripped over in a variety of ways, either by hitting BUG_ON(!ret) in
> fill_holes() after the search, or in btrfs_set_item_key_safe() in
> btrfs_drop_extent() at a later time by an unrelated task.  Fix this by only
> setting drop_end to the last extent we did actually drop.  This way our holes
> are filled in properly for the range that we did drop, and the rest of the range
> that remains to be dropped is actually dropped.  Thanks,
>
> Signed-off-by: Josef Bacik <jbacik@fb.com>

Thanks for tracking this down Josef.  We should mark it for stable too.

-chris

Re: [PATCH 1/2] Btrfs: fix file extent corruption

[Liu Bo]

On Mon, Nov 14, 2016 at 02:06:21PM -0500, Josef Bacik wrote:
> In order to do hole punching we have a block reserve to hold the reservation we
> need to drop the extents in our range.  Since we could end up dropping a lot of
> extents we set rsv->failfast so we can just loop around again and drop the
> remaining of the range.  Unfortunately we unconditionally fill the hole extents
> in and start from the last extent we encountered, which we may or may not have
> dropped.  So this can result in overlapping file extent entries, which can be
> tripped over in a variety of ways, either by hitting BUG_ON(!ret) in
> fill_holes() after the search, or in btrfs_set_item_key_safe() in
> btrfs_drop_extent() at a later time by an unrelated task.  Fix this by only
> setting drop_end to the last extent we did actually drop.  This way our holes
> are filled in properly for the range that we did drop, and the rest of the range
> that remains to be dropped is actually dropped.  Thanks,

Can you pleaes share the reproducer?

Thanks,

-liubo
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  fs/btrfs/file.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index cbefdc8..1c15a98 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -706,6 +706,7 @@ int __btrfs_drop_extents(struct btrfs_trans_handle *trans,
>  	u64 num_bytes = 0;
>  	u64 extent_offset = 0;
>  	u64 extent_end = 0;
> +	u64 last_end = 0;
>  	int del_nr = 0;
>  	int del_slot = 0;
>  	int extent_type;
> @@ -797,8 +798,10 @@ next_slot:
>  		 * extent item in the call to setup_items_for_insert() later
>  		 * in this function.
>  		 */
> -		if (extent_end == key.offset && extent_end >= search_start)
> +		if (extent_end == key.offset && extent_end >= search_start) {
> +			last_end = extent_end;
>  			goto delete_extent_item;
> +		}
>  
>  		if (extent_end <= search_start) {
>  			path->slots[0]++;
> @@ -861,6 +864,12 @@ next_slot:
>  			key.offset = start;
>  		}
>  		/*
> +		 * From here on out we will have actually dropped something, so
> +		 * last_end can be updated.
> +		 */
> +		last_end = extent_end;
> +
> +		/*
>  		 *  | ---- range to drop ----- |
>  		 *      | -------- extent -------- |
>  		 */
> @@ -1010,7 +1019,7 @@ delete_extent_item:
>  	if (!replace_extent || !(*key_inserted))
>  		btrfs_release_path(path);
>  	if (drop_end)
> -		*drop_end = found ? min(end, extent_end) : end;
> +		*drop_end = found ? min(end, last_end) : end;
>  	return ret;
>  }
>  
> -- 
> 2.7.4
> 
> --
> 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

Re: [PATCH 1/2] Btrfs: fix file extent corruption

[Josef Bacik]

On 11/14/2016 06:11 PM, Liu Bo wrote:
> On Mon, Nov 14, 2016 at 02:06:21PM -0500, Josef Bacik wrote:
>> In order to do hole punching we have a block reserve to hold the reservation we
>> need to drop the extents in our range.  Since we could end up dropping a lot of
>> extents we set rsv->failfast so we can just loop around again and drop the
>> remaining of the range.  Unfortunately we unconditionally fill the hole extents
>> in and start from the last extent we encountered, which we may or may not have
>> dropped.  So this can result in overlapping file extent entries, which can be
>> tripped over in a variety of ways, either by hitting BUG_ON(!ret) in
>> fill_holes() after the search, or in btrfs_set_item_key_safe() in
>> btrfs_drop_extent() at a later time by an unrelated task.  Fix this by only
>> setting drop_end to the last extent we did actually drop.  This way our holes
>> are filled in properly for the range that we did drop, and the rest of the range
>> that remains to be dropped is actually dropped.  Thanks,
>
> Can you pleaes share the reproducer?
>

Yup here you go

https://paste.fedoraproject.org/483195/30633414

Thanks,

Josef

[PATCH 1/2][V2] Btrfs: fix file extent corruption

[Josef Bacik]

In order to do hole punching we have a block reserve to hold the reservation we
need to drop the extents in our range.  Since we could end up dropping a lot of
extents we set rsv->failfast so we can just loop around again and drop the
remaining of the range.  Unfortunately we unconditionally fill the hole extents
in and start from the last extent we encountered, which we may or may not have
dropped.  So this can result in overlapping file extent entries, which can be
tripped over in a variety of ways, either by hitting BUG_ON(!ret) in
fill_holes() after the search, or in btrfs_set_item_key_safe() in
btrfs_drop_extent() at a later time by an unrelated task.  Fix this by only
setting drop_end to the last extent we did actually drop.  This way our holes
are filled in properly for the range that we did drop, and the rest of the range
that remains to be dropped is actually dropped.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
V1->V2:
- don't call fill_holes if our drop_end is == start.

 fs/btrfs/file.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index cbefdc8..23859e7 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -706,6 +706,7 @@ int __btrfs_drop_extents(struct btrfs_trans_handle *trans,
 	u64 num_bytes = 0;
 	u64 extent_offset = 0;
 	u64 extent_end = 0;
+	u64 last_end = start;
 	int del_nr = 0;
 	int del_slot = 0;
 	int extent_type;
@@ -797,8 +798,10 @@ next_slot:
 		 * extent item in the call to setup_items_for_insert() later
 		 * in this function.
 		 */
-		if (extent_end == key.offset && extent_end >= search_start)
+		if (extent_end == key.offset && extent_end >= search_start) {
+			last_end = extent_end;
 			goto delete_extent_item;
+		}
 
 		if (extent_end <= search_start) {
 			path->slots[0]++;
@@ -861,6 +864,12 @@ next_slot:
 			key.offset = start;
 		}
 		/*
+		 * From here on out we will have actually dropped something, so
+		 * last_end can be updated.
+		 */
+		last_end = extent_end;
+
+		/*
 		 *  | ---- range to drop ----- |
 		 *      | -------- extent -------- |
 		 */
@@ -1010,7 +1019,7 @@ delete_extent_item:
 	if (!replace_extent || !(*key_inserted))
 		btrfs_release_path(path);
 	if (drop_end)
-		*drop_end = found ? min(end, extent_end) : end;
+		*drop_end = found ? min(end, last_end) : end;
 	return ret;
 }
 
@@ -2526,7 +2535,7 @@ static int btrfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 
 		trans->block_rsv = &root->fs_info->trans_block_rsv;
 
-		if (cur_offset < ino_size) {
+		if (cur_offset < drop_end && cur_offset < ino_size) {
 			ret = fill_holes(trans, inode, path, cur_offset,
 					 drop_end);
 			if (ret) {
-- 
2.7.4