On 08/10/2018 21:18, Michael Ellerman wrote:

Serhii Popovych [off-list ref] writes:

quoted

Alexey Kardashevskiy wrote:

quoted

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 <redacted>
---
 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