linux-kernel - Re: [RFC PATCH] extcon: Fix sleeping in atomic context after connecting USB cable

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-id: <1411986225.6035.4.camel@AMDC1943>
Date:	Mon, 29 Sep 2014 12:23:45 +0200
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	myungjoo.ham@...sung.com
Cc:	최찬우 <cw00.choi@...sung.com>,
	Felipe Balbi <balbi@...com>,
	우상정 <sangjung.woo@...sung.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	한진구 <jg1.han@...sung.com>,
	박경민 <kyungmin.park@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: [RFC PATCH] extcon: Fix sleeping in atomic context after
 connecting USB cable

On pon, 2014-09-29 at 04:25 +0000, 함명주 wrote:
> >   
> >  Kernel built with extcon and charger-manager.
> > 
> > After connecting the USB cable sleeping function was called from atomic
> > context:
> > [   63.328648] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
> []
> > [   63.388743] Workqueue: events max14577_muic_irq_work
> > [   63.393707] [<c00167a4>] (unwind_backtrace) from [<c0012c08>] (show_stack+0x10/0x14)
> > [   63.401422] [<c0012c08>] (show_stack) from [<c06232ec>] (dump_stack+0x70/0xbc)
> > [   63.408625] [<c06232ec>] (dump_stack) from [<c0629d0c>] (mutex_lock_nested+0x24/0x410)
> > [   63.416525] [<c0629d0c>] (mutex_lock_nested) from [<c0354474>] (regmap_read+0x30/0x60)
> > [   63.424424] [<c0354474>] (regmap_read) from [<c0406b84>] (max14577_charger_get_property+0x1e4/0x250)
> > [   63.433535] [<c0406b84>] (max14577_charger_get_property) from [<c0403834>] (is_ext_pwr_online+0x30/0x6c)
> > [   63.442994] [<c0403834>] (is_ext_pwr_online) from [<c0404a54>] (charger_extcon_notifier+0x40/0x70)
> > [   63.451934] [<c0404a54>] (charger_extcon_notifier) from [<c044cbe4>] (_call_per_cable+0x40/0x4c)
> > [   63.460704] [<c044cbe4>] (_call_per_cable) from [<c0051560>] (notifier_call_chain+0x64/0x128)
> > [   63.469209] [<c0051560>] (notifier_call_chain) from [<c0051640>] (raw_notifier_call_chain+0x18/0x20)
> > [   63.478321] [<c0051640>] (raw_notifier_call_chain) from [<c044d0fc>] (extcon_update_state+0xa8/0x204)
> > [   63.487522] [<c044d0fc>] (extcon_update_state) from [<c044e5f8>] (max14577_muic_chg_handler+0xdc/0x180)
> > [   63.496897] [<c044e5f8>] (max14577_muic_chg_handler) from [<c044e924>] (max14577_muic_irq_work+0x7c/0xd8)
> > [   63.506445] [<c044e924>] (max14577_muic_irq_work) from [<c004480c>] (process_one_work+0x198/0x66c)
> > [   63.515385] [<c004480c>] (process_one_work) from [<c0044d4c>] (worker_thread+0x38/0x564)
> > [   63.523455] [<c0044d4c>] (worker_thread) from [<c004c4ec>] (kthread+0xcc/0xe8)
> > [   63.530661] [<c004c4ec>] (kthread) from [<c000f1f8>] (ret_from_fork+0x14/0x3c)
> > [   63.543926] charger_manager: Set current limit of CHARGER : 450000uA ~ 450000uA
> > [   63.548870] extcon-port jack: jack event CHGDET=usb
> > [   63.550592] extcon-port jack: jack event CHGDET=charger
> > [   64.188607] charger-manager charger-manager@0: Failed to get battery temperature
> > [   64.200684] charger-manager charger-manager@0: CHARGING
> > 
> > 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).
> > 
> > The extcon_update_state() uses spin locks which are not necessary
> > because the function is not called from interrupt service routines.
> > Instead, the extcon_update_state() is called from:
> > 1. Threaded interrupt handlers.
> > 2. Work queues.
> > 
> > Replace the spin lock with mutex and update the documentation of this
> > function.
> 
> No. You've done it in the opposite way.
> 
> update_state is often called in a interrupt handler that cannot sleep.
> 
> For the context you've mentioned, we'd need workqueue invoked by
> _call_per_cable or notifier callback.

I'm still not convinced that calling raw_notifier_chain under spin lock
in extcon_update_state() is safe. I think it is not safe because current
design implies silently that all notifiers registered with
extcon_register_notifier() must not sleep.

_call_per_cable() is just one of ways to trigger this issue.

Best regards,
Krzysztof



--
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/