| Message ID | 20200415090513.5133-2-johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce Zone Append for writing to zoned block devices | expand |
On Wed, Apr 15, 2020 at 06:05:03PM +0900, Johannes Thumshirn wrote: > In case scsi_setup_fs_cmnd() fails we're not freeing the sgtables > allocated by scsi_init_io(), thus we leak the allocated memory. > > So free the sgtables allocated by scsi_init_io() in case > scsi_setup_fs_cmnd() fails. > > Technically scsi_setup_scsi_cmnd() does not suffer from this problem, as > it can only fail if scsi_init_io() fails, so it does not have sgtables > allocated. But to maintain symmetry and as a measure of defensive > programming, free the sgtables on scsi_setup_scsi_cmnd() failure as well. > scsi_mq_free_sgtables() has safeguards against double-freeing of memory so > this is safe to do. > > While we're at it, rename scsi_mq_free_sgtables() to scsi_free_sgtables(). Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Apr 15, 2020 at 06:05:03PM +0900, Johannes Thumshirn wrote: > In case scsi_setup_fs_cmnd() fails we're not freeing the sgtables > allocated by scsi_init_io(), thus we leak the allocated memory. > > So free the sgtables allocated by scsi_init_io() in case > scsi_setup_fs_cmnd() fails. > > Technically scsi_setup_scsi_cmnd() does not suffer from this problem, as > it can only fail if scsi_init_io() fails, so it does not have sgtables > allocated. But to maintain symmetry and as a measure of defensive > programming, free the sgtables on scsi_setup_scsi_cmnd() failure as well. > scsi_mq_free_sgtables() has safeguards against double-freeing of memory so > this is safe to do. > > While we're at it, rename scsi_mq_free_sgtables() to scsi_free_sgtables(). > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> I am not sure if the renaming should be part of this fix as it might be something which should backported to stable. Anyway, looks good to me. Reviewed-by: Daniel Wagner <dwagner@suse.de>
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 47835c4b4ee0..ad97369ffabd 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -548,7 +548,7 @@ static void scsi_uninit_cmd(struct scsi_cmnd *cmd) } } -static void scsi_mq_free_sgtables(struct scsi_cmnd *cmd) +static void scsi_free_sgtables(struct scsi_cmnd *cmd) { if (cmd->sdb.table.nents) sg_free_table_chained(&cmd->sdb.table, @@ -560,7 +560,7 @@ static void scsi_mq_free_sgtables(struct scsi_cmnd *cmd) static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd) { - scsi_mq_free_sgtables(cmd); + scsi_free_sgtables(cmd); scsi_uninit_cmd(cmd); } @@ -1059,7 +1059,7 @@ blk_status_t scsi_init_io(struct scsi_cmnd *cmd) return BLK_STS_OK; out_free_sgtables: - scsi_mq_free_sgtables(cmd); + scsi_free_sgtables(cmd); return ret; } EXPORT_SYMBOL(scsi_init_io); @@ -1190,6 +1190,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; @@ -1199,9 +1200,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
In case scsi_setup_fs_cmnd() fails we're not freeing the sgtables allocated by scsi_init_io(), thus we leak the allocated memory. So free the sgtables allocated by scsi_init_io() in case scsi_setup_fs_cmnd() fails. Technically scsi_setup_scsi_cmnd() does not suffer from this problem, as it can only fail if scsi_init_io() fails, so it does not have sgtables allocated. But to maintain symmetry and as a measure of defensive programming, free the sgtables on scsi_setup_scsi_cmnd() failure as well. scsi_mq_free_sgtables() has safeguards against double-freeing of memory so this is safe to do. While we're at it, rename scsi_mq_free_sgtables() to scsi_free_sgtables(). Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- drivers/scsi/scsi_lib.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)