[PATCH] ata: fix a race condition when internal cmd time out

[PATCH] ata: fix a race condition when internal cmd time out

[Jason Yan]

For internal cmds, we will unmap DMA memory associated with the cmd
before we abort the cmd. If DMA transfering data before the aborting,
bus error will occured.

ata_exec_internal_sg
  ->ata_port_freeze if timeout
    ->ata_qc_complete
      ->ata_sg_clean
                                      dma transfering data = bus error
  ->ap->ops->post_internal_cmd
    ->sas_ata_post_internal
      ->sas_ata_internal_abort
        ->abort the cmd

Fix this by move post_internal_cmd() before unmapping the DMA memory
when time out. Notice that we have to set ATA_QCFLAG_FAILED flag before
calling post_internal_cmd() so that the aborting will work.

Reported-by: luojian <luojian5@huawei.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/ata/libata-core.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index adf28788cab5..d0ff5711e805 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1665,6 +1665,13 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 		 */
 		if (qc->flags & ATA_QCFLAG_ACTIVE) {
 			qc->err_mask |= AC_ERR_TIMEOUT;
+			qc->flags |= ATA_QCFLAG_FAILED;
+
+			spin_unlock_irqrestore(ap->lock, flags);
+			/* do post_internal_cmd */
+			if (ap->ops->post_internal_cmd)
+				ap->ops->post_internal_cmd(qc);
+			spin_lock_irqsave(ap->lock, flags);
 
 			if (ap->ops->error_handler)
 				ata_port_freeze(ap);
@@ -1679,9 +1686,10 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 		spin_unlock_irqrestore(ap->lock, flags);
 	}
 
-	/* do post_internal_cmd */
-	if (ap->ops->post_internal_cmd)
-		ap->ops->post_internal_cmd(qc);
+	if (!(qc->err_mask & AC_ERR_TIMEOUT))
+		/* do post_internal_cmd */
+		if (ap->ops->post_internal_cmd)
+			ap->ops->post_internal_cmd(qc);
 
 	/* perform minimal error analysis */
 	if (qc->flags & ATA_QCFLAG_FAILED) {
-- 
2.14.4

Re: [PATCH] ata: fix a race condition when internal cmd time out

[Sergei Shtylyov]

Hello!

On 16.02.2019 6:42, Jason Yan wrote:

> For internal cmds, we will unmap DMA memory associated with the cmd
> before we abort the cmd. If DMA transfering data before the aborting,

    Transferring.

> bus error will occured.

    Occur.

> ata_exec_internal_sg
>    ->ata_port_freeze if timeout
>      ->ata_qc_complete
>        ->ata_sg_clean
>                                        dma transfering data = bus error
>    ->ap->ops->post_internal_cmd
>      ->sas_ata_post_internal
>        ->sas_ata_internal_abort
>          ->abort the cmd
> 
> Fix this by move post_internal_cmd() before unmapping the DMA memory

    Moving.

> when time out. Notice that we have to set ATA_QCFLAG_FAILED flag before
> calling post_internal_cmd() so that the aborting will work.
> 
> Reported-by: luojian <luojian5@huawei.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
[...]

MBR, Sergei

Re: [PATCH] ata: fix a race condition when internal cmd time out

[Jason Yan]

Hi, Sergei

Thanks for the review.

On 2019/2/16 15:45, Sergei Shtylyov wrote:
> Hello!
> 
> On 16.02.2019 6:42, Jason Yan wrote:
> 
>> For internal cmds, we will unmap DMA memory associated with the cmd
>> before we abort the cmd. If DMA transfering data before the aborting,
> 
>     Transferring.
> 
>> bus error will occured.
> 
>     Occur.
> 
>> ata_exec_internal_sg
>>    ->ata_port_freeze if timeout
>>      ->ata_qc_complete
>>        ->ata_sg_clean
>>                                        dma transfering data = bus error
>>    ->ap->ops->post_internal_cmd
>>      ->sas_ata_post_internal
>>        ->sas_ata_internal_abort
>>          ->abort the cmd
>>
>> Fix this by move post_internal_cmd() before unmapping the DMA memory
> 
>     Moving.
> 
>> when time out. Notice that we have to set ATA_QCFLAG_FAILED flag before
>> calling post_internal_cmd() so that the aborting will work.
>>
>> Reported-by: luojian <luojian5@huawei.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> [...]
> 
> MBR, Sergei
> 
>