[PATCH] bcache: Fixup error handling in register_cache()

[PATCH] bcache: Fixup error handling in register_cache()

[Jan Kara]

Coverity has noticed that the printing of error message in
register_cache() uses already freed bdev_handle to get to bdev. In fact
the problem has been there even before commit "bcache: Convert to
bdev_open_by_path()" just a bit more subtle one - cache object itself
could have been freed by the time we looked at ca->bdev and we don't
hold any reference to bdev either so even that could in principle go
away (due to device unplug or similar). Fix all these problems by
printing the error message before closing the bdev.

Fixes: dc893f51d24a ("bcache: Convert to bdev_open_by_path()")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 drivers/md/bcache/super.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Hello Christian!

Can you please add this to patch to the bdev_handle conversion series? Either
append it at the end of the series or just fold it into the bcache conversion.
Whatever looks better for you.


diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index c11ac86be72b..a30c8d4f2ac8 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2354,6 +2354,13 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
 
 	ret = cache_alloc(ca);
 	if (ret != 0) {
+		if (ret == -ENOMEM)
+			err = "cache_alloc(): -ENOMEM";
+		else if (ret == -EPERM)
+			err = "cache_alloc(): cache device is too small";
+		else
+			err = "cache_alloc(): unknown error";
+		pr_notice("error %pg: %s\n", bdev_handle->bdev, err);
 		/*
 		 * If we failed here, it means ca->kobj is not initialized yet,
 		 * kobject_put() won't be called and there is no chance to
@@ -2361,17 +2368,12 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
 		 * we explicitly call bdev_release() here.
 		 */
 		bdev_release(bdev_handle);
-		if (ret == -ENOMEM)
-			err = "cache_alloc(): -ENOMEM";
-		else if (ret == -EPERM)
-			err = "cache_alloc(): cache device is too small";
-		else
-			err = "cache_alloc(): unknown error";
-		goto err;
+		return ret;
 	}
 
 	if (kobject_add(&ca->kobj, bdev_kobj(bdev_handle->bdev), "bcache")) {
-		err = "error calling kobject_add";
+		pr_notice("error %pg: error calling kobject_add\n",
+			  bdev_handle->bdev);
 		ret = -ENOMEM;
 		goto out;
 	}
@@ -2389,11 +2391,6 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
 
 out:
 	kobject_put(&ca->kobj);
-
-err:
-	if (err)
-		pr_notice("error %pg: %s\n", ca->bdev_handle->bdev, err);
-
 	return ret;
 }
 
-- 
2.35.3

Re: [PATCH] bcache: Fixup error handling in register_cache()

[Coly Li]

> 2023年10月4日 17:37，Jan Kara <jack@suse.cz> 写道：
> 
> Coverity has noticed that the printing of error message in
> register_cache() uses already freed bdev_handle to get to bdev. In fact
> the problem has been there even before commit "bcache: Convert to
> bdev_open_by_path()" just a bit more subtle one - cache object itself
> could have been freed by the time we looked at ca->bdev and we don't
> hold any reference to bdev either so even that could in principle go
> away (due to device unplug or similar). Fix all these problems by
> printing the error message before closing the bdev.
> 
> Fixes: dc893f51d24a ("bcache: Convert to bdev_open_by_path()")
> Signed-off-by: Jan Kara <jack@suse.cz>

Asked-by: Coly Li <colyli@suse.de <mailto:colyli@suse.de>>

Thanks.

Coly Li

> ---
> drivers/md/bcache/super.c | 23 ++++++++++-------------
> 1 file changed, 10 insertions(+), 13 deletions(-)
> 
> Hello Christian!
> 
> Can you please add this to patch to the bdev_handle conversion series? Either
> append it at the end of the series or just fold it into the bcache conversion.
> Whatever looks better for you.
> 
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index c11ac86be72b..a30c8d4f2ac8 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -2354,6 +2354,13 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
> 
> ret = cache_alloc(ca);
> if (ret != 0) {
> + if (ret == -ENOMEM)
> + err = "cache_alloc(): -ENOMEM";
> + else if (ret == -EPERM)
> + err = "cache_alloc(): cache device is too small";
> + else
> + err = "cache_alloc(): unknown error";
> + pr_notice("error %pg: %s\n", bdev_handle->bdev, err);
> /*
> * If we failed here, it means ca->kobj is not initialized yet,
> * kobject_put() won't be called and there is no chance to
> @@ -2361,17 +2368,12 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
> * we explicitly call bdev_release() here.
> */
> bdev_release(bdev_handle);
> - if (ret == -ENOMEM)
> - err = "cache_alloc(): -ENOMEM";
> - else if (ret == -EPERM)
> - err = "cache_alloc(): cache device is too small";
> - else
> - err = "cache_alloc(): unknown error";
> - goto err;
> + return ret;
> }
> 
> if (kobject_add(&ca->kobj, bdev_kobj(bdev_handle->bdev), "bcache")) {
> - err = "error calling kobject_add";
> + pr_notice("error %pg: error calling kobject_add\n",
> +  bdev_handle->bdev);
> ret = -ENOMEM;
> goto out;
> }
> @@ -2389,11 +2391,6 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
> 
> out:
> kobject_put(&ca->kobj);
> -
> -err:
> - if (err)
> - pr_notice("error %pg: %s\n", ca->bdev_handle->bdev, err);
> -
> return ret;
> }
> 
> -- 
> 2.35.3
> 

Re: [PATCH] bcache: Fixup error handling in register_cache()

[Christian Brauner]

On Wed, 04 Oct 2023 11:37:57 +0200, Jan Kara wrote:
> Coverity has noticed that the printing of error message in
> register_cache() uses already freed bdev_handle to get to bdev. In fact
> the problem has been there even before commit "bcache: Convert to
> bdev_open_by_path()" just a bit more subtle one - cache object itself
> could have been freed by the time we looked at ca->bdev and we don't
> hold any reference to bdev either so even that could in principle go
> away (due to device unplug or similar). Fix all these problems by
> printing the error message before closing the bdev.
> 
> [...]

Applied to the vfs.super branch of the vfs/vfs.git tree.
Patches in the vfs.super branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.super

[1/1] bcache: Fixup error handling in register_cache()
      https://git.kernel.org/vfs/vfs/c/cc82d9a7a3d0