| Message ID | 20190211183209.GA13934@kadam (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: drop the lock on error in btrfs_dev_replace_cancel() | expand |
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.
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>
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; }
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(+)