2015-07-16 15:46:41

by Tony Battersby

Subject: [PATCH] scsi: fix memory leak with scsi-mq

Fix a memory leak with scsi-mq triggered by commands with large data
transfer length.

__sg_alloc_table() sets both table->nents and table->orig_nents to the
same value. When the scatterlist is DMA-mapped, table->nents is
overwritten with the (possibly smaller) size of the DMA-mapped
scatterlist, while table->orig_nents retains the original size of the
allocated scatterlist. scsi_free_sgtable() should therefore check
orig_nents instead of nents, and all code that initializes sdb->table
without calling __sg_alloc_table() should set both nents and orig_nents.

Fixes: d285203cf647 ("scsi: add support for a blk-mq based I/O path.")
Cc: <[email protected]> # 3.17+
Signed-off-by: Tony Battersby <[email protected]>
---

For immediate inclusion.

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 106884a..cfadcce 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -944,7 +944,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
scmd->sdb.length);
scmd->sdb.table.sgl = &ses->sense_sgl;
scmd->sc_data_direction = DMA_FROM_DEVICE;
- scmd->sdb.table.nents = 1;
+ scmd->sdb.table.nents = scmd->sdb.table.orig_nents = 1;
scmd->cmnd[0] = REQUEST_SENSE;
scmd->cmnd[4] = scmd->sdb.length;
scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b1a2631..448ebda 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -583,7 +583,7 @@ static struct scatterlist *scsi_sg_alloc(unsigned int nents, gfp_t gfp_mask)

static void scsi_free_sgtable(struct scsi_data_buffer *sdb, bool mq)
{
- if (mq && sdb->table.nents <= SCSI_MAX_SG_SEGMENTS)
+ if (mq && sdb->table.orig_nents <= SCSI_MAX_SG_SEGMENTS)
return;
__sg_free_table(&sdb->table, SCSI_MAX_SG_SEGMENTS, mq, scsi_sg_free);
}
@@ -597,8 +597,8 @@ static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb, int nents, bool mq)

if (mq) {
if (nents <= SCSI_MAX_SG_SEGMENTS) {
- sdb->table.nents = nents;
- sg_init_table(sdb->table.sgl, sdb->table.nents);
+ sdb->table.nents = sdb->table.orig_nents = nents;
+ sg_init_table(sdb->table.sgl, nents);
return 0;
}
first_chunk = sdb->table.sgl;

2015-07-21 14:28:54

by Christoph Hellwig

Subject: Re: [PATCH] scsi: fix memory leak with scsi-mq

Thanks Tony,

this looks good to me. In the long run I'll see how we can hide
these implementation details in the lib/scatterlist.c code instead
of burdening it on the users.

Reviewed-by: Christoph Hellwig <[email protected]>

2015-07-27 17:52:09

by Ewan Milne

Subject: Re: [PATCH] scsi: fix memory leak with scsi-mq

On Thu, 2015-07-16 at 11:40 -0400, Tony Battersby wrote:
> Fix a memory leak with scsi-mq triggered by commands with large data
> transfer length.
>
> __sg_alloc_table() sets both table->nents and table->orig_nents to the
> same value. When the scatterlist is DMA-mapped, table->nents is
> overwritten with the (possibly smaller) size of the DMA-mapped
> scatterlist, while table->orig_nents retains the original size of the
> allocated scatterlist. scsi_free_sgtable() should therefore check
> orig_nents instead of nents, and all code that initializes sdb->table
> without calling __sg_alloc_table() should set both nents and orig_nents.
>
> Fixes: d285203cf647 ("scsi: add support for a blk-mq based I/O path.")
> Cc: <[email protected]> # 3.17+
> Signed-off-by: Tony Battersby <[email protected]>
> ---
>
> For immediate inclusion.
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 106884a..cfadcce 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -944,7 +944,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
> scmd->sdb.length);
> scmd->sdb.table.sgl = &ses->sense_sgl;
> scmd->sc_data_direction = DMA_FROM_DEVICE;
> - scmd->sdb.table.nents = 1;
> + scmd->sdb.table.nents = scmd->sdb.table.orig_nents = 1;
> scmd->cmnd[0] = REQUEST_SENSE;
> scmd->cmnd[4] = scmd->sdb.length;
> scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index b1a2631..448ebda 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -583,7 +583,7 @@ static struct scatterlist *scsi_sg_alloc(unsigned int nents, gfp_t gfp_mask)
>
> static void scsi_free_sgtable(struct scsi_data_buffer *sdb, bool mq)
> {
> - if (mq && sdb->table.nents <= SCSI_MAX_SG_SEGMENTS)
> + if (mq && sdb->table.orig_nents <= SCSI_MAX_SG_SEGMENTS)
> return;
> __sg_free_table(&sdb->table, SCSI_MAX_SG_SEGMENTS, mq, scsi_sg_free);
> }
> @@ -597,8 +597,8 @@ static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb, int nents, bool mq)
>
> if (mq) {
> if (nents <= SCSI_MAX_SG_SEGMENTS) {
> - sdb->table.nents = nents;
> - sg_init_table(sdb->table.sgl, sdb->table.nents);
> + sdb->table.nents = sdb->table.orig_nents = nents;
> + sg_init_table(sdb->table.sgl, nents);
> return 0;
> }
> first_chunk = sdb->table.sgl;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Looks good. James, can we get this in please?

Reviewed-by: Ewan D. Milne <[email protected]>