fs: Fix a missing check bug
diff mbox series

Message ID 20191207144126.14320-1-dinghao.liu@zju.edu.cn
State New
Headers show
Series
  • fs: Fix a missing check bug
Related show

Commit Message

Dinghao Liu Dec. 7, 2019, 2:41 p.m. UTC
The return value of link_free_space(ctl, info) is checked out-sync. Only one branch of an if statement checks this return value after WARN_ON(ret).

Since this path pair is similar in semantic, there might be a missing check bug.

Fix this by simply adding a check on ret.

Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
---
 fs/btrfs/free-space-cache.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Filipe Manana Dec. 7, 2019, 5:16 p.m. UTC | #1
On Sat, Dec 7, 2019 at 3:03 PM Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
>
> The return value of link_free_space(ctl, info) is checked out-sync. Only one branch of an if statement checks this return value after WARN_ON(ret).
>
> Since this path pair is similar in semantic, there might be a missing check bug.
>
> Fix this by simply adding a check on ret.
>
> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> ---
>  fs/btrfs/free-space-cache.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> index 3283da419200..acbb3a59d344 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -2437,6 +2437,8 @@ int btrfs_remove_free_space(struct btrfs_block_group *block_group,
>                         if (info->bytes) {
>                                 ret = link_free_space(ctl, info);
>                                 WARN_ON(ret);
> +                               if (ret)

I think the WARN_ON() can go away as well.
The only possible error is -EEXIST, coming from tree_insert_offset().
When that happens tree_insert_offset() already emits a warning.

Also, the free space entry needs to be freed, otherwise we leak
memory. So it should be something like this:

if (ret) {
    kmem_cache_free(btrfs_free_space_cachep, info);
    goto out_unlock;
}

Further the subject should be prefixed with "btrfs: " and not "fs: ",
since this is a btrfs specific patch.
Something like the following for example:

"btrfs: add missing error handling when removing free space"

Thanks.

> +                                       goto out_lock;
>                         } else {
>                                 kmem_cache_free(btrfs_free_space_cachep, info);
>                         }
> --
> 2.21.0 (Apple Git-122)
>
Qu Wenruo Dec. 8, 2019, 5:02 a.m. UTC | #2
On 2019/12/7 下午10:41, Dinghao Liu wrote:
> The return value of link_free_space(ctl, info) is checked out-sync. Only one branch of an if statement checks this return value after WARN_ON(ret).
> 
> Since this path pair is similar in semantic, there might be a missing check bug.
> 
> Fix this by simply adding a check on ret.

The main failure mode for link_free_space() is -EEXIST, which means
there is already free space in the cache.

Here EEXIST may not be a big problem, and we may really want to continue
the iteration other than error out.


Would you explain in details about why you believe error out is the
correct way other than current continue behavior?

Thanks,
Qu

> 
> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> ---
>  fs/btrfs/free-space-cache.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> index 3283da419200..acbb3a59d344 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -2437,6 +2437,8 @@ int btrfs_remove_free_space(struct btrfs_block_group *block_group,
>  			if (info->bytes) {
>  				ret = link_free_space(ctl, info);
>  				WARN_ON(ret);
> +				if (ret)
> +					goto out_lock;
>  			} else {
>  				kmem_cache_free(btrfs_free_space_cachep, info);
>  			}
>

Patch
diff mbox series

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 3283da419200..acbb3a59d344 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -2437,6 +2437,8 @@  int btrfs_remove_free_space(struct btrfs_block_group *block_group,
 			if (info->bytes) {
 				ret = link_free_space(ctl, info);
 				WARN_ON(ret);
+				if (ret)
+					goto out_lock;
 			} else {
 				kmem_cache_free(btrfs_free_space_cachep, info);
 			}