Lorenzo Stoakes <lstoakes@xxxxxxxxx> 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.
