Re: [PATCH 09/17] mm: export access_remote_vm() symbol

[Alistair Popple <apopple@nvidia.com>]

"Yu, Fenghua" <fenghua.yu@intel.com> 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