On 10/04/2020 08:39, Christoph Hellwig wrote:
> Looking more the situation seems even worse. If scsi_mq_prep_fn
> isn't successfull we never seem to free the sgtables, even for fatal
> errors. So I think we need a real bug fix here in front of the series
If I'm not missing something all that needs to be done to fix it is:
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 4724002627cd..5e6165246f77 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1191,6 +1191,7 @@ static blk_status_t scsi_setup_cmnd(struct
scsi_device *sdev,
struct request *req)
{
struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
+ blk_status_t ret;
if (!blk_rq_bytes(req))
cmd->sc_data_direction = DMA_NONE;
@@ -1200,9 +1201,14 @@ static blk_status_t scsi_setup_cmnd(struct
scsi_device *sdev,
cmd->sc_data_direction = DMA_FROM_DEVICE;
if (blk_rq_is_scsi(req))
- return scsi_setup_scsi_cmnd(sdev, req);
+ ret = scsi_setup_scsi_cmnd(sdev, req);
else
- return scsi_setup_fs_cmnd(sdev, req);
+ ret = scsi_setup_fs_cmnd(sdev, req);
+
+ if (ret != BLK_STS_OK)
+ scsi_free_sgtables(cmd);
+
+ return ret;
}
static blk_status_t
Theoretically it's enough to catch errors from scsi_setup_fs_cmnd() as
scsi_setup_scsi_cmnd() either fails scsi_init_io() which means no
sgtables are allocated or returns BLK_STS_OK.
But for the sake of symmetry and defensive programming I think we can
also check the return of scsi_setup_scsi_cmnd(). I've checked
scsi_free_sgtables() and __sg_free_table() and they're double-free safe.
Thoughts?
