"Yu, Fenghua" <fenghua.yu@xxxxxxxxx> writes:
> Hi, Lorenzo,
>
>> Hey Fenghua :)
>>
>> > access_remote_vm(mm) directly call __access_remote_vm(mm).
>> > access_process_vm(tsk) calls mm=get_task_mm() then
>> __access_remote_vm(mm).
>> >
>> > So instead of access_remote_vm(mm), it's access_process_vm(tsk) that
>> > holds a reference count on the mm, right?
>>
>> Indeed!
>>
>> >
>> > > >
>> > > > Is there a reason you can't use access_process_vm() which is
>> > > > exported and additionally handles the refrencing?
>> >
>> > IDXD interrupt handler starts a work which needs to access remote vm.
>> > The remote mm is found by PASID which is saved in device event log.
>> >
>> > In the work, it's hard to get the remote mm from a task because
>> > mm->owner could be NULL but the mm is still existing.
>>
>> That makes sense, however I do feel nervous about exporting something that
>> that relies on this reference.
>>
>> The issue is ensuring that the mm can't be taken from underneath you, the only
>> user of access_remote_vm(), procfs, does a careful dance using get_task_mm()
>> and
>> mm_access() to ensure this can't happen, if _sometimes_ the remote mm might
>> have an owner and _sometimes_ not it feels like any exported function needs to
>> be equally careful?
I think the point is the remote mm should be valid as long as the PASID
is valid because it doesn't make sense to have a PASID without
associated memory map. iommu_sva_find() does mmget_not_zero() to ensure
that.
Obviously something must still be holding a mmgrab() though. That should
happen as part of the PASID allocation done by iommu_sva_bind_device().
>> I definitely don't feel as if simply exporting this is a safe option, and you would in
>> that case need a new function that handles different scenarios of mm
>> ownership/not.
Note this isn't that different from get_user_pages_remote().
>> I may be missing something here and I will wait for others to chime in but I think
>> we would definitely need something more than simply exporting this.
>
> I may define and export a new wrapper access_remote_vm_ref() which will hold
> mm's reference count before accessing it:
> int access_remote_vm_ref(mm)
> {
> int ret;
>
> if (mm == &init_mm)
> return 0;
>
> mmget(mm);
> ret = access_remote_vm(mm);
> mmput(mm);
>
> return ret;
> }
> EXPORT_SYMBOL_GPL(access_remote_vm_ref);
>
> IDXD or any driver calls this and holds mm reference count while accesses the mm.
> This is useful for caller to directly access mm even if mm's owner could be NULL.
I'm not sure that helps much. A driver would still need to hold a
mm_count to ensure the struct_mm itself can't go away anyway so it may
as well do the mmget() IMHO (although it really should be
mmget_not_zero()).
In any case though iommu_sva_find() already takes care of doing
mmget_not_zero(). I wonder if it makes more sense to define a wrapper
(eg. iommu_access_pasid) that takes a PASID and does the mm
lookup/access_vm/mmput?
> Do you think this is sufficient to take care of the mm reference and is a good way to go?
>
> Thanks.
>
> -Fenghua
