diff mbox

[04/10] aacraid: Fix memory leak in aac_fib_map_free

Message ID 1448973589-9216-5-git-send-email-RaghavaAditya.Renukunta@pmcs.com (mailing list archive)
State New, archived
Headers show

Commit Message

Raghava Aditya Renukunta Dec. 1, 2015, 12:39 p.m. UTC 
From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

aac_fib_map_free() calls pci_free_consistent() without checking that
dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
are indeed NULL/0, this will result in a hang as pci_free_consistent()
will attempt to invalidate cache for the entire 64-bit address space
(which would take a very long time).

Fixed by adding a check to make sure that dev->hw_fib_va and
dev->max_fib_size are not NULL and 0 respectively.

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/commsup.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Johannes Thumshirn Dec. 2, 2015, 9:44 a.m. UTC | #1 
On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> aac_fib_map_free() calls pci_free_consistent() without checking that
> dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
> are indeed NULL/0, this will result in a hang as pci_free_consistent()
> will attempt to invalidate cache for the entire 64-bit address space
> (which would take a very long time).
> 
> Fixed by adding a check to make sure that dev->hw_fib_va and
> dev->max_fib_size are not NULL and 0 respectively.
> 
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/commsup.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
> index b257d3b..9533f47 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
>  
>  void aac_fib_map_free(struct aac_dev *dev)
>  {
> -	pci_free_consistent(dev->pdev,
> -	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue +
> AAC_NUM_MGT_FIB),
> -	  dev->hw_fib_va, dev->hw_fib_pa);
> +	if (dev->hw_fib_va && dev->max_fib_size) {
> +		pci_free_consistent(dev->pdev,
> +		(dev->max_fib_size *
> +		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
> +		dev->hw_fib_va, dev->hw_fib_pa);
> +	}
>  	dev->hw_fib_va = NULL;
>  	dev->hw_fib_pa = 0;
>  }

Fixes: 9ad5204d6 - "[SCSI] aacraid: incorrect dma mapping mask during blinkled
recover or user initiated reset"
Cc: stable@vger.kernel.org
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomas Henzl Dec. 4, 2015, 2:34 p.m. UTC | #2 
On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
>
> aac_fib_map_free() calls pci_free_consistent() without checking that
> dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
> are indeed NULL/0, this will result in a hang as pci_free_consistent()
> will attempt to invalidate cache for the entire 64-bit address space
> (which would take a very long time).
>
> Fixed by adding a check to make sure that dev->hw_fib_va and
> dev->max_fib_size are not NULL and 0 respectively.
>
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

Reviewed-by: Tomas Henzl <thenzl@redhat.com>

Is the can_queue constant during the driver's life, or is it possible
to manipulate it (aac_change_queue_depth)?

Tomas

