Re: [PATCH V6 2/7] vfio/type1: prevent underflow of locked_vm via exec()

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 20 Dec 2022 10:01:21 -0500
Steven Sistare <steven.sistare@oracle.com> wrote:

> On 12/19/2022 2:48 AM, Tian, Kevin wrote:
> >> From: Steve Sistare <steven.sistare@oracle.com>
> >> Sent: Saturday, December 17, 2022 2:51 AM
> >> @@ -1664,15 +1666,7 @@ static int vfio_dma_do_map(struct vfio_iommu
> >> *iommu,
> >>  	 * against the locked memory limit and we need to be able to do both
> >>  	 * outside of this call path as pinning can be asynchronous via the
> >>  	 * external interfaces for mdev devices.  RLIMIT_MEMLOCK requires
> >> a
> >> -	 * task_struct and VM locked pages requires an mm_struct, however
> >> -	 * holding an indefinite mm reference is not recommended,
> >> therefore we
> >> -	 * only hold a reference to a task.  We could hold a reference to
> >> -	 * current, however QEMU uses this call path through vCPU threads,
> >> -	 * which can be killed resulting in a NULL mm and failure in the
> >> unmap
> >> -	 * path when called via a different thread.  Avoid this problem by
> >> -	 * using the group_leader as threads within the same group require
> >> -	 * both CLONE_THREAD and CLONE_VM and will therefore use the
> >> same
> >> -	 * mm_struct.
> >> +	 * task_struct and VM locked pages requires an mm_struct.  
> > 
> > IMHO the rationale why choosing group_leader still applies...  
> 
> I don't see why it still applies.  With the new code, we may save a reference 
> to current or current->group_leader, without error.  "NULL mm and failure in the 
> unmap path" will not happen with mmgrab.  task->signal->rlimit is shared, so it 
> does not matter which task we use, or whether the task is dead, as long as 
> one of the tasks lives, which is guaranteed by the mmget_not_zero() guard.  Am
> I missing something?
> 
> I kept current->group_leader for ease of debugging, so that all dma's are owned 
> by the same task.

That much at least would be a good comment to add since the above
removes all justification for why we're storing group_leader as the
task rather than current.  Ex:

	QEMU typically calls this path through vCPU threads, which can
	terminate due to vCPU hotplug, therefore historically we've used
	group_leader for task tracking.  With the addition of grabbing
	the mm for the life of the DMA tracking structure, this is not
	so much a concern, but we continue to use group_leader for
	debug'ability, ie. all DMA tracking is owned by the same task.

Given the upcoming holidays and reviewers starting to disappear, I
suggest we take this up as a fixes series for v6.2 after the new year.
The remainder of the vfio next branch for v6.2 has already been merged.
Thanks,

Alex