Requests to the mmc layer usually come through a block device.
The exceptions are the RPMB chardev and debugfs, which issue their
own blk_mq requests through blk_execute_rq and do not query
the BLK_STS error but the mmc-internal drv_op_result.
This patch ensures that drv_op_result is set as error whenever
a BLK_STS error is set.
The behavior leads to a bug where the request never sees the error,
e.g. by directly erroring out at mmc_blk_mq_issue_rq if
mmc_blk_part_switch fails. The ioctl caller of the rpmb chardev then
can never see the error and thus may assume that their call executed
successfully when it did not.
While always checking the blk_execute_rq return value would be
advised, let's eliminate the error completely by always setting
drv_op_result in case of a BLK_STS error.
Signed-off-by: Christian Loehle <[email protected]>
---
drivers/mmc/core/queue.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index b396e3900717..8240962e28f3 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -334,6 +334,9 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
WRITE_ONCE(mq->busy, false);
}
+ /* Ensure request error propagates to non-blk callers, too. */
+ if (!req_to_mmc_queue_req(req)->drv_op_result && ret)
+ req_to_mmc_queue_req(req)->drv_op_result = ret;
return ret;
}
--
2.37.3
Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782
On 24/04/23 19:22, Christian Loehle wrote:
> Requests to the mmc layer usually come through a block device.
> The exceptions are the RPMB chardev and debugfs, which issue their
> own blk_mq requests through blk_execute_rq and do not query
> the BLK_STS error but the mmc-internal drv_op_result.
> This patch ensures that drv_op_result is set as error whenever
> a BLK_STS error is set.
>
> The behavior leads to a bug where the request never sees the error,
> e.g. by directly erroring out at mmc_blk_mq_issue_rq if
> mmc_blk_part_switch fails. The ioctl caller of the rpmb chardev then
> can never see the error and thus may assume that their call executed
> successfully when it did not.
>
> While always checking the blk_execute_rq return value would be
> advised, let's eliminate the error completely by always setting
> drv_op_result in case of a BLK_STS error.
>
> Signed-off-by: Christian Loehle <[email protected]>
Fixes tag?
> ---
> drivers/mmc/core/queue.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index b396e3900717..8240962e28f3 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -334,6 +334,9 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
> WRITE_ONCE(mq->busy, false);
> }
>
> + /* Ensure request error propagates to non-blk callers, too. */
> + if (!req_to_mmc_queue_req(req)->drv_op_result && ret)
> + req_to_mmc_queue_req(req)->drv_op_result = ret;
That is not the only path out.
It might be better to initialize drv_op_result to an
error code to start with e.g.
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 00c33edb9fb9..d920c4178389 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -264,6 +264,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
goto out_put;
}
req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_BOOT_WP;
+ req_to_mmc_queue_req(req)->drv_op_result = -EIO;
blk_execute_rq(req, false);
ret = req_to_mmc_queue_req(req)->drv_op_result;
blk_mq_free_request(req);
@@ -651,6 +652,7 @@ static int mmc_blk_ioctl_cmd(struct mmc_blk_data *md,
idatas[0] = idata;
req_to_mmc_queue_req(req)->drv_op =
rpmb ? MMC_DRV_OP_IOCTL_RPMB : MMC_DRV_OP_IOCTL;
+ req_to_mmc_queue_req(req)->drv_op_result = -EIO;
req_to_mmc_queue_req(req)->drv_op_data = idatas;
req_to_mmc_queue_req(req)->ioc_count = 1;
blk_execute_rq(req, false);
@@ -722,6 +724,7 @@ static int mmc_blk_ioctl_multi_cmd(struct mmc_blk_data *md,
}
req_to_mmc_queue_req(req)->drv_op =
rpmb ? MMC_DRV_OP_IOCTL_RPMB : MMC_DRV_OP_IOCTL;
+ req_to_mmc_queue_req(req)->drv_op_result = -EIO;
req_to_mmc_queue_req(req)->drv_op_data = idata;
req_to_mmc_queue_req(req)->ioc_count = n;
blk_execute_rq(req, false);
@@ -2806,6 +2809,7 @@ static int mmc_dbg_card_status_get(void *data, u64 *val)
if (IS_ERR(req))
return PTR_ERR(req);
req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_GET_CARD_STATUS;
+ req_to_mmc_queue_req(req)->drv_op_result = -EIO;
blk_execute_rq(req, false);
ret = req_to_mmc_queue_req(req)->drv_op_result;
if (ret >= 0) {
@@ -2844,6 +2848,7 @@ static int mmc_ext_csd_open(struct inode *inode, struct file *filp)
goto out_free;
}
req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_GET_EXT_CSD;
+ req_to_mmc_queue_req(req)->drv_op_result = -EIO;
req_to_mmc_queue_req(req)->drv_op_data = &ext_csd;
blk_execute_rq(req, false);
err = req_to_mmc_queue_req(req)->drv_op_result;
> return ret;
> }
>