diff mbox series

[v7,01/11] scsi: free sgtables in case command setup fails

Message ID 20200417121536.5393-2-johannes.thumshirn@wdc.com (mailing list archive)
State New, archived
Headers show
Series Introduce Zone Append for writing to zoned block devices | expand

Commit Message

Johannes Thumshirn April 17, 2020, 12:15 p.m. UTC 
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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
---
 drivers/scsi/scsi_lib.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Bart Van Assche April 18, 2020, 4:02 p.m. UTC | #1 
On 2020-04-17 05:15, Johannes Thumshirn wrote:
> @@ -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;
>  }

If this patch fixes the bug reported in
https://bugzilla.kernel.org/show_bug.cgi?id=205595, please mention this.

How about adding __must_check to scsi_setup_fs_cmnd()?

Thanks,

Bart.
Johannes Thumshirn April 20, 2020, 7:10 a.m. UTC | #2 
On 18/04/2020 18:02, Bart Van Assche wrote:
> If this patch fixes the bug reported in
> https://bugzilla.kernel.org/show_bug.cgi?id=205595, please mention this.
> 
> How about adding __must_check to scsi_setup_fs_cmnd()?

Oh thanks for the Link, I didn't know there was a bug report for the 
memory leak.

I've encountered the leak in testing this series with custom error 
injection via SG_IO.

I'll add the link to the bug report if I need a re-send.
Johannes Thumshirn April 20, 2020, 10:46 a.m. UTC | #3 
On 18/04/2020 18:02, Bart Van Assche wrote:
> How about adding __must_check to scsi_setup_fs_cmnd()?

I'm actually not sure if __must_check helps us anything given that with 
this patch applied:

johannes@redsun60:linux(zone-append-wip)$ git --no-pager grep -n \
                                        scsi_setup_fs_cmnd drivers/scsi
drivers/scsi/scsi_lib.c:1173:
static blk_status_t scsi_setup_fs_cmnd(struct scsi_device *sdev,
drivers/scsi/scsi_lib.c:1205:
ret = scsi_setup_fs_cmnd(sdev, req);

there's only one caller of scsi_setup_fs_cmnd(), in the same file, 32 
lines below the implementation.

I do agree about the Link or an eventual Cc: stable, but I think Martin 
or Jens can add this when applying the patch.
Christoph Hellwig April 22, 2020, 6:44 a.m. UTC | #4 
On Mon, Apr 20, 2020 at 10:46:21AM +0000, Johannes Thumshirn wrote:
> On 18/04/2020 18:02, Bart Van Assche wrote:
> > How about adding __must_check to scsi_setup_fs_cmnd()?
> 
> I'm actually not sure if __must_check helps us anything given that with 
> this patch applied:

It doesn't.  __must_check on static functions with a single caller does
not make any sense.
diff mbox series

Patch

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