On 12/20/2022 4:59 PM, Alex Williamson wrote: > On Tue, 20 Dec 2022 10:01:21 -0500 > Steven Sistare <steven.sistare@xxxxxxxxxx> wrote: > >> On 12/19/2022 2:48 AM, Tian, Kevin wrote: >>>> From: Steve Sistare <steven.sistare@xxxxxxxxxx> >>>> 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
