Commit bd4e2d29 authored by Nicholas Bellinger's avatar Nicholas Bellinger

target: Fix NULL dereference during LUN lookup + active I/O shutdown

When transport_clear_lun_ref() is shutting down a se_lun via
configfs with new I/O in-flight, it's possible to trigger a
NULL pointer dereference in transport_lookup_cmd_lun() due
to the fact percpu_ref_get() doesn't do any __PERCPU_REF_DEAD
checking before incrementing lun->lun_ref.count after
lun->lun_ref has switched to atomic_t mode.

This results in a NULL pointer dereference as LUN shutdown
code in core_tpg_remove_lun() continues running after the
existing ->release() -> core_tpg_lun_ref_release() callback
completes, and clears the RCU protected se_lun->lun_se_dev
pointer.

During the OOPs, the state of lun->lun_ref in the process
which triggered the NULL pointer dereference looks like
the following on v4.1.y stable code:

struct se_lun {
  lun_link_magic = 4294932337,
  lun_status = TRANSPORT_LUN_STATUS_FREE,

  .....

  lun_se_dev = 0x0,
  lun_sep = 0x0,

  .....

  lun_ref = {
    count = {
      counter = 1
    },
    percpu_count_ptr = 3,
    release = 0xffffffffa02fa1e0 <core_tpg_lun_ref_release>,
    confirm_switch = 0x0,
    force_atomic = false,
    rcu = {
      next = 0xffff88154fa1a5d0,
      func = 0xffffffff8137c4c0 <percpu_ref_switch_to_atomic_rcu>
    }
  }
}

To address this bug, use percpu_ref_tryget_live() to ensure
once __PERCPU_REF_DEAD is visable on all CPUs and ->lun_ref
has switched to atomic_t, all new I/Os will fail to obtain
a new lun->lun_ref reference.

