| Message ID | 20210115092643.728-3-zhukeqian1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/iommu_type1: some fixes | expand |
On Fri, 15 Jan 2021 17:26:43 +0800 Keqian Zhu <zhukeqian1@huawei.com> wrote: > vfio_sanity_check_pfn_list() is used to check whether pfn_list of > vfio_dma is empty when remove the external domain, so it makes a > wrong assumption that only external domain will add pfn to dma pfn_list. > > Now we apply this check when remove a specific vfio_dma and extract > the notifier check just for external domain. The page pinning interface is gated by having a notifier registered for unmaps, therefore non-external domains would also need to register a notifier. There's currently no other way to add entries to the pfn_list. So if we allow pinning for such domains, then it's wrong to WARN_ON() when the notifier list is not-empty when removing an external domain. Long term we should probably extend page {un}pinning for the caller to pass their notifier to be validated against the notifier list rather than just allowing page pinning if *any* notifier is registered. Thanks, Alex > Fixes: a54eb55045ae ("vfio iommu type1: Add support for mediated devices") > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > --- > drivers/vfio/vfio_iommu_type1.c | 24 +++++------------------- > 1 file changed, 5 insertions(+), 19 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 4e82b9a3440f..a9bc15e84a4e 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -958,6 +958,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, > > static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma) > { > + WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list); > vfio_unmap_unpin(iommu, dma, true); > vfio_unlink_dma(iommu, dma); > put_task_struct(dma->task); > @@ -2251,23 +2252,6 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) > } > } > > -static void vfio_sanity_check_pfn_list(struct vfio_iommu *iommu) > -{ > - struct rb_node *n; > - > - n = rb_first(&iommu->dma_list); > - for (; n; n = rb_next(n)) { > - struct vfio_dma *dma; > - > - dma = rb_entry(n, struct vfio_dma, node); > - > - if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list))) > - break; > - } > - /* mdev vendor driver must unregister notifier */ > - WARN_ON(iommu->notifier.head); > -} > - > /* > * Called when a domain is removed in detach. It is possible that > * the removed domain decided the iova aperture window. Modify the > @@ -2367,7 +2351,8 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, > kfree(group); > > if (list_empty(&iommu->external_domain->group_list)) { > - vfio_sanity_check_pfn_list(iommu); > + /* mdev vendor driver must unregister notifier */ > + WARN_ON(iommu->notifier.head); > > if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) > vfio_iommu_unmap_unpin_all(iommu); > @@ -2491,7 +2476,8 @@ static void vfio_iommu_type1_release(void *iommu_data) > > if (iommu->external_domain) { > vfio_release_domain(iommu->external_domain, true); > - vfio_sanity_check_pfn_list(iommu); > + /* mdev vendor driver must unregister notifier */ > + WARN_ON(iommu->notifier.head); > kfree(iommu->external_domain); > } >
On 2021/1/16 3:14, Alex Williamson wrote: > On Fri, 15 Jan 2021 17:26:43 +0800 > Keqian Zhu <zhukeqian1@huawei.com> wrote: > >> vfio_sanity_check_pfn_list() is used to check whether pfn_list of >> vfio_dma is empty when remove the external domain, so it makes a >> wrong assumption that only external domain will add pfn to dma pfn_list. >> >> Now we apply this check when remove a specific vfio_dma and extract >> the notifier check just for external domain. > > The page pinning interface is gated by having a notifier registered for > unmaps, therefore non-external domains would also need to register a > notifier. There's currently no other way to add entries to the > pfn_list. So if we allow pinning for such domains, then it's wrong to > WARN_ON() when the notifier list is not-empty when removing an external > domain. Long term we should probably extend page {un}pinning for the > caller to pass their notifier to be validated against the notifier list > rather than just allowing page pinning if *any* notifier is registered. > Thanks, I was misled by the code comments. So when the commit a54eb55045ae is added, the only user of pin interface is mdev vendor driver, but now we also allow iommu backed group to use this interface to constraint dirty scope. Is vfio_iommu_unmap_unpin_all() a proper place to put this WARN()? Thanks, Keqian > > Alex > >> Fixes: a54eb55045ae ("vfio iommu type1: Add support for mediated devices") >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> --- >> drivers/vfio/vfio_iommu_type1.c | 24 +++++------------------- >> 1 file changed, 5 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >> index 4e82b9a3440f..a9bc15e84a4e 100644 >> --- a/drivers/vfio/vfio_iommu_type1.c >> +++ b/drivers/vfio/vfio_iommu_type1.c >> @@ -958,6 +958,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, >> >> static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma) >> { >> + WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list); >> vfio_unmap_unpin(iommu, dma, true); >> vfio_unlink_dma(iommu, dma); >> put_task_struct(dma->task); >> @@ -2251,23 +2252,6 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) >> } >> } >> >> -static void vfio_sanity_check_pfn_list(struct vfio_iommu *iommu) >> -{ >> - struct rb_node *n; >> - >> - n = rb_first(&iommu->dma_list); >> - for (; n; n = rb_next(n)) { >> - struct vfio_dma *dma; >> - >> - dma = rb_entry(n, struct vfio_dma, node); >> - >> - if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list))) >> - break; >> - } >> - /* mdev vendor driver must unregister notifier */ >> - WARN_ON(iommu->notifier.head); >> -} >> - >> /* >> * Called when a domain is removed in detach. It is possible that >> * the removed domain decided the iova aperture window. Modify the >> @@ -2367,7 +2351,8 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, >> kfree(group); >> >> if (list_empty(&iommu->external_domain->group_list)) { >> - vfio_sanity_check_pfn_list(iommu); >> + /* mdev vendor driver must unregister notifier */ >> + WARN_ON(iommu->notifier.head); >> >> if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) >> vfio_iommu_unmap_unpin_all(iommu); >> @@ -2491,7 +2476,8 @@ static void vfio_iommu_type1_release(void *iommu_data) >> >> if (iommu->external_domain) { >> vfio_release_domain(iommu->external_domain, true); >> - vfio_sanity_check_pfn_list(iommu); >> + /* mdev vendor driver must unregister notifier */ >> + WARN_ON(iommu->notifier.head); >> kfree(iommu->external_domain); >> } >> > > . >
On Mon, 18 Jan 2021 21:16:08 +0800 Keqian Zhu <zhukeqian1@huawei.com> wrote: > On 2021/1/16 3:14, Alex Williamson wrote: > > On Fri, 15 Jan 2021 17:26:43 +0800 > > Keqian Zhu <zhukeqian1@huawei.com> wrote: > > > >> vfio_sanity_check_pfn_list() is used to check whether pfn_list of > >> vfio_dma is empty when remove the external domain, so it makes a > >> wrong assumption that only external domain will add pfn to dma pfn_list. > >> > >> Now we apply this check when remove a specific vfio_dma and extract > >> the notifier check just for external domain. > > > > The page pinning interface is gated by having a notifier registered for > > unmaps, therefore non-external domains would also need to register a > > notifier. There's currently no other way to add entries to the > > pfn_list. So if we allow pinning for such domains, then it's wrong to > > WARN_ON() when the notifier list is not-empty when removing an external > > domain. Long term we should probably extend page {un}pinning for the > > caller to pass their notifier to be validated against the notifier list > > rather than just allowing page pinning if *any* notifier is registered. > > Thanks, > I was misled by the code comments. So when the commit a54eb55045ae is > added, the only user of pin interface is mdev vendor driver, but now > we also allow iommu backed group to use this interface to constraint > dirty scope. Is vfio_iommu_unmap_unpin_all() a proper place to put > this WARN()? vfio_iommu_unmap_unpin_all() deals with removing vfio_dmas, it's logically unrelated to whether any driver is registered to receive unmap notifications. Thanks, Alex
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 4e82b9a3440f..a9bc15e84a4e 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -958,6 +958,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma) { + WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list); vfio_unmap_unpin(iommu, dma, true); vfio_unlink_dma(iommu, dma); put_task_struct(dma->task); @@ -2251,23 +2252,6 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) } } -static void vfio_sanity_check_pfn_list(struct vfio_iommu *iommu) -{ - struct rb_node *n; - - n = rb_first(&iommu->dma_list); - for (; n; n = rb_next(n)) { - struct vfio_dma *dma; - - dma = rb_entry(n, struct vfio_dma, node); - - if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list))) - break; - } - /* mdev vendor driver must unregister notifier */ - WARN_ON(iommu->notifier.head); -} - /* * Called when a domain is removed in detach. It is possible that * the removed domain decided the iova aperture window. Modify the @@ -2367,7 +2351,8 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, kfree(group); if (list_empty(&iommu->external_domain->group_list)) { - vfio_sanity_check_pfn_list(iommu); + /* mdev vendor driver must unregister notifier */ + WARN_ON(iommu->notifier.head); if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) vfio_iommu_unmap_unpin_all(iommu); @@ -2491,7 +2476,8 @@ static void vfio_iommu_type1_release(void *iommu_data) if (iommu->external_domain) { vfio_release_domain(iommu->external_domain, true); - vfio_sanity_check_pfn_list(iommu); + /* mdev vendor driver must unregister notifier */ + WARN_ON(iommu->notifier.head); kfree(iommu->external_domain); }
vfio_sanity_check_pfn_list() is used to check whether pfn_list of vfio_dma is empty when remove the external domain, so it makes a wrong assumption that only external domain will add pfn to dma pfn_list. Now we apply this check when remove a specific vfio_dma and extract the notifier check just for external domain. Fixes: a54eb55045ae ("vfio iommu type1: Add support for mediated devices") Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> --- drivers/vfio/vfio_iommu_type1.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-)