* Re: [PATCH] mmc: queue: ensure error propagation for non-blk
2023-04-24 16:22 [PATCH] mmc: queue: ensure error propagation for non-blk Christian Loehle
@ 2023-04-25 14:01 ` Adrian Hunter
0 siblings, 0 replies; 2+ messages in thread
From: Adrian Hunter @ 2023-04-25 14:01 UTC (permalink / raw)
To: Christian Loehle, ulf hansson, linux-mmc,
linux-kernel@vger.kernel.org
Cc: Avri Altman, Christoph Hellwig
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 <cloehle@hyperstone.com>
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;
> }
>
^ permalink raw reply related [flat|nested] 2+ messages in thread