Also use an explicit percpu_ref_kill_and_confirm() callback
to block on ->lun_ref_comp to allow the first stage and
associated RCU grace period to complete, and then block on
->lun_ref_shutdown waiting for the final percpu_ref_put()
to drop the last reference via transport_lun_remove_cmd()
before continuing with core_tpg_remove_lun() shutdown.
Reported-by: default avatarRob Millner <rlm@daterainc.com>
Tested-by: default avatarRob Millner <rlm@daterainc.com>
Cc: Rob Millner <rlm@daterainc.com>
Tested-by: default avatarVaibhav Tandon <vst@datera.io>
Cc: Vaibhav Tandon <vst@datera.io>
Tested-by: default avatarBryant G. Ly <bryantly@linux.vnet.ibm.com>
Cc: <stable@vger.kernel.org> # v3.14+
Signed-off-by: default avatarNicholas Bellinger <nab@linux-iscsi.org>
parent 51ec502a
...@@ -78,12 +78,16 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) ...@@ -78,12 +78,16 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
&deve->read_bytes); &deve->read_bytes);
se_lun = rcu_dereference(deve->se_lun); se_lun = rcu_dereference(deve->se_lun);
if (!percpu_ref_tryget_live(&se_lun->lun_ref)) {
se_lun = NULL;
goto out_unlock;
}
se_cmd->se_lun = rcu_dereference(deve->se_lun); se_cmd->se_lun = rcu_dereference(deve->se_lun);
se_cmd->pr_res_key = deve->pr_res_key; se_cmd->pr_res_key = deve->pr_res_key;
se_cmd->orig_fe_lun = unpacked_lun; se_cmd->orig_fe_lun = unpacked_lun;
se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD; se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
percpu_ref_get(&se_lun->lun_ref);
se_cmd->lun_ref_active = true; se_cmd->lun_ref_active = true;
if ((se_cmd->data_direction == DMA_TO_DEVICE) && if ((se_cmd->data_direction == DMA_TO_DEVICE) &&
...@@ -97,6 +101,7 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) ...@@ -97,6 +101,7 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
goto ref_dev; goto ref_dev;
} }
} }
out_unlock:
rcu_read_unlock(); rcu_read_unlock();
if (!se_lun) { if (!se_lun) {
...@@ -815,6 +820,7 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name) ...@@ -815,6 +820,7 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
xcopy_lun = &dev->xcopy_lun; xcopy_lun = &dev->xcopy_lun;
rcu_assign_pointer(xcopy_lun->lun_se_dev, dev); rcu_assign_pointer(xcopy_lun->lun_se_dev, dev);
init_completion(&xcopy_lun->lun_ref_comp); init_completion(&xcopy_lun->lun_ref_comp);
init_completion(&xcopy_lun->lun_shutdown_comp);
INIT_LIST_HEAD(&xcopy_lun->lun_deve_list); INIT_LIST_HEAD(&xcopy_lun->lun_deve_list);
INIT_LIST_HEAD(&xcopy_lun->lun_dev_link); INIT_LIST_HEAD(&xcopy_lun->lun_dev_link);
mutex_init(&xcopy_lun->lun_tg_pt_md_mutex); mutex_init(&xcopy_lun->lun_tg_pt_md_mutex);
......
...@@ -445,7 +445,7 @@ static void core_tpg_lun_ref_release(struct percpu_ref *ref) ...@@ -445,7 +445,7 @@ static void core_tpg_lun_ref_release(struct percpu_ref *ref)
{ {
struct se_lun *lun = container_of(ref, struct se_lun, lun_ref); struct se_lun *lun = container_of(ref, struct se_lun, lun_ref);
complete(&lun->lun_ref_comp); complete(&lun->lun_shutdown_comp);
} }
int core_tpg_register( int core_tpg_register(
...@@ -571,6 +571,7 @@ struct se_lun *core_tpg_alloc_lun( ...@@ -571,6 +571,7 @@ struct se_lun *core_tpg_alloc_lun(
lun->lun_link_magic = SE_LUN_LINK_MAGIC; lun->lun_link_magic = SE_LUN_LINK_MAGIC;
atomic_set(&lun->lun_acl_count, 0); atomic_set(&lun->lun_acl_count, 0);
init_completion(&lun->lun_ref_comp); init_completion(&lun->lun_ref_comp);
init_completion(&lun->lun_shutdown_comp);
INIT_LIST_HEAD(&lun->lun_deve_list); INIT_LIST_HEAD(&lun->lun_deve_list);
INIT_LIST_HEAD(&lun->lun_dev_link); INIT_LIST_HEAD(&lun->lun_dev_link);
atomic_set(&lun->lun_tg_pt_secondary_offline, 0); atomic_set(&lun->lun_tg_pt_secondary_offline, 0);
......
...@@ -2700,10 +2700,39 @@ void target_wait_for_sess_cmds(struct se_session *se_sess) ...@@ -2700,10 +2700,39 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
} }
EXPORT_SYMBOL(target_wait_for_sess_cmds); EXPORT_SYMBOL(target_wait_for_sess_cmds);
static void target_lun_confirm(struct percpu_ref *ref)
{
struct se_lun *lun = container_of(ref, struct se_lun, lun_ref);
complete(&lun->lun_ref_comp);
}
void transport_clear_lun_ref(struct se_lun *lun) void transport_clear_lun_ref(struct se_lun *lun)
{ {
percpu_ref_kill(&lun->lun_ref); /*
* Mark the percpu-ref as DEAD, switch to atomic_t mode, drop
* the initial reference and schedule confirm kill to be
* executed after one full RCU grace period has completed.
*/
percpu_ref_kill_and_confirm(&lun->lun_ref, target_lun_confirm);
/*
* The first completion waits for percpu_ref_switch_to_atomic_rcu()
* to call target_lun_confirm after lun->lun_ref has been marked
* as __PERCPU_REF_DEAD on all CPUs, and switches to atomic_t
* mode so that percpu_ref_tryget_live() lookup of lun->lun_ref
* fails for all new incoming I/O.
*/
wait_for_completion(&lun->lun_ref_comp); wait_for_completion(&lun->lun_ref_comp);
/*
* The second completion waits for percpu_ref_put_many() to
* invoke ->release() after lun->lun_ref has switched to
* atomic_t mode, and lun->lun_ref.count has reached zero.
*
* At this point all target-core lun->lun_ref references have
* been dropped via transport_lun_remove_cmd(), and it's safe
* to proceed with the remaining LUN shutdown.
*/
wait_for_completion(&lun->lun_shutdown_comp);
} }
static bool static bool
......
...@@ -730,6 +730,7 @@ struct se_lun { ...@@ -730,6 +730,7 @@ struct se_lun {
struct config_group lun_group; struct config_group lun_group;
struct se_port_stat_grps port_stat_grps; struct se_port_stat_grps port_stat_grps;
struct completion lun_ref_comp; struct completion lun_ref_comp;
struct completion lun_shutdown_comp;
struct percpu_ref lun_ref; struct percpu_ref lun_ref;
struct list_head lun_dev_link; struct list_head lun_dev_link;
struct hlist_node link; struct hlist_node link;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment