Skip to content

Commit

Permalink
libata: fix sff host state machine locking while polling
Browse files Browse the repository at this point in the history 
The bulk of ATA host state machine is implemented by
ata_sff_hsm_move().  The function is called from either the interrupt
handler or, if polling, a work item.  Unlike from the interrupt path,
the polling path calls the function without holding the host lock and
ata_sff_hsm_move() selectively grabs the lock.

This is completely broken.  If an IRQ triggers while polling is in
progress, the two can easily race and end up accessing the hardware
and updating state machine state at the same time.  This can put the
state machine in an illegal state and lead to a crash like the
following.

  kernel BUG at drivers/ata/libata-sff.c:1302!
  invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
  Modules linked in:
  CPU: 1 PID: 10679 Comm: syz-executor Not tainted 4.5.0-rc1+ torvalds#300
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
  task: ffff88002bd00000 ti: ffff88002e048000 task.ti: ffff88002e048000
  RIP: 0010:[<ffffffff83a83409>]  [<ffffffff83a83409>] ata_sff_hsm_move+0x619/0x1c60
  ...
  Call Trace:
   <IRQ>
   [<ffffffff83a84c31>] __ata_sff_port_intr+0x1e1/0x3a0 drivers/ata/libata-sff.c:1584
   [<ffffffff83a85611>] ata_bmdma_port_intr+0x71/0x400 drivers/ata/libata-sff.c:2877
   [<     inline     >] __ata_sff_interrupt drivers/ata/libata-sff.c:1629
   [<ffffffff83a85bf3>] ata_bmdma_interrupt+0x253/0x580 drivers/ata/libata-sff.c:2902
   [<ffffffff81479f98>] handle_irq_event_percpu+0x108/0x7e0 kernel/irq/handle.c:157
   [<ffffffff8147a717>] handle_irq_event+0xa7/0x140 kernel/irq/handle.c:205
   [<ffffffff81484573>] handle_edge_irq+0x1e3/0x8d0 kernel/irq/chip.c:623
   [<     inline     >] generic_handle_irq_desc include/linux/irqdesc.h:146
   [<ffffffff811a92bc>] handle_irq+0x10c/0x2a0 arch/x86/kernel/irq_64.c:78
   [<ffffffff811a7e4d>] do_IRQ+0x7d/0x1a0 arch/x86/kernel/irq.c:240
   [<ffffffff86653d4c>] common_interrupt+0x8c/0x8c arch/x86/entry/entry_64.S:520
   <EOI>
   [<     inline     >] rcu_lock_acquire include/linux/rcupdate.h:490
   [<     inline     >] rcu_read_lock include/linux/rcupdate.h:874
   [<ffffffff8164b4a1>] filemap_map_pages+0x131/0xba0 mm/filemap.c:2145
   [<     inline     >] do_fault_around mm/memory.c:2943
   [<     inline     >] do_read_fault mm/memory.c:2962
   [<     inline     >] do_fault mm/memory.c:3133
   [<     inline     >] handle_pte_fault mm/memory.c:3308
   [<     inline     >] __handle_mm_fault mm/memory.c:3418
   [<ffffffff816efb16>] handle_mm_fault+0x2516/0x49a0 mm/memory.c:3447
   [<ffffffff8127dc16>] __do_page_fault+0x376/0x960 arch/x86/mm/fault.c:1238
   [<ffffffff8127e358>] trace_do_page_fault+0xe8/0x420 arch/x86/mm/fault.c:1331
   [<ffffffff8126f514>] do_async_page_fault+0x14/0xd0 arch/x86/kernel/kvm.c:264
   [<ffffffff86655578>] async_page_fault+0x28/0x30 arch/x86/entry/entry_64.S:986

Fix it by ensuring that the polling path is holding the host lock
before entering ata_sff_hsm_move() so that all hardware accesses and
state updates are performed under the host lock.

Signed-off-by: Tejun Heo <[email protected]>
Reported-and-tested-by: Dmitry Vyukov <[email protected]>
Link: http://lkml.kernel.org/g/CACT4Y+b_JsOxJu2EZyEf+mOXORc_zid5V1-pLZSroJVxyWdSpw@mail.gmail.com
Cc: [email protected]
  • Loading branch information
@htejun
htejun committed Feb 1, 2016
1 parent a588afc commit 8eee1d3
Showing 1 changed file with 11 additions and 21 deletions.
32 changes: 11 additions & 21 deletions drivers/ata/libata-sff.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -997,12 +997,9 @@ static inline int ata_hsm_ok_in_wq(struct ata_port *ap,
static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
{
struct ata_port *ap = qc->ap;
unsigned long flags;

if (ap->ops->error_handler) {
if (in_wq) {
spin_lock_irqsave(ap->lock, flags);

/* EH might have kicked in while host lock is
* released.
*/
Expand All @@ -1014,8 +1011,6 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
} else
ata_port_freeze(ap);
}

spin_unlock_irqrestore(ap->lock, flags);
} else {
if (likely(!(qc->err_mask & AC_ERR_HSM)))
ata_qc_complete(qc);
Expand All @@ -1024,10 +1019,8 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
}
} else {
if (in_wq) {
spin_lock_irqsave(ap->lock, flags);
ata_sff_irq_on(ap);
ata_qc_complete(qc);
spin_unlock_irqrestore(ap->lock, flags);
} else
ata_qc_complete(qc);
}
Expand All @@ -1048,9 +1041,10 @@ int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
{
struct ata_link *link = qc->dev->link;
struct ata_eh_info *ehi = &link->eh_info;
unsigned long flags = 0;
int poll_next;

lockdep_assert_held(ap->lock);

WARN_ON_ONCE((qc->flags & ATA_QCFLAG_ACTIVE) == 0);

/* Make sure ata_sff_qc_issue() does not throw things
Expand Down Expand Up @@ -1112,14 +1106,6 @@ int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
}
}

/* Send the CDB (atapi) or the first data block (ata pio out).
* During the state transition, interrupt handler shouldn't
* be invoked before the data transfer is complete and
* hsm_task_state is changed. Hence, the following locking.
*/
if (in_wq)
spin_lock_irqsave(ap->lock, flags);

if (qc->tf.protocol == ATA_PROT_PIO) {
/* PIO data out protocol.
* send first data block.
Expand All @@ -1135,9 +1121,6 @@ int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
/* send CDB */
atapi_send_cdb(ap, qc);

if (in_wq)
spin_unlock_irqrestore(ap->lock, flags);

/* if polling, ata_sff_pio_task() handles the rest.
* otherwise, interrupt handler takes over from here.
*/
Expand Down Expand Up @@ -1362,12 +1345,14 @@ static void ata_sff_pio_task(struct work_struct *work)
u8 status;
int poll_next;

spin_lock_irq(ap->lock);

BUG_ON(ap->sff_pio_task_link == NULL);
/* qc can be NULL if timeout occurred */
qc = ata_qc_from_tag(ap, link->active_tag);
if (!qc) {
ap->sff_pio_task_link = NULL;
return;
goto out_unlock;
}

fsm_start:
Expand All @@ -1382,11 +1367,14 @@ static void ata_sff_pio_task(struct work_struct *work)
*/
status = ata_sff_busy_wait(ap, ATA_BUSY, 5);
if (status & ATA_BUSY) {
spin_unlock_irq(ap->lock);
ata_msleep(ap, 2);
spin_lock_irq(ap->lock);

status = ata_sff_busy_wait(ap, ATA_BUSY, 10);
if (status & ATA_BUSY) {
ata_sff_queue_pio_task(link, ATA_SHORT_PAUSE);
return;
goto out_unlock;
}
}

Expand All @@ -1403,6 +1391,8 @@ static void ata_sff_pio_task(struct work_struct *work)
*/
if (poll_next)
goto fsm_start;
out_unlock:
spin_unlock_irq(ap->lock);
}

/**
Expand Down

0 comments on commit 8eee1d3

Please sign in to comment.