* [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@redhat.com, lixiubo@cmss.chinamobile.com, nab@linux-iscsi.org Cc: linux-scsi@vger.kernel.org, target-devel@vger.kernel.org, bryantly@linux.vnet.ibm.com 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@redhat.com, nab@linux-iscsi.org Cc: linux-scsi@vger.kernel.org, target-devel@vger.kernel.org, bryantly@linux.vnet.ibm.com 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@redhat.com, lixiubo@cmss.chinamobile.com, linux-scsi@vger.kernel.org, target-devel@vger.kernel.org, bryantly@linux.vnet.ibm.com 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@redhat.com, linux-scsi@vger.kernel.org, target-devel@vger.kernel.org, bryantly@linux.vnet.ibm.com 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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox