Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751598AbdFIF3U (ORCPT ); Fri, 9 Jun 2017 01:29:20 -0400 Received: from mail.linux-iscsi.org ([67.23.28.174]:36204 "EHLO linux-iscsi.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751501AbdFIF3S (ORCPT ); Fri, 9 Jun 2017 01:29:18 -0400 Message-ID: <1496986156.28997.18.camel@haakon3.risingtidesystems.com> Subject: Re: [PATCH] target: Fix kref->refcount underflow in transport_cmd_finish_abort From: "Nicholas A. Bellinger" To: target-devel Cc: linux-scsi , lkml , Mike Christie , Hannes Reinecke , Christoph Hellwig , Himanshu Madhani , Sagi Grimberg , "Tran, Quinn" Date: Thu, 08 Jun 2017 22:29:16 -0700 In-Reply-To: <1496524144-25580-1-git-send-email-nab@linux-iscsi.org> References: <1496524144-25580-1-git-send-email-nab@linux-iscsi.org> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.4.4-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5471 Lines: 142 (Adding Quinn CC') Reviews please..? On Sat, 2017-06-03 at 21:09 +0000, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger > > This patch fixes a se_cmd->cmd_kref underflow during CMD_T_ABORTED > when a fabric driver drops it's second reference from below the > target_core_tmr.c based callers of transport_cmd_finish_abort(). > > Recently with the conversion of kref to refcount_t, this bug was > manifesting itself as: > > [705519.601034] refcount_t: underflow; use-after-free. > [705519.604034] INFO: NMI handler (kgdb_nmi_handler) took too long to run: 20116.512 msecs > [705539.719111] ------------[ cut here ]------------ > [705539.719117] WARNING: CPU: 3 PID: 26510 at lib/refcount.c:184 refcount_sub_and_test+0x33/0x51 > > Since the original kref atomic_t based kref_put() didn't check for > underflow and only invoked the final callback when zero was reached, > this bug did not manifest in practice since all se_cmd memory is > using preallocated tags. > > To address this, go ahead and propigate the existing return from > transport_put_cmd() up via transport_cmd_finish_abort(), and > change transport_cmd_finish_abort() + core_tmr_handle_tas_abort() > callers to only do their local target_put_sess_cmd() if necessary. > > Reported-by: Bart Van Assche > Cc: Mike Christie > Cc: Hannes Reinecke > Cc: Christoph Hellwig > Cc: Himanshu Madhani > Cc: Sagi Grimberg > Cc: stable@vger.kernel.org # 3.14+ > Signed-off-by: Nicholas Bellinger > --- > drivers/target/target_core_internal.h | 2 +- > drivers/target/target_core_tmr.c | 16 ++++++++-------- > drivers/target/target_core_transport.c | 9 ++++++--- > 3 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h > index 9ab7090..0912de7 100644 > --- a/drivers/target/target_core_internal.h > +++ b/drivers/target/target_core_internal.h > @@ -136,7 +136,7 @@ struct se_node_acl *core_tpg_add_initiator_node_acl(struct se_portal_group *tpg, > void release_se_kmem_caches(void); > u32 scsi_get_new_index(scsi_index_t); > void transport_subsystem_check_init(void); > -void transport_cmd_finish_abort(struct se_cmd *, int); > +int transport_cmd_finish_abort(struct se_cmd *, int); > unsigned char *transport_dump_cmd_direction(struct se_cmd *); > void transport_dump_dev_state(struct se_device *, char *, int *); > void transport_dump_dev_info(struct se_device *, struct se_lun *, > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c > index dce1e1b..13f47bf 100644 > --- a/drivers/target/target_core_tmr.c > +++ b/drivers/target/target_core_tmr.c > @@ -75,7 +75,7 @@ void core_tmr_release_req(struct se_tmr_req *tmr) > kfree(tmr); > } > > -static void core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas) > +static int core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas) > { > unsigned long flags; > bool remove = true, send_tas; > @@ -91,7 +91,7 @@ static void core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas) > transport_send_task_abort(cmd); > } > > - transport_cmd_finish_abort(cmd, remove); > + return transport_cmd_finish_abort(cmd, remove); > } > > static int target_check_cdb_and_preempt(struct list_head *list, > @@ -184,8 +184,8 @@ void core_tmr_abort_task( > cancel_work_sync(&se_cmd->work); > transport_wait_for_tasks(se_cmd); > > - transport_cmd_finish_abort(se_cmd, true); > - target_put_sess_cmd(se_cmd); > + if (!transport_cmd_finish_abort(se_cmd, true)) > + target_put_sess_cmd(se_cmd); > > printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for" > " ref_tag: %llu\n", ref_tag); > @@ -281,8 +281,8 @@ static void core_tmr_drain_tmr_list( > cancel_work_sync(&cmd->work); > transport_wait_for_tasks(cmd); > > - transport_cmd_finish_abort(cmd, 1); > - target_put_sess_cmd(cmd); > + if (!transport_cmd_finish_abort(cmd, 1)) > + target_put_sess_cmd(cmd); > } > } > > @@ -380,8 +380,8 @@ static void core_tmr_drain_state_list( > cancel_work_sync(&cmd->work); > transport_wait_for_tasks(cmd); > > - core_tmr_handle_tas_abort(cmd, tas); > - target_put_sess_cmd(cmd); > + if (!core_tmr_handle_tas_abort(cmd, tas)) > + target_put_sess_cmd(cmd); > } > } > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 6025935..f1b3a46 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -651,9 +651,10 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd) > percpu_ref_put(&lun->lun_ref); > } > > -void transport_cmd_finish_abort(struct se_cmd *cmd, int remove) > +int transport_cmd_finish_abort(struct se_cmd *cmd, int remove) > { > bool ack_kref = (cmd->se_cmd_flags & SCF_ACK_KREF); > + int ret = 0; > > if (cmd->se_cmd_flags & SCF_SE_LUN_CMD) > transport_lun_remove_cmd(cmd); > @@ -665,9 +666,11 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove) > cmd->se_tfo->aborted_task(cmd); > > if (transport_cmd_check_stop_to_fabric(cmd)) > - return; > + return 1; > if (remove && ack_kref) > - transport_put_cmd(cmd); > + ret = transport_put_cmd(cmd); > + > + return ret; > } > > static void target_complete_failure_work(struct work_struct *work)