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

[Tomas Henzl <thenzl@redhat.com>]

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