Skip to content

Commit 3aa9945

Browse files
ickleickle
committed
drm/i915: Separate GEM context construction and registration to userspace
In later patches, it became apparent that userspace can see a partially constructed GEM context and begin using it before it was ready, to much hilarity. Close this window of opportunity by lifting the registration of the context with userspace (the insertion of the context into the filp's idr) to the very end of the CONTEXT_CREATE ioctl. Signed-off-by: Chris Wilson <[email protected]> Cc: Tvrtko Ursulin <[email protected]> Reviewed-by: Tvrtko Ursulin <[email protected]> Link: https://patchwork.freedesktop.org/patch/msgid/[email protected]
1 parent 401f147 commit 3aa9945

File tree

7 files changed

+111
-75
lines changed

7 files changed

+111
-75
lines changed

‎drivers/gpu/drm/i915/i915_gem_context.c

Lines changed: 80 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -337,15 +337,13 @@ static u32 default_desc_template(const struct drm_i915_private *i915,
337337
}
338338

339339
static struct i915_gem_context *
340-
__create_hw_context(struct drm_i915_private *dev_priv,
341-
struct drm_i915_file_private *file_priv)
340+
__create_context(struct drm_i915_private *dev_priv)
342341
{
343342
struct i915_gem_context *ctx;
344-
int ret;
345343
int i;
346344

347345
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
348-
if (ctx == NULL)
346+
if (!ctx)
349347
return ERR_PTR(-ENOMEM);
350348

351349
kref_init(&ctx->ref);
@@ -362,29 +360,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
362360
INIT_LIST_HEAD(&ctx->handles_list);
363361
INIT_LIST_HEAD(&ctx->hw_id_link);
364362

365-
/* Default context will never have a file_priv */
366-
ret = DEFAULT_CONTEXT_HANDLE;
367-
if (file_priv) {
368-
ret = idr_alloc(&file_priv->context_idr, ctx,
369-
DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
370-
if (ret < 0)
371-
goto err_lut;
372-
}
373-
ctx->user_handle = ret;
374-
375-
ctx->file_priv = file_priv;
376-
if (file_priv) {
377-
ctx->pid = get_task_pid(current, PIDTYPE_PID);
378-
ctx->name = kasprintf(GFP_KERNEL, "%s[%d]/%x",
379-
current->comm,
380-
pid_nr(ctx->pid),
381-
ctx->user_handle);
382-
if (!ctx->name) {
383-
ret = -ENOMEM;
384-
goto err_pid;
385-
}
386-
}
387-
388363
/* NB: Mark all slices as needing a remap so that when the context first
389364
* loads it will restore whatever remap state already exists. If there
390365
* is no remap info, it will be a NOP. */
@@ -401,25 +376,10 @@ __create_hw_context(struct drm_i915_private *dev_priv,
401376
ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
402377

403378
return ctx;
404-
405-
err_pid:
406-
put_pid(ctx->pid);
407-
idr_remove(&file_priv->context_idr, ctx->user_handle);
408-
err_lut:
409-
context_close(ctx);
410-
return ERR_PTR(ret);
411-
}
412-
413-
static void __destroy_hw_context(struct i915_gem_context *ctx,
414-
struct drm_i915_file_private *file_priv)
415-
{
416-
idr_remove(&file_priv->context_idr, ctx->user_handle);
417-
context_close(ctx);
418379
}
419380

420381
static struct i915_gem_context *
421-
i915_gem_create_context(struct drm_i915_private *dev_priv,
422-
struct drm_i915_file_private *file_priv)
382+
i915_gem_create_context(struct drm_i915_private *dev_priv)
423383
{
424384
struct i915_gem_context *ctx;
425385

@@ -428,18 +388,18 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
428388
/* Reap the most stale context */
429389
contexts_free_first(dev_priv);
430390

431-
ctx = __create_hw_context(dev_priv, file_priv);
391+
ctx = __create_context(dev_priv);
432392
if (IS_ERR(ctx))
433393
return ctx;
434394

435395
if (HAS_FULL_PPGTT(dev_priv)) {
436396
struct i915_hw_ppgtt *ppgtt;
437397

438-
ppgtt = i915_ppgtt_create(dev_priv, file_priv);
398+
ppgtt = i915_ppgtt_create(dev_priv);
439399
if (IS_ERR(ppgtt)) {
440400
DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n",
441401
PTR_ERR(ppgtt));
442-
__destroy_hw_context(ctx, file_priv);
402+
context_close(ctx);
443403
return ERR_CAST(ppgtt);
444404
}
445405

@@ -475,7 +435,7 @@ i915_gem_context_create_gvt(struct drm_device *dev)
475435
if (ret)
476436
return ERR_PTR(ret);
477437

478-
ctx = i915_gem_create_context(to_i915(dev), NULL);
438+
ctx = i915_gem_create_context(to_i915(dev));
479439
if (IS_ERR(ctx))
480440
goto out;
481441

@@ -511,7 +471,7 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
511471
struct i915_gem_context *ctx;
512472
int err;
513473

514-
ctx = i915_gem_create_context(i915, NULL);
474+
ctx = i915_gem_create_context(i915);
515475
if (IS_ERR(ctx))
516476
return ctx;
517477

@@ -625,25 +585,74 @@ static int context_idr_cleanup(int id, void *p, void *data)
625585
return 0;
626586
}
627587

588+
static int gem_context_register(struct i915_gem_context *ctx,
589+
struct drm_i915_file_private *fpriv)
590+
{
591+
int ret;
592+
593+
ctx->file_priv = fpriv;
594+
if (ctx->ppgtt)
595+
ctx->ppgtt->vm.file = fpriv;
596+
597+
ctx->pid = get_task_pid(current, PIDTYPE_PID);
598+
ctx->name = kasprintf(GFP_KERNEL, "%s[%d]",
599+
current->comm, pid_nr(ctx->pid));
600+
if (!ctx->name) {
601+
ret = -ENOMEM;
602+
goto err_pid;
603+
}
604+
605+
/* And finally expose ourselves to userspace via the idr */
606+
ret = idr_alloc(&fpriv->context_idr, ctx,
607+
DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
608+
if (ret < 0)
609+
goto err_name;
610+
611+
ctx->user_handle = ret;
612+
613+
return 0;
614+
615+
err_name:
616+
kfree(fetch_and_zero(&ctx->name));
617+
err_pid:
618+
put_pid(fetch_and_zero(&ctx->pid));
619+
return ret;
620+
}
621+
628622
int i915_gem_context_open(struct drm_i915_private *i915,
629623
struct drm_file *file)
630624
{
631625
struct drm_i915_file_private *file_priv = file->driver_priv;
632626
struct i915_gem_context *ctx;
627+
int err;
633628

634629
idr_init(&file_priv->context_idr);
635630

636631
mutex_lock(&i915->drm.struct_mutex);
637-
ctx = i915_gem_create_context(i915, file_priv);
638-
mutex_unlock(&i915->drm.struct_mutex);
632+
633+
ctx = i915_gem_create_context(i915);
639634
if (IS_ERR(ctx)) {
640-
idr_destroy(&file_priv->context_idr);
641-
return PTR_ERR(ctx);
635+
err = PTR_ERR(ctx);
636+
goto err;
642637
}
643638

639+
err = gem_context_register(ctx, file_priv);
640+
if (err)
641+
goto err_ctx;
642+
643+
GEM_BUG_ON(ctx->user_handle != DEFAULT_CONTEXT_HANDLE);
644644
GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
645645

646+
mutex_unlock(&i915->drm.struct_mutex);
647+
646648
return 0;
649+
650+
err_ctx:
651+
context_close(ctx);
652+
err:
653+
mutex_unlock(&i915->drm.struct_mutex);
654+
idr_destroy(&file_priv->context_idr);
655+
return PTR_ERR(ctx);
647656
}
648657

649658
void i915_gem_context_close(struct drm_file *file)
@@ -835,17 +844,28 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
835844
if (ret)
836845
return ret;
837846

838-
ctx = i915_gem_create_context(i915, file_priv);
839-
mutex_unlock(&dev->struct_mutex);
840-
if (IS_ERR(ctx))
841-
return PTR_ERR(ctx);
847+
ctx = i915_gem_create_context(i915);
848+
if (IS_ERR(ctx)) {
849+
ret = PTR_ERR(ctx);
850+
goto err_unlock;
851+
}
842852

843-
GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
853+
ret = gem_context_register(ctx, file_priv);
854+
if (ret)
855+
goto err_ctx;
856+
857+
mutex_unlock(&dev->struct_mutex);
844858

845859
args->ctx_id = ctx->user_handle;
846860
DRM_DEBUG("HW context %d created\n", args->ctx_id);
847861

848862
return 0;
863+
864+
err_ctx:
865+
context_close(ctx);
866+
err_unlock:
867+
mutex_unlock(&dev->struct_mutex);
868+
return ret;
849869
}
850870

851871
int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
@@ -870,7 +890,9 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
870890
if (ret)
871891
goto out;
872892

873-
__destroy_hw_context(ctx, file_priv);
893+
idr_remove(&file_priv->context_idr, ctx->user_handle);
894+
context_close(ctx);
895+
874896
mutex_unlock(&dev->struct_mutex);
875897

876898
out:

‎drivers/gpu/drm/i915/i915_gem_gtt.c

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2069,17 +2069,14 @@ __hw_ppgtt_create(struct drm_i915_private *i915)
20692069
}
20702070

20712071
struct i915_hw_ppgtt *
2072-
i915_ppgtt_create(struct drm_i915_private *i915,
2073-
struct drm_i915_file_private *fpriv)
2072+
i915_ppgtt_create(struct drm_i915_private *i915)
20742073
{
20752074
struct i915_hw_ppgtt *ppgtt;
20762075

20772076
ppgtt = __hw_ppgtt_create(i915);
20782077
if (IS_ERR(ppgtt))
20792078
return ppgtt;
20802079

2081-
ppgtt->vm.file = fpriv;
2082-
20832080
trace_i915_ppgtt_create(&ppgtt->vm);
20842081

20852082
return ppgtt;
@@ -2657,7 +2654,7 @@ int i915_gem_init_aliasing_ppgtt(struct drm_i915_private *i915)
26572654
struct i915_hw_ppgtt *ppgtt;
26582655
int err;
26592656

2660-
ppgtt = i915_ppgtt_create(i915, ERR_PTR(-EPERM));
2657+
ppgtt = i915_ppgtt_create(i915);
26612658
if (IS_ERR(ppgtt))
26622659
return PTR_ERR(ppgtt);
26632660

‎drivers/gpu/drm/i915/i915_gem_gtt.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -603,15 +603,17 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv);
603603
void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv);
604604

605605
int i915_ppgtt_init_hw(struct drm_i915_private *dev_priv);
606-
void i915_ppgtt_release(struct kref *kref);
607-
struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_i915_private *dev_priv,
608-
struct drm_i915_file_private *fpriv);
606+
607+
struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_i915_private *dev_priv);
609608
void i915_ppgtt_close(struct i915_address_space *vm);
609+
void i915_ppgtt_release(struct kref *kref);
610+
610611
static inline void i915_ppgtt_get(struct i915_hw_ppgtt *ppgtt)
611612
{
612613
if (ppgtt)
613614
kref_get(&ppgtt->ref);
614615
}
616+
615617
static inline void i915_ppgtt_put(struct i915_hw_ppgtt *ppgtt)
616618
{
617619
if (ppgtt)

‎drivers/gpu/drm/i915/selftests/huge_pages.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1713,7 +1713,7 @@ int i915_gem_huge_page_mock_selftests(void)
17131713
mkwrite_device_info(dev_priv)->ppgtt_size = 48;
17141714

17151715
mutex_lock(&dev_priv->drm.struct_mutex);
1716-
ppgtt = i915_ppgtt_create(dev_priv, ERR_PTR(-ENODEV));
1716+
ppgtt = i915_ppgtt_create(dev_priv);
17171717
if (IS_ERR(ppgtt)) {
17181718
err = PTR_ERR(ppgtt);
17191719
goto out_unlock;

‎drivers/gpu/drm/i915/selftests/i915_gem_context.c

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ static int live_nop_switch(void *arg)
7676
}
7777

7878
for (n = 0; n < nctx; n++) {
79-
ctx[n] = i915_gem_create_context(i915, file->driver_priv);
79+
ctx[n] = live_context(i915, file);
8080
if (IS_ERR(ctx[n])) {
8181
err = PTR_ERR(ctx[n]);
8282
goto out_unlock;
@@ -513,7 +513,7 @@ static int igt_ctx_exec(void *arg)
513513
struct i915_gem_context *ctx;
514514
unsigned int id;
515515

516-
ctx = i915_gem_create_context(i915, file->driver_priv);
516+
ctx = live_context(i915, file);
517517
if (IS_ERR(ctx)) {
518518
err = PTR_ERR(ctx);
519519
goto out_unlock;
@@ -962,7 +962,7 @@ __igt_ctx_sseu(struct drm_i915_private *i915,
962962

963963
mutex_lock(&i915->drm.struct_mutex);
964964

965-
ctx = i915_gem_create_context(i915, file->driver_priv);
965+
ctx = live_context(i915, file);
966966
if (IS_ERR(ctx)) {
967967
ret = PTR_ERR(ctx);
968968
goto out_unlock;
@@ -1072,7 +1072,7 @@ static int igt_ctx_readonly(void *arg)
10721072
if (err)
10731073
goto out_unlock;
10741074

1075-
ctx = i915_gem_create_context(i915, file->driver_priv);
1075+
ctx = live_context(i915, file);
10761076
if (IS_ERR(ctx)) {
10771077
err = PTR_ERR(ctx);
10781078
goto out_unlock;
@@ -1397,13 +1397,13 @@ static int igt_vm_isolation(void *arg)
13971397
if (err)
13981398
goto out_unlock;
13991399

1400-
ctx_a = i915_gem_create_context(i915, file->driver_priv);
1400+
ctx_a = live_context(i915, file);
14011401
if (IS_ERR(ctx_a)) {
14021402
err = PTR_ERR(ctx_a);
14031403
goto out_unlock;
14041404
}
14051405

1406-
ctx_b = i915_gem_create_context(i915, file->driver_priv);
1406+
ctx_b = live_context(i915, file);
14071407
if (IS_ERR(ctx_b)) {
14081408
err = PTR_ERR(ctx_b);
14091409
goto out_unlock;

‎drivers/gpu/drm/i915/selftests/i915_gem_gtt.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1010,7 +1010,7 @@ static int exercise_ppgtt(struct drm_i915_private *dev_priv,
10101010
return PTR_ERR(file);
10111011

10121012
mutex_lock(&dev_priv->drm.struct_mutex);
1013-
ppgtt = i915_ppgtt_create(dev_priv, file->driver_priv);
1013+
ppgtt = i915_ppgtt_create(dev_priv);
10141014
if (IS_ERR(ppgtt)) {
10151015
err = PTR_ERR(ppgtt);
10161016
goto out_unlock;

‎drivers/gpu/drm/i915/selftests/mock_context.c

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,24 @@ void mock_init_contexts(struct drm_i915_private *i915)
8888
struct i915_gem_context *
8989
live_context(struct drm_i915_private *i915, struct drm_file *file)
9090
{
91+
struct i915_gem_context *ctx;
92+
int err;
93+
9194
lockdep_assert_held(&i915->drm.struct_mutex);
9295

93-
return i915_gem_create_context(i915, file->driver_priv);
96+
ctx = i915_gem_create_context(i915);
97+
if (IS_ERR(ctx))
98+
return ctx;
99+
100+
err = gem_context_register(ctx, file->driver_priv);
101+
if (err)
102+
goto err_ctx;
103+
104+
return ctx;
105+
106+
err_ctx:
107+
context_close(ctx);
108+
return ERR_PTR(err);
94109
}
95110

96111
struct i915_gem_context *

0 commit comments

Comments
 (0)