The bo->resv pointer could be NULL, leading to kernel oopses like the one below. It looks like .fb_probe -> drm_fbdev_cma_create() -> drm_gem_cma_create() would end up not initializing resv, because we're doing that in vc4_bo_create(), not vc4_create_object().
This patch ensures that bo->resv is always set in vc4_create_object ensuring that it is never NULL.
Thanks to Eric Anholt for pointing to the correct solution.
[ 19.738487] Unable to handle kernel NULL pointer dereference at virtual address 00000000 [ 19.746805] pgd = ffff8000275fc000 [ 19.750319] [00000000] *pgd=0000000000000000 [ 19.754715] Internal error: Oops: 96000004 [#1] PREEMPT SMP [ 19.760369] Modules linked in: smsc95xx usbnet vc4 drm_kms_helper drm pwm_bcm2835 i2c_bcm2835 bcm2835_rng rng_core bcm2835_dma virt_dma [ 19.772767] CPU: 0 PID: 1297 Comm: Xorg Not tainted 4.12.0-rc1-rpi3 #58 [ 19.779476] Hardware name: Raspberry Pi 3 Model B (DT) [ 19.784688] task: ffff800028268000 task.stack: ffff800026c08000 [ 19.790705] PC is at ww_mutex_lock_interruptible+0x14/0xc0 [ 19.796329] LR is at vc4_submit_cl_ioctl+0x4fc/0x998 [vc4] [ 19.801892] pc : [<ffff0000088975f4>] lr : [<ffff0000009b3ea4>] pstate: a0000145 [ 19.809391] sp : ffff800026c0bc30 [ 19.812750] x29: ffff800026c0bc30 x28: 0000000000000000 [ 19.818142] x27: ffff80003a385a00 x26: 0000000000000000 [ 19.823534] x25: 00000000ffffffff x24: 0000000000000000 [ 19.828925] x23: ffff800027510200 x22: ffff800026393800 [ 19.834316] x21: ffff800027510270 x20: ffff800027510b00 [ 19.839707] x19: ffff80003a00a418 x18: 0000000000000011 [ 19.845099] x17: 0000000000000008 x16: 000000000000001d [ 19.850490] x15: 000000000000001b x14: 000000000000001c [ 19.855881] x13: 0000000000000000 x12: 0000000000000001 [ 19.861272] x11: 000000000000001a x10: 0000000000000000 [ 19.866662] x9 : 0000000000000000 x8 : ffff80003a385a00 [ 19.872053] x7 : 0000000000000073 x6 : 0000000000000000 [ 19.877445] x5 : 0000000000000001 x4 : 0000000000000019 [ 19.882835] x3 : ffff800028268000 x2 : 0000000000000000 [ 19.888226] x1 : ffff800026c0bce8 x0 : 0000000000000000 [ 19.893621] Process Xorg (pid: 1297, stack limit = 0xffff800026c08000) [ 19.900244] Stack: (0xffff800026c0bc30 to 0xffff800026c0c000) [ 19.906073] bc20: ffff800026c0bc60 ffff0000009b3ea4 [ 19.914019] bc40: 0000000000000000 0000000000000000 ffff80002809fc00 00000000000015f5 [ 19.921964] bc60: ffff800026c0bd00 ffff0000008f75f8 ffff0000009bdd80 00000000c0a06440 [ 19.929910] bc80: 00000000000000a0 ffff800027a58600 0000ffffeb48d398 00000000000000a0 [ 19.937856] bca0: ffff00000091ad58 0000000000000040 ffff800026393800 ffff800027510b00 [ 19.945802] bcc0: ffff0000009bdd80 00000000c0a06440 ffff800027a58650 ffff800027c42684 [ 19.953747] bce0: 0000ffffeb48d398 ffff800028268000 0000000000000001 0000aaaa00000000 [ 19.961693] bd00: ffff800026c0be00 ffff00000821383c ffff800028abb8d8 ffff80003a31eb00 [ 19.969638] bd20: 0000ffffeb48d398 ffff80003a31eb00 00000000c0a06440 0000ffffeb48d398 [ 19.977585] bd40: 0000000000000124 000000000000001d ffff0000088b2000 ffff800028268000 [ 19.985530] bd60: ffff800026c0bd80 00000000000000a0 ffff0000009b39a8 ffff800026c0bd80 [ 19.993476] bd80: 00000000007f8000 000000000000000a ffff800026c0bda0 ffff000008097b58 [ 20.001422] bda0: ffff800026c0be00 ffff0000080812cc ffff0000088bb308 0000000082000007 [ 20.009366] bdc0: ffff0000088bb438 0000ffffa9b56178 ffff800026c0bec0 0000000000000020 [ 20.017313] bde0: 0000000082000007 0000ffffa9b56178 0000000000000001 ffff800028268000 [ 20.025258] be00: ffff800026c0be80 ffff000008213fe4 0000000000000000 0000800032374000 [ 20.033204] be20: ffff80003a31eb00 0000000000000018 0000000080000000 ffff800026c0be30 [ 20.041150] be40: ffff800026c0be80 ffff000008213fa8 0000000000000000 ffff80003a31eb00 [ 20.049096] be60: ffff800026c0be70 ffff000008220300 ffff800026c0be80 ffff000008213f88 [ 20.057042] be80: 0000000000000000 ffff000008082f30 0000000000000000 0000800032374000 [ 20.064988] bea0: ffffffffffffffff 0000ffffaad04cdc 0000000040000000 0000000000000015 [ 20.072933] bec0: 0000000000000018 00000000c0a06440 0000ffffeb48d398 0000000000000000 [ 20.080879] bee0: 0000aaaae7550b00 0000000000000000 0000000000000040 0000aaaae7550910 [ 20.088824] bf00: 000000000000001d 0000aaaae7550b50 0000000000000000 0000000000000000 [ 20.096769] bf20: 0000000000000000 0000000000000000 0000000000000008 0000ffffeb48d1a8 [ 20.104714] bf40: 0000ffffab0472a0 0000ffffaad04cd0 0000aaaae754a3a0 0000ffffaa21d000 [ 20.112659] bf60: 0000ffffeb48d398 00000000c0a06440 0000000000000018 0000ffffaa21f000 [ 20.120605] bf80: 0000aaaae754bd80 0000ffffeb48d498 0000aaaae751e8c0 0000aaaae6d346b0 [ 20.128551] bfa0: 0000aaaae6d4f3f0 0000ffffeb48d310 0000ffffab02d884 0000ffffeb48d310 [ 20.136496] bfc0: 0000ffffaad04cdc 0000000040000000 0000000000000018 000000000000001d [ 20.144441] bfe0: 0000000000000000 0000000000000000 1000006238744821 d61f00208b218841 [ 20.152382] Call trace: [ 20.154863] Exception stack(0xffff800026c0ba50 to 0xffff800026c0bb80) [ 20.161397] ba40: ffff80003a00a418 0001000000000000 [ 20.169342] ba60: ffff800026c0bc30 ffff0000088975f4 00000000a0000145 0000000000000000 [ 20.177287] ba80: 0000000000000000 0000000000000000 0000000000000009 0000000000000ba5 [ 20.185232] baa0: ffff800027510b00 0000000000000000 ffff00000cd0e000 0000000000000073 [ 20.193177] bac0: 0000000000000000 0000000000000000 0000000000000001 000000000000001a [ 20.201121] bae0: 0000000000000000 0000000000000000 000000000000001c 000000000000001b [ 20.209067] bb00: 0000000000000000 ffff800026c0bce8 0000000000000000 ffff800028268000 [ 20.217012] bb20: 0000000000000019 0000000000000001 0000000000000000 0000000000000073 [ 20.224957] bb40: ffff80003a385a00 0000000000000000 0000000000000000 000000000000001a [ 20.232901] bb60: 0000000000000001 0000000000000000 000000000000001c 000000000000001b [ 20.240855] [<ffff0000088975f4>] ww_mutex_lock_interruptible+0x14/0xc0 [ 20.247528] [<ffff0000009b3ea4>] vc4_submit_cl_ioctl+0x4fc/0x998 [vc4] [ 20.254372] [<ffff0000008f75f8>] drm_ioctl+0x180/0x438 [drm] [ 20.260120] [<ffff00000821383c>] do_vfs_ioctl+0xa4/0x7d0 [ 20.265510] [<ffff000008213fe4>] SyS_ioctl+0x7c/0x98 [ 20.270550] [<ffff000008082f30>] el0_svc_naked+0x24/0x28 [ 20.275941] Code: d2800002 d5384103 910003fd f9800011 (c85ffc04) [ 20.282527] ---[ end trace 1f6bd640ff32ae12 ]---
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com --- drivers/gpu/drm/vc4/vc4_bo.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c index 80b2f9e55c5c..2231ee76cd8d 100644 --- a/drivers/gpu/drm/vc4/vc4_bo.c +++ b/drivers/gpu/drm/vc4/vc4_bo.c @@ -91,8 +91,7 @@ static void vc4_bo_destroy(struct vc4_bo *bo) vc4->bo_stats.num_allocated--; vc4->bo_stats.size_allocated -= obj->size;
- if (bo->resv == &bo->_resv) - reservation_object_fini(bo->resv); + reservation_object_fini(bo->resv);
drm_gem_cma_free_object(obj); } @@ -212,6 +211,8 @@ struct drm_gem_object *vc4_create_object(struct drm_device *dev, size_t size) vc4->bo_stats.num_allocated++; vc4->bo_stats.size_allocated += size; mutex_unlock(&vc4->bo_lock); + bo->resv = &bo->_resv; + reservation_object_init(bo->resv);
return &bo->base.base; } @@ -250,12 +251,7 @@ struct vc4_bo *vc4_bo_create(struct drm_device *dev, size_t unaligned_size, return ERR_PTR(-ENOMEM); } } - bo = to_vc4_bo(&cma_obj->base); - - bo->resv = &bo->_resv; - reservation_object_init(bo->resv); - - return bo; + return to_vc4_bo(&cma_obj->base); }
int vc4_dumb_create(struct drm_file *file_priv,
Hans Verkuil hverkuil@xs4all.nl writes:
The bo->resv pointer could be NULL, leading to kernel oopses like the one below. It looks like .fb_probe -> drm_fbdev_cma_create() -> drm_gem_cma_create() would end up not initializing resv, because we're doing that in vc4_bo_create(), not vc4_create_object().
This patch ensures that bo->resv is always set in vc4_create_object ensuring that it is never NULL.
Thanks to Eric Anholt for pointing to the correct solution.
Folks that know dma-buf better: Will having two reservations around for the lifetime of the BO in the case of CMA dma-buf imports be a bad thing? I'm assuming reservations are cheap, given that we're unconditionally allocating them per BO for un-shared BOs already.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com
drivers/gpu/drm/vc4/vc4_bo.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c index 80b2f9e55c5c..2231ee76cd8d 100644 --- a/drivers/gpu/drm/vc4/vc4_bo.c +++ b/drivers/gpu/drm/vc4/vc4_bo.c @@ -91,8 +91,7 @@ static void vc4_bo_destroy(struct vc4_bo *bo) vc4->bo_stats.num_allocated--; vc4->bo_stats.size_allocated -= obj->size;
- if (bo->resv == &bo->_resv)
reservation_object_fini(bo->resv);
- reservation_object_fini(bo->resv);
This one would need to be "reservation_object_fini(&bo->_resv);" -- we need to free the reservation that we created (and left unused in the case of an import), not the one that was imported from someone else.
drm_gem_cma_free_object(obj); } @@ -212,6 +211,8 @@ struct drm_gem_object *vc4_create_object(struct drm_device *dev, size_t size) vc4->bo_stats.num_allocated++; vc4->bo_stats.size_allocated += size; mutex_unlock(&vc4->bo_lock);
bo->resv = &bo->_resv;
reservation_object_init(bo->resv);
return &bo->base.base; }
@@ -250,12 +251,7 @@ struct vc4_bo *vc4_bo_create(struct drm_device *dev, size_t unaligned_size, return ERR_PTR(-ENOMEM); } }
- bo = to_vc4_bo(&cma_obj->base);
- bo->resv = &bo->_resv;
- reservation_object_init(bo->resv);
- return bo;
return to_vc4_bo(&cma_obj->base); }
int vc4_dumb_create(struct drm_file *file_priv,
-- 2.11.0
On 06/02/2017 11:48 PM, Eric Anholt wrote:
Hans Verkuil hverkuil@xs4all.nl writes:
The bo->resv pointer could be NULL, leading to kernel oopses like the one below. It looks like .fb_probe -> drm_fbdev_cma_create() -> drm_gem_cma_create() would end up not initializing resv, because we're doing that in vc4_bo_create(), not vc4_create_object().
This patch ensures that bo->resv is always set in vc4_create_object ensuring that it is never NULL.
Thanks to Eric Anholt for pointing to the correct solution.
Folks that know dma-buf better: Will having two reservations around for the lifetime of the BO in the case of CMA dma-buf imports be a bad thing? I'm assuming reservations are cheap, given that we're unconditionally allocating them per BO for un-shared BOs already.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com
drivers/gpu/drm/vc4/vc4_bo.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c index 80b2f9e55c5c..2231ee76cd8d 100644 --- a/drivers/gpu/drm/vc4/vc4_bo.c +++ b/drivers/gpu/drm/vc4/vc4_bo.c @@ -91,8 +91,7 @@ static void vc4_bo_destroy(struct vc4_bo *bo) vc4->bo_stats.num_allocated--; vc4->bo_stats.size_allocated -= obj->size;
- if (bo->resv == &bo->_resv)
reservation_object_fini(bo->resv);
- reservation_object_fini(bo->resv);
This one would need to be "reservation_object_fini(&bo->_resv);" -- we need to free the reservation that we created (and left unused in the case of an import), not the one that was imported from someone else.
Of course, fixed. I'll wait a bit to see if anyone replies to your query above, and will post a v2 some time next week.
Regards,
Hans
drm_gem_cma_free_object(obj);
} @@ -212,6 +211,8 @@ struct drm_gem_object *vc4_create_object(struct drm_device *dev, size_t size) vc4->bo_stats.num_allocated++; vc4->bo_stats.size_allocated += size; mutex_unlock(&vc4->bo_lock);
bo->resv = &bo->_resv;
reservation_object_init(bo->resv);
return &bo->base.base; }
@@ -250,12 +251,7 @@ struct vc4_bo *vc4_bo_create(struct drm_device *dev, size_t unaligned_size, return ERR_PTR(-ENOMEM); } }
- bo = to_vc4_bo(&cma_obj->base);
- bo->resv = &bo->_resv;
- reservation_object_init(bo->resv);
- return bo;
return to_vc4_bo(&cma_obj->base); }
int vc4_dumb_create(struct drm_file *file_priv,
-- 2.11.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org