[patch 0/5] scsi/bnx2*: Plug hotplug race, correct locking and simplify hotplug code

[patch 0/5] scsi/bnx2*: Plug hotplug race, correct locking and simplify hotplug code

[Thomas Gleixner]

The conversion of the cpu hotplug locking to a percpu rwsem does not longer
allow recursive locking of the hotplug lock.

The BNX2I and BNX2FC drivers install/remove hotplug states with the hotplug
lock held. The install/removal code acquired the hotplug lock as well.

While looking into this, I noticed an interesting hotplug race in the
BNX2FC driver, which could result in dereferencing a NULL pointer or freed
and potentially reused memory.

The following series addresses these problems and as a final step on top it
simplifies the hotplug code in both drivers.

Thanks,

	tglx

----
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c |   68 ++++++++------------------------------
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |   45 ++++++++++++-------------
 drivers/scsi/bnx2i/bnx2i_init.c   |   64 ++++++++---------------------------
 include/linux/cpuhotplug.h        |    2 -
 4 files changed, 53 insertions(+), 126 deletions(-)

[patch 1/5] scsi/bnx2fc: Plug CPU hotplug race

[Thomas Gleixner]

[-- Attachment #1: scsi-bnx2fc--Plug-CPU-hotplug-race.patch --]
[-- Type: text/plain, Size: 2332 bytes --]

bnx2fc_process_new_cqes() has protection against CPU hotplug, which relies
on the per cpu thread pointer. This protection is racy because it happens
only partially with the per cpu fp_work_lock held.

If the CPU is unplugged after the lock is dropped, the wakeup code can
dereference a NULL pointer or access freed and potentially reused memory.

Restructure the code so the thread check and wakeup happens with the
fp_work_lock held.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_hwi.c |   45 +++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -1008,6 +1008,28 @@ static struct bnx2fc_work *bnx2fc_alloc_
 	return work;
 }
 
+/* Pending work request completion */
+static void bnx2fc_pending_work(struct bnx2fc_rport *tgt, unsigned int wqe)
+{
+	unsigned int cpu = wqe % num_possible_cpus();
+	struct bnx2fc_percpu_s *fps;
+	struct bnx2fc_work *work;
+
+	fps = &per_cpu(bnx2fc_percpu, cpu);
+	spin_lock_bh(&fps->fp_work_lock);
+	if (fps->iothread) {
+		work = bnx2fc_alloc_work(tgt, wqe);
+		if (work) {
+			list_add_tail(&work->list, &fps->work_list);
+			wake_up_process(fps->iothread);
+			spin_unlock_bh(&fps->fp_work_lock);
+			return;
+		}
+	}
+	spin_unlock_bh(&fps->fp_work_lock);
+	bnx2fc_process_cq_compl(tgt, wqe);
+}
+
 int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt)
 {
 	struct fcoe_cqe *cq;
@@ -1042,28 +1064,7 @@ int bnx2fc_process_new_cqes(struct bnx2f
 			/* Unsolicited event notification */
 			bnx2fc_process_unsol_compl(tgt, wqe);
 		} else {
-			/* Pending work request completion */
-			struct bnx2fc_work *work = NULL;
-			struct bnx2fc_percpu_s *fps = NULL;
-			unsigned int cpu = wqe % num_possible_cpus();
-
-			fps = &per_cpu(bnx2fc_percpu, cpu);
-			spin_lock_bh(&fps->fp_work_lock);
-			if (unlikely(!fps->iothread))
-				goto unlock;
-
-			work = bnx2fc_alloc_work(tgt, wqe);
-			if (work)
-				list_add_tail(&work->list,
-					      &fps->work_list);
-unlock:
-			spin_unlock_bh(&fps->fp_work_lock);
-
-			/* Pending work request completion */
-			if (fps->iothread && work)
-				wake_up_process(fps->iothread);
-			else
-				bnx2fc_process_cq_compl(tgt, wqe);
+			bnx2fc_pending_work(tgt, wqe);
 			num_free_sqes++;
 		}
 		cqe++;

[patch 2/5] scsi/bnx2fc: Prevent recursive cpuhotplug locking

[Thomas Gleixner]

[-- Attachment #1: scsi-bnx2fc--Prevent-recursive-cpuhotplug-locking.patch --]
[-- Type: text/plain, Size: 1737 bytes --]

The BNX2FC module init/exit code installs/removes the hotplug callbacks with
the cpu hotplug lock held. This worked with the old CPU locking
implementation which allowed recursive locking, but with the new percpu
rwsem based mechanism this is not longer allowed.

Use the _cpuslocked() variants to fix this.

Reported-by: kernel test robot <fengguang.wu@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -2766,15 +2766,16 @@ static int __init bnx2fc_mod_init(void)
 	for_each_online_cpu(cpu)
 		bnx2fc_percpu_thread_create(cpu);
 
-	rc = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
-				       "scsi/bnx2fc:online",
-				       bnx2fc_cpu_online, NULL);
+	rc = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
+						  "scsi/bnx2fc:online",
+						  bnx2fc_cpu_online, NULL);
 	if (rc < 0)
 		goto stop_threads;
 	bnx2fc_online_state = rc;
 
