mbox series

[V8,0/7] fixes for virtual address update

Message ID 1675184289-267876-1-git-send-email-steven.sistare@oracle.com (mailing list archive)
Headers show
Series fixes for virtual address update | expand

Message

Steven Sistare Jan. 31, 2023, 4:58 p.m. UTC
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.  Also fix a pre-existing bug
that allows locked_vm underflow.

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

Changes in V5 (thanks Jason and Kevin):
  * grab mm and use it for locked_vm accounting
  * separate patches for underflow and restoring locked_vm
  * account for reserved pages
  * improve error messages

Changes in V6:
  * drop "count reserved pages" patch
  * add "track locked_vm" patch
  * grab current->mm not group_leader->mm
  * simplify vfio_change_dma_owner
  * fix commit messages

Changes in v7:
  * compare current->mm not group_leader->mm (missed one)
  * misc cosmetic changes

Changes in v8:
  * updated group_leader comment in vfio_dma_do_map
  * delete async arg from mm_lock_acct
  * pass async=false to vfio_lock_acct in vfio_pin_page_external
  * locked_vm becomes size_t
  * improved commit message in "restore locked_vm"
  * simplified flow in vfio_change_dma_owner
  * rebase to v6.2-rc6

Steve Sistare (7):
  vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR
  vfio/type1: prevent underflow of locked_vm via exec()
  vfio/type1: track locked_vm per dma
  vfio/type1: restore locked_vm
  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 | 248 ++++++++++++++++++----------------------
 include/uapi/linux/vfio.h       |  15 ++-
 4 files changed, 120 insertions(+), 155 deletions(-)

Comments

Alex Williamson Feb. 2, 2023, 9:25 p.m. UTC | #1
On Tue, 31 Jan 2023 08:58:02 -0800
Steve Sistare <steven.sistare@oracle.com> wrote:
> Changes in v8:
>   * updated group_leader comment in vfio_dma_do_map
>   * delete async arg from mm_lock_acct
>   * pass async=false to vfio_lock_acct in vfio_pin_page_external
>   * locked_vm becomes size_t
>   * improved commit message in "restore locked_vm"
>   * simplified flow in vfio_change_dma_owner
>   * rebase to v6.2-rc6
> 
> Steve Sistare (7):
>   vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR
>   vfio/type1: prevent underflow of locked_vm via exec()
>   vfio/type1: track locked_vm per dma
>   vfio/type1: restore locked_vm
>   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 | 248 ++++++++++++++++++----------------------
>  include/uapi/linux/vfio.h       |  15 ++-
>  4 files changed, 120 insertions(+), 155 deletions(-)

LGTM, I'll pause to give Jason a chance to review as well.  Thanks for
working on this!

Alex
Jason Gunthorpe Feb. 3, 2023, 2:35 p.m. UTC | #2
On Tue, Jan 31, 2023 at 08:58:02AM -0800, Steve Sistare 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.  Also fix a pre-existing bug
> that allows locked_vm underflow.
> 
> 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")

I would still rather we delete this API. Something that doesn't work
with iommufd, and doesn't work with mdevs doesn't seem like it should
be in the kernel.

But it is up to Alex, and the code looks fine:

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason
Alex Williamson Feb. 9, 2023, 9:03 p.m. UTC | #3
On Tue, 31 Jan 2023 08:58:02 -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.  Also fix a pre-existing bug
> that allows locked_vm underflow.
> 
> 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
> 
> Changes in V5 (thanks Jason and Kevin):
>   * grab mm and use it for locked_vm accounting
>   * separate patches for underflow and restoring locked_vm
>   * account for reserved pages
>   * improve error messages
> 
> Changes in V6:
>   * drop "count reserved pages" patch
>   * add "track locked_vm" patch
>   * grab current->mm not group_leader->mm
>   * simplify vfio_change_dma_owner
>   * fix commit messages
> 
> Changes in v7:
>   * compare current->mm not group_leader->mm (missed one)
>   * misc cosmetic changes
> 
> Changes in v8:
>   * updated group_leader comment in vfio_dma_do_map
>   * delete async arg from mm_lock_acct
>   * pass async=false to vfio_lock_acct in vfio_pin_page_external
>   * locked_vm becomes size_t
>   * improved commit message in "restore locked_vm"
>   * simplified flow in vfio_change_dma_owner
>   * rebase to v6.2-rc6
> 
> Steve Sistare (7):
>   vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR
>   vfio/type1: prevent underflow of locked_vm via exec()
>   vfio/type1: track locked_vm per dma
>   vfio/type1: restore locked_vm
>   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 | 248 ++++++++++++++++++----------------------
>  include/uapi/linux/vfio.h       |  15 ++-
>  4 files changed, 120 insertions(+), 155 deletions(-)
> 

Applied to vfio next branch for v6.3.  Thanks,

Alex