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

["Yu, Fenghua" <fenghua.yu@intel.com>]

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