On Tue, Dec 20, 2022 at 12:39:20PM -0800, Steve Sistare wrote:
> When a vfio container is preserved across exec, the task does not change,
> but it gets a new mm with locked_vm=0, and loses the count from existing
> dma mappings. If the user later unmaps a dma mapping, locked_vm underflows
> to a large unsigned value, and a subsequent dma map request fails with
> ENOMEM in __account_locked_vm.
>
> To avoid underflow, grab and save the mm at the time a dma is mapped.
> Use that mm when adjusting locked_vm, rather than re-acquiring the saved
> task's mm, which may have changed. If the saved mm is dead, do nothing.
>
> locked_vm is incremented for existing mappings in a subsequent patch.
>
> Fixes: 73fa0d10d077 ("vfio: Type1 IOMMU implementation")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Steve Sistare <steven.sistare@xxxxxxxxxx>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
> drivers/vfio/vfio_iommu_type1.c | 27 +++++++++++----------------
> 1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 144f5bb..71f980b 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -100,6 +100,7 @@ struct vfio_dma {
> struct task_struct *task;
> struct rb_root pfn_list; /* Ex-user pinned pfn list */
> unsigned long *bitmap;
> + struct mm_struct *mm;
> };
>
> struct vfio_batch {
> @@ -420,8 +421,8 @@ static int vfio_lock_acct(struct vfio_dma *dma, long npage, bool async)
> if (!npage)
> return 0;
>
> - mm = async ? get_task_mm(dma->task) : dma->task->mm;
> - if (!mm)
> + mm = dma->mm;
> + if (async && !mmget_not_zero(mm))
> return -ESRCH; /* process exited */
Just delete the async, the lock_acct always acts on the dma which
always has a singular mm.
FIx the few callers that need it to do the mmget_no_zero() before
calling in.
> @@ -794,8 +795,8 @@ static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr,
> struct mm_struct *mm;
> int ret;
>
> - mm = get_task_mm(dma->task);
> - if (!mm)
> + mm = dma->mm;
> + if (!mmget_not_zero(mm))
> return -ENODEV;
eg things get all confused here where we have the mmget_not_zero
But then we go ahead and call vfio_lock_acct() with true
Jason
