Serhii Popovych <spopovyc@xxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
cheers