> ---
>  drivers/scsi/aacraid/commsup.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
> index b257d3b..9533f47 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
>  
>  void aac_fib_map_free(struct aac_dev *dev)
>  {
> -	pci_free_consistent(dev->pdev,
> -	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB),
> -	  dev->hw_fib_va, dev->hw_fib_pa);
> +	if (dev->hw_fib_va && dev->max_fib_size) {
> +		pci_free_consistent(dev->pdev,
> +		(dev->max_fib_size *
> +		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
> +		dev->hw_fib_va, dev->hw_fib_pa);
> +	}
>  	dev->hw_fib_va = NULL;
>  	dev->hw_fib_pa = 0;
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Raghava Aditya Renukunta Dec. 5, 2015, 12:40 a.m. UTC | #3 
Hello Tomas,


> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Friday, December 4, 2015 6:35 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free
> 
> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> >
> > aac_fib_map_free() calls pci_free_consistent() without checking that
> > dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
> > are indeed NULL/0, this will result in a hang as pci_free_consistent()
> > will attempt to invalidate cache for the entire 64-bit address space
> > (which would take a very long time).
> >
> > Fixed by adding a check to make sure that dev->hw_fib_va and
> > dev->max_fib_size are not NULL and 0 respectively.
> >
> > Signed-off-by: Raghava Aditya Renukunta
> <raghavaaditya.renukunta@pmcs.com>
> 
> Reviewed-by: Tomas Henzl <thenzl@redhat.com>
> 
> Is the can_queue constant during the driver's life, or is it possible
> to manipulate it (aac_change_queue_depth)?
> 
> Tomas

can_queue is only changed in aac_init_adapter. Do you want to save 
 (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB) in a variable
So that the whole can_queue dereference need not be used?

Regards,
Raghava Aditya

> > ---
> >  drivers/scsi/aacraid/commsup.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/aacraid/commsup.c
> b/drivers/scsi/aacraid/commsup.c
> > index b257d3b..9533f47 100644
> > --- a/drivers/scsi/aacraid/commsup.c
> > +++ b/drivers/scsi/aacraid/commsup.c
> > @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
> >
> >  void aac_fib_map_free(struct aac_dev *dev)
> >  {
> > -	pci_free_consistent(dev->pdev,
> > -	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue +
> AAC_NUM_MGT_FIB),
> > -	  dev->hw_fib_va, dev->hw_fib_pa);
> > +	if (dev->hw_fib_va && dev->max_fib_size) {
> > +		pci_free_consistent(dev->pdev,
> > +		(dev->max_fib_size *
> > +		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
> > +		dev->hw_fib_va, dev->hw_fib_pa);
> > +	}
> >  	dev->hw_fib_va = NULL;
> >  	dev->hw_fib_pa = 0;
> >  }

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomas Henzl Dec. 7, 2015, 2:05 p.m. UTC | #4 
On 5.12.2015 01:40, Raghava Aditya Renukunta wrote:
> Hello Tomas,
>
>
>> -----Original Message-----
>> From: Tomas Henzl [mailto:thenzl@redhat.com]
>> Sent: Friday, December 4, 2015 6:35 AM
>> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
>> scsi@vger.kernel.org
>> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
>> aacraid@pmc-sierra.com; Rich Bono
>> Subject: Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free
>>
>> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
>>> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
>>>
>>> aac_fib_map_free() calls pci_free_consistent() without checking that
>>> dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
>>> are indeed NULL/0, this will result in a hang as pci_free_consistent()
>>> will attempt to invalidate cache for the entire 64-bit address space
>>> (which would take a very long time).
>>>
>>> Fixed by adding a check to make sure that dev->hw_fib_va and
>>> dev->max_fib_size are not NULL and 0 respectively.
>>>
>>> Signed-off-by: Raghava Aditya Renukunta
>> <raghavaaditya.renukunta@pmcs.com>
>>
>> Reviewed-by: Tomas Henzl <thenzl@redhat.com>
>>
>> Is the can_queue constant during the driver's life, or is it possible
>> to manipulate it (aac_change_queue_depth)?
>>
>> Tomas
> can_queue is only changed in aac_init_adapter. Do you want to save 
>  (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB) in a variable
> So that the whole can_queue dereference need not be used?

It's fine as it is, (I thought it may change elsewhere in your code
but now I think that I was wrong).

--tm

