Re: [PATCH v5 1/7] iommu: Fix a leak in iommu_insert_resv_region

[Auger Eric]

Hi Christoph,

On 5/29/19 8:17 AM, Christoph Hellwig wrote:
>>  		} else if ((start >= a) && (end <= b)) {
>>  			if (new->type == type)
>> -				goto done;
>> +				return 0;
>>  			else
>>  				pos = pos->next;
> 
> Please remove the pointless else after the return statement.
Sorry I don't get your point here. For both cases, we only return if
both types match. Otherwise we continue parsing the list as there may be
a region of the same type as @new following the current interval that
may need to be merged with @new. Let's consider the insertion of those
resv regions:

add_resv(head, 0xa001000, 0xa001FFF, IOMMU_RESV_TYPE1);
head -> 0x000000000a001000 0x000000000a001fff type1

add_resv(head, 0xa003000, 0xa003FFF, IOMMU_RESV_TYPE1);
head ->
0x000000000a001000 0x000000000a001fff type1 ->
0x000000000a003000 0x000000000a003fff type1

add_resv(head, 0xa002000, 0xa004FFF, IOMMU_RESV_TYPE2);
head ->
0x000000000a001000 0x000000000a001fff type1 ->
0x000000000a003000 0x000000000a003fff type1 ->
0x000000000a002000 0x000000000a004fff type2 ->

add_resv(head, 0xa0010FF, 0xa003000, IOMMU_RESV_TYPE2);

head ->
0x000000000a001000 0x000000000a001fff type1 ->
0x000000000a003000 0x000000000a003fff type1 ->
0x000000000a0010ff 0x000000000a004fff type2 <- merge

Or maybe I simply missed your point. Please let me know.

Thanks

Eric

> 
>>  		} else {
>>  			if (new->type == type) {
>>  				phys_addr_t new_start = min(a, start);
>>  				phys_addr_t new_end = max(b, end);
>> +				int ret;
>>  
>>  				list_del(&entry->list);
>>  				entry->start = new_start;
>>  				entry->length = new_end - new_start + 1;
>> -				iommu_insert_resv_region(entry, regions);
>> +				ret = iommu_insert_resv_region(entry, regions);
>> +				kfree(entry);
>> +				return ret;
>>  			} else {
>>  				pos = pos->next;
>>  			}
> 
> Same here.
>