From: Rob Clark <[email protected]>
Adding the vm to the vm_xa table makes it visible to userspace, which
could try to race with us to close the vm. So we need to take our extra
reference before putting it in the table.
Signed-off-by: Rob Clark <[email protected]>
---
Note, you could list commit e1a7ab4fca0c ("drm/i915: Remove the vm open
count") as the "fixed" commit, but really the issue seems to go back
much further (with the fix needing some backporting in the process).
drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 6250de9b9196..e4b78ab4773b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1861,11 +1861,19 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv,
vm = ctx->vm;
GEM_BUG_ON(!vm);
+ /*
+ * Get a reference for the allocated handle. Once the handle is
+ * visible in the vm_xa table, userspace could try to close it
+ * from under our feet, so we need to hold the extra reference
+ * first.
+ */
+ i915_vm_get(vm);
+
err = xa_alloc(&file_priv->vm_xa, &id, vm, xa_limit_32b, GFP_KERNEL);
- if (err)
+ if (err) {
+ i915_vm_put(vm);
return err;
-
- i915_vm_get(vm);
+ }
GEM_BUG_ON(id == 0); /* reserved for invalid/unassigned ppgtt */
args->value = id;
--
2.38.1
+ some more people based on e1a7ab4fca0c
On 19/01/2023 17:32, Rob Clark wrote:
> From: Rob Clark <[email protected]>
>
> Adding the vm to the vm_xa table makes it visible to userspace, which
> could try to race with us to close the vm. So we need to take our extra
> reference before putting it in the table.
>
> Signed-off-by: Rob Clark <[email protected]>
> ---
> Note, you could list commit e1a7ab4fca0c ("drm/i915: Remove the vm open
> count") as the "fixed" commit, but really the issue seems to go back
> much further (with the fix needing some backporting in the process).
It would probably be rather essential to identify the correct Fixes: tag.
Since Thomas, Matt and Niranjana you were directly involved in the patch
which changed significantly how this works, perhaps there is something
still somewhat easily retrievable from your memory lanes to help with this?
Regards,
Tvrtko
>
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 6250de9b9196..e4b78ab4773b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1861,11 +1861,19 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv,
> vm = ctx->vm;
> GEM_BUG_ON(!vm);
>
> + /*
> + * Get a reference for the allocated handle. Once the handle is
> + * visible in the vm_xa table, userspace could try to close it
> + * from under our feet, so we need to hold the extra reference
> + * first.
> + */
> + i915_vm_get(vm);
> +
> err = xa_alloc(&file_priv->vm_xa, &id, vm, xa_limit_32b, GFP_KERNEL);
> - if (err)
> + if (err) {
> + i915_vm_put(vm);
> return err;
> -
> - i915_vm_get(vm);
> + }
>
> GEM_BUG_ON(id == 0); /* reserved for invalid/unassigned ppgtt */
> args->value = id;
On Mon, 23 Jan 2023 at 16:57, Tvrtko Ursulin
<[email protected]> wrote:
>
>
> + some more people based on e1a7ab4fca0c
>
> On 19/01/2023 17:32, Rob Clark wrote:
> > From: Rob Clark <[email protected]>
> >
> > Adding the vm to the vm_xa table makes it visible to userspace, which
> > could try to race with us to close the vm. So we need to take our extra
> > reference before putting it in the table.
> >
> > Signed-off-by: Rob Clark <[email protected]>
> > ---
> > Note, you could list commit e1a7ab4fca0c ("drm/i915: Remove the vm open
> > count") as the "fixed" commit, but really the issue seems to go back
> > much further (with the fix needing some backporting in the process).
>
> It would probably be rather essential to identify the correct Fixes: tag.
>
> Since Thomas, Matt and Niranjana you were directly involved in the patch
> which changed significantly how this works, perhaps there is something
> still somewhat easily retrievable from your memory lanes to help with this?
Sorry for the delay. Fix looks good to me,
Reviewed-by: Matthew Auld <[email protected]>
Looking at the git history, the fixes tag I think needs to be:
Fixes: 9ec8795e7d91 ("drm/i915: Drop __rcu from gem_context->vm")
Cc: <[email protected]> # v5.16+
>
> Regards,
>
> Tvrtko
>
> >
> > drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 6250de9b9196..e4b78ab4773b 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -1861,11 +1861,19 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv,
> > vm = ctx->vm;
> > GEM_BUG_ON(!vm);
> >
> > + /*
> > + * Get a reference for the allocated handle. Once the handle is
> > + * visible in the vm_xa table, userspace could try to close it
> > + * from under our feet, so we need to hold the extra reference
> > + * first.
> > + */
> > + i915_vm_get(vm);
> > +
> > err = xa_alloc(&file_priv->vm_xa, &id, vm, xa_limit_32b, GFP_KERNEL);
> > - if (err)
> > + if (err) {
> > + i915_vm_put(vm);
> > return err;
> > -
> > - i915_vm_get(vm);
> > + }
> >
> > GEM_BUG_ON(id == 0); /* reserved for invalid/unassigned ppgtt */
> > args->value = id;
On 27/01/2023 13:10, Matthew Auld wrote:
> On Mon, 23 Jan 2023 at 16:57, Tvrtko Ursulin
> <[email protected]> wrote:
>>
>>
>> + some more people based on e1a7ab4fca0c
>>
>> On 19/01/2023 17:32, Rob Clark wrote:
>>> From: Rob Clark <[email protected]>
>>>
>>> Adding the vm to the vm_xa table makes it visible to userspace, which
>>> could try to race with us to close the vm. So we need to take our extra
>>> reference before putting it in the table.
>>>
>>> Signed-off-by: Rob Clark <[email protected]>
>>> ---
>>> Note, you could list commit e1a7ab4fca0c ("drm/i915: Remove the vm open
>>> count") as the "fixed" commit, but really the issue seems to go back
>>> much further (with the fix needing some backporting in the process).
>>
>> It would probably be rather essential to identify the correct Fixes: tag.
>>
>> Since Thomas, Matt and Niranjana you were directly involved in the patch
>> which changed significantly how this works, perhaps there is something
>> still somewhat easily retrievable from your memory lanes to help with this?
>
> Sorry for the delay. Fix looks good to me,
> Reviewed-by: Matthew Auld <[email protected]>
>
> Looking at the git history, the fixes tag I think needs to be:
>
> Fixes: 9ec8795e7d91 ("drm/i915: Drop __rcu from gem_context->vm")
> Cc: <[email protected]> # v5.16+
As discussed offline this looks correct to me too. Thanks for looking
into it!
Since the CI was green I have now merged the patch. Thanks for the fix Rob!
Regards,
Tvrtko
P.S. Backport to kernels which do not contain e1a7ab4fca0c ("drm/i915:
Remove the vm open count"), so 5.16 to 5.18, will require a slightly
different patch as Matt has also mentioned offline.
>
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++++++++++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> index 6250de9b9196..e4b78ab4773b 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> @@ -1861,11 +1861,19 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv,
>>> vm = ctx->vm;
>>> GEM_BUG_ON(!vm);
>>>
>>> + /*
>>> + * Get a reference for the allocated handle. Once the handle is
>>> + * visible in the vm_xa table, userspace could try to close it
>>> + * from under our feet, so we need to hold the extra reference
>>> + * first.
>>> + */
>>> + i915_vm_get(vm);
>>> +
>>> err = xa_alloc(&file_priv->vm_xa, &id, vm, xa_limit_32b, GFP_KERNEL);
>>> - if (err)
>>> + if (err) {
>>> + i915_vm_put(vm);
>>> return err;
>>> -
>>> - i915_vm_get(vm);
>>> + }
>>>
>>> GEM_BUG_ON(id == 0); /* reserved for invalid/unassigned ppgtt */
>>> args->value = id;