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 <[email protected]>
---
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);
}
--
2.21.0 (Apple Git-122)
On Sat, Dec 7, 2019 at 3:03 PM Dinghao Liu <[email protected]> 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 <[email protected]>
> ---
> 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)
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
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 <[email protected]>
> ---
> 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);
> }
>