[PATCH] btrfs: drop the lock on error in btrfs_dev_replace_cancel()

[PATCH] btrfs: drop the lock on error in btrfs_dev_replace_cancel()

[Dan Carpenter]

We should drop the lock on this error path.  This is from static
analysis and I don't know if it's possible to hit this error path in
real life.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/btrfs/dev-replace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 13863354ff9d..ee193c5222b2 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -862,6 +862,7 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 			btrfs_destroy_dev_replace_tgtdev(tgt_device);
 		break;
 	default:
+		up_write(&dev_replace->rwsem);
 		result = -EINVAL;
 	}
 
-- 
2.17.1

Re: [PATCH] btrfs: drop the lock on error in btrfs_dev_replace_cancel()

[David Sterba]

On Mon, Feb 11, 2019 at 09:32:10PM +0300, Dan Carpenter wrote:
> We should drop the lock on this error path.  This is from static
> analysis and I don't know if it's possible to hit this error path in
> real life.

Yes the lock needs to be released, it's there to protect access to the
dev_replace members and is not supposed to be left locked. The value of
state that's being switched would need to be artifically changed to an
invalid value so the default: branch is taken.

It's been introduced by d189dd70e25561817325 in 5.0-rc1 so it counts as
a regression but I don't think it's urgent enough to be sent to a late
rc. It'll go through the stable tree channel. Thanks.

Re: [PATCH] btrfs: drop the lock on error in btrfs_dev_replace_cancel()

[Anand Jain]

On 2/20/19 3:04 AM, David Sterba wrote:
> On Mon, Feb 11, 2019 at 09:32:10PM +0300, Dan Carpenter wrote:
>> We should drop the lock on this error path.  This is from static
>> analysis and I don't know if it's possible to hit this error path in
>> real life.
> 
> Yes the lock needs to be released, it's there to protect access to the
> dev_replace members and is not supposed to be left locked. The value of
> state that's being switched would need to be artifically changed to an
> invalid value so the default: branch is taken.
> 
> It's been introduced by d189dd70e25561817325 in 5.0-rc1 so it counts as
> a regression but I don't think it's urgent enough to be sent to a late
> rc. It'll go through the stable tree channel. Thanks.
> 

oops I missed this email. Thanks Dan and David.

Reviewed-by: Anand Jain <anand.jain@oracle.com>