-	cpuhp_setup_state_nocalls(CPUHP_SCSI_BNX2FC_DEAD, "scsi/bnx2fc:dead",
-				  NULL, bnx2fc_cpu_dead);
+	cpuhp_setup_state_nocalls_cpuslocked(CPUHP_SCSI_BNX2FC_DEAD,
+					     "scsi/bnx2fc:dead",
+					     NULL, bnx2fc_cpu_dead);
 	put_online_cpus();
 
 	cnic_register_driver(CNIC_ULP_FCOE, &bnx2fc_cnic_cb);
@@ -2850,8 +2851,8 @@ static void __exit bnx2fc_mod_exit(void)
 		bnx2fc_percpu_thread_destroy(cpu);
 	}
 
-	cpuhp_remove_state_nocalls(bnx2fc_online_state);
-	cpuhp_remove_state_nocalls(CPUHP_SCSI_BNX2FC_DEAD);
+	cpuhp_remove_state_nocalls_cpuslocked(bnx2fc_online_state);
+	cpuhp_remove_state_nocalls_cpuslocked(CPUHP_SCSI_BNX2FC_DEAD);
 
 	put_online_cpus();
 

[patch 3/5] scsi/bnx2i: Prevent recursive cpuhotplug locking

[Thomas Gleixner]

[-- Attachment #1: scsu-bnx2i--Prevent-recursive-cpuhotplug-locking.patch --]
[-- Type: text/plain, Size: 1740 bytes --]

The BNX2I module init/exit code installs/removes the hotplug callbacks with
the cpu hotplug lock held. This worked with the old CPU locking
implementation which allowed recursive locking, but with the new percpu
rwsem based mechanism this is not longer allowed.

Use the _cpuslocked() variants to fix this.

Reported-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/scsi/bnx2i/bnx2i_init.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

--- a/drivers/scsi/bnx2i/bnx2i_init.c
+++ b/drivers/scsi/bnx2i/bnx2i_init.c
@@ -516,15 +516,16 @@ static int __init bnx2i_mod_init(void)
 	for_each_online_cpu(cpu)
 		bnx2i_percpu_thread_create(cpu);
 
-	err = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
-				       "scsi/bnx2i:online",
-				       bnx2i_cpu_online, NULL);
+	err = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
+						   "scsi/bnx2i:online",
+						   bnx2i_cpu_online, NULL);
 	if (err < 0)
 		goto remove_threads;
 	bnx2i_online_state = err;
 
-	cpuhp_setup_state_nocalls(CPUHP_SCSI_BNX2I_DEAD, "scsi/bnx2i:dead",
-				  NULL, bnx2i_cpu_dead);
+	cpuhp_setup_state_nocalls_cpuslocked(CPUHP_SCSI_BNX2I_DEAD,
+					     "scsi/bnx2i:dead",
+					     NULL, bnx2i_cpu_dead);
 	put_online_cpus();
 	return 0;
 
@@ -574,8 +575,8 @@ static void __exit bnx2i_mod_exit(void)
 	for_each_online_cpu(cpu)
 		bnx2i_percpu_thread_destroy(cpu);
 
-	cpuhp_remove_state_nocalls(bnx2i_online_state);
-	cpuhp_remove_state_nocalls(CPUHP_SCSI_BNX2I_DEAD);
+	cpuhp_remove_state_nocalls_cpuslocked(bnx2i_online_state);
+	cpuhp_remove_state_nocalls_cpuslocked(CPUHP_SCSI_BNX2I_DEAD);
 	put_online_cpus();
 
 	iscsi_unregister_transport(&bnx2i_iscsi_transport);

[patch 4/5] scsi/bnx2fc: Simplify CPU hotplug code

[Thomas Gleixner]

[-- Attachment #1: scsi-bnx2fc--Simplify-CPU-hotplug-code.patch --]
[-- Type: text/plain, Size: 4378 bytes --]

The CPU hotplug related code of this driver can be simplified by:

1) Consolidating the callbacks into a single state. The CPU thread can be
   torn down on the CPU which goes offline. There is no point in delaying
   that to the CPU dead state

2) Let the core code invoke the online/offline callbacks and remove the
   extra for_each_online_cpu() loops.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c |   69 ++++++++------------------------------
 include/linux/cpuhotplug.h        |    1 
 2 files changed, 15 insertions(+), 55 deletions(-)

--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -2624,12 +2624,11 @@ static struct fcoe_transport bnx2fc_tran
 };
 
 /**
- * bnx2fc_percpu_thread_create - Create a receive thread for an
- *				 online CPU
+ * bnx2fc_cpu_online - Create a receive thread for an  online CPU
  *
  * @cpu: cpu index for the online cpu
  */
-static void bnx2fc_percpu_thread_create(unsigned int cpu)
+static int bnx2fc_cpu_online(unsigned int cpu)
 {
 	struct bnx2fc_percpu_s *p;
 	struct task_struct *thread;
@@ -2639,15 +2638,17 @@ static void bnx2fc_percpu_thread_create(
 	thread = kthread_create_on_node(bnx2fc_percpu_io_thread,
 					(void *)p, cpu_to_node(cpu),
 					"bnx2fc_thread/%d", cpu);
+	if (IS_ERR(thread))
+		return PTR_ERR(thread);
+
 	/* bind thread to the cpu */
-	if (likely(!IS_ERR(thread))) {
-		kthread_bind(thread, cpu);
-		p->iothread = thread;
-		wake_up_process(thread);
-	}
+	kthread_bind(thread, cpu);
+	p->iothread = thread;
+	wake_up_process(thread);
+	return 0;
 }
 
-static void bnx2fc_percpu_thread_destroy(unsigned int cpu)
+static int bnx2fc_cpu_offline(unsigned int cpu)
 {
 	struct bnx2fc_percpu_s *p;
 	struct task_struct *thread;
@@ -2661,7 +2662,6 @@ static void bnx2fc_percpu_thread_destroy
 	thread = p->iothread;
 	p->iothread = NULL;
 
-
 	/* Free all work in the list */
 	list_for_each_entry_safe(work, tmp, &p->work_list, list) {
 		list_del_init(&work->list);
@@ -2673,20 +2673,6 @@ static void bnx2fc_percpu_thread_destroy
 
 	if (thread)
 		kthread_stop(thread);
-}
-
-
-static int bnx2fc_cpu_online(unsigned int cpu)
-{
-	printk(PFX "CPU %x online: Create Rx thread\n", cpu);
-	bnx2fc_percpu_thread_create(cpu);
-	return 0;
-}
-
-static int bnx2fc_cpu_dead(unsigned int cpu)
-{
-	printk(PFX "CPU %x offline: Remove Rx thread\n", cpu);
-	bnx2fc_percpu_thread_destroy(cpu);
 	return 0;
 }
 
@@ -2761,31 +2747,16 @@ static int __init bnx2fc_mod_init(void)
 		spin_lock_init(&p->fp_work_lock);
 	}
 
-	get_online_cpus();
-
-	for_each_online_cpu(cpu)
-		bnx2fc_percpu_thread_create(cpu);
-
-	rc = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
-						  "scsi/bnx2fc:online",
-						  bnx2fc_cpu_online, NULL);
+	rc = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "scsi/bnx2fc:online",
+			       bnx2fc_cpu_online, bnx2fc_cpu_offline);
 	if (rc < 0)
-		goto stop_threads;
+		goto stop_thread;
 	bnx2fc_online_state = rc;
 
-	cpuhp_setup_state_nocalls_cpuslocked(CPUHP_SCSI_BNX2FC_DEAD,
-					     "scsi/bnx2fc:dead",
-					     NULL, bnx2fc_cpu_dead);
-	put_online_cpus();
-
 	cnic_register_driver(CNIC_ULP_FCOE, &bnx2fc_cnic_cb);
-
 	return 0;
 
-stop_threads:
-	for_each_online_cpu(cpu)
-		bnx2fc_percpu_thread_destroy(cpu);
-	put_online_cpus();
+stop_thread:
 	kthread_stop(l2_thread);
 free_wq:
 	destroy_workqueue(bnx2fc_wq);
@@ -2804,7 +2775,6 @@ static void __exit bnx2fc_mod_exit(void)
 	struct fcoe_percpu_s *bg;
 	struct task_struct *l2_thread;
 	struct sk_buff *skb;
-	unsigned int cpu = 0;
 
 	/*
 	 * NOTE: Since cnic calls register_driver routine rtnl_lock,
@@ -2845,16 +2815,7 @@ static void __exit bnx2fc_mod_exit(void)
 	if (l2_thread)
 		kthread_stop(l2_thread);
 
-	get_online_cpus();
-	/* Destroy per cpu threads */
-	for_each_online_cpu(cpu) {
-		bnx2fc_percpu_thread_destroy(cpu);
-	}
-
-	cpuhp_remove_state_nocalls_cpuslocked(bnx2fc_online_state);
-	cpuhp_remove_state_nocalls_cpuslocked(CPUHP_SCSI_BNX2FC_DEAD);
-
-	put_online_cpus();
+	cpuhp_remove_state(bnx2fc_online_state);
 
 	destroy_workqueue(bnx2fc_wq);
 	/*
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -39,7 +39,6 @@ enum cpuhp_state {
 	CPUHP_PCI_XGENE_DEAD,
 	CPUHP_IOMMU_INTEL_DEAD,
 	CPUHP_LUSTRE_CFS_DEAD,
-	CPUHP_SCSI_BNX2FC_DEAD,
 	CPUHP_SCSI_BNX2I_DEAD,
 	CPUHP_WORKQUEUE_PREP,
 	CPUHP_POWER_NUMA_PREPARE,

[patch 5/5] scsi/bnx2i: Simplify cpu hotplug code

[Thomas Gleixner]

[-- Attachment #1: scsci-bxn2i--Simplify-cpu-hotplug-code.patch --]
[-- Type: text/plain, Size: 4080 bytes --]

The CPU hotplug related code of this driver can be simplified by:

1) Consolidating the callbacks into a single state. The CPU thread can be
   torn down on the CPU which goes offline. There is no point in delaying
   that to the CPU dead state

2) Let the core code invoke the online/offline callbacks and remove the
   extra for_each_online_cpu() loops.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/scsi/bnx2i/bnx2i_init.c |   65 +++++++++-------------------------------
 include/linux/cpuhotplug.h      |    1 
 2 files changed, 15 insertions(+), 51 deletions(-)

--- a/drivers/scsi/bnx2i/bnx2i_init.c
+++ b/drivers/scsi/bnx2i/bnx2i_init.c
@@ -404,12 +404,11 @@ int bnx2i_get_stats(void *handle)
 
 
 /**
- * bnx2i_percpu_thread_create - Create a receive thread for an
- *				online CPU
+ * bnx2i_cpu_online - Create a receive thread for an online CPU
  *
  * @cpu:	cpu index for the online cpu
  */
-static void bnx2i_percpu_thread_create(unsigned int cpu)
+static int bnx2i_cpu_online(unsigned int cpu)
 {
 	struct bnx2i_percpu_s *p;
 	struct task_struct *thread;
@@ -419,16 +418,17 @@ static void bnx2i_percpu_thread_create(u
 	thread = kthread_create_on_node(bnx2i_percpu_io_thread, (void *)p,
 					cpu_to_node(cpu),
 					"bnx2i_thread/%d", cpu);
+	if (IS_ERR(thread))
+		return PTR_ERR(thread);
+
 	/* bind thread to the cpu */
-	if (likely(!IS_ERR(thread))) {
-		kthread_bind(thread, cpu);
-		p->iothread = thread;
-		wake_up_process(thread);
-	}
+	kthread_bind(thread, cpu);
+	p->iothread = thread;
+	wake_up_process(thread);
+	return 0;
 }
 
-
-static void bnx2i_percpu_thread_destroy(unsigned int cpu)
+static int bnx2i_cpu_offline(unsigned int cpu)
 {
 	struct bnx2i_percpu_s *p;
 	struct task_struct *thread;
@@ -451,19 +451,6 @@ static void bnx2i_percpu_thread_destroy(
 	spin_unlock_bh(&p->p_work_lock);
 	if (thread)
 		kthread_stop(thread);
-}
-
-static int bnx2i_cpu_online(unsigned int cpu)
-{
-	pr_info("bnx2i: CPU %x online: Create Rx thread\n", cpu);
-	bnx2i_percpu_thread_create(cpu);
-	return 0;
-}
-
-static int bnx2i_cpu_dead(unsigned int cpu)
-{
-	pr_info("CPU %x offline: Remove Rx thread\n", cpu);
-	bnx2i_percpu_thread_destroy(cpu);
 	return 0;
 }
 
@@ -511,28 +498,14 @@ static int __init bnx2i_mod_init(void)
 		p->iothread = NULL;
 	}
 
-	get_online_cpus();
-
-	for_each_online_cpu(cpu)
-		bnx2i_percpu_thread_create(cpu);
-
-	err = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
-						   "scsi/bnx2i:online",
-						   bnx2i_cpu_online, NULL);
+	err = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "scsi/bnx2i:online",
+				bnx2i_cpu_online, bnx2i_cpu_offline);
 	if (err < 0)
-		goto remove_threads;
+		goto unreg_driver;
 	bnx2i_online_state = err;
-
-	cpuhp_setup_state_nocalls_cpuslocked(CPUHP_SCSI_BNX2I_DEAD,
-					     "scsi/bnx2i:dead",
-					     NULL, bnx2i_cpu_dead);
-	put_online_cpus();
 	return 0;
 
-remove_threads:
-	for_each_online_cpu(cpu)
-		bnx2i_percpu_thread_destroy(cpu);
-	put_online_cpus();
+unreg_driver:
 	cnic_unregister_driver(CNIC_ULP_ISCSI);
 unreg_xport:
 	iscsi_unregister_transport(&bnx2i_iscsi_transport);
