| Message ID | 20190720030637.14447-3-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/scsi/dm-rq: fix leak of request private data in dm-mpath | expand |
On 7/19/19 8:06 PM, Ming Lei wrote: > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index e1da8c70a266..52537c145762 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -154,12 +154,9 @@ scsi_set_blocked(struct scsi_cmnd *cmd, int reason) > > static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd) > { > - if (cmd->request->rq_flags & RQF_DONTPREP) { > - cmd->request->rq_flags &= ~RQF_DONTPREP; > - scsi_mq_uninit_cmd(cmd); > - } else { > - WARN_ON_ONCE(true); > - } > + WARN_ON_ONCE(!(cmd->request->rq_flags & RQF_DONTPREP)); > + > + scsi_mq_uninit_cmd(cmd); > blk_mq_requeue_request(cmd->request, true); > } The above changes are independent of this patch series. Have you considered to move these into a separate patch? > +/* > + * Only called when the request isn't completed by SCSI, and not freed by > + * SCSI > + */ > +static void scsi_cleanup_rq(struct request *rq) > +{ > + struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq); > + > + scsi_mq_uninit_cmd(cmd); > +} Is the comment above this function correct? The previous patch adds an unconditional call to mq_ops->cleanup_rq() in blk_mq_free_request(). Thanks, Bart.
On Mon, Jul 22, 2019 at 08:40:23AM -0700, Bart Van Assche wrote: > On 7/19/19 8:06 PM, Ming Lei wrote: > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > > index e1da8c70a266..52537c145762 100644 > > --- a/drivers/scsi/scsi_lib.c > > +++ b/drivers/scsi/scsi_lib.c > > @@ -154,12 +154,9 @@ scsi_set_blocked(struct scsi_cmnd *cmd, int reason) > > static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd) > > { > > - if (cmd->request->rq_flags & RQF_DONTPREP) { > > - cmd->request->rq_flags &= ~RQF_DONTPREP; > > - scsi_mq_uninit_cmd(cmd); > > - } else { > > - WARN_ON_ONCE(true); > > - } > > + WARN_ON_ONCE(!(cmd->request->rq_flags & RQF_DONTPREP)); > > + > > + scsi_mq_uninit_cmd(cmd); > > blk_mq_requeue_request(cmd->request, true); > > } > > The above changes are independent of this patch series. Have you considered > to move these into a separate patch? OK. > > > +/* > > + * Only called when the request isn't completed by SCSI, and not freed by > > + * SCSI > > + */ > > +static void scsi_cleanup_rq(struct request *rq) > > +{ > > + struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq); > > + > > + scsi_mq_uninit_cmd(cmd); > > +} > > Is the comment above this function correct? The previous patch adds an > unconditional call to mq_ops->cleanup_rq() in blk_mq_free_request(). You are right, will fix it in V3. Thanks, Ming
Hi, Thanks for your report! On Thu, Jul 25, 2019 at 06:46:29PM +0800, kernel test robot wrote: > FYI, we noticed the following commit (built with gcc-7): > > commit: ae86a1c5530b52dc44a280e78feb0c4eb2dd8595 ("[PATCH V2 2/2] scsi: implement .cleanup_rq callback") > url: https://github.com/0day-ci/linux/commits/Ming-Lei/blk-mq-add-callback-of-cleanup_rq/20190720-133431 > > > in testcase: blktests > with following parameters: > > disk: 1SSD > test: block-group1 > > > > on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 4G > > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace): > > > +---------------------------------------------------------------------------------------------------------------+------------+------------+ > | | bd222ca85f | ae86a1c553 | > +---------------------------------------------------------------------------------------------------------------+------------+------------+ > | boot_successes | 0 | 0 | > | boot_failures | 11 | 14 | > | BUG:kernel_reboot-without-warning_in_test_stage | 11 | 1 | > | BUG:kernel_NULL_pointer_dereference,address | 0 | 4 | > | Oops:#[##] | 0 | 4 | > | RIP:scsi_queue_rq | 0 | 4 | > | Kernel_panic-not_syncing:Fatal_exception | 0 | 4 | > | invoked_oom-killer:gfp_mask=0x | 0 | 9 | > | Mem-Info | 0 | 9 | > | page_allocation_failure:order:#,mode:#(GFP_KERNEL|__GFP_RETRY_MAYFAIL),nodemask=(null),cpuset=/,mems_allowed= | 0 | 2 | > +---------------------------------------------------------------------------------------------------------------+------------+------------+ > > > If you fix the issue, kindly add following tag > Reported-by: kernel test robot <rong.a.chen@intel.com> > > > [ 140.974865] BUG: kernel NULL pointer dereference, address: 000000000000001c Yeah, I know this issue, and it has been fixed in either V3 or V4. Thanks, Ming
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index e1da8c70a266..52537c145762 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -154,12 +154,9 @@ scsi_set_blocked(struct scsi_cmnd *cmd, int reason) static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd) { - if (cmd->request->rq_flags & RQF_DONTPREP) { - cmd->request->rq_flags &= ~RQF_DONTPREP; - scsi_mq_uninit_cmd(cmd); - } else { - WARN_ON_ONCE(true); - } + WARN_ON_ONCE(!(cmd->request->rq_flags & RQF_DONTPREP)); + + scsi_mq_uninit_cmd(cmd); blk_mq_requeue_request(cmd->request, true); } @@ -563,9 +560,13 @@ static void scsi_mq_free_sgtables(struct scsi_cmnd *cmd) static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd) { + if (!(cmd->request->rq_flags & RQF_DONTPREP)) + return; + scsi_mq_free_sgtables(cmd); scsi_uninit_cmd(cmd); scsi_del_cmd_from_list(cmd); + cmd->request->rq_flags &= ~RQF_DONTPREP; } /* Returns false when no more bytes to process, true if there are more */ @@ -1089,6 +1090,17 @@ static void scsi_initialize_rq(struct request *rq) cmd->retries = 0; } +/* + * Only called when the request isn't completed by SCSI, and not freed by + * SCSI + */ +static void scsi_cleanup_rq(struct request *rq) +{ + struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq); + + scsi_mq_uninit_cmd(cmd); +} + /* Add a command to the list used by the aacraid and dpt_i2o drivers */ void scsi_add_cmd_to_list(struct scsi_cmnd *cmd) { @@ -1708,8 +1720,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx, * we hit an error, as we will never see this command * again. */ - if (req->rq_flags & RQF_DONTPREP) - scsi_mq_uninit_cmd(cmd); + scsi_mq_uninit_cmd(cmd); break; } return ret; @@ -1816,6 +1827,7 @@ static const struct blk_mq_ops scsi_mq_ops = { .init_request = scsi_mq_init_request, .exit_request = scsi_mq_exit_request, .initialize_rq_fn = scsi_initialize_rq, + .cleanup_rq = scsi_cleanup_rq, .busy = scsi_mq_lld_busy, .map_queues = scsi_map_queues, };
Implement .cleanup_rq() callback for freeing driver private part of the request. Then we can avoid to leak request private data if the request isn't completed by SCSI, and freed by blk-mq or upper layer(such as dm-rq) finally. Cc: Ewan D. Milne <emilne@redhat.com> Cc: Bart Van Assche <bvanassche@acm.org> Cc: Hannes Reinecke <hare@suse.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Mike Snitzer <snitzer@redhat.com> Cc: dm-devel@redhat.com Cc: <stable@vger.kernel.org> Fixes: 396eaf21ee17 ("blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback") Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/scsi/scsi_lib.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)