From: Nicholas Bellinger <[email protected]>
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 <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Himanshu Madhani <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: [email protected] # 3.14+
Signed-off-by: Nicholas Bellinger <[email protected]>
---
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)
--
1.9.1
(Adding Quinn CC')
Reviews please..?
On Sat, 2017-06-03 at 21:09 +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[email protected]>
>
> 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 <[email protected]>
> Cc: Mike Christie <[email protected]>
> Cc: Hannes Reinecke <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Himanshu Madhani <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> Cc: [email protected] # 3.14+
> Signed-off-by: Nicholas Bellinger <[email protected]>
> ---
> 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)