* [PATCH] target: Wait RCU grace-period before backend/fabric unload
@ 2015-07-30 6:15 Nicholas A. Bellinger
2015-07-30 13:07 ` Paul E. McKenney
0 siblings, 1 reply; 4+ messages in thread
From: Nicholas A. Bellinger @ 2015-07-30 6:15 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, Nicholas Bellinger, Paul E. McKenney,
Christoph Hellwig, Hannes Reinecke, Sagi Grimberg
From: Nicholas Bellinger <nab@linux-iscsi.org>
This patch addresses a v4.2-rc1 regression where backend driver
struct module unload immediately after ->free_device() has done
an internal call_rcu(), results in IRQ rcu_process_callbacks()
use-after-free paging OOPsen.
It adds a explicit synchronize_rcu() in target_backend_unregister()
to wait a full RCU grace period before releasing target_backend_ops
memory, and allowing TBO->module exit to proceed.
Also, go ahead and do the same for target_unregister_template()
to ensure se_deve_entry->rcu_head -> kfree_rcu() grace period has
passed, before allowing target_core_fabric_ops->owner module exit
to proceed.
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Sagi Grimberg <sagig@mellanox.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/target/target_core_configfs.c | 10 +++++++++-
drivers/target/target_core_hba.c | 10 +++++++++-
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index c2e9fea..b4c3ae0 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -457,8 +457,16 @@ void target_unregister_template(const struct target_core_fabric_ops *fo)
if (!strcmp(t->tf_ops->name, fo->name)) {
BUG_ON(atomic_read(&t->tf_access_cnt));
list_del(&t->tf_list);
+ mutex_unlock(&g_tf_lock);
+ /*
+ * Allow any outstanding fabric se_deve_entry->rcu_head
+ * grace periods to expire post kfree_rcu(), before allowing
+ * fabric driver unload of target_core_fabric_ops->module
+ * to proceed.
+ */
+ synchronize_rcu();
kfree(t);
- break;
+ return;
}
}
mutex_unlock(&g_tf_lock);
diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c
index 62ea4e8..0fb830b 100644
--- a/drivers/target/target_core_hba.c
+++ b/drivers/target/target_core_hba.c
@@ -84,8 +84,16 @@ void target_backend_unregister(const struct target_backend_ops *ops)
list_for_each_entry(tb, &backend_list, list) {
if (tb->ops == ops) {
list_del(&tb->list);
+ mutex_unlock(&backend_mutex);
+ /*
+ * Allow any outstanding backend driver ->rcu_head grace
+ * period to expire post ->free_device() -> call_rcu(),
+ * before allowing backend driver module unload of
+ * target_backend_ops->owner to proceed.
+ */
+ synchronize_rcu();
kfree(tb);
- break;
+ return;
}
}
mutex_unlock(&backend_mutex);
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] target: Wait RCU grace-period before backend/fabric unload
2015-07-30 6:15 [PATCH] target: Wait RCU grace-period before backend/fabric unload Nicholas A. Bellinger
@ 2015-07-30 13:07 ` Paul E. McKenney
2015-07-31 0:23 ` Nicholas A. Bellinger
0 siblings, 1 reply; 4+ messages in thread
From: Paul E. McKenney @ 2015-07-30 13:07 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, Nicholas Bellinger, Christoph Hellwig,
Hannes Reinecke, Sagi Grimberg
On Thu, Jul 30, 2015 at 06:15:23AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch addresses a v4.2-rc1 regression where backend driver
> struct module unload immediately after ->free_device() has done
> an internal call_rcu(), results in IRQ rcu_process_callbacks()
> use-after-free paging OOPsen.
>
> It adds a explicit synchronize_rcu() in target_backend_unregister()
> to wait a full RCU grace period before releasing target_backend_ops
> memory, and allowing TBO->module exit to proceed.
Good catch, but...
You need rcu_barrier() rather than synchronize_rcu() in this case.
All that synchronize_rcu() does is wait for pre-existing RCU readers,
when what is needed is to wait for all pre-existing RCU callbacks
to be invoked.
So please replace the two synchronize_rcu() calls with rcu_barrier().
Thanx, Paul
> Also, go ahead and do the same for target_unregister_template()
> to ensure se_deve_entry->rcu_head -> kfree_rcu() grace period has
> passed, before allowing target_core_fabric_ops->owner module exit
> to proceed.
>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
> drivers/target/target_core_configfs.c | 10 +++++++++-
> drivers/target/target_core_hba.c | 10 +++++++++-
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index c2e9fea..b4c3ae0 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -457,8 +457,16 @@ void target_unregister_template(const struct target_core_fabric_ops *fo)
> if (!strcmp(t->tf_ops->name, fo->name)) {
> BUG_ON(atomic_read(&t->tf_access_cnt));
> list_del(&t->tf_list);
> + mutex_unlock(&g_tf_lock);
> + /*
> + * Allow any outstanding fabric se_deve_entry->rcu_head
> + * grace periods to expire post kfree_rcu(), before allowing
> + * fabric driver unload of target_core_fabric_ops->module
> + * to proceed.
> + */
> + synchronize_rcu();
> kfree(t);
> - break;
> + return;
> }
> }
> mutex_unlock(&g_tf_lock);
> diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c
> index 62ea4e8..0fb830b 100644
> --- a/drivers/target/target_core_hba.c
> +++ b/drivers/target/target_core_hba.c
> @@ -84,8 +84,16 @@ void target_backend_unregister(const struct target_backend_ops *ops)
> list_for_each_entry(tb, &backend_list, list) {
> if (tb->ops == ops) {
> list_del(&tb->list);
> + mutex_unlock(&backend_mutex);
> + /*
> + * Allow any outstanding backend driver ->rcu_head grace
> + * period to expire post ->free_device() -> call_rcu(),
> + * before allowing backend driver module unload of
> + * target_backend_ops->owner to proceed.
> + */
> + synchronize_rcu();
> kfree(tb);
> - break;
> + return;
> }
> }
> mutex_unlock(&backend_mutex);
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] target: Wait RCU grace-period before backend/fabric unload
2015-07-30 13:07 ` Paul E. McKenney
@ 2015-07-31 0:23 ` Nicholas A. Bellinger
2015-07-31 4:10 ` Paul E. McKenney
0 siblings, 1 reply; 4+ messages in thread
From: Nicholas A. Bellinger @ 2015-07-31 0:23 UTC (permalink / raw)
To: paulmck
Cc: Nicholas A. Bellinger, target-devel, linux-scsi,
Christoph Hellwig, Hannes Reinecke, Sagi Grimberg
On Thu, 2015-07-30 at 06:07 -0700, Paul E. McKenney wrote:
> On Thu, Jul 30, 2015 at 06:15:23AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch addresses a v4.2-rc1 regression where backend driver
> > struct module unload immediately after ->free_device() has done
> > an internal call_rcu(), results in IRQ rcu_process_callbacks()
> > use-after-free paging OOPsen.
> >
> > It adds a explicit synchronize_rcu() in target_backend_unregister()
> > to wait a full RCU grace period before releasing target_backend_ops
> > memory, and allowing TBO->module exit to proceed.
>
> Good catch, but...
>
> You need rcu_barrier() rather than synchronize_rcu() in this case.
> All that synchronize_rcu() does is wait for pre-existing RCU readers,
> when what is needed is to wait for all pre-existing RCU callbacks
> to be invoked.
>
Ah, was getting confused by rcu_barrier_tasks() being specific to
CONFIG_TASKS_RCU in update.c code, and missing rcu_barrier() in
tree_plugin.h.
Should have taken a look at Documentation/RCU/rcubarrier.txt..
> So please replace the two synchronize_rcu() calls with rcu_barrier().
>
<nod>, below is the updated version.
Thanks for the review!
--nab
>From 9721910116f9883c7afded613ec88dc2de12339a Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Wed, 29 Jul 2015 22:27:13 -0700
Subject: [PATCH] target: Perform RCU callback barrier before backend/fabric
unload
This patch addresses a v4.2-rc1 regression where backend driver
module unload happening immediately after TBO->free_device()
does internal call_rcu(), will currently result in IRQ context
rcu_process_callbacks() use-after-free paging OOPsen.
It adds the missing rcu_barrier() in target_backend_unregister()
to perform an explicit RCU barrier waiting for all RCU callbacks
to complete before releasing target_backend_ops memory, and
allowing TBO->module exit to proceed.
Also, do the same for fabric drivers in target_unregister_template()
to ensure se_deve_entry->rcu_head -> kfree_rcu() callbacks have
completed, before allowing target_core_fabric_ops->owner module
exit to proceed.
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Sagi Grimberg <sagig@mellanox.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/target/target_core_configfs.c | 9 ++++++++-
drivers/target/target_core_hba.c | 10 +++++++++-
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index c2e9fea..860e840 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -457,8 +457,15 @@ void target_unregister_template(const struct target_core_fabric_ops *fo)
if (!strcmp(t->tf_ops->name, fo->name)) {
BUG_ON(atomic_read(&t->tf_access_cnt));
list_del(&t->tf_list);
+ mutex_unlock(&g_tf_lock);
+ /*
+ * Wait for any outstanding fabric se_deve_entry->rcu_head
+ * callbacks to complete post kfree_rcu(), before allowing
+ * fabric driver unload of TFO->module to proceed.
+ */
+ rcu_barrier();
kfree(t);
- break;
+ return;
}
}
mutex_unlock(&g_tf_lock);
diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c
index 62ea4e8..be9cefc 100644
--- a/drivers/target/target_core_hba.c
+++ b/drivers/target/target_core_hba.c
@@ -84,8 +84,16 @@ void target_backend_unregister(const struct target_backend_ops *ops)
list_for_each_entry(tb, &backend_list, list) {
if (tb->ops == ops) {
list_del(&tb->list);
+ mutex_unlock(&backend_mutex);
+ /*
+ * Wait for any outstanding backend driver ->rcu_head
+ * callbacks to complete post TBO->free_device() ->
+ * call_rcu(), before allowing backend driver module
+ * unload of target_backend_ops->owner to proceed.
+ */
+ rcu_barrier();
kfree(tb);
- break;
+ return;
}
}
mutex_unlock(&backend_mutex);
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] target: Wait RCU grace-period before backend/fabric unload
2015-07-31 0:23 ` Nicholas A. Bellinger
@ 2015-07-31 4:10 ` Paul E. McKenney
0 siblings, 0 replies; 4+ messages in thread
From: Paul E. McKenney @ 2015-07-31 4:10 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Nicholas A. Bellinger, target-devel, linux-scsi,
Christoph Hellwig, Hannes Reinecke, Sagi Grimberg
On Thu, Jul 30, 2015 at 05:23:43PM -0700, Nicholas A. Bellinger wrote:
> On Thu, 2015-07-30 at 06:07 -0700, Paul E. McKenney wrote:
> > On Thu, Jul 30, 2015 at 06:15:23AM +0000, Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > >
> > > This patch addresses a v4.2-rc1 regression where backend driver
> > > struct module unload immediately after ->free_device() has done
> > > an internal call_rcu(), results in IRQ rcu_process_callbacks()
> > > use-after-free paging OOPsen.
> > >
> > > It adds a explicit synchronize_rcu() in target_backend_unregister()
> > > to wait a full RCU grace period before releasing target_backend_ops
> > > memory, and allowing TBO->module exit to proceed.
> >
> > Good catch, but...
> >
> > You need rcu_barrier() rather than synchronize_rcu() in this case.
> > All that synchronize_rcu() does is wait for pre-existing RCU readers,
> > when what is needed is to wait for all pre-existing RCU callbacks
> > to be invoked.
> >
>
> Ah, was getting confused by rcu_barrier_tasks() being specific to
> CONFIG_TASKS_RCU in update.c code, and missing rcu_barrier() in
> tree_plugin.h.
>
> Should have taken a look at Documentation/RCU/rcubarrier.txt..
>
> > So please replace the two synchronize_rcu() calls with rcu_barrier().
> >
>
> <nod>, below is the updated version.
>
> Thanks for the review!
Much better!
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> --nab
>
> >From 9721910116f9883c7afded613ec88dc2de12339a Mon Sep 17 00:00:00 2001
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> Date: Wed, 29 Jul 2015 22:27:13 -0700
> Subject: [PATCH] target: Perform RCU callback barrier before backend/fabric
> unload
>
> This patch addresses a v4.2-rc1 regression where backend driver
> module unload happening immediately after TBO->free_device()
> does internal call_rcu(), will currently result in IRQ context
> rcu_process_callbacks() use-after-free paging OOPsen.
>
> It adds the missing rcu_barrier() in target_backend_unregister()
> to perform an explicit RCU barrier waiting for all RCU callbacks
> to complete before releasing target_backend_ops memory, and
> allowing TBO->module exit to proceed.
>
> Also, do the same for fabric drivers in target_unregister_template()
> to ensure se_deve_entry->rcu_head -> kfree_rcu() callbacks have
> completed, before allowing target_core_fabric_ops->owner module
> exit to proceed.
>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
> drivers/target/target_core_configfs.c | 9 ++++++++-
> drivers/target/target_core_hba.c | 10 +++++++++-
> 2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index c2e9fea..860e840 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -457,8 +457,15 @@ void target_unregister_template(const struct target_core_fabric_ops *fo)
> if (!strcmp(t->tf_ops->name, fo->name)) {
> BUG_ON(atomic_read(&t->tf_access_cnt));
> list_del(&t->tf_list);
> + mutex_unlock(&g_tf_lock);
> + /*
> + * Wait for any outstanding fabric se_deve_entry->rcu_head
> + * callbacks to complete post kfree_rcu(), before allowing
> + * fabric driver unload of TFO->module to proceed.
> + */
> + rcu_barrier();
> kfree(t);
> - break;
> + return;
> }
> }
> mutex_unlock(&g_tf_lock);
> diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c
> index 62ea4e8..be9cefc 100644
> --- a/drivers/target/target_core_hba.c
> +++ b/drivers/target/target_core_hba.c
> @@ -84,8 +84,16 @@ void target_backend_unregister(const struct target_backend_ops *ops)
> list_for_each_entry(tb, &backend_list, list) {
> if (tb->ops == ops) {
> list_del(&tb->list);
> + mutex_unlock(&backend_mutex);
> + /*
> + * Wait for any outstanding backend driver ->rcu_head
> + * callbacks to complete post TBO->free_device() ->
> + * call_rcu(), before allowing backend driver module
> + * unload of target_backend_ops->owner to proceed.
> + */
> + rcu_barrier();
> kfree(tb);
> - break;
> + return;
> }
> }
> mutex_unlock(&backend_mutex);
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-07-31 4:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-30 6:15 [PATCH] target: Wait RCU grace-period before backend/fabric unload Nicholas A. Bellinger
2015-07-30 13:07 ` Paul E. McKenney
2015-07-31 0:23 ` Nicholas A. Bellinger
2015-07-31 4:10 ` Paul E. McKenney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox