lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Date:	Tue, 14 Oct 2014 17:22:43 +0200
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	MyungJoo Ham <myungjoo.ham@...sung.com>,
	Chanwoo Choi <cw00.choi@...sung.com>,
	Felipe Balbi <balbi@...com>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Sangjung Woo <sangjung.woo@...sung.com>,
	linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jingoo Han <jg1.han@...sung.com>
Cc:	Kyungmin Park <kyungmin.park@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	stable@...r.kernel.org
Subject: [PATCH v2] extcon: Fix sleeping in atomic context after connecting USB
 cable

Sleeping in atomic context happens because:
1. Extcon is notifying with raw notifier chain under spin lock.
2. Notified charger-manager calls sleeping functions.

Solve the problem in a generic way (not charger-manager specific)
because the extcon is exporting as API its raw notifier chain with
extcon_register_notifier().

Notify on cable change not directly in extcon_update_state(), but from
scheduled work on ordered workqueue.

Details on issue:
=================
Kernel built with extcon and charger-manager. After connecting the USB
cable:
[    6.534930] max77693-muic max77693-muic: external connector is attached(chg_type:0x1, prev_chg_type:0x1)
[    6.544043] max77693-muic max77693-muic: CONTROL1 : 0x09, CONTROL2 : 0x04, state : attached
[    6.551539] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:615
[    6.559691] in_atomic(): 1, irqs_disabled(): 128, pid: 1248, name: kworker/2:1
[    6.566892] 4 locks held by kworker/2:1/1248:
[    6.571229]  #0:  ("events"){.+.+.+}, at: [<c0039648>] process_one_work+0x114/0x3f4
[    6.578867]  #1:  ((&info->irq_work)){+.+...}, at: [<c0039648>] process_one_work+0x114/0x3f4
[    6.587287]  #2:  (&info->mutex){+.+...}, at: [<c038e56c>] max77693_muic_irq_work+0x28/0x120
[    6.595706]  #3:  (&(&edev->lock)->rlock){......}, at: [<c038c3b8>] extcon_update_state+0x20/0x204
[    6.604648] irq event stamp: 3944
[    6.607945] hardirqs last  enabled at (3943): [<c0068fb4>] vprintk_emit+0x1dc/0x5c0
[    6.615584] hardirqs last disabled at (3944): [<c048b5ac>] _raw_spin_lock_irqsave+0x1c/0x5c
[    6.623917] softirqs last  enabled at (0): [<c0020c7c>] copy_process.part.54+0x3f4/0x1520
[    6.632076] softirqs last disabled at (0): [<  (null)>]   (null)
[    6.638065] Preemption disabled at:[<  (null)>]   (null)
[    6.643359]
[    6.644843] CPU: 2 PID: 1248 Comm: kworker/2:1 Not tainted 3.17.0-next-20141007-00047-g1b95596dfa88 #302
[    6.654307] Workqueue: events max77693_muic_irq_work
[    6.659277] [<c0014d2c>] (unwind_backtrace) from [<c0011c98>] (show_stack+0x10/0x14)
[    6.666986] [<c0011c98>] (show_stack) from [<c0484d18>] (dump_stack+0x70/0xbc)
[    6.674186] [<c0484d18>] (dump_stack) from [<c0487b38>] (mutex_lock_nested+0x24/0x458)
[    6.682087] [<c0487b38>] (mutex_lock_nested) from [<c028ef6c>] (regmap_read+0x30/0x60)
[    6.689990] [<c028ef6c>] (regmap_read) from [<c033d830>] (max77693_charger_get_property+0x20c/0x244)
[    6.699113] [<c033d830>] (max77693_charger_get_property) from [<c03365f8>] (power_supply_get_property+0x20/0x2c)
[    6.709255] [<c03365f8>] (power_supply_get_property) from [<c033a0a4>] (is_ext_pwr_online+0x30/0xb8)
[    6.718364] [<c033a0a4>] (is_ext_pwr_online) from [<c033b6ac>] (charger_extcon_notifier+0x40/0x70)
[    6.727306] [<c033b6ac>] (charger_extcon_notifier) from [<c038bf4c>] (_call_per_cable+0x40/0x4c)
[    6.736080] [<c038bf4c>] (_call_per_cable) from [<c00402b4>] (notifier_call_chain+0x64/0xd4)
[    6.744492] [<c00402b4>] (notifier_call_chain) from [<c0040340>] (raw_notifier_call_chain+0x18/0x20)
[    6.753607] [<c0040340>] (raw_notifier_call_chain) from [<c038c440>] (extcon_update_state+0xa8/0x204)
[    6.762807] [<c038c440>] (extcon_update_state) from [<c038de44>] (max77693_muic_chg_handler+0x1f4/0x25c)
[    6.772267] [<c038de44>] (max77693_muic_chg_handler) from [<c038e650>] (max77693_muic_irq_work+0x10c/0x120)
[    6.781992] [<c038e650>] (max77693_muic_irq_work) from [<c00396b4>] (process_one_work+0x180/0x3f4)
[    6.790930] [<c00396b4>] (process_one_work) from [<c003998c>] (worker_thread+0x30/0x458)
[    6.799000] [<c003998c>] (worker_thread) from [<c003f374>] (kthread+0xd8/0xf4)
[    6.806209] [<c003f374>] (kthread) from [<c000f268>] (ret_from_fork+0x14/0x2c)

The first sleeping function is is_ext_pwr_online()
(drivers/power/charger-manager.c). The atomic context initiating the
flow is set up in extcon_update_state() (drivers/extcon/extcon-class.c).

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
Cc: <stable@...r.kernel.org>
Fixes: 0ea625037826 ("Extcon: renamed files to comply with the standard naming.")

---

Changes since v1:
1. Re-work after discussions MyungJoo Ham. Don't change spin locks into
   mutexes but completely move raw_notifier_call_chain out of atomic
   context.
2. Mark cc-stable.
---
 drivers/extcon/extcon-class.c | 47 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/extcon.h        |  1 +
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
index 4c2f2c543bb7..e3aaa02954bb 100644
--- a/drivers/extcon/extcon-class.c
+++ b/drivers/extcon/extcon-class.c
@@ -34,6 +34,16 @@
 #include <linux/of.h>
 
 /*
+ * Not part of API.
+ * Work item for notifing on update state.
+ */
+struct notifier_work {
+	struct extcon_dev *edev;
+	u32 old_state;
+	struct work_struct work;
+};
+
+/*
  * extcon_cable_name suggests the standard cable names for commonly used
  * cable types.
  *
@@ -187,6 +197,32 @@ static ssize_t cable_state_show(struct device *dev,
 					       cable->cable_index));
 }
 
+static void update_state_notifier_worker(struct work_struct *_work)
+{
+	struct notifier_work *work = container_of(_work, struct notifier_work,
+			work);
+
+	raw_notifier_call_chain(&work->edev->nh, work->old_state, work->edev);
+	kfree(work);
+}
+
+static int queue_update_state_notifier(struct extcon_dev *edev, u32 old_state)
+{
+	struct notifier_work *work;
+
+	work = kzalloc(sizeof(*work), GFP_NOWAIT);
+	if (!work)
+		return -ENOMEM;
+
+	work->edev = edev;
+	work->old_state = old_state;
+	INIT_WORK(&work->work, update_state_notifier_worker);
+
+	queue_work(edev->noti_workqueue, &work->work);
+
+	return 0;
+}
+
 /**
  * extcon_update_state() - Update the cable attach states of the extcon device
  *			   only for the masked bits.
@@ -216,6 +252,7 @@ int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
 
 	if (edev->state != ((edev->state & ~mask) | (state & mask))) {
 		u32 old_state = edev->state;
+		int ret;
 
 		if (check_mutually_exclusive(edev, (edev->state & ~mask) |
 						   (state & mask))) {
@@ -226,7 +263,12 @@ int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
 		edev->state &= ~mask;
 		edev->state |= state & mask;
 
-		raw_notifier_call_chain(&edev->nh, old_state, edev);
+		ret = queue_update_state_notifier(edev, old_state);
+		if (ret) {
+			spin_unlock_irqrestore(&edev->lock, flags);
+			return ret;
+		}
+
 		/* This could be in interrupt handler */
 		prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
 		if (prop_buf) {
@@ -588,6 +630,8 @@ struct extcon_dev *extcon_dev_allocate(const char **supported_cable)
 	edev->max_supported = 0;
 	edev->supported_cable = supported_cable;
 
+	edev->noti_workqueue = create_singlethread_workqueue("extcon");
+
 	return edev;
 }
 
@@ -597,6 +641,7 @@ struct extcon_dev *extcon_dev_allocate(const char **supported_cable)
  */
 void extcon_dev_free(struct extcon_dev *edev)
 {
+	destroy_workqueue(edev->noti_workqueue);
 	kfree(edev);
 }
 EXPORT_SYMBOL_GPL(extcon_dev_free);
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 36f49c405dfb..327bf5d85920 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -123,6 +123,7 @@ struct extcon_dev {
 	/* Internal data. Please do not set. */
 	struct device dev;
 	struct raw_notifier_head nh;
+	struct workqueue_struct *noti_workqueue;
 	struct list_head entry;
 	int max_supported;
 	spinlock_t lock;	/* could be called by irq handler */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists