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

[Alistair Popple <apopple@nvidia.com>]

Lorenzo Stoakes <lstoakes@gmail.com> writes:

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

I'm more familiar with the ARM side of things, but we can safely assume
we always have a valid mmgrab()/mm_count while the PASID is bound because
iommu_sva_bind_device() -> iommu_sva_domain_alloc() -> mmgrab().

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

Yeah, iommu_sva_find() does take care of that though:

 * On success a reference to the mm is taken, and must be released with mmput().

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

Oh that makes sense.

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

Ok, although I think making this an iommu specific wrapper taking a
PASID rather than mm_struct would make the API more specific and less
likely to be misused as the mm_count/users lifetime issues could be
dealt with inside the core IOMMU code.

> 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

Seems reasonable to me at least, but I don't have any strong opinions as
I only noticed this thread while trying to catch up on IOMMU
developments.