[patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails

[patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails

[Dan Carpenter]

In the current code, if blk_mq_alloc_tag_set() fails then it returns
zero (success) instead of preserving the error code.  The caller is not
expecting that and the kernel could be left in an inconsistent state.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Static analysis stuff.  Not tested.

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index cb529e9..2ff5efc 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2152,7 +2152,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE;
 	dev->tagset.driver_data = dev;
 
-	if (blk_mq_alloc_tag_set(&dev->tagset))
+	res = blk_mq_alloc_tag_set(&dev->tagset);
+	if (res)
 		goto out;
 
 	id_ns = mem;

[patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails

[Dan Carpenter]

In the current code, if blk_mq_alloc_tag_set() fails then it returns
zero (success) instead of preserving the error code.  The caller is not
expecting that and the kernel could be left in an inconsistent state.

Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com>
---
Static analysis stuff.  Not tested.

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index cb529e9..2ff5efc 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2152,7 +2152,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE;
 	dev->tagset.driver_data = dev;
 
-	if (blk_mq_alloc_tag_set(&dev->tagset))
+	res = blk_mq_alloc_tag_set(&dev->tagset);
+	if (res)
 		goto out;
 
 	id_ns = mem;

[patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails

[Jens Axboe]

On 01/19/2015 07:43 AM, Dan Carpenter wrote:
> In the current code, if blk_mq_alloc_tag_set() fails then it returns
> zero (success) instead of preserving the error code.  The caller is not
> expecting that and the kernel could be left in an inconsistent state.
>
> Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com>

Looks good to me, Keith, could you ack/review it? Leaving it below...

> Static analysis stuff.  Not tested.
>
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index cb529e9..2ff5efc 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -2152,7 +2152,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
>   	dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE;
>   	dev->tagset.driver_data = dev;
>
> -	if (blk_mq_alloc_tag_set(&dev->tagset))
> +	res = blk_mq_alloc_tag_set(&dev->tagset);
> +	if (res)
>   		goto out;
>
>   	id_ns = mem;
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
>


-- 
Jens Axboe

[patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails

[Keith Busch]

On Wed, 21 Jan 2015, Jens Axboe wrote:
> On 01/19/2015 07:43 AM, Dan Carpenter wrote:
>> In the current code, if blk_mq_alloc_tag_set() fails then it returns
>> zero (success) instead of preserving the error code.  The caller is not
>> expecting that and the kernel could be left in an inconsistent state.
>> 
>> Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com>
>
> Looks good to me, Keith, could you ack/review it? Leaving it below...

Should we bail on the device if tagset allocation fails? If so, the patch
is good, but I thought it was a concious descision to not return error
here so the controller can be managed. Capabilities would be limited
and a failure here probably means there's a bigger problem, so I'm okay
either way.

>> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
>> index cb529e9..2ff5efc 100644
>> --- a/drivers/block/nvme-core.c
>> +++ b/drivers/block/nvme-core.c
>> @@ -2152,7 +2152,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
>>   	dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE;
>>   	dev->tagset.driver_data = dev;
>> 
>> -	if (blk_mq_alloc_tag_set(&dev->tagset))
>> +	res = blk_mq_alloc_tag_set(&dev->tagset);
>> +	if (res)
>>   		goto out;
>>
>>   	id_ns = mem;

[patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails

[Jens Axboe]

On 01/22/2015 09:48 AM, Keith Busch wrote:
> On Wed, 21 Jan 2015, Jens Axboe wrote:
>> On 01/19/2015 07:43 AM, Dan Carpenter wrote:
>>> In the current code, if blk_mq_alloc_tag_set() fails then it returns
>>> zero (success) instead of preserving the error code.  The caller is not
>>> expecting that and the kernel could be left in an inconsistent state.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com>
>>
>> Looks good to me, Keith, could you ack/review it? Leaving it below...
> 
> Should we bail on the device if tagset allocation fails? If so, the patch
> is good, but I thought it was a concious descision to not return error
> here so the controller can be managed. Capabilities would be limited
> and a failure here probably means there's a bigger problem, so I'm okay
> either way.

That's a good point, you could still send admin IO through the ioctls
even if this fails. Looking at the rest of the function, the error
handling is a bit strange. If we fail the nvme_identify(), we'll just
march on. If the next works, we're good, we return success. But if the
failed one happens to be the last one, we return error.

So we need to clean it up a bit regardless. Question is, what errors
constitute general failure, and which ones we want to allow. If the
rationale is wanting to still access the device and do admin IO, then
none of them should be hard failures. But they should be reported. I can
imagine cases where the device is mostly screwed and you just want to
get the driver loaded successfully to reset/format/fw-update (actually I
don't need to imagine, I've been in those situations!).


-- 
Jens Axboe

[patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails

[Keith Busch]

On Thu, 22 Jan 2015, Jens Axboe wrote:
> On 01/22/2015 09:48 AM, Keith Busch wrote:
>> Should we bail on the device if tagset allocation fails? If so, the patch
>> is good, but I thought it was a concious descision to not return error
>> here so the controller can be managed. Capabilities would be limited
>> and a failure here probably means there's a bigger problem, so I'm okay
>> either way.
>
> That's a good point, you could still send admin IO through the ioctls
> even if this fails. Looking at the rest of the function, the error
> handling is a bit strange. If we fail the nvme_identify(), we'll just
> march on. If the next works, we're good, we return success. But if the
> failed one happens to be the last one, we return error.

It's a little simpler than that. The return value is unconditionally
cleared to 0 at the end so we always return success if the first
nvme_identify() was successful. I wouldn't trust a controller that
couldn't identify itself, but the remaining commands are for namespace
identification which can legitimately return errors.

[patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails

[Jens Axboe]

On 01/22/2015 04:11 PM, Keith Busch wrote:
> On Thu, 22 Jan 2015, Jens Axboe wrote:
>> On 01/22/2015 09:48 AM, Keith Busch wrote:
>>> Should we bail on the device if tagset allocation fails? If so, the
>>> patch
>>> is good, but I thought it was a concious descision to not return error
>>> here so the controller can be managed. Capabilities would be limited
>>> and a failure here probably means there's a bigger problem, so I'm okay
>>> either way.
>>
>> That's a good point, you could still send admin IO through the ioctls
>> even if this fails. Looking at the rest of the function, the error
>> handling is a bit strange. If we fail the nvme_identify(), we'll just
>> march on. If the next works, we're good, we return success. But if the
>> failed one happens to be the last one, we return error.
>
> It's a little simpler than that. The return value is unconditionally
> cleared to 0 at the end so we always return success if the first
> nvme_identify() was successful. I wouldn't trust a controller that
> couldn't identify itself, but the remaining commands are for namespace
> identification which can legitimately return errors.

Ah correct, I missed that unconditional clear. I think it looks fine then.

-- 
Jens Axboe