@@ -552,7 +525,6 @@ static int __init bnx2i_mod_init(void)
 static void __exit bnx2i_mod_exit(void)
 {
 	struct bnx2i_hba *hba;
-	unsigned cpu = 0;
 
 	mutex_lock(&bnx2i_dev_lock);
 	while (!list_empty(&adapter_list)) {
@@ -570,14 +542,7 @@ static void __exit bnx2i_mod_exit(void)
 	}
 	mutex_unlock(&bnx2i_dev_lock);
 
-	get_online_cpus();
-
-	for_each_online_cpu(cpu)
-		bnx2i_percpu_thread_destroy(cpu);
-
-	cpuhp_remove_state_nocalls_cpuslocked(bnx2i_online_state);
-	cpuhp_remove_state_nocalls_cpuslocked(CPUHP_SCSI_BNX2I_DEAD);
-	put_online_cpus();
+	cpuhp_remove_state(bnx2i_online_state);
 
 	iscsi_unregister_transport(&bnx2i_iscsi_transport);
 	cnic_unregister_driver(CNIC_ULP_ISCSI);
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -39,7 +39,6 @@ enum cpuhp_state {
 	CPUHP_PCI_XGENE_DEAD,
 	CPUHP_IOMMU_INTEL_DEAD,
 	CPUHP_LUSTRE_CFS_DEAD,
-	CPUHP_SCSI_BNX2I_DEAD,
 	CPUHP_WORKQUEUE_PREP,
 	CPUHP_POWER_NUMA_PREPARE,
 	CPUHP_HRTIMERS_PREPARE,

Re: [patch 0/5] scsi/bnx2*: Plug hotplug race, correct locking and simplify hotplug code

[Chad Dupuis]

On Mon, 24 Jul 2017, 6:52am, Thomas Gleixner wrote:

> The conversion of the cpu hotplug locking to a percpu rwsem does not longer
> allow recursive locking of the hotplug lock.
> 
> The BNX2I and BNX2FC drivers install/remove hotplug states with the hotplug
> lock held. The install/removal code acquired the hotplug lock as well.
> 
> While looking into this, I noticed an interesting hotplug race in the
> BNX2FC driver, which could result in dereferencing a NULL pointer or freed
> and potentially reused memory.
> 
> The following series addresses these problems and as a final step on top it
> simplifies the hotplug code in both drivers.
> 
> Thanks,
> 
> 	tglx
> 
> ----
>  drivers/scsi/bnx2fc/bnx2fc_fcoe.c |   68 ++++++++------------------------------
>  drivers/scsi/bnx2fc/bnx2fc_hwi.c  |   45 ++++++++++++-------------
>  drivers/scsi/bnx2i/bnx2i_init.c   |   64 ++++++++---------------------------
>  include/linux/cpuhotplug.h        |    2 -
>  4 files changed, 53 insertions(+), 126 deletions(-)
> 

We tested the series and everything was fine.  Ack to the series.

Acked-by: Chad Dupuis <chad.dupuis@cavium.com>

Re: [patch 0/5] scsi/bnx2*: Plug hotplug race, correct locking and simplify hotplug code

[Martin K. Petersen]

Thomas,

> The conversion of the cpu hotplug locking to a percpu rwsem does not
> longer allow recursive locking of the hotplug lock.
>
> The BNX2I and BNX2FC drivers install/remove hotplug states with the
> hotplug lock held. The install/removal code acquired the hotplug lock
> as well.
>
> While looking into this, I noticed an interesting hotplug race in the
> BNX2FC driver, which could result in dereferencing a NULL pointer or
> freed and potentially reused memory.
>
> The following series addresses these problems and as a final step on
> top it simplifies the hotplug code in both drivers.

Applied to 4.13/scsi-fixes. Thank you!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [patch 3/5] scsi/bnx2i: Prevent recursive cpuhotplug locking

[Steven Rostedt]

On Mon, 24 Jul 2017 12:52:58 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> The BNX2I module init/exit code installs/removes the hotplug callbacks with
> the cpu hotplug lock held. This worked with the old CPU locking
> implementation which allowed recursive locking, but with the new percpu
> rwsem based mechanism this is not longer allowed.
> 
> Use the _cpuslocked() variants to fix this.
> 
> Reported-by: Steven Rostedt <rostedt@goodmis.org>

Tested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

(makes the lockdep splat go away)

-- Steve

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/scsi/bnx2i/bnx2i_init.c |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> --- a/drivers/scsi/bnx2i/bnx2i_init.c
> +++ b/drivers/scsi/bnx2i/bnx2i_init.c
> @@ -516,15 +516,16 @@ static int __init bnx2i_mod_init(void)
>  	for_each_online_cpu(cpu)
>  		bnx2i_percpu_thread_create(cpu);
>  
> -	err = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> -				       "scsi/bnx2i:online",
> -				       bnx2i_cpu_online, NULL);
> +	err = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
> +						   "scsi/bnx2i:online",
> +						   bnx2i_cpu_online, NULL);
>  	if (err < 0)
>  		goto remove_threads;
>  	bnx2i_online_state = err;
>  
> -	cpuhp_setup_state_nocalls(CPUHP_SCSI_BNX2I_DEAD, "scsi/bnx2i:dead",
> -				  NULL, bnx2i_cpu_dead);
> +	cpuhp_setup_state_nocalls_cpuslocked(CPUHP_SCSI_BNX2I_DEAD,
> +					     "scsi/bnx2i:dead",
> +					     NULL, bnx2i_cpu_dead);
>  	put_online_cpus();
>  	return 0;
>  
> @@ -574,8 +575,8 @@ static void __exit bnx2i_mod_exit(void)
>  	for_each_online_cpu(cpu)
>  		bnx2i_percpu_thread_destroy(cpu);
>  
> -	cpuhp_remove_state_nocalls(bnx2i_online_state);
> -	cpuhp_remove_state_nocalls(CPUHP_SCSI_BNX2I_DEAD);
> +	cpuhp_remove_state_nocalls_cpuslocked(bnx2i_online_state);
> +	cpuhp_remove_state_nocalls_cpuslocked(CPUHP_SCSI_BNX2I_DEAD);
>  	put_online_cpus();
>  
>  	iscsi_unregister_transport(&bnx2i_iscsi_transport);
>