NVMe: return an error code if blk_mq_alloc_tag_set() fails
Subsystems (3)
PATCH 1/1 NVMe: return an error code if blk_mq_alloc_tag_set() fails
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.
---
Static analysis stuff. Not tested.
drivers/block/nvme-core.c +2 -1
@@ -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;
Jens Axboe
maintainerPATCH 1/1 NVMe: return an error code if blk_mq_alloc_tag_set() fails
Quoted 5 lines · 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>
Looks good to me, Keith, could you ack/review it? Leaving it below...
Quoted 22 lines
Static analysis stuff. Not tested.
drivers/block/nvme-core.c +2
-1@@ -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
Signature · 1 line
Jens Axboe
PATCH 1/1 NVMe: return an error code if blk_mq_alloc_tag_set() fails
Quoted 8 lines · Jens Axboe
Quoted 5 lines · 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>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.
Quoted 14 lines
Quoted 14 lines
drivers/block/nvme-core.c +2
-1@@ -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;
Jens Axboe
maintainerPATCH 1/1 NVMe: return an error code if blk_mq_alloc_tag_set() fails
Quoted 8 lines · Keith Busch
Quoted 7 lines · Jens Axboe
Quoted 5 lines · 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>
Quoted 7 lines
Quoted 1 line
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!).
Signature · 1 line
Jens Axboe
PATCH 1/1 NVMe: return an error code if blk_mq_alloc_tag_set() fails
Quoted 7 lines · Jens Axboe
Quoted 5 lines · Keith Busch
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.
Quoted 5 lines
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.
Jens Axboe
maintainerPATCH 1/1 NVMe: return an error code if blk_mq_alloc_tag_set() fails
Quoted 15 lines · Keith Busch
Quoted 8 lines · Jens Axboe
Quoted 6 lines · Keith Busch
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.Quoted 5 lines
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.
Quoted 5 lines
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.
Signature · 1 line
Jens Axboe