| Message ID | 1671053097-138498-1-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fixes for virtual address update | expand |
On Wed, 14 Dec 2022 13:24:52 -0800 Steve Sistare <steven.sistare@oracle.com> wrote: > Fix bugs in the interfaces that allow the underlying memory object of an > iova range to be mapped in a new address space. They allow userland to > indefinitely block vfio mediated device kernel threads, and do not > propagate the locked_vm count to a new mm. > > The fixes impose restrictions that eliminate waiting conditions, so > revert the dead code: > commit 898b9eaeb3fe ("vfio/type1: block on invalid vaddr") > commit 487ace134053 ("vfio/type1: implement notify callback") > commit ec5e32940cc9 ("vfio: iommu driver notify callback") > > Changes in V2 (thanks Alex): > * do not allow group attach while vaddrs are invalid > * add patches to delete dead code > * add WARN_ON for never-should-happen conditions > * check for changed mm in unmap. > * check for vfio_lock_acct failure in remap > > Changes in V3 (ditto!): > * return errno at WARN_ON sites, and make it unique > * correctly check for dma task mm change > * change dma owner to current when vaddr is updated > * add Fixes to commit messages > * refactored new code in vfio_dma_do_map > > Changes in V4: > * misc cosmetic changes > > Steve Sistare (5): > vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR > vfio/type1: prevent locked_vm underflow > vfio/type1: revert "block on invalid vaddr" > vfio/type1: revert "implement notify callback" > vfio: revert "iommu driver notify callback" > > drivers/vfio/container.c | 5 - > drivers/vfio/vfio.h | 7 -- > drivers/vfio/vfio_iommu_type1.c | 199 +++++++++++++++++++--------------------- > include/uapi/linux/vfio.h | 15 +-- > 4 files changed, 103 insertions(+), 123 deletions(-) > Looks ok to me. Jason, Kevin, I'd appreciate your reviews regarding whether this sufficiently addresses the outstanding issues to keep this interface around until we have a re-implementation in iommufd. Thanks, Alex
On Wed, Dec 14, 2022 at 02:42:29PM -0700, Alex Williamson wrote: > On Wed, 14 Dec 2022 13:24:52 -0800 > Steve Sistare <steven.sistare@oracle.com> wrote: > > > Fix bugs in the interfaces that allow the underlying memory object of an > > iova range to be mapped in a new address space. They allow userland to > > indefinitely block vfio mediated device kernel threads, and do not > > propagate the locked_vm count to a new mm. > > > > The fixes impose restrictions that eliminate waiting conditions, so > > revert the dead code: > > commit 898b9eaeb3fe ("vfio/type1: block on invalid vaddr") > > commit 487ace134053 ("vfio/type1: implement notify callback") > > commit ec5e32940cc9 ("vfio: iommu driver notify callback") > > > > Changes in V2 (thanks Alex): > > * do not allow group attach while vaddrs are invalid > > * add patches to delete dead code > > * add WARN_ON for never-should-happen conditions > > * check for changed mm in unmap. > > * check for vfio_lock_acct failure in remap > > > > Changes in V3 (ditto!): > > * return errno at WARN_ON sites, and make it unique > > * correctly check for dma task mm change > > * change dma owner to current when vaddr is updated > > * add Fixes to commit messages > > * refactored new code in vfio_dma_do_map > > > > Changes in V4: > > * misc cosmetic changes > > > > Steve Sistare (5): > > vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR > > vfio/type1: prevent locked_vm underflow > > vfio/type1: revert "block on invalid vaddr" > > vfio/type1: revert "implement notify callback" > > vfio: revert "iommu driver notify callback" > > > > drivers/vfio/container.c | 5 - > > drivers/vfio/vfio.h | 7 -- > > drivers/vfio/vfio_iommu_type1.c | 199 +++++++++++++++++++--------------------- > > include/uapi/linux/vfio.h | 15 +-- > > 4 files changed, 103 insertions(+), 123 deletions(-) > > > > Looks ok to me. Jason, Kevin, I'd appreciate your reviews regarding > whether this sufficiently addresses the outstanding issues to keep this > interface around until we have a re-implementation in iommufd. Thanks, Well, patch 3 undeniably solves the deadlock problem. I still don't like that we are keeping this, an interface that doesn't support mdevs has no future and can't really be used in any general purpose way. Can we at least protect it with a kconfig && CONFIG_EXPERIMENTAL so it is disabled in most builds? Jason
On Thu, 15 Dec 2022 10:25:11 -0400 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Wed, Dec 14, 2022 at 02:42:29PM -0700, Alex Williamson wrote: > > On Wed, 14 Dec 2022 13:24:52 -0800 > > Steve Sistare <steven.sistare@oracle.com> wrote: > > > > > Fix bugs in the interfaces that allow the underlying memory object of an > > > iova range to be mapped in a new address space. They allow userland to > > > indefinitely block vfio mediated device kernel threads, and do not > > > propagate the locked_vm count to a new mm. > > > > > > The fixes impose restrictions that eliminate waiting conditions, so > > > revert the dead code: > > > commit 898b9eaeb3fe ("vfio/type1: block on invalid vaddr") > > > commit 487ace134053 ("vfio/type1: implement notify callback") > > > commit ec5e32940cc9 ("vfio: iommu driver notify callback") > > > > > > Changes in V2 (thanks Alex): > > > * do not allow group attach while vaddrs are invalid > > > * add patches to delete dead code > > > * add WARN_ON for never-should-happen conditions > > > * check for changed mm in unmap. > > > * check for vfio_lock_acct failure in remap > > > > > > Changes in V3 (ditto!): > > > * return errno at WARN_ON sites, and make it unique > > > * correctly check for dma task mm change > > > * change dma owner to current when vaddr is updated > > > * add Fixes to commit messages > > > * refactored new code in vfio_dma_do_map > > > > > > Changes in V4: > > > * misc cosmetic changes > > > > > > Steve Sistare (5): > > > vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR > > > vfio/type1: prevent locked_vm underflow > > > vfio/type1: revert "block on invalid vaddr" > > > vfio/type1: revert "implement notify callback" > > > vfio: revert "iommu driver notify callback" > > > > > > drivers/vfio/container.c | 5 - > > > drivers/vfio/vfio.h | 7 -- > > > drivers/vfio/vfio_iommu_type1.c | 199 +++++++++++++++++++--------------------- > > > include/uapi/linux/vfio.h | 15 +-- > > > 4 files changed, 103 insertions(+), 123 deletions(-) > > > > > > > Looks ok to me. Jason, Kevin, I'd appreciate your reviews regarding > > whether this sufficiently addresses the outstanding issues to keep this > > interface around until we have a re-implementation in iommufd. Thanks, > > Well, patch 3 undeniably solves the deadlock problem. > > I still don't like that we are keeping this, an interface that doesn't > support mdevs has no future and can't really be used in any general > purpose way. > > Can we at least protect it with a kconfig && CONFIG_EXPERIMENTAL so it > is disabled in most builds? We no longer have that tool, CONFIG_EXPERIMENTAL hasn't existed for almost a decade. I don't think anyone would argue this as a perfect solution, but at the same time we're putting Steve in a position where we want to remove the vulnerabilities, we're divesting development in type1 with the introduction of iommufd, and there's clearly some utility remaining in the interface, even without mdev support. It's not that different from migration in fact. In order to migrate a VM with vfio devices, all those devices need to support migration. In order to live update a VM, all the device need to support live update, where we have an implementation of generic support here for non-mdev devices. It looks like we need some more refinement of the patches, but I think there's enough here to show there's an interim, restricted use case solution short of reverting the feature entirely. Thanks, Alex
Fix bugs in the interfaces that allow the underlying memory object of an iova range to be mapped in a new address space. They allow userland to indefinitely block vfio mediated device kernel threads, and do not propagate the locked_vm count to a new mm. The fixes impose restrictions that eliminate waiting conditions, so revert the dead code: commit 898b9eaeb3fe ("vfio/type1: block on invalid vaddr") commit 487ace134053 ("vfio/type1: implement notify callback") commit ec5e32940cc9 ("vfio: iommu driver notify callback") Changes in V2 (thanks Alex): * do not allow group attach while vaddrs are invalid * add patches to delete dead code * add WARN_ON for never-should-happen conditions * check for changed mm in unmap. * check for vfio_lock_acct failure in remap Changes in V3 (ditto!): * return errno at WARN_ON sites, and make it unique * correctly check for dma task mm change * change dma owner to current when vaddr is updated * add Fixes to commit messages * refactored new code in vfio_dma_do_map Changes in V4: * misc cosmetic changes Steve Sistare (5): vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR vfio/type1: prevent locked_vm underflow vfio/type1: revert "block on invalid vaddr" vfio/type1: revert "implement notify callback" vfio: revert "iommu driver notify callback" drivers/vfio/container.c | 5 - drivers/vfio/vfio.h | 7 -- drivers/vfio/vfio_iommu_type1.c | 199 +++++++++++++++++++--------------------- include/uapi/linux/vfio.h | 15 +-- 4 files changed, 103 insertions(+), 123 deletions(-)