| Message ID | 017804b2198a906463d634f84777b6087c9b4a40.1553093421.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: untag user pointers passed to the kernel | expand |
On Wed, Mar 20, 2019 at 03:51:28PM +0100, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > amdgpu_ttm_tt_get_user_pages() uses provided user pointers for vma > lookups, which can only by done with untagged pointers. > > Untag user pointers in this function. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 73e71e61dc99..891b027fa33b 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -751,10 +751,11 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages) > * check that we only use anonymous memory to prevent problems > * with writeback > */ > - unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE; > + unsigned long userptr = untagged_addr(gtt->userptr); > + unsigned long end = userptr + ttm->num_pages * PAGE_SIZE; > struct vm_area_struct *vma; > > - vma = find_vma(mm, gtt->userptr); > + vma = find_vma(mm, userptr); > if (!vma || vma->vm_file || vma->vm_end < end) { > up_read(&mm->mmap_sem); > return -EPERM; I tried to track this down but I failed to see whether user could provide an tagged pointer here (under the restrictions as per Vincenzo's ABI document).
On 22/03/2019 15:59, Catalin Marinas wrote: > On Wed, Mar 20, 2019 at 03:51:28PM +0100, Andrey Konovalov wrote: >> This patch is a part of a series that extends arm64 kernel ABI to allow to >> pass tagged user pointers (with the top byte set to something else other >> than 0x00) as syscall arguments. >> >> amdgpu_ttm_tt_get_user_pages() uses provided user pointers for vma >> lookups, which can only by done with untagged pointers. >> >> Untag user pointers in this function. >> >> Signed-off-by: Andrey Konovalov <andreyknvl@google.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> index 73e71e61dc99..891b027fa33b 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> @@ -751,10 +751,11 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages) >> * check that we only use anonymous memory to prevent problems >> * with writeback >> */ >> - unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE; >> + unsigned long userptr = untagged_addr(gtt->userptr); >> + unsigned long end = userptr + ttm->num_pages * PAGE_SIZE; >> struct vm_area_struct *vma; >> >> - vma = find_vma(mm, gtt->userptr); >> + vma = find_vma(mm, userptr); >> if (!vma || vma->vm_file || vma->vm_end < end) { >> up_read(&mm->mmap_sem); >> return -EPERM; > I tried to track this down but I failed to see whether user could > provide an tagged pointer here (under the restrictions as per Vincenzo's > ABI document). ->userptr is set by radeon_ttm_tt_set_userptr(), itself called from radeon_gem_userptr_ioctl(). Any page-aligned value is allowed. Kevin
On 2019-03-20 10:51 a.m., Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > amdgpu_ttm_tt_get_user_pages() uses provided user pointers for vma > lookups, which can only by done with untagged pointers. > > Untag user pointers in this function. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 73e71e61dc99..891b027fa33b 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -751,10 +751,11 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages) > * check that we only use anonymous memory to prevent problems > * with writeback > */ > - unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE; > + unsigned long userptr = untagged_addr(gtt->userptr); > + unsigned long end = userptr + ttm->num_pages * PAGE_SIZE; > struct vm_area_struct *vma; > > - vma = find_vma(mm, gtt->userptr); > + vma = find_vma(mm, userptr); > if (!vma || vma->vm_file || vma->vm_end < end) { > up_read(&mm->mmap_sem); > return -EPERM; We'll need to be careful that we don't break your change when the following commit gets applied through drm-next for Linux 5.2: https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next-5.2-wip&id=915d3eecfa23693bac9e54cdacf84fb4efdcc5c4 Would it make sense to apply the untagging in amdgpu_ttm_tt_set_userptr instead? That would avoid this conflict and I think it would clearly put the untagging into the user mode code path where the tagged pointer originates. In amdgpu_gem_userptr_ioctl and amdgpu_amdkfd_gpuvm.c (init_user_pages) we also set up an MMU notifier with the (tagged) pointer from user mode. That should probably also use the untagged address so that MMU notifiers for the untagged address get correctly matched up with the right BO. I'd move the untagging further up the call stack to cover that. For the GEM case I think amdgpu_gem_userptr_ioctl would be the right place. For the KFD case, I'd do this in amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu. Regards, Felix
On Mon, Mar 25, 2019 at 11:21 PM Kuehling, Felix <Felix.Kuehling@amd.com> wrote: > > On 2019-03-20 10:51 a.m., Andrey Konovalov wrote: > > This patch is a part of a series that extends arm64 kernel ABI to allow to > > pass tagged user pointers (with the top byte set to something else other > > than 0x00) as syscall arguments. > > > > amdgpu_ttm_tt_get_user_pages() uses provided user pointers for vma > > lookups, which can only by done with untagged pointers. > > > > Untag user pointers in this function. > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > > index 73e71e61dc99..891b027fa33b 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > > @@ -751,10 +751,11 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages) > > * check that we only use anonymous memory to prevent problems > > * with writeback > > */ > > - unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE; > > + unsigned long userptr = untagged_addr(gtt->userptr); > > + unsigned long end = userptr + ttm->num_pages * PAGE_SIZE; > > struct vm_area_struct *vma; > > > > - vma = find_vma(mm, gtt->userptr); > > + vma = find_vma(mm, userptr); > > if (!vma || vma->vm_file || vma->vm_end < end) { > > up_read(&mm->mmap_sem); > > return -EPERM; > > We'll need to be careful that we don't break your change when the > following commit gets applied through drm-next for Linux 5.2: > > https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next-5.2-wip&id=915d3eecfa23693bac9e54cdacf84fb4efdcc5c4 > > Would it make sense to apply the untagging in amdgpu_ttm_tt_set_userptr > instead? That would avoid this conflict and I think it would clearly put > the untagging into the user mode code path where the tagged pointer > originates. > > In amdgpu_gem_userptr_ioctl and amdgpu_amdkfd_gpuvm.c (init_user_pages) > we also set up an MMU notifier with the (tagged) pointer from user mode. > That should probably also use the untagged address so that MMU notifiers > for the untagged address get correctly matched up with the right BO. I'd > move the untagging further up the call stack to cover that. For the GEM > case I think amdgpu_gem_userptr_ioctl would be the right place. For the > KFD case, I'd do this in amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu. Will do in v14, thanks a lot for looking at this! Is this applicable to the radeon driver (drivers/gpu/drm/radeon) as well? It seems to be using very similar structure. > > Regards, > Felix >
On 2019-04-02 10:37 a.m., Andrey Konovalov wrote: > On Mon, Mar 25, 2019 at 11:21 PM Kuehling, Felix <Felix.Kuehling@amd.com> wrote: >> On 2019-03-20 10:51 a.m., Andrey Konovalov wrote: >>> This patch is a part of a series that extends arm64 kernel ABI to allow to >>> pass tagged user pointers (with the top byte set to something else other >>> than 0x00) as syscall arguments. >>> >>> amdgpu_ttm_tt_get_user_pages() uses provided user pointers for vma >>> lookups, which can only by done with untagged pointers. >>> >>> Untag user pointers in this function. >>> >>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> index 73e71e61dc99..891b027fa33b 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> @@ -751,10 +751,11 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages) >>> * check that we only use anonymous memory to prevent problems >>> * with writeback >>> */ >>> - unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE; >>> + unsigned long userptr = untagged_addr(gtt->userptr); >>> + unsigned long end = userptr + ttm->num_pages * PAGE_SIZE; >>> struct vm_area_struct *vma; >>> >>> - vma = find_vma(mm, gtt->userptr); >>> + vma = find_vma(mm, userptr); >>> if (!vma || vma->vm_file || vma->vm_end < end) { >>> up_read(&mm->mmap_sem); >>> return -EPERM; >> We'll need to be careful that we don't break your change when the >> following commit gets applied through drm-next for Linux 5.2: >> >> https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next-5.2-wip&id=915d3eecfa23693bac9e54cdacf84fb4efdcc5c4 >> >> Would it make sense to apply the untagging in amdgpu_ttm_tt_set_userptr >> instead? That would avoid this conflict and I think it would clearly put >> the untagging into the user mode code path where the tagged pointer >> originates. >> >> In amdgpu_gem_userptr_ioctl and amdgpu_amdkfd_gpuvm.c (init_user_pages) >> we also set up an MMU notifier with the (tagged) pointer from user mode. >> That should probably also use the untagged address so that MMU notifiers >> for the untagged address get correctly matched up with the right BO. I'd >> move the untagging further up the call stack to cover that. For the GEM >> case I think amdgpu_gem_userptr_ioctl would be the right place. For the >> KFD case, I'd do this in amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu. > Will do in v14, thanks a lot for looking at this! > > Is this applicable to the radeon driver (drivers/gpu/drm/radeon) as > well? It seems to be using very similar structure. I think so. Radeon doesn't have the KFD bits any more. But the GEM interface and MMU notifier are very similar. Regards, Felix
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 73e71e61dc99..891b027fa33b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -751,10 +751,11 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages) * check that we only use anonymous memory to prevent problems * with writeback */ - unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE; + unsigned long userptr = untagged_addr(gtt->userptr); + unsigned long end = userptr + ttm->num_pages * PAGE_SIZE; struct vm_area_struct *vma; - vma = find_vma(mm, gtt->userptr); + vma = find_vma(mm, userptr); if (!vma || vma->vm_file || vma->vm_end < end) { up_read(&mm->mmap_sem); return -EPERM;
This patch is a part of a series that extends arm64 kernel ABI to allow to pass tagged user pointers (with the top byte set to something else other than 0x00) as syscall arguments. amdgpu_ttm_tt_get_user_pages() uses provided user pointers for vma lookups, which can only by done with untagged pointers. Untag user pointers in this function. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)