Btrfs devel - Jan 2013 - [PATCH] btrfs: fix btrfs_cont_expand() freeing IS_ERR em

btrfs_cont_expand() tries to free an IS_ERR em as it gets an error from
btrfs_get_extent() and breaks out of its loop.

An instance of -EEXIST was reported in the wild:

  https://bugzilla.redhat.com/show_bug.cgi?id=874407

I have no idea if that -EEXIST is surprising, or not.  Regardless, this
error handling should be cleaned up to handle other reasonable errors
(ENOMEM, EIO; whatever).

This seemed to be the only buggy freeing of the relatively rare IS_ERR
em so I opted to fix the caller rather than teach free_extent_map() to
use IS_ERR_OR_NULL().

Signed-off-by: Zach Brown <zab@redhat.com>
---
 fs/btrfs/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index edd30ae..0bc7bb3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3690,6 +3690,7 @@ int btrfs_cont_expand(struct inode *inode, loff_t oldsize,
loff_t size)
 				block_end - cur_offset, 0);
 		if (IS_ERR(em)) {
 			err = PTR_ERR(em);
+			em = NULL;
 			break;
 		}
 		last_byte = min(extent_map_end(em), block_end);
-- 
1.7.11.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 1/8/13 1:37 PM, Zach Brown wrote:
> btrfs_cont_expand() tries to free an IS_ERR em as it gets an error from
> btrfs_get_extent() and breaks out of its loop.
> 
> An instance of -EEXIST was reported in the wild:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=874407
> 
> I have no idea if that -EEXIST is surprising, or not.  Regardless, this
> error handling should be cleaned up to handle other reasonable errors
> (ENOMEM, EIO; whatever).
> 
> This seemed to be the only buggy freeing of the relatively rare IS_ERR
> em so I opted to fix the caller rather than teach free_extent_map() to
> use IS_ERR_OR_NULL().
> 
> Signed-off-by: Zach Brown <zab@redhat.com>
Looks good (maybe IS_ERR_OR_NULL would be more future-proof, but *shrug*)

Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/btrfs/inode.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index edd30ae..0bc7bb3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3690,6 +3690,7 @@ int btrfs_cont_expand(struct inode *inode, loff_t
oldsize, loff_t size)
>  				block_end - cur_offset, 0);
>  		if (IS_ERR(em)) {
>  			err = PTR_ERR(em);
> +			em = NULL;
>  			break;
>  		}
>  		last_byte = min(extent_map_end(em), block_end);
> 
--
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

> Looks good (maybe IS_ERR_OR_NULL would be more future-proof, but *shrug*)
Yeah.  I don''t feel strongly about it.  I''m happy to do that,
and dance
around the tree pulling out redundant ''em = NULL'' bits that
wouldn''t be
needed, if anyone feels strongly about it.

- z
(*shrug*)
--
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