[RFC PATCH kernel] vfio/spapr_tce: Get rid of possible infinite loop

[Alexey Kardashevskiy]

On 08/10/2018 21:18, Michael Ellerman wrote:
> Serhii Popovych <spopovyc at redhat.com> writes:
>> Alexey Kardashevskiy wrote:
>>> As a part of cleanup, the SPAPR TCE IOMMU subdriver releases preregistered
>>> memory. If there is a bug in memory release, the loop in
>>> tce_iommu_release() becomes infinite; this actually happened to me.
>>>
>>> This makes the loop finite and prints a warning on every failure to make
>>> the code more bug prone.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik at ozlabs.ru>
>>> ---
>>>  drivers/vfio/vfio_iommu_spapr_tce.c | 10 +++-------
>>>  1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>>> index b1a8ab3..ece0651 100644
>>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>>> @@ -393,13 +394,8 @@ static void tce_iommu_release(void *iommu_data)
>>>  		tce_iommu_free_table(container, tbl);
>>>  	}
>>>  
>>> -	while (!list_empty(&container->prereg_list)) {
>>> -		struct tce_iommu_prereg *tcemem;
>>> -
>>> -		tcemem = list_first_entry(&container->prereg_list,
>>> -				struct tce_iommu_prereg, next);
>>> -		WARN_ON_ONCE(tce_iommu_prereg_free(container, tcemem));
>>> -	}
>>> +	list_for_each_entry_safe(tcemem, tmtmp, &container->prereg_list, next)
>>> +		WARN_ON(tce_iommu_prereg_free(container, tcemem));
>>
>> I'm not sure that tce_iommu_prereg_free() call under WARN_ON() is good
>> idea because WARN_ON() is a preprocessor macro:
>>
>>   if CONFIG_WARN=n is added by the analogy with CONFIG_BUG=n defining
>>   WARN_ON() as empty we will loose call to tce_iommu_prereg_free()
>>   leaking resources.
> 
> I don't think that's likely to ever happen though, we have a large
> number of uses that would need to be checked one-by-one:
> 
>   $ git grep "if (WARN_ON(" | wc -l
>   2853
> 
> 
> So if we ever did add CONFIG_WARN, I think it would still need to
> evaluate the condition, just not emit a warning.

Is anyone taking this?


-- 
Alexey