* [PATCH] target: Fix NULL dereference during LUN lookup + active I/O shutdown
@ 2017-02-23 6:56 Nicholas A. Bellinger
2017-02-23 17:46 ` Bryant G. Ly
0 siblings, 1 reply; 3+ messages in thread
From: Nicholas A. Bellinger @ 2017-02-23 6:56 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, lkml, Nicholas Bellinger, Rob Millner, Vaibhav Tandon
From: Nicholas Bellinger <nab@linux-iscsi.org>
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: Rob Millner <rlm@daterainc.com>
Tested-by: Rob Millner <rlm@daterainc.com>
Cc: Rob Millner <rlm@daterainc.com>
Tested-by: Vaibhav Tandon <vst@datera.io>
Cc: Vaibhav Tandon <vst@datera.io>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/target/target_core_device.c | 10 ++++++++--
drivers/target/target_core_tpg.c | 3 ++-
drivers/target/target_core_transport.c | 31 ++++++++++++++++++++++++++++++-
include/target/target_core_base.h | 1 +
4 files changed, 41 insertions(+), 4 deletions(-)
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index cb7047d..c754ae3 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -78,12 +78,16 @@
&deve->read_bytes);
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->pr_res_key = deve->pr_res_key;
se_cmd->orig_fe_lun = unpacked_lun;
se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
-
- percpu_ref_get(&se_lun->lun_ref);
se_cmd->lun_ref_active = true;
if ((se_cmd->data_direction == DMA_TO_DEVICE) &&
@@ -97,6 +101,7 @@
goto ref_dev;
}
}
+out_unlock:
rcu_read_unlock();
if (!se_lun) {
@@ -815,6 +820,7 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
xcopy_lun = &dev->xcopy_lun;
rcu_assign_pointer(xcopy_lun->lun_se_dev, dev);
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_dev_link);
mutex_init(&xcopy_lun->lun_tg_pt_md_mutex);
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index d99752c..2744251 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -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);
- complete(&lun->lun_ref_comp);
+ complete(&lun->lun_shutdown_comp);
}
int core_tpg_register(
@@ -571,6 +571,7 @@ struct se_lun *core_tpg_alloc_lun(
lun->lun_link_magic = SE_LUN_LINK_MAGIC;
atomic_set(&lun->lun_acl_count, 0);
init_completion(&lun->lun_ref_comp);
+ init_completion(&lun->lun_shutdown_comp);
INIT_LIST_HEAD(&lun->lun_deve_list);
INIT_LIST_HEAD(&lun->lun_dev_link);
atomic_set(&lun->lun_tg_pt_secondary_offline, 0);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index efb9e6f..434d9d6 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2700,10 +2700,39 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
}
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)
{
- 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);
+ /*
+ * 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
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index d7336f3..16d3be8 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -730,6 +730,7 @@ struct se_lun {
struct config_group lun_group;
struct se_port_stat_grps port_stat_grps;
struct completion lun_ref_comp;
+ struct completion lun_shutdown_comp;
struct percpu_ref lun_ref;
struct list_head lun_dev_link;
struct hlist_node link;
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] target: Fix NULL dereference during LUN lookup + active I/O shutdown
2017-02-23 6:56 [PATCH] target: Fix NULL dereference during LUN lookup + active I/O shutdown Nicholas A. Bellinger
@ 2017-02-23 17:46 ` Bryant G. Ly
2017-03-02 7:09 ` Nicholas A. Bellinger
0 siblings, 1 reply; 3+ messages in thread
From: Bryant G. Ly @ 2017-02-23 17:46 UTC (permalink / raw)
To: Nicholas A. Bellinger, target-devel
Cc: linux-scsi, lkml, Rob Millner, Vaibhav Tandon
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> 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: Rob Millner <rlm@daterainc.com>
> Tested-by: Rob Millner <rlm@daterainc.com>
> Cc: Rob Millner <rlm@daterainc.com>
> Tested-by: Vaibhav Tandon <vst@datera.io>
> Cc: Vaibhav Tandon <vst@datera.io>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
> drivers/target/target_core_device.c | 10 ++++++++--
> drivers/target/target_core_tpg.c | 3 ++-
> drivers/target/target_core_transport.c | 31 ++++++++++++++++++++++++++++++-
> include/target/target_core_base.h | 1 +
> 4 files changed, 41 insertions(+), 4 deletions(-)
>
I have seen this and have tested this with our custom kernel.
So this looks good from me!
-Bryant
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] target: Fix NULL dereference during LUN lookup + active I/O shutdown
2017-02-23 17:46 ` Bryant G. Ly
@ 2017-03-02 7:09 ` Nicholas A. Bellinger
0 siblings, 0 replies; 3+ messages in thread
From: Nicholas A. Bellinger @ 2017-03-02 7:09 UTC (permalink / raw)
To: Bryant G. Ly; +Cc: target-devel, linux-scsi, lkml, Rob Millner, Vaibhav Tandon
On Thu, 2017-02-23 at 11:46 -0600, Bryant G. Ly wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > 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: Rob Millner <rlm@daterainc.com>
> > Tested-by: Rob Millner <rlm@daterainc.com>
> > Cc: Rob Millner <rlm@daterainc.com>
> > Tested-by: Vaibhav Tandon <vst@datera.io>
> > Cc: Vaibhav Tandon <vst@datera.io>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> > drivers/target/target_core_device.c | 10 ++++++++--
> > drivers/target/target_core_tpg.c | 3 ++-
> > drivers/target/target_core_transport.c | 31 ++++++++++++++++++++++++++++++-
> > include/target/target_core_base.h | 1 +
> > 4 files changed, 41 insertions(+), 4 deletions(-)
> >
> I have seen this and have tested this with our custom kernel.
>
> So this looks good from me!
>
Added your Tested-by to the patch.
Thanks Bryant.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-03-02 7:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-23 6:56 [PATCH] target: Fix NULL dereference during LUN lookup + active I/O shutdown Nicholas A. Bellinger
2017-02-23 17:46 ` Bryant G. Ly
2017-03-02 7:09 ` Nicholas A. Bellinger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).