On 08/10/2018 21:18, Michael Ellerman wrote:
> 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.
Is anyone taking this?
--
Alexey
