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

[Lorenzo Stoakes <lstoakes@gmail.com>]

On Wed, Jan 04, 2023 at 05:12:31PM +1100, Alistair Popple wrote:
> 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'm not familiar with the iommu code, but a brief glance suggests that no
mmgrab() is being performed for intel devices? I may have missed something
though.

We do need to be absolutely sure the mm sticks around (hence the
grab) but if we need userland mappings that we have to have a subsequent
mmget_not_zero().

> >> 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().

get_user_pages_remote() differs in that, most importantly, it requires the
mm_lock is held on invocation (implying that the mm will stick around), which is
not the case for access_remote_vm() (as __access_remote_vm() subsequently
obtains it), but also in that it pins pages but doesn't copy things to a buffer
(rather returning VMAs or struct page objects).

Also note the comment around get_user_pages_remote() saying nobody should be
using it due to lack of FAULT_FLAG_ALLOW_RETRY handling :) yes it feels like GUP
is a bit of a mess.

> 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?

My concern is exposing something highly delicate _which accesses remote mas a public API with implicit
assumptions whose one and only (core kernel) user treats with enormous
caution. Even if this iommu code were to use it correctly, we'd end up with an
interface which could be subject to real risks which other drivers may misuse.

Another question is - why can't we:-

1. mmgrab() [or safely assume we already have a reference] + mmget_not_zero()
2. acquire mm read lock to stop VMAs disappearing underneath us and pin pages with get_user_pages_remote()
3. copy what we need using e.g. copy_from_user()/copy_to_user()
4. unwind