| Message ID | 20181002032231.7494-1-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 517ad4ae8aa93dccdb9a88c27257ecb421c9e848 |
| Headers | show |
| Series | [RFC,kernel] vfio/spapr_tce: Get rid of possible infinite loop | expand |
On Tue, Oct 02, 2018 at 01:22:31PM +1000, 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@ozlabs.ru> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> It does improve the current behaviour. I do suspect, however, that leaving the failed regions in the list will probably cause another failure later on. > --- > 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 > @@ -371,6 +371,7 @@ static void tce_iommu_release(void *iommu_data) > { > struct tce_container *container = iommu_data; > struct tce_iommu_group *tcegrp; > + struct tce_iommu_prereg *tcemem, *tmtmp; > long i; > > while (tce_groups_attached(container)) { > @@ -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)); > > tce_iommu_disable(container); > if (container->mm)
On Tue, 2 Oct 2018 13:22:31 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> 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@ozlabs.ru> > --- > drivers/vfio/vfio_iommu_spapr_tce.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) Should this have a stable/fixes tag? Looks like it's relevant to: 4b6fad7097f8 powerpc/mm/iommu, vfio/spapr: Put pages on VFIO container shutdown Also, not sure who you're wanting to take this since it was sent to ppc lists. If it's me, let me know, otherwise Acked-by: Alex Williamson <alex.williamson@redhat.com> Thanks, Alex > 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 > @@ -371,6 +371,7 @@ static void tce_iommu_release(void *iommu_data) > { > struct tce_container *container = iommu_data; > struct tce_iommu_group *tcegrp; > + struct tce_iommu_prereg *tcemem, *tmtmp; > long i; > > while (tce_groups_attached(container)) { > @@ -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)); > > tce_iommu_disable(container); > if (container->mm)
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@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 > @@ -371,6 +371,7 @@ static void tce_iommu_release(void *iommu_data) > { > struct tce_container *container = iommu_data; > struct tce_iommu_group *tcegrp; > + struct tce_iommu_prereg *tcemem, *tmtmp; > long i; > > while (tce_groups_attached(container)) { > @@ -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. There is no problem at the moment: WARN_ON() defined for PPC in arch/powerpc/include/asm/bug.h unconditionally. So your first version with intermediate variable looks better to me. > > tce_iommu_disable(container); > if (container->mm) >
Serhii Popovych <spopovyc@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@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. cheers
On 08/10/2018 21:18, Michael Ellerman wrote: > Serhii Popovych <spopovyc@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@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?
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 @@ -371,6 +371,7 @@ static void tce_iommu_release(void *iommu_data) { struct tce_container *container = iommu_data; struct tce_iommu_group *tcegrp; + struct tce_iommu_prereg *tcemem, *tmtmp; long i; while (tce_groups_attached(container)) { @@ -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)); tce_iommu_disable(container); if (container->mm)
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@ozlabs.ru> --- drivers/vfio/vfio_iommu_spapr_tce.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)