| Message ID | 20230510141932.413348-3-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/xe: Fix unprotected rebind_list accesses | expand |
On Wed, May 10, 2023 at 04:19:32PM +0200, Thomas Hellström wrote: > If a vma was destroyed with the bo evicted, it might happen that we forget > to remove it from the notifer::rebind_list. Fix to make sure that really > happens. > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/xe/xe_vm.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c > index 5f93d78c2e58..f54b3b7566c9 100644 > --- a/drivers/gpu/drm/xe/xe_vm.c > +++ b/drivers/gpu/drm/xe/xe_vm.c > @@ -978,6 +978,15 @@ static void xe_vma_destroy(struct xe_vma *vma, struct dma_fence *fence) > } else { > xe_bo_assert_held(vma->bo); > list_del(&vma->bo_link); > + /* > + * TODO: We can do an advisory check for list link empty here, > + * if this lock becomes too costly. Nobody can re-add to the > + * bo to the vm::notifier::rebind_list at this point since we > + * have the bo lock. > + */ IMO grab isn't a big deal, not sure this is worth such a lengthly comment. > + spin_lock(&vm->notifier.list_lock); > + list_del(&vma->notifier.rebind_link); Can you safe call list_del on an empty list? I thought that call blows up hence we have a bunch of if (!list_empty()) checks before calling list_del all over the driver. Matt > + spin_unlock(&vm->notifier.list_lock); > if (!vma->bo->vm) > vm_remove_extobj(vma); > } > -- > 2.39.2 >
On 5/11/23 16:54, Matthew Brost wrote: > On Wed, May 10, 2023 at 04:19:32PM +0200, Thomas Hellström wrote: >> If a vma was destroyed with the bo evicted, it might happen that we forget >> to remove it from the notifer::rebind_list. Fix to make sure that really >> happens. >> >> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> --- >> drivers/gpu/drm/xe/xe_vm.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c >> index 5f93d78c2e58..f54b3b7566c9 100644 >> --- a/drivers/gpu/drm/xe/xe_vm.c >> +++ b/drivers/gpu/drm/xe/xe_vm.c >> @@ -978,6 +978,15 @@ static void xe_vma_destroy(struct xe_vma *vma, struct dma_fence *fence) >> } else { >> xe_bo_assert_held(vma->bo); >> list_del(&vma->bo_link); >> + /* >> + * TODO: We can do an advisory check for list link empty here, >> + * if this lock becomes too costly. Nobody can re-add to the >> + * bo to the vm::notifier::rebind_list at this point since we >> + * have the bo lock. >> + */ > IMO grab isn't a big deal, not sure this is worth such a lengthly comment. Ok, I'll remove it. > >> + spin_lock(&vm->notifier.list_lock); >> + list_del(&vma->notifier.rebind_link); > Can you safe call list_del on an empty list? I thought that call blows > up hence we have a bunch of if (!list_empty()) checks before calling > list_del all over the driver. Good question. Looking at the implementation it definitely looks possible, and I have LIST_DEBUG turned on when testing, so I assume it would have blown up otherwise. /Thomas > > Matt > >> + spin_unlock(&vm->notifier.list_lock); >> if (!vma->bo->vm) >> vm_remove_extobj(vma); >> } >> -- >> 2.39.2 >>
On Thu, May 11, 2023 at 05:38:11PM +0200, Thomas Hellström wrote: > > On 5/11/23 16:54, Matthew Brost wrote: > > On Wed, May 10, 2023 at 04:19:32PM +0200, Thomas Hellström wrote: > > > If a vma was destroyed with the bo evicted, it might happen that we forget > > > to remove it from the notifer::rebind_list. Fix to make sure that really > > > happens. > > > > > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > > --- > > > drivers/gpu/drm/xe/xe_vm.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c > > > index 5f93d78c2e58..f54b3b7566c9 100644 > > > --- a/drivers/gpu/drm/xe/xe_vm.c > > > +++ b/drivers/gpu/drm/xe/xe_vm.c > > > @@ -978,6 +978,15 @@ static void xe_vma_destroy(struct xe_vma *vma, struct dma_fence *fence) > > > } else { > > > xe_bo_assert_held(vma->bo); > > > list_del(&vma->bo_link); > > > + /* > > > + * TODO: We can do an advisory check for list link empty here, > > > + * if this lock becomes too costly. Nobody can re-add to the > > > + * bo to the vm::notifier::rebind_list at this point since we > > > + * have the bo lock. > > > + */ > > IMO grab isn't a big deal, not sure this is worth such a lengthly comment. > > Ok, I'll remove it. > > > > > > > + spin_lock(&vm->notifier.list_lock); > > > + list_del(&vma->notifier.rebind_link); > > Can you safe call list_del on an empty list? I thought that call blows > > up hence we have a bunch of if (!list_empty()) checks before calling > > list_del all over the driver. > > Good question. Looking at the implementation it definitely looks possible, > and I have LIST_DEBUG turned on when testing, so I assume it would have > blown up otherwise. > It looks like 2 deletes will blow up but delete on a empty list is fine. With that: Reviewed-by: Matthew Brost <matthew.brost@intel.com> > /Thomas > > > > > > Matt > > > > > + spin_unlock(&vm->notifier.list_lock); > > > if (!vma->bo->vm) > > > vm_remove_extobj(vma); > > > } > > > -- > > > 2.39.2 > > >
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index 5f93d78c2e58..f54b3b7566c9 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -978,6 +978,15 @@ static void xe_vma_destroy(struct xe_vma *vma, struct dma_fence *fence) } else { xe_bo_assert_held(vma->bo); list_del(&vma->bo_link); + /* + * TODO: We can do an advisory check for list link empty here, + * if this lock becomes too costly. Nobody can re-add to the + * bo to the vm::notifier::rebind_list at this point since we + * have the bo lock. + */ + spin_lock(&vm->notifier.list_lock); + list_del(&vma->notifier.rebind_link); + spin_unlock(&vm->notifier.list_lock); if (!vma->bo->vm) vm_remove_extobj(vma); }
If a vma was destroyed with the bo evicted, it might happen that we forget to remove it from the notifer::rebind_list. Fix to make sure that really happens. Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/xe/xe_vm.c | 9 +++++++++ 1 file changed, 9 insertions(+)