>
> Regards,
> Raghava Aditya
>
>>> ---
>>>  drivers/scsi/aacraid/commsup.c | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/aacraid/commsup.c
>> b/drivers/scsi/aacraid/commsup.c
>>> index b257d3b..9533f47 100644
>>> --- a/drivers/scsi/aacraid/commsup.c
>>> +++ b/drivers/scsi/aacraid/commsup.c
>>> @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
>>>
>>>  void aac_fib_map_free(struct aac_dev *dev)
>>>  {
>>> -	pci_free_consistent(dev->pdev,
>>> -	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue +
>> AAC_NUM_MGT_FIB),
>>> -	  dev->hw_fib_va, dev->hw_fib_pa);
>>> +	if (dev->hw_fib_va && dev->max_fib_size) {
>>> +		pci_free_consistent(dev->pdev,
>>> +		(dev->max_fib_size *
>>> +		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
>>> +		dev->hw_fib_va, dev->hw_fib_pa);
>>> +	}
>>>  	dev->hw_fib_va = NULL;
>>>  	dev->hw_fib_pa = 0;
>>>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Raghava Aditya Renukunta Dec. 7, 2015, 7:07 p.m. UTC | #5 
Hello Tomas,

> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Monday, December 7, 2015 6:06 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free
> 
> On 5.12.2015 01:40, Raghava Aditya Renukunta wrote:
> > Hello Tomas,
> >
> >
> >> -----Original Message-----
> >> From: Tomas Henzl [mailto:thenzl@redhat.com]
> >> Sent: Friday, December 4, 2015 6:35 AM
> >> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> >> scsi@vger.kernel.org
> >> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> >> aacraid@pmc-sierra.com; Rich Bono
> >> Subject: Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free
> >>
> >> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> >>> From: Raghava Aditya Renukunta
> <raghavaaditya.renukunta@pmcs.com>
> >>>
> >>> aac_fib_map_free() calls pci_free_consistent() without checking that
> >>> dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
> >>> are indeed NULL/0, this will result in a hang as pci_free_consistent()
> >>> will attempt to invalidate cache for the entire 64-bit address space
> >>> (which would take a very long time).
> >>>
> >>> Fixed by adding a check to make sure that dev->hw_fib_va and
> >>> dev->max_fib_size are not NULL and 0 respectively.
> >>>
> >>> Signed-off-by: Raghava Aditya Renukunta
> >> <raghavaaditya.renukunta@pmcs.com>
> >>
> >> Reviewed-by: Tomas Henzl <thenzl@redhat.com>
> >>
> >> Is the can_queue constant during the driver's life, or is it possible
> >> to manipulate it (aac_change_queue_depth)?
> >>
> >> Tomas
> > can_queue is only changed in aac_init_adapter. Do you want to save
> >  (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB) in a variable
> > So that the whole can_queue dereference need not be used?
> 
> It's fine as it is, (I thought it may change elsewhere in your code
> but now I think that I was wrong).
> 
> --tm

I will leave it as it is then.


> 
> >
> > Regards,
> > Raghava Aditya
> >
> >>> ---
> >>>  drivers/scsi/aacraid/commsup.c | 9 ++++++---
> >>>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/scsi/aacraid/commsup.c
> >> b/drivers/scsi/aacraid/commsup.c
> >>> index b257d3b..9533f47 100644
> >>> --- a/drivers/scsi/aacraid/commsup.c
> >>> +++ b/drivers/scsi/aacraid/commsup.c
> >>> @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
> >>>
> >>>  void aac_fib_map_free(struct aac_dev *dev)
> >>>  {
> >>> -	pci_free_consistent(dev->pdev,
> >>> -	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue +
> >> AAC_NUM_MGT_FIB),
> >>> -	  dev->hw_fib_va, dev->hw_fib_pa);
> >>> +	if (dev->hw_fib_va && dev->max_fib_size) {
> >>> +		pci_free_consistent(dev->pdev,
> >>> +		(dev->max_fib_size *
> >>> +		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
> >>> +		dev->hw_fib_va, dev->hw_fib_pa);
> >>> +	}
> >>>  	dev->hw_fib_va = NULL;
> >>>  	dev->hw_fib_pa = 0;
> >>>  }
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index b257d3b..9533f47 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -83,9 +83,12 @@  static int fib_map_alloc(struct aac_dev *dev)
 
 void aac_fib_map_free(struct aac_dev *dev)
 {
-	pci_free_consistent(dev->pdev,
-	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB),
-	  dev->hw_fib_va, dev->hw_fib_pa);
+	if (dev->hw_fib_va && dev->max_fib_size) {
+		pci_free_consistent(dev->pdev,
+		(dev->max_fib_size *
+		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
+		dev->hw_fib_va, dev->hw_fib_pa);
+	}
 	dev->hw_fib_va = NULL;
 	dev->hw_fib_pa = 0;
 }