* [PATCH] tcmu: Fix possbile memory leak when recalculating the cmd base size
@ 2017-07-11 8:05 lixiubo
2017-07-11 9:06 ` Xiubo Li
0 siblings, 1 reply; 6+ messages in thread
From: lixiubo @ 2017-07-11 8:05 UTC (permalink / raw)
To: nab, mchristi; +Cc: bryantly, linux-scsi, target-devel, Xiubo Li
From: Xiubo Li <lixiubo@cmss.chinamobile.com>
For all the entries allocated from the ring cmd area, the memory
is something like the stack, which will reserve the old data, so
the entry->req.iov_bidi_cnt maybe none zero.
To fix this, just memset all the entry memory before using it, and
also to be more readable we adjust the bidi code.
Fixed: fe25cc34795(tcmu: Recalculate the tcmu_cmd size to save cmd area
memories)
Reported-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
---
drivers/target/target_core_user.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index 2f1fa92..be62c86 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -840,6 +840,7 @@ static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
}
entry = (void *) mb + CMDR_OFF + cmd_head;
+ memset(entry, 0, command_size);
tcmu_hdr_set_op(&entry->hdr.len_op, TCMU_OP_CMD);
entry->hdr.cmd_id = tcmu_cmd->cmd_id;
entry->hdr.kflags = 0;
@@ -865,8 +866,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
entry->req.iov_dif_cnt = 0;
/* Handle BIDI commands */
+ iov_cnt = 0;
if (se_cmd->se_cmd_flags & SCF_BIDI) {
- iov_cnt = 0;
iov++;
ret = scatter_data_area(udev, tcmu_cmd,
se_cmd->t_bidi_data_sg,
@@ -879,8 +880,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
pr_err("tcmu: alloc and scatter bidi data failed\n");
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
}
- entry->req.iov_bidi_cnt = iov_cnt;
}
+ entry->req.iov_bidi_cnt = iov_cnt;
/*
* Recalaulate the command's base size and size according
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tcmu: Fix possbile memory leak when recalculating the cmd base size
2017-07-11 8:05 [PATCH] tcmu: Fix possbile memory leak when recalculating the cmd base size lixiubo
@ 2017-07-11 9:06 ` Xiubo Li
2017-07-11 9:24 ` Damien Le Moal
0 siblings, 1 reply; 6+ messages in thread
From: Xiubo Li @ 2017-07-11 9:06 UTC (permalink / raw)
To: nab, mchristi, Damien Le Moal; +Cc: bryantly, linux-scsi, target-devel
To Damien,
Please test this, I think this maybe helpful.
Thanks,
BRs
On 2017年07月11日 16:05, lixiubo@cmss.chinamobile.com wrote:
> From: Xiubo Li <lixiubo@cmss.chinamobile.com>
>
> For all the entries allocated from the ring cmd area, the memory
> is something like the stack, which will reserve the old data, so
> the entry->req.iov_bidi_cnt maybe none zero.
>
> To fix this, just memset all the entry memory before using it, and
> also to be more readable we adjust the bidi code.
>
> Fixed: fe25cc34795(tcmu: Recalculate the tcmu_cmd size to save cmd area
> memories)
> Reported-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
> ---
> drivers/target/target_core_user.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 2f1fa92..be62c86 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -840,6 +840,7 @@ static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
> }
>
> entry = (void *) mb + CMDR_OFF + cmd_head;
> + memset(entry, 0, command_size);
> tcmu_hdr_set_op(&entry->hdr.len_op, TCMU_OP_CMD);
> entry->hdr.cmd_id = tcmu_cmd->cmd_id;
> entry->hdr.kflags = 0;
> @@ -865,8 +866,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
> entry->req.iov_dif_cnt = 0;
>
> /* Handle BIDI commands */
> + iov_cnt = 0;
> if (se_cmd->se_cmd_flags & SCF_BIDI) {
> - iov_cnt = 0;
> iov++;
> ret = scatter_data_area(udev, tcmu_cmd,
> se_cmd->t_bidi_data_sg,
> @@ -879,8 +880,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
> pr_err("tcmu: alloc and scatter bidi data failed\n");
> return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> }
> - entry->req.iov_bidi_cnt = iov_cnt;
> }
> + entry->req.iov_bidi_cnt = iov_cnt;
>
> /*
> * Recalaulate the command's base size and size according
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tcmu: Fix possbile memory leak when recalculating the cmd base size
2017-07-11 9:06 ` Xiubo Li
@ 2017-07-11 9:24 ` Damien Le Moal
2017-07-11 9:33 ` [PATCH] tcmu: Fix possbile memory leak when recalculating the cmdbase size Xiubo Li
2017-07-11 9:34 ` [PATCH] tcmu: Fix possbile memory leak when recalculating the cmd base size Nicholas A. Bellinger
0 siblings, 2 replies; 6+ messages in thread
From: Damien Le Moal @ 2017-07-11 9:24 UTC (permalink / raw)
To: mchristi, lixiubo, nab; +Cc: linux-scsi, target-devel, bryantly
Xiubo,
Well done ! This fixed my problem. The ZBC test suite now passes all tests on
my target without crashing the kernel.
Please see some comments/nitpicks below.
Otherwise, please feel free to add my "tested-by"
On Tue, 2017-07-11 at 17:06 +0800, Xiubo Li wrote:
> On 2017年07月11日 16:05, lixiubo@cmss.chinamobile.com wrote:
> > From: Xiubo Li <lixiubo@cmss.chinamobile.com>
> >
> > For all the entries allocated from the ring cmd area, the memory
> > is something like the stack, which will reserve the old data, so
> > the entry->req.iov_bidi_cnt maybe none zero.
> >
> > To fix this, just memset all the entry memory before using it, and
> > also to be more readable we adjust the bidi code.
> >
> > Fixed: fe25cc34795(tcmu: Recalculate the tcmu_cmd size to save cmd area
> > memories)
> > Reported-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> > Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
> > ---
> > drivers/target/target_core_user.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/target/target_core_user.c
> > b/drivers/target/target_core_user.c
> > index 2f1fa92..be62c86 100644
> > --- a/drivers/target/target_core_user.c
> > +++ b/drivers/target/target_core_user.c
> > @@ -840,6 +840,7 @@ static inline size_t tcmu_cmd_get_cmd_size(struct
> > tcmu_cmd *tcmu_cmd,
> > }
> >
> > entry = (void *) mb + CMDR_OFF + cmd_head;
> > + memset(entry, 0, command_size);
> > tcmu_hdr_set_op(&entry->hdr.len_op, TCMU_OP_CMD);
> > entry->hdr.cmd_id = tcmu_cmd->cmd_id;
> > entry->hdr.kflags = 0;
The added memset allows removing the 0 assignment here, and the one that
follows this one too.
> > @@ -865,8 +866,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct
> > tcmu_cmd *tcmu_cmd,
> > entry->req.iov_dif_cnt = 0;
> >
> > /* Handle BIDI commands */
> > + iov_cnt = 0;
> > if (se_cmd->se_cmd_flags & SCF_BIDI) {
> > - iov_cnt = 0;
> > iov++;
> > ret = scatter_data_area(udev, tcmu_cmd,
> > se_cmd->t_bidi_data_sg,
> > @@ -879,8 +880,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct
> > tcmu_cmd *tcmu_cmd,
> > pr_err("tcmu: alloc and scatter bidi data
> > failed\n");
> > return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> > }
> > - entry->req.iov_bidi_cnt = iov_cnt;
> > }
> > + entry->req.iov_bidi_cnt = iov_cnt;
> >
> > /*
> > * Recalaulate the command's base size and size according
For reference, here is the actual patch I used for testing:
diff --git a/drivers/target/target_core_user.c
b/drivers/target/target_core_user.c
index 2f1fa92..3b25ef3 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -563,7 +563,7 @@ static int scatter_data_area(struct tcmu_dev *udev,
to_offset = get_block_offset_user(udev, dbi,
block_remaining);
offset = DATA_BLOCK_SIZE - block_remaining;
- to = (void *)(unsigned long)to + offset;
+ to += offset;
if (*iov_cnt != 0 &&
to_offset == iov_tail(udev, *iov)) {
@@ -636,7 +636,7 @@ static void gather_data_area(struct tcmu_dev *udev, struct
tcmu_cmd *cmd,
copy_bytes = min_t(size_t, sg_remaining,
block_remaining);
offset = DATA_BLOCK_SIZE - block_remaining;
- from = (void *)(unsigned long)from + offset;
+ from += offset;
tcmu_flush_dcache_range(from, copy_bytes);
memcpy(to + sg->length - sg_remaining, from,
copy_bytes);
@@ -840,10 +840,9 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
}
entry = (void *) mb + CMDR_OFF + cmd_head;
+ memset(entry, 0, command_size);
tcmu_hdr_set_op(&entry->hdr.len_op, TCMU_OP_CMD);
entry->hdr.cmd_id = tcmu_cmd->cmd_id;
- entry->hdr.kflags = 0;
- entry->hdr.uflags = 0;
/* Handle allocating space from the data area */
tcmu_cmd_reset_dbi_cur(tcmu_cmd);
@@ -862,11 +861,10 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
}
entry->req.iov_cnt = iov_cnt;
- entry->req.iov_dif_cnt = 0;
/* Handle BIDI commands */
+ iov_cnt = 0;
if (se_cmd->se_cmd_flags & SCF_BIDI) {
- iov_cnt = 0;
iov++;
ret = scatter_data_area(udev, tcmu_cmd,
se_cmd->t_bidi_data_sg,
@@ -879,8 +877,8 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
pr_err("tcmu: alloc and scatter bidi data failed\n");
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
}
- entry->req.iov_bidi_cnt = iov_cnt;
}
+ entry->req.iov_bidi_cnt = iov_cnt;
/*
* Recalaulate the command's base size and size according
--
Best regards.
--
Damien Le Moal
Western Digital
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tcmu: Fix possbile memory leak when recalculating the cmdbase size
2017-07-11 9:24 ` Damien Le Moal
@ 2017-07-11 9:33 ` Xiubo Li
2017-07-11 9:34 ` [PATCH] tcmu: Fix possbile memory leak when recalculating the cmd base size Nicholas A. Bellinger
1 sibling, 0 replies; 6+ messages in thread
From: Xiubo Li @ 2017-07-11 9:33 UTC (permalink / raw)
To: Damien Le Moal, mchristi, nab; +Cc: linux-scsi, target-devel, bryantly
Hi Damien,
Good news.
Thanks very much for you help to test about this.
On 2017年07月11日 17:24, Damien Le Moal wrote:
> Xiubo,
>
> Well done ! This fixed my problem. The ZBC test suite now passes all tests on
> my target without crashing the kernel.
>
> Please see some comments/nitpicks below.
>
> Otherwise, please feel free to add my "tested-by"
>
>
> On Tue, 2017-07-11 at 17:06 +0800, Xiubo Li wrote:
>> On 2017年07月11日 16:05, lixiubo@cmss.chinamobile.com wrote:
>>> From: Xiubo Li <lixiubo@cmss.chinamobile.com>
>>>
>>> For all the entries allocated from the ring cmd area, the memory
>>> is something like the stack, which will reserve the old data, so
>>> the entry->req.iov_bidi_cnt maybe none zero.
>>>
>>> To fix this, just memset all the entry memory before using it, and
>>> also to be more readable we adjust the bidi code.
>>>
>>> Fixed: fe25cc34795(tcmu: Recalculate the tcmu_cmd size to save cmd area
>>> memories)
>>> Reported-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
>>> Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
>>> ---
>>> drivers/target/target_core_user.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/target/target_core_user.c
>>> b/drivers/target/target_core_user.c
>>> index 2f1fa92..be62c86 100644
>>> --- a/drivers/target/target_core_user.c
>>> +++ b/drivers/target/target_core_user.c
>>> @@ -840,6 +840,7 @@ static inline size_t tcmu_cmd_get_cmd_size(struct
>>> tcmu_cmd *tcmu_cmd,
>>> }
>>>
>>> entry = (void *) mb + CMDR_OFF + cmd_head;
>>> + memset(entry, 0, command_size);
>>> tcmu_hdr_set_op(&entry->hdr.len_op, TCMU_OP_CMD);
>>> entry->hdr.cmd_id = tcmu_cmd->cmd_id;
>>> entry->hdr.kflags = 0;
> The added memset allows removing the 0 assignment here, and the one that
> follows this one too.
>
Yes, it is. Maybe reserving them could make the code more readable ?
Thanks,
BRs
Xiubo
>>> @@ -865,8 +866,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct
>>> tcmu_cmd *tcmu_cmd,
>>> entry->req.iov_dif_cnt = 0;
>>>
>>> /* Handle BIDI commands */
>>> + iov_cnt = 0;
>>> if (se_cmd->se_cmd_flags & SCF_BIDI) {
>>> - iov_cnt = 0;
>>> iov++;
>>> ret = scatter_data_area(udev, tcmu_cmd,
>>> se_cmd->t_bidi_data_sg,
>>> @@ -879,8 +880,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct
>>> tcmu_cmd *tcmu_cmd,
>>> pr_err("tcmu: alloc and scatter bidi data
>>> failed\n");
>>> return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>>> }
>>> - entry->req.iov_bidi_cnt = iov_cnt;
>>> }
>>> + entry->req.iov_bidi_cnt = iov_cnt;
>>>
>>> /*
>>> * Recalaulate the command's base size and size according
> For reference, here is the actual patch I used for testing:
>
> diff --git a/drivers/target/target_core_user.c
> b/drivers/target/target_core_user.c
> index 2f1fa92..3b25ef3 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -563,7 +563,7 @@ static int scatter_data_area(struct tcmu_dev *udev,
> to_offset = get_block_offset_user(udev, dbi,
> block_remaining);
> offset = DATA_BLOCK_SIZE - block_remaining;
> - to = (void *)(unsigned long)to + offset;
> + to += offset;
>
> if (*iov_cnt != 0 &&
> to_offset == iov_tail(udev, *iov)) {
> @@ -636,7 +636,7 @@ static void gather_data_area(struct tcmu_dev *udev, struct
> tcmu_cmd *cmd,
> copy_bytes = min_t(size_t, sg_remaining,
> block_remaining);
> offset = DATA_BLOCK_SIZE - block_remaining;
> - from = (void *)(unsigned long)from + offset;
> + from += offset;
> tcmu_flush_dcache_range(from, copy_bytes);
> memcpy(to + sg->length - sg_remaining, from,
> copy_bytes);
> @@ -840,10 +840,9 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
> }
>
> entry = (void *) mb + CMDR_OFF + cmd_head;
> + memset(entry, 0, command_size);
> tcmu_hdr_set_op(&entry->hdr.len_op, TCMU_OP_CMD);
> entry->hdr.cmd_id = tcmu_cmd->cmd_id;
> - entry->hdr.kflags = 0;
> - entry->hdr.uflags = 0;
>
> /* Handle allocating space from the data area */
> tcmu_cmd_reset_dbi_cur(tcmu_cmd);
> @@ -862,11 +861,10 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
> return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> }
> entry->req.iov_cnt = iov_cnt;
> - entry->req.iov_dif_cnt = 0;
>
> /* Handle BIDI commands */
> + iov_cnt = 0;
> if (se_cmd->se_cmd_flags & SCF_BIDI) {
> - iov_cnt = 0;
> iov++;
> ret = scatter_data_area(udev, tcmu_cmd,
> se_cmd->t_bidi_data_sg,
> @@ -879,8 +877,8 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
> pr_err("tcmu: alloc and scatter bidi data failed\n");
> return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> }
> - entry->req.iov_bidi_cnt = iov_cnt;
> }
> + entry->req.iov_bidi_cnt = iov_cnt;
>
> /*
> * Recalaulate the command's base size and size according
> --
>
> Best regards.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tcmu: Fix possbile memory leak when recalculating the cmd base size
2017-07-11 9:24 ` Damien Le Moal
2017-07-11 9:33 ` [PATCH] tcmu: Fix possbile memory leak when recalculating the cmdbase size Xiubo Li
@ 2017-07-11 9:34 ` Nicholas A. Bellinger
2017-07-11 9:37 ` [PATCH] tcmu: Fix possbile memory leak when recalculating thecmd " Xiubo Li
1 sibling, 1 reply; 6+ messages in thread
From: Nicholas A. Bellinger @ 2017-07-11 9:34 UTC (permalink / raw)
To: Damien Le Moal; +Cc: mchristi, lixiubo, linux-scsi, target-devel, bryantly
On Tue, 2017-07-11 at 09:24 +0000, Damien Le Moal wrote:
> Xiubo,
>
> Well done ! This fixed my problem. The ZBC test suite now passes all tests on
> my target without crashing the kernel.
>
> Please see some comments/nitpicks below.
>
> Otherwise, please feel free to add my "tested-by"
>
Great, thanks for the quick turn-around.
Xiubo, please pick-up Damien's extra bits and repost for v2. Also,
please update the patch topic + commit log to mention it addresses a
general protection fault OOPs, so others can find it easily when looking
through git log.
Damien's OOPs would probably be the best one to include in the commit
log, since Bryant's was on POWER. ;)
Btw, since it's a regression fix related to commit fe25cc34795, I assume
it should be CC'ed to stable for v4.12.y, right..?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tcmu: Fix possbile memory leak when recalculating thecmd base size
2017-07-11 9:34 ` [PATCH] tcmu: Fix possbile memory leak when recalculating the cmd base size Nicholas A. Bellinger
@ 2017-07-11 9:37 ` Xiubo Li
0 siblings, 0 replies; 6+ messages in thread
From: Xiubo Li @ 2017-07-11 9:37 UTC (permalink / raw)
To: Nicholas A. Bellinger, Damien Le Moal
Cc: mchristi, linux-scsi, target-devel, bryantly
On 2017年07月11日 17:34, Nicholas A. Bellinger wrote:
> On Tue, 2017-07-11 at 09:24 +0000, Damien Le Moal wrote:
>> Xiubo,
>>
>> Well done ! This fixed my problem. The ZBC test suite now passes all tests on
>> my target without crashing the kernel.
>>
>> Please see some comments/nitpicks below.
>>
>> Otherwise, please feel free to add my "tested-by"
>>
> Great, thanks for the quick turn-around.
>
> Xiubo, please pick-up Damien's extra bits and repost for v2. Also,
> please update the patch topic + commit log to mention it addresses a
> general protection fault OOPs, so others can find it easily when looking
> through git log.
>
> Damien's OOPs would probably be the best one to include in the commit
> log, since Bryant's was on POWER. ;)
Sure, I will repost for v2 later.
> Btw, since it's a regression fix related to commit fe25cc34795, I assume
> it should be CC'ed to stable for v4.12.y, right..?
>
Yes, it is.
Thanks
BRs
Xiubo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-11 9:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-11 8:05 [PATCH] tcmu: Fix possbile memory leak when recalculating the cmd base size lixiubo
2017-07-11 9:06 ` Xiubo Li
2017-07-11 9:24 ` Damien Le Moal
2017-07-11 9:33 ` [PATCH] tcmu: Fix possbile memory leak when recalculating the cmdbase size Xiubo Li
2017-07-11 9:34 ` [PATCH] tcmu: Fix possbile memory leak when recalculating the cmd base size Nicholas A. Bellinger
2017-07-11 9:37 ` [PATCH] tcmu: Fix possbile memory leak when recalculating thecmd " Xiubo Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.