From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
stable@vger.kernel.org, Andrea Righi <andrea.righi@canonical.com>,
Pavel Machek <pavel@ucw.cz>
Subject: [PATCH 4.9 18/32] leds: trigger: fix potential deadlock with libata
Date: Tue, 2 Feb 2021 14:38:41 +0100 [thread overview]
Message-ID: <20210202132942.753514333@linuxfoundation.org> (raw)
In-Reply-To: <20210202132942.035179752@linuxfoundation.org>
From: Andrea Righi <andrea.righi@canonical.com>
commit 27af8e2c90fba242460b01fa020e6e19ed68c495 upstream.
We have the following potential deadlock condition:
========================================================
WARNING: possible irq lock inversion dependency detected
5.10.0-rc2+ #25 Not tainted
--------------------------------------------------------
swapper/3/0 just changed the state of lock:
ffff8880063bd618 (&host->lock){-...}-{2:2}, at: ata_bmdma_interrupt+0x27/0x200
but this lock took another, HARDIRQ-READ-unsafe lock in the past:
(&trig->leddev_list_lock){.+.?}-{2:2}
and interrupts could create inverse lock ordering between them.
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&trig->leddev_list_lock);
local_irq_disable();
lock(&host->lock);
lock(&trig->leddev_list_lock);
<Interrupt>
lock(&host->lock);
*** DEADLOCK ***
no locks held by swapper/3/0.
the shortest dependencies between 2nd lock and 1st lock:
-> (&trig->leddev_list_lock){.+.?}-{2:2} ops: 46 {
HARDIRQ-ON-R at:
lock_acquire+0x15f/0x420
_raw_read_lock+0x42/0x90
led_trigger_event+0x2b/0x70
rfkill_global_led_trigger_worker+0x94/0xb0
process_one_work+0x240/0x560
worker_thread+0x58/0x3d0
kthread+0x151/0x170
ret_from_fork+0x1f/0x30
IN-SOFTIRQ-R at:
lock_acquire+0x15f/0x420
_raw_read_lock+0x42/0x90
led_trigger_event+0x2b/0x70
kbd_bh+0x9e/0xc0
tasklet_action_common.constprop.0+0xe9/0x100
tasklet_action+0x22/0x30
__do_softirq+0xcc/0x46d
run_ksoftirqd+0x3f/0x70
smpboot_thread_fn+0x116/0x1f0
kthread+0x151/0x170
ret_from_fork+0x1f/0x30
SOFTIRQ-ON-R at:
lock_acquire+0x15f/0x420
_raw_read_lock+0x42/0x90
led_trigger_event+0x2b/0x70
rfkill_global_led_trigger_worker+0x94/0xb0
process_one_work+0x240/0x560
worker_thread+0x58/0x3d0
kthread+0x151/0x170
ret_from_fork+0x1f/0x30
INITIAL READ USE at:
lock_acquire+0x15f/0x420
_raw_read_lock+0x42/0x90
led_trigger_event+0x2b/0x70
rfkill_global_led_trigger_worker+0x94/0xb0
process_one_work+0x240/0x560
worker_thread+0x58/0x3d0
kthread+0x151/0x170
ret_from_fork+0x1f/0x30
}
... key at: [<ffffffff83da4c00>] __key.0+0x0/0x10
... acquired at:
_raw_read_lock+0x42/0x90
led_trigger_blink_oneshot+0x3b/0x90
ledtrig_disk_activity+0x3c/0xa0
ata_qc_complete+0x26/0x450
ata_do_link_abort+0xa3/0xe0
ata_port_freeze+0x2e/0x40
ata_hsm_qc_complete+0x94/0xa0
ata_sff_hsm_move+0x177/0x7a0
ata_sff_pio_task+0xc7/0x1b0
process_one_work+0x240/0x560
worker_thread+0x58/0x3d0
kthread+0x151/0x170
ret_from_fork+0x1f/0x30
-> (&host->lock){-...}-{2:2} ops: 69 {
IN-HARDIRQ-W at:
lock_acquire+0x15f/0x420
_raw_spin_lock_irqsave+0x52/0xa0
ata_bmdma_interrupt+0x27/0x200
__handle_irq_event_percpu+0xd5/0x2b0
handle_irq_event+0x57/0xb0
handle_edge_irq+0x8c/0x230
asm_call_irq_on_stack+0xf/0x20
common_interrupt+0x100/0x1c0
asm_common_interrupt+0x1e/0x40
native_safe_halt+0xe/0x10
arch_cpu_idle+0x15/0x20
default_idle_call+0x59/0x1c0
do_idle+0x22c/0x2c0
cpu_startup_entry+0x20/0x30
start_secondary+0x11d/0x150
secondary_startup_64_no_verify+0xa6/0xab
INITIAL USE at:
lock_acquire+0x15f/0x420
_raw_spin_lock_irqsave+0x52/0xa0
ata_dev_init+0x54/0xe0
ata_link_init+0x8b/0xd0
ata_port_alloc+0x1f1/0x210
ata_host_alloc+0xf1/0x130
ata_host_alloc_pinfo+0x14/0xb0
ata_pci_sff_prepare_host+0x41/0xa0
ata_pci_bmdma_prepare_host+0x14/0x30
piix_init_one+0x21f/0x600
local_pci_probe+0x48/0x80
pci_device_probe+0x105/0x1c0
really_probe+0x221/0x490
driver_probe_device+0xe9/0x160
device_driver_attach+0xb2/0xc0
__driver_attach+0x91/0x150
bus_for_each_dev+0x81/0xc0
driver_attach+0x1e/0x20
bus_add_driver+0x138/0x1f0
driver_register+0x91/0xf0
__pci_register_driver+0x73/0x80
piix_init+0x1e/0x2e
do_one_initcall+0x5f/0x2d0
kernel_init_freeable+0x26f/0x2cf
kernel_init+0xe/0x113
ret_from_fork+0x1f/0x30
}
... key at: [<ffffffff83d9fdc0>] __key.6+0x0/0x10
... acquired at:
__lock_acquire+0x9da/0x2370
lock_acquire+0x15f/0x420
_raw_spin_lock_irqsave+0x52/0xa0
ata_bmdma_interrupt+0x27/0x200
__handle_irq_event_percpu+0xd5/0x2b0
handle_irq_event+0x57/0xb0
handle_edge_irq+0x8c/0x230
asm_call_irq_on_stack+0xf/0x20
common_interrupt+0x100/0x1c0
asm_common_interrupt+0x1e/0x40
native_safe_halt+0xe/0x10
arch_cpu_idle+0x15/0x20
default_idle_call+0x59/0x1c0
do_idle+0x22c/0x2c0
cpu_startup_entry+0x20/0x30
start_secondary+0x11d/0x150
secondary_startup_64_no_verify+0xa6/0xab
This lockdep splat is reported after:
commit e918188611f0 ("locking: More accurate annotations for read_lock()")
To clarify:
- read-locks are recursive only in interrupt context (when
in_interrupt() returns true)
- after acquiring host->lock in CPU1, another cpu (i.e. CPU2) may call
write_lock(&trig->leddev_list_lock) that would be blocked by CPU0
that holds trig->leddev_list_lock in read-mode
- when CPU1 (ata_ac_complete()) tries to read-lock
trig->leddev_list_lock, it would be blocked by the write-lock waiter
on CPU2 (because we are not in interrupt context, so the read-lock is
not recursive)
- at this point if an interrupt happens on CPU0 and
ata_bmdma_interrupt() is executed it will try to acquire host->lock,
that is held by CPU1, that is currently blocked by CPU2, so:
* CPU0 blocked by CPU1
* CPU1 blocked by CPU2
* CPU2 blocked by CPU0
*** DEADLOCK ***
The deadlock scenario is better represented by the following schema
(thanks to Boqun Feng <boqun.feng@gmail.com> for the schema and the
detailed explanation of the deadlock condition):
CPU 0: CPU 1: CPU 2:
----- ----- -----
led_trigger_event():
read_lock(&trig->leddev_list_lock);
<workqueue>
ata_hsm_qc_complete():
spin_lock_irqsave(&host->lock);
write_lock(&trig->leddev_list_lock);
ata_port_freeze():
ata_do_link_abort():
ata_qc_complete():
ledtrig_disk_activity():
led_trigger_blink_oneshot():
read_lock(&trig->leddev_list_lock);
// ^ not in in_interrupt() context, so could get blocked by CPU 2
<interrupt>
ata_bmdma_interrupt():
spin_lock_irqsave(&host->lock);
Fix by using read_lock_irqsave/irqrestore() in led_trigger_event(), so
that no interrupt can happen in between, preventing the deadlock
condition.
Apply the same change to led_trigger_blink_setup() as well, since the
same deadlock scenario can also happen in power_supply_update_bat_leds()
-> led_trigger_blink() -> led_trigger_blink_setup() (workqueue context),
and potentially prevent other similar usages.
Link: https://lore.kernel.org/lkml/20201101092614.GB3989@xps-13-7390/
Fixes: eb25cb9956cc ("leds: convert IDE trigger to common disk trigger")
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/leds/led-triggers.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -283,14 +283,15 @@ void led_trigger_event(struct led_trigge
enum led_brightness brightness)
{
struct led_classdev *led_cdev;
+ unsigned long flags;
if (!trig)
return;
- read_lock(&trig->leddev_list_lock);
+ read_lock_irqsave(&trig->leddev_list_lock, flags);
list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
led_set_brightness(led_cdev, brightness);
- read_unlock(&trig->leddev_list_lock);
+ read_unlock_irqrestore(&trig->leddev_list_lock, flags);
}
EXPORT_SYMBOL_GPL(led_trigger_event);
@@ -301,11 +302,12 @@ static void led_trigger_blink_setup(stru
int invert)
{
struct led_classdev *led_cdev;
+ unsigned long flags;
if (!trig)
return;
- read_lock(&trig->leddev_list_lock);
+ read_lock_irqsave(&trig->leddev_list_lock, flags);
list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
if (oneshot)
led_blink_set_oneshot(led_cdev, delay_on, delay_off,
@@ -313,7 +315,7 @@ static void led_trigger_blink_setup(stru
else
led_blink_set(led_cdev, delay_on, delay_off);
}
- read_unlock(&trig->leddev_list_lock);
+ read_unlock_irqrestore(&trig->leddev_list_lock, flags);
}
void led_trigger_blink(struct led_trigger *trig,
next prev parent reply other threads:[~2021-02-02 17:52 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-02 13:38 [PATCH 4.9 00/32] 4.9.255-rc1 review Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 01/32] ACPI: sysfs: Prefer "compatible" modalias Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 02/32] wext: fix NULL-ptr-dereference with cfg80211s lack of commit() Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 03/32] net: usb: qmi_wwan: added support for Thales Cinterion PLSx3 modem family Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 04/32] y2038: futex: Move compat implementation into futex.c Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 05/32] futex: Move futex exit handling into futex code Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 06/32] futex: Replace PF_EXITPIDONE with a state Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 07/32] exit/exec: Seperate mm_release() Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 08/32] futex: Split futex_mm_release() for exit/exec Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 09/32] futex: Set task::futex_state to DEAD right after handling futex exit Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 10/32] futex: Mark the begin of futex exit explicitly Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 11/32] futex: Sanitize exit state handling Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 12/32] futex: Provide state handling for exec() as well Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 13/32] futex: Add mutex around futex exit Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 14/32] futex: Provide distinct return value when owner is exiting Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 15/32] futex: Prevent exit livelock Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 16/32] KVM: x86/pmu: Fix HW_REF_CPU_CYCLES event pseudo-encoding in intel_arch_events[] Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 17/32] KVM: x86: get smi pending status correctly Greg Kroah-Hartman
2021-02-02 13:38 ` Greg Kroah-Hartman [this message]
2021-02-02 13:38 ` [PATCH 4.9 19/32] mt7601u: fix kernel crash unplugging the device Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 20/32] mt7601u: fix rx buffer refcounting Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 21/32] ARM: imx: build suspend-imx6.S with arm instruction set Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 22/32] netfilter: nft_dynset: add timeout extension to template Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 23/32] xfrm: Fix oops in xfrm_replay_advance_bmp Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 24/32] RDMA/cxgb4: Fix the reported max_recv_sge value Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 25/32] iwlwifi: pcie: use jiffies for memory read spin time limit Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 26/32] iwlwifi: pcie: reschedule in long-running memory reads Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 27/32] mac80211: pause TX while changing interface type Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 28/32] can: dev: prevent potential information leak in can_fill_info() Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 29/32] iommu/vt-d: Gracefully handle DMAR units with no supported address widths Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 30/32] iommu/vt-d: Dont dereference iommu_device if IOMMU_API is not built Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 31/32] NFC: fix resource leak when target index is invalid Greg Kroah-Hartman
2021-02-02 13:38 ` [PATCH 4.9 32/32] NFC: fix possible resource leak Greg Kroah-Hartman
2021-02-02 20:20 ` [PATCH 4.9 00/32] 4.9.255-rc1 review Jon Hunter
2021-02-03 9:53 ` Naresh Kamboju
2021-02-03 15:42 ` Shuah Khan
2021-02-03 20:41 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210202132942.753514333@linuxfoundation.org \
--to=gregkh@linuxfoundation.org \
--cc=andrea.righi@canonical.com \
--cc=linux-kernel@vger.kernel.org \
--cc=pavel@ucw.cz \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.