Stable Archive on lore.kernel.org
 help / Atom feed
* [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()
@ 2019-02-11 15:05 Ewan D. Milne
  2019-02-11 19:19 ` James Smart
  2019-02-12  8:06 ` Christoph Hellwig
  0 siblings, 2 replies; 7+ messages in thread
From: Ewan D. Milne @ 2019-02-11 15:05 UTC (permalink / raw)
  To: linux-scsi, linux-nvme
  Cc: stable, james.smart, dick.kennedy, martin.petersen, jejb

The change to use dma_set_mask_and_coherent() incorrectly made a second
call with the 32 bit DMA mask value when the call with the 64 bit DMA
mask value succeeded.  This resulted in NVMe/FC connections failing due
to corrupted data buffers, and various other SCSI/FCP I/O errors.

Fixes: f30e1bfd6154 ("scsi: lpfc: use dma_set_mask_and_coherent")
Cc: <stable@vger.kernel.org>
Suggested-by: Don Dutile <ddutile@redhat.com>
Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/lpfc/lpfc_init.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index bede11e..60c178d 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -7367,9 +7367,9 @@ struct lpfc_rpi_hdr *
 		return error;
 
 	/* Set the device DMA mask size */
-	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
-	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
-		return error;
+	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
+		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)
+			return error;
 
 	/* Get the bus address of Bar0 and Bar2 and the number of bytes
 	 * required by each mapping.
@@ -9745,9 +9745,9 @@ struct lpfc_cq_event *
 		return error;
 
 	/* Set the device DMA mask size */
-	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
-	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
-		return error;
+	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
+		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)
+			return error;
 
 	/*
 	 * The BARs and register set definitions and offset locations are
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()
  2019-02-11 15:05 [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent() Ewan D. Milne
@ 2019-02-11 19:19 ` James Smart
  2019-02-12  8:06 ` Christoph Hellwig
  1 sibling, 0 replies; 7+ messages in thread
From: James Smart @ 2019-02-11 19:19 UTC (permalink / raw)
  To: Ewan D. Milne, linux-scsi, linux-nvme
  Cc: stable, dick.kennedy, martin.petersen, jejb

On 2/11/2019 7:05 AM, Ewan D. Milne wrote:
> The change to use dma_set_mask_and_coherent() incorrectly made a second
> call with the 32 bit DMA mask value when the call with the 64 bit DMA
> mask value succeeded.  This resulted in NVMe/FC connections failing due
> to corrupted data buffers, and various other SCSI/FCP I/O errors.
>
> Fixes: f30e1bfd6154 ("scsi: lpfc: use dma_set_mask_and_coherent")
> Cc: <stable@vger.kernel.org>
> Suggested-by: Don Dutile <ddutile@redhat.com>
> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> ---
>   drivers/scsi/lpfc/lpfc_init.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index bede11e..60c178d 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -7367,9 +7367,9 @@ struct lpfc_rpi_hdr *
>   		return error;
>   
>   	/* Set the device DMA mask size */
> -	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
> -	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
> -		return error;
> +	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
> +		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)
> +			return error;
>   
>   	/* Get the bus address of Bar0 and Bar2 and the number of bytes
>   	 * required by each mapping.
> @@ -9745,9 +9745,9 @@ struct lpfc_cq_event *
>   		return error;
>   
>   	/* Set the device DMA mask size */
> -	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
> -	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
> -		return error;
> +	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
> +		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)
> +			return error;
>   
>   	/*
>   	 * The BARs and register set definitions and offset locations are

Thanks Ewan.


Signed-off-by:  James Smart  <james.smart@broadcom.com>

-- james


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()
  2019-02-11 15:05 [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent() Ewan D. Milne
  2019-02-11 19:19 ` James Smart
@ 2019-02-12  8:06 ` Christoph Hellwig
  2019-02-12 15:37   ` Ewan D. Milne
  1 sibling, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2019-02-12  8:06 UTC (permalink / raw)
  To: Ewan D. Milne
  Cc: linux-scsi, linux-nvme, stable, james.smart, dick.kennedy,
	martin.petersen, jejb

On Mon, Feb 11, 2019 at 10:05:02AM -0500, Ewan D. Milne wrote:
> The change to use dma_set_mask_and_coherent() incorrectly made a second
> call with the 32 bit DMA mask value when the call with the 64 bit DMA
> mask value succeeded.  This resulted in NVMe/FC connections failing due
> to corrupted data buffers, and various other SCSI/FCP I/O errors.

Ooops, sorry.

>  	/* Set the device DMA mask size */
> -	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
> -	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
> -		return error;
> +	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
> +		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)

But this still looks obsfuctating, why not:

	error = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
	if (error)
		error = dma_set_mask_and_coherent(&pdev->dev,
				DMA_BIT_MASK(32)));
	if (error)
		return error;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()
  2019-02-12  8:06 ` Christoph Hellwig
@ 2019-02-12 15:37   ` Ewan D. Milne
  2019-02-12 18:34     ` Christoph Hellwig
  2019-02-13  3:10     ` Martin K. Petersen
  0 siblings, 2 replies; 7+ messages in thread
From: Ewan D. Milne @ 2019-02-12 15:37 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-scsi, linux-nvme, stable, james.smart, dick.kennedy,
	martin.petersen, jejb

On Tue, 2019-02-12 at 00:06 -0800, Christoph Hellwig wrote:
> On Mon, Feb 11, 2019 at 10:05:02AM -0500, Ewan D. Milne wrote:
> > The change to use dma_set_mask_and_coherent() incorrectly made a second
> > call with the 32 bit DMA mask value when the call with the 64 bit DMA
> > mask value succeeded.  This resulted in NVMe/FC connections failing due
> > to corrupted data buffers, and various other SCSI/FCP I/O errors.
> 
> Ooops, sorry.
> 
> >  	/* Set the device DMA mask size */
> > -	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
> > -	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
> > -		return error;
> > +	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
> > +		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)
> 
> But this still looks obsfuctating, why not:
> 
> 	error = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> 	if (error)
> 		error = dma_set_mask_and_coherent(&pdev->dev,
> 				DMA_BIT_MASK(32)));
> 	if (error)
> 		return error;

I agree that making the flow of control explicit would be good, but...

It looks like this would introduce a different problem.  "error" is set to
-ENODEV earlier in the two functions so that the various error paths will return
that value.  If it is overwritten with a successful result from the call to
dma_set_mask_and_coherent() but a later call to e.g. ioremap() fails, the
functions will incorrectly return a value indicating that they have succeeded.

It also appears as if the patches to hptiop, hisi_sas and bfa need to be fixed up.
I don't have a test environment for these, although I might be able to modify the
test environment for bfa.  Martin/James, what about the others?

-Ewan


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()
  2019-02-12 15:37   ` Ewan D. Milne
@ 2019-02-12 18:34     ` Christoph Hellwig
  2019-02-13  3:10     ` Martin K. Petersen
  1 sibling, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2019-02-12 18:34 UTC (permalink / raw)
  To: Ewan D. Milne
  Cc: Christoph Hellwig, linux-scsi, linux-nvme, stable, james.smart,
	dick.kennedy, martin.petersen, jejb

On Tue, Feb 12, 2019 at 10:37:45AM -0500, Ewan D. Milne wrote:
> 
> It looks like this would introduce a different problem.  "error" is set to
> -ENODEV earlier in the two functions so that the various error paths will return
> that value.  If it is overwritten with a successful result from the call to
> dma_set_mask_and_coherent() but a later call to e.g. ioremap() fails, the
> functions will incorrectly return a value indicating that they have succeeded.
> 
> It also appears as if the patches to hptiop, hisi_sas and bfa need to be fixed up.
> I don't have a test environment for these, although I might be able to modify the
> test environment for bfa.  Martin/James, what about the others?

I don't have a test environment for them either, and I doubt many
people have.  If you don't feel comfortable I can send a fix for
them.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()
  2019-02-12 15:37   ` Ewan D. Milne
  2019-02-12 18:34     ` Christoph Hellwig
@ 2019-02-13  3:10     ` Martin K. Petersen
  2019-02-13  9:18       ` John Garry
  1 sibling, 1 reply; 7+ messages in thread
From: Martin K. Petersen @ 2019-02-13  3:10 UTC (permalink / raw)
  To: Ewan D. Milne
  Cc: Christoph Hellwig, linux-scsi, linux-nvme, stable, james.smart,
	dick.kennedy, martin.petersen, jejb, John Garry


Ewan,

> It also appears as if the patches to hptiop, hisi_sas and bfa need to
> be fixed up.  I don't have a test environment for these, although I
> might be able to modify the test environment for bfa.  Martin/James,
> what about the others?

I'm afraid I don't have any of those. But I'm sure John would be happy
to check hisi_sas.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()
  2019-02-13  3:10     ` Martin K. Petersen
@ 2019-02-13  9:18       ` John Garry
  0 siblings, 0 replies; 7+ messages in thread
From: John Garry @ 2019-02-13  9:18 UTC (permalink / raw)
  To: Martin K. Petersen, Ewan D. Milne, Christoph Hellwig
  Cc: linux-scsi, linux-nvme, stable, james.smart, dick.kennedy, jejb

On 13/02/2019 03:10, Martin K. Petersen wrote:
>
> Ewan,
>
>> It also appears as if the patches to hptiop, hisi_sas and bfa need to
>> be fixed up.  I don't have a test environment for these, although I
>> might be able to modify the test environment for bfa.  Martin/James,
>> what about the others?
>
> I'm afraid I don't have any of those. But I'm sure John would be happy
> to check hisi_sas.
>

Happy to test. Or I can just send the fix myself. So what is the 
preferred pattern for setting the DMA mask?

John


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-11 15:05 [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent() Ewan D. Milne
2019-02-11 19:19 ` James Smart
2019-02-12  8:06 ` Christoph Hellwig
2019-02-12 15:37   ` Ewan D. Milne
2019-02-12 18:34     ` Christoph Hellwig
2019-02-13  3:10     ` Martin K. Petersen
2019-02-13  9:18       ` John Garry

Stable Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://lore.kernel.org/stable \
		stable@vger.kernel.org stable@archiver.kernel.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox