Hi, Alistair and Lorenzo,
> >> > 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().
That's right. IDXD driver calls iommu_sva_find() which holds mm reference before
access_remote_vm(mm) and puts the count after.
And comment of access_remote_vm() explicitly says "The caller must hold a reference on @mm.".
IDXD follows the mm reference policy. There is no need to have a different wrapper.
So the current patch is good without any change, right?
> 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?
Currently access_remove_vm() is called only once in IDXD. And the calling code
is clearly to have mmget() and mmput() already. The proposed
wrapper iommu_access_pasid() may not be very useful. We may add the wrapper
in the future if there are more usages. Is that OK?
Thanks.
-Fenghua
