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

[Steven Sistare <steven.sistare@oracle.com>]

On 12/20/2022 4:59 PM, Alex Williamson wrote:
> 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,

Will do - Steve