* [BUG] drm: vc4: refcount_t: increment on 0; use-after-free.
@ 2017-11-22 16:43 Stefan Wahren
2017-11-22 17:51 ` Boris Brezillon
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Wahren @ 2017-11-22 16:43 UTC (permalink / raw)
To: Boris Brezillon, Eric Anholt; +Cc: David Airlie, Daniel Vetter, dri-devel
Hi Boris,
if i boot Raspberry Pi 3 (ARM64, defconfig, linux-next-20171122) with sufficient CMA memory (32 MB), i'll get this warning during boot:
[ 7.623510] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
[ 7.632453] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
[ 7.639707] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
[ 7.647364] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
[ 7.655451] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
[ 7.663415] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
[ 7.730580] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
[ 7.743965] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
[ 7.750841] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[ 7.757620] [drm] Driver supports precise vblank timestamp query.
[ 7.811775] ------------[ cut here ]------------
[ 7.811780] refcount_t: increment on 0; use-after-free.
[ 7.811881] WARNING: CPU: 2 PID: 2188 at lib/refcount.c:153 refcount_inc+0x44/0x50
[ 7.811884] Modules linked in: vc4(+) cfg80211 cec drm_kms_helper smsc95xx usbnet drm rfkill brcmutil bcm2835_rng rng_core bcm2835_dma crc32_ce i2c_bcm2835 pwm_bcm2835 ip_tables x_tables ipv6
[ 7.811950] CPU: 2 PID: 2188 Comm: systemd-udevd Not tainted 4.14.0-next-20171122 #1
[ 7.811953] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
[ 7.811958] task: ffff800036b91c00 task.stack: ffff00000d6f0000
[ 7.811967] pstate: 20000005 (nzCv daif -PAN -UAO)
[ 7.811974] pc : refcount_inc+0x44/0x50
[ 7.811981] lr : refcount_inc+0x44/0x50
[ 7.811984] sp : ffff00000d6f3300
[ 7.811988] x29: ffff00000d6f3300 x28: 0000000000000000
[ 7.811996] x27: 0000000000000003 x26: ffff800037107800
[ 7.812004] x25: 0000000000000001 x24: ffff800035afdc00
[ 7.812012] x23: 0000000000000000 x22: ffff800035dfa600
[ 7.812020] x21: ffff800035afd9b0 x20: ffff800035afd9a4
[ 7.812027] x19: 0000000000000000 x18: 0000000000000000
[ 7.812034] x17: 0000000000000001 x16: 0000000000000019
[ 7.812042] x15: 0000000000000001 x14: 00000000fffffff0
[ 7.812049] x13: ffff0000090ae840 x12: ffff000008fa2d50
[ 7.812057] x11: ffff000008fa2000 x10: ffff0000090ad000
[ 7.812064] x9 : 0000000000000000 x8 : ffff0000090b5c2f
[ 7.812072] x7 : 0000000000000000 x6 : 000000000015ee00
[ 7.812079] x5 : 0000000000000000 x4 : 0000000000000000
[ 7.812087] x3 : ffffffffffffffff x2 : 0000800030047000
[ 7.812094] x1 : ffff800036b91c00 x0 : 000000000000002b
[ 7.812102] Call trace:
[ 7.812109] refcount_inc+0x44/0x50
[ 7.812226] vc4_bo_inc_usecnt+0x84/0x88 [vc4]
[ 7.812310] vc4_prepare_fb+0x40/0xf0 [vc4]
[ 7.812460] drm_atomic_helper_prepare_planes+0x54/0xf0 [drm_kms_helper]
[ 7.812543] vc4_atomic_commit+0x88/0x130 [vc4]
[ 7.812868] drm_atomic_commit+0x48/0x68 [drm]
[ 7.813002] restore_fbdev_mode_atomic+0x1d8/0x1e8 [drm_kms_helper]
[ 7.813113] restore_fbdev_mode+0x28/0x160 [drm_kms_helper]
[ 7.813223] drm_fb_helper_restore_fbdev_mode_unlocked.part.24+0x28/0x90 [drm_kms_helper]
[ 7.813331] drm_fb_helper_set_par+0x54/0xa8 [drm_kms_helper]
[ 7.813346] fbcon_init+0x4e8/0x538
[ 7.813357] visual_init+0xb4/0x108
[ 7.813366] do_bind_con_driver+0x1b8/0x3a0
[ 7.813373] do_take_over_console+0x150/0x1d0
[ 7.813380] do_fbcon_takeover+0x6c/0xf0
[ 7.813387] fbcon_event_notify+0x8fc/0x928
[ 7.813399] notifier_call_chain+0x50/0x90
[ 7.813406] __blocking_notifier_call_chain+0x4c/0x90
[ 7.813413] blocking_notifier_call_chain+0x14/0x20
[ 7.813420] fb_notifier_call_chain+0x1c/0x28
[ 7.813426] register_framebuffer+0x1d0/0x2d8
[ 7.813533] __drm_fb_helper_initial_config_and_unlock+0x1e8/0x350 [drm_kms_helper]
[ 7.813639] drm_fb_helper_initial_config+0x40/0x58 [drm_kms_helper]
[ 7.813747] drm_fbdev_cma_init_with_funcs+0x88/0x158 [drm_kms_helper]
[ 7.813855] drm_fbdev_cma_init+0x14/0x28 [drm_kms_helper]
[ 7.813943] vc4_kms_load+0xa4/0xf0 [vc4]
[ 7.814026] vc4_drm_bind+0x100/0x168 [vc4]
[ 7.814038] try_to_bring_up_master+0x144/0x1a8
[ 7.814044] component_master_add_with_match+0x9c/0xe0
[ 7.814128] vc4_platform_drm_probe+0xb4/0xf0 [vc4]
[ 7.814137] platform_drv_probe+0x58/0xc0
[ 7.814146] driver_probe_device+0x224/0x308
[ 7.814153] __driver_attach+0xac/0xb0
[ 7.814161] bus_for_each_dev+0x64/0xa0
[ 7.814169] driver_attach+0x20/0x28
[ 7.814177] bus_add_driver+0x108/0x228
[ 7.814184] driver_register+0x60/0xf8
[ 7.814190] __platform_driver_register+0x40/0x48
[ 7.814274] vc4_drm_register+0x38/0x1000 [vc4]
[ 7.814283] do_one_initcall+0x38/0x120
[ 7.814295] do_init_module+0x58/0x1b8
[ 7.814304] load_module+0x1fa8/0x2280
[ 7.814311] SyS_finit_module+0xc0/0xd0
[ 7.814318] __sys_trace_return+0x0/0x4
[ 7.814325] ---[ end trace 3348554eb91e19a1 ]---
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [BUG] drm: vc4: refcount_t: increment on 0; use-after-free.
2017-11-22 16:43 [BUG] drm: vc4: refcount_t: increment on 0; use-after-free Stefan Wahren
@ 2017-11-22 17:51 ` Boris Brezillon
2017-11-22 18:13 ` Daniel Vetter
2017-11-22 18:17 ` Stefan Wahren
0 siblings, 2 replies; 10+ messages in thread
From: Boris Brezillon @ 2017-11-22 17:51 UTC (permalink / raw)
To: Stefan Wahren; +Cc: David Airlie, dri-devel, Daniel Vetter
Hi Stefan,
On Wed, 22 Nov 2017 17:43:35 +0100 (CET)
Stefan Wahren <stefan.wahren@i2se.com> wrote:
> Hi Boris,
> if i boot Raspberry Pi 3 (ARM64, defconfig, linux-next-20171122) with sufficient CMA memory (32 MB), i'll get this warning during boot:
>
> [ 7.623510] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
> [ 7.632453] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
> [ 7.639707] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
> [ 7.647364] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
> [ 7.655451] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
> [ 7.663415] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
> [ 7.730580] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
> [ 7.743965] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
> [ 7.750841] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [ 7.757620] [drm] Driver supports precise vblank timestamp query.
> [ 7.811775] ------------[ cut here ]------------
> [ 7.811780] refcount_t: increment on 0; use-after-free.
> [ 7.811881] WARNING: CPU: 2 PID: 2188 at lib/refcount.c:153 refcount_inc+0x44/0x50
> [ 7.811884] Modules linked in: vc4(+) cfg80211 cec drm_kms_helper smsc95xx usbnet drm rfkill brcmutil bcm2835_rng rng_core bcm2835_dma crc32_ce i2c_bcm2835 pwm_bcm2835 ip_tables x_tables ipv6
> [ 7.811950] CPU: 2 PID: 2188 Comm: systemd-udevd Not tainted 4.14.0-next-20171122 #1
> [ 7.811953] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
> [ 7.811958] task: ffff800036b91c00 task.stack: ffff00000d6f0000
> [ 7.811967] pstate: 20000005 (nzCv daif -PAN -UAO)
> [ 7.811974] pc : refcount_inc+0x44/0x50
> [ 7.811981] lr : refcount_inc+0x44/0x50
> [ 7.811984] sp : ffff00000d6f3300
> [ 7.811988] x29: ffff00000d6f3300 x28: 0000000000000000
> [ 7.811996] x27: 0000000000000003 x26: ffff800037107800
> [ 7.812004] x25: 0000000000000001 x24: ffff800035afdc00
> [ 7.812012] x23: 0000000000000000 x22: ffff800035dfa600
> [ 7.812020] x21: ffff800035afd9b0 x20: ffff800035afd9a4
> [ 7.812027] x19: 0000000000000000 x18: 0000000000000000
> [ 7.812034] x17: 0000000000000001 x16: 0000000000000019
> [ 7.812042] x15: 0000000000000001 x14: 00000000fffffff0
> [ 7.812049] x13: ffff0000090ae840 x12: ffff000008fa2d50
> [ 7.812057] x11: ffff000008fa2000 x10: ffff0000090ad000
> [ 7.812064] x9 : 0000000000000000 x8 : ffff0000090b5c2f
> [ 7.812072] x7 : 0000000000000000 x6 : 000000000015ee00
> [ 7.812079] x5 : 0000000000000000 x4 : 0000000000000000
> [ 7.812087] x3 : ffffffffffffffff x2 : 0000800030047000
> [ 7.812094] x1 : ffff800036b91c00 x0 : 000000000000002b
> [ 7.812102] Call trace:
> [ 7.812109] refcount_inc+0x44/0x50
> [ 7.812226] vc4_bo_inc_usecnt+0x84/0x88 [vc4]
> [ 7.812310] vc4_prepare_fb+0x40/0xf0 [vc4]
> [ 7.812460] drm_atomic_helper_prepare_planes+0x54/0xf0 [drm_kms_helper]
> [ 7.812543] vc4_atomic_commit+0x88/0x130 [vc4]
> [ 7.812868] drm_atomic_commit+0x48/0x68 [drm]
> [ 7.813002] restore_fbdev_mode_atomic+0x1d8/0x1e8 [drm_kms_helper]
> [ 7.813113] restore_fbdev_mode+0x28/0x160 [drm_kms_helper]
> [ 7.813223] drm_fb_helper_restore_fbdev_mode_unlocked.part.24+0x28/0x90 [drm_kms_helper]
> [ 7.813331] drm_fb_helper_set_par+0x54/0xa8 [drm_kms_helper]
> [ 7.813346] fbcon_init+0x4e8/0x538
> [ 7.813357] visual_init+0xb4/0x108
> [ 7.813366] do_bind_con_driver+0x1b8/0x3a0
> [ 7.813373] do_take_over_console+0x150/0x1d0
> [ 7.813380] do_fbcon_takeover+0x6c/0xf0
> [ 7.813387] fbcon_event_notify+0x8fc/0x928
> [ 7.813399] notifier_call_chain+0x50/0x90
> [ 7.813406] __blocking_notifier_call_chain+0x4c/0x90
> [ 7.813413] blocking_notifier_call_chain+0x14/0x20
> [ 7.813420] fb_notifier_call_chain+0x1c/0x28
> [ 7.813426] register_framebuffer+0x1d0/0x2d8
> [ 7.813533] __drm_fb_helper_initial_config_and_unlock+0x1e8/0x350 [drm_kms_helper]
> [ 7.813639] drm_fb_helper_initial_config+0x40/0x58 [drm_kms_helper]
> [ 7.813747] drm_fbdev_cma_init_with_funcs+0x88/0x158 [drm_kms_helper]
> [ 7.813855] drm_fbdev_cma_init+0x14/0x28 [drm_kms_helper]
> [ 7.813943] vc4_kms_load+0xa4/0xf0 [vc4]
> [ 7.814026] vc4_drm_bind+0x100/0x168 [vc4]
> [ 7.814038] try_to_bring_up_master+0x144/0x1a8
> [ 7.814044] component_master_add_with_match+0x9c/0xe0
> [ 7.814128] vc4_platform_drm_probe+0xb4/0xf0 [vc4]
> [ 7.814137] platform_drv_probe+0x58/0xc0
> [ 7.814146] driver_probe_device+0x224/0x308
> [ 7.814153] __driver_attach+0xac/0xb0
> [ 7.814161] bus_for_each_dev+0x64/0xa0
> [ 7.814169] driver_attach+0x20/0x28
> [ 7.814177] bus_add_driver+0x108/0x228
> [ 7.814184] driver_register+0x60/0xf8
> [ 7.814190] __platform_driver_register+0x40/0x48
> [ 7.814274] vc4_drm_register+0x38/0x1000 [vc4]
> [ 7.814283] do_one_initcall+0x38/0x120
> [ 7.814295] do_init_module+0x58/0x1b8
> [ 7.814304] load_module+0x1fa8/0x2280
> [ 7.814311] SyS_finit_module+0xc0/0xd0
> [ 7.814318] __sys_trace_return+0x0/0x4
> [ 7.814325] ---[ end trace 3348554eb91e19a1 ]---
Looks like I didn't test this code with CONFIG_REFCOUNT_FULL enabled :-/.
Anyway, can you try to apply the following diff and let me know if it
fixes the problem?
Thanks,
Boris
--->8---
diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
index 4ae45d7dac42..2decc8e2c79f 100644
--- a/drivers/gpu/drm/vc4/vc4_bo.c
+++ b/drivers/gpu/drm/vc4/vc4_bo.c
@@ -637,7 +637,8 @@ int vc4_bo_inc_usecnt(struct vc4_bo *bo)
mutex_lock(&bo->madv_lock);
switch (bo->madv) {
case VC4_MADV_WILLNEED:
- refcount_inc(&bo->usecnt);
+ if (!refcount_inc_not_zero(&bo->usecnt))
+ refcount_set(&bo->usecnt, 1);
ret = 0;
break;
case VC4_MADV_DONTNEED:
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [BUG] drm: vc4: refcount_t: increment on 0; use-after-free.
2017-11-22 17:51 ` Boris Brezillon
@ 2017-11-22 18:13 ` Daniel Vetter
2017-11-22 18:21 ` Boris Brezillon
2017-11-22 18:17 ` Stefan Wahren
1 sibling, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2017-11-22 18:13 UTC (permalink / raw)
To: Boris Brezillon; +Cc: Stefan Wahren, David Airlie, dri-devel, Daniel Vetter
On Wed, Nov 22, 2017 at 6:51 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Stefan,
>
> On Wed, 22 Nov 2017 17:43:35 +0100 (CET)
> Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
>> Hi Boris,
>> if i boot Raspberry Pi 3 (ARM64, defconfig, linux-next-20171122) with sufficient CMA memory (32 MB), i'll get this warning during boot:
>>
>> [ 7.623510] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
>> [ 7.632453] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
>> [ 7.639707] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
>> [ 7.647364] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
>> [ 7.655451] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
>> [ 7.663415] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
>> [ 7.730580] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
>> [ 7.743965] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
>> [ 7.750841] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>> [ 7.757620] [drm] Driver supports precise vblank timestamp query.
>> [ 7.811775] ------------[ cut here ]------------
>> [ 7.811780] refcount_t: increment on 0; use-after-free.
>> [ 7.811881] WARNING: CPU: 2 PID: 2188 at lib/refcount.c:153 refcount_inc+0x44/0x50
>> [ 7.811884] Modules linked in: vc4(+) cfg80211 cec drm_kms_helper smsc95xx usbnet drm rfkill brcmutil bcm2835_rng rng_core bcm2835_dma crc32_ce i2c_bcm2835 pwm_bcm2835 ip_tables x_tables ipv6
>> [ 7.811950] CPU: 2 PID: 2188 Comm: systemd-udevd Not tainted 4.14.0-next-20171122 #1
>> [ 7.811953] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
>> [ 7.811958] task: ffff800036b91c00 task.stack: ffff00000d6f0000
>> [ 7.811967] pstate: 20000005 (nzCv daif -PAN -UAO)
>> [ 7.811974] pc : refcount_inc+0x44/0x50
>> [ 7.811981] lr : refcount_inc+0x44/0x50
>> [ 7.811984] sp : ffff00000d6f3300
>> [ 7.811988] x29: ffff00000d6f3300 x28: 0000000000000000
>> [ 7.811996] x27: 0000000000000003 x26: ffff800037107800
>> [ 7.812004] x25: 0000000000000001 x24: ffff800035afdc00
>> [ 7.812012] x23: 0000000000000000 x22: ffff800035dfa600
>> [ 7.812020] x21: ffff800035afd9b0 x20: ffff800035afd9a4
>> [ 7.812027] x19: 0000000000000000 x18: 0000000000000000
>> [ 7.812034] x17: 0000000000000001 x16: 0000000000000019
>> [ 7.812042] x15: 0000000000000001 x14: 00000000fffffff0
>> [ 7.812049] x13: ffff0000090ae840 x12: ffff000008fa2d50
>> [ 7.812057] x11: ffff000008fa2000 x10: ffff0000090ad000
>> [ 7.812064] x9 : 0000000000000000 x8 : ffff0000090b5c2f
>> [ 7.812072] x7 : 0000000000000000 x6 : 000000000015ee00
>> [ 7.812079] x5 : 0000000000000000 x4 : 0000000000000000
>> [ 7.812087] x3 : ffffffffffffffff x2 : 0000800030047000
>> [ 7.812094] x1 : ffff800036b91c00 x0 : 000000000000002b
>> [ 7.812102] Call trace:
>> [ 7.812109] refcount_inc+0x44/0x50
>> [ 7.812226] vc4_bo_inc_usecnt+0x84/0x88 [vc4]
>> [ 7.812310] vc4_prepare_fb+0x40/0xf0 [vc4]
>> [ 7.812460] drm_atomic_helper_prepare_planes+0x54/0xf0 [drm_kms_helper]
>> [ 7.812543] vc4_atomic_commit+0x88/0x130 [vc4]
>> [ 7.812868] drm_atomic_commit+0x48/0x68 [drm]
>> [ 7.813002] restore_fbdev_mode_atomic+0x1d8/0x1e8 [drm_kms_helper]
>> [ 7.813113] restore_fbdev_mode+0x28/0x160 [drm_kms_helper]
>> [ 7.813223] drm_fb_helper_restore_fbdev_mode_unlocked.part.24+0x28/0x90 [drm_kms_helper]
>> [ 7.813331] drm_fb_helper_set_par+0x54/0xa8 [drm_kms_helper]
>> [ 7.813346] fbcon_init+0x4e8/0x538
>> [ 7.813357] visual_init+0xb4/0x108
>> [ 7.813366] do_bind_con_driver+0x1b8/0x3a0
>> [ 7.813373] do_take_over_console+0x150/0x1d0
>> [ 7.813380] do_fbcon_takeover+0x6c/0xf0
>> [ 7.813387] fbcon_event_notify+0x8fc/0x928
>> [ 7.813399] notifier_call_chain+0x50/0x90
>> [ 7.813406] __blocking_notifier_call_chain+0x4c/0x90
>> [ 7.813413] blocking_notifier_call_chain+0x14/0x20
>> [ 7.813420] fb_notifier_call_chain+0x1c/0x28
>> [ 7.813426] register_framebuffer+0x1d0/0x2d8
>> [ 7.813533] __drm_fb_helper_initial_config_and_unlock+0x1e8/0x350 [drm_kms_helper]
>> [ 7.813639] drm_fb_helper_initial_config+0x40/0x58 [drm_kms_helper]
>> [ 7.813747] drm_fbdev_cma_init_with_funcs+0x88/0x158 [drm_kms_helper]
>> [ 7.813855] drm_fbdev_cma_init+0x14/0x28 [drm_kms_helper]
>> [ 7.813943] vc4_kms_load+0xa4/0xf0 [vc4]
>> [ 7.814026] vc4_drm_bind+0x100/0x168 [vc4]
>> [ 7.814038] try_to_bring_up_master+0x144/0x1a8
>> [ 7.814044] component_master_add_with_match+0x9c/0xe0
>> [ 7.814128] vc4_platform_drm_probe+0xb4/0xf0 [vc4]
>> [ 7.814137] platform_drv_probe+0x58/0xc0
>> [ 7.814146] driver_probe_device+0x224/0x308
>> [ 7.814153] __driver_attach+0xac/0xb0
>> [ 7.814161] bus_for_each_dev+0x64/0xa0
>> [ 7.814169] driver_attach+0x20/0x28
>> [ 7.814177] bus_add_driver+0x108/0x228
>> [ 7.814184] driver_register+0x60/0xf8
>> [ 7.814190] __platform_driver_register+0x40/0x48
>> [ 7.814274] vc4_drm_register+0x38/0x1000 [vc4]
>> [ 7.814283] do_one_initcall+0x38/0x120
>> [ 7.814295] do_init_module+0x58/0x1b8
>> [ 7.814304] load_module+0x1fa8/0x2280
>> [ 7.814311] SyS_finit_module+0xc0/0xd0
>> [ 7.814318] __sys_trace_return+0x0/0x4
>> [ 7.814325] ---[ end trace 3348554eb91e19a1 ]---
>
> Looks like I didn't test this code with CONFIG_REFCOUNT_FULL enabled :-/.
>
> Anyway, can you try to apply the following diff and let me know if it
> fixes the problem?
>
> Thanks,
>
> Boris
>
> --->8---
> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 4ae45d7dac42..2decc8e2c79f 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> @@ -637,7 +637,8 @@ int vc4_bo_inc_usecnt(struct vc4_bo *bo)
> mutex_lock(&bo->madv_lock);
> switch (bo->madv) {
> case VC4_MADV_WILLNEED:
> - refcount_inc(&bo->usecnt);
> + if (!refcount_inc_not_zero(&bo->usecnt))
> + refcount_set(&bo->usecnt, 1);
This is racy. You need a refcount_inc_allow_zero (if that exists).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [BUG] drm: vc4: refcount_t: increment on 0; use-after-free.
2017-11-22 17:51 ` Boris Brezillon
2017-11-22 18:13 ` Daniel Vetter
@ 2017-11-22 18:17 ` Stefan Wahren
1 sibling, 0 replies; 10+ messages in thread
From: Stefan Wahren @ 2017-11-22 18:17 UTC (permalink / raw)
To: Boris Brezillon; +Cc: David Airlie, dri-devel, Daniel Vetter
Hi Boris,
> Boris Brezillon <boris.brezillon@free-electrons.com> hat am 22. November 2017 um 18:51 geschrieben:
>
>
> Hi Stefan,
>
> On Wed, 22 Nov 2017 17:43:35 +0100 (CET)
> Stefan Wahren <stefan.wahren@i2se.com> wrote:
> ...
>
> Looks like I didn't test this code with CONFIG_REFCOUNT_FULL enabled :-/.
>
> Anyway, can you try to apply the following diff and let me know if it
> fixes the problem?
yes, this fixes the problem.
>
> Thanks,
>
> Boris
>
> --->8---
> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 4ae45d7dac42..2decc8e2c79f 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> @@ -637,7 +637,8 @@ int vc4_bo_inc_usecnt(struct vc4_bo *bo)
> mutex_lock(&bo->madv_lock);
> switch (bo->madv) {
> case VC4_MADV_WILLNEED:
> - refcount_inc(&bo->usecnt);
> + if (!refcount_inc_not_zero(&bo->usecnt))
> + refcount_set(&bo->usecnt, 1);
> ret = 0;
> break;
> case VC4_MADV_DONTNEED:
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [BUG] drm: vc4: refcount_t: increment on 0; use-after-free.
2017-11-22 18:13 ` Daniel Vetter
@ 2017-11-22 18:21 ` Boris Brezillon
2017-11-23 7:57 ` Daniel Vetter
0 siblings, 1 reply; 10+ messages in thread
From: Boris Brezillon @ 2017-11-22 18:21 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Stefan Wahren, David Airlie, dri-devel, Daniel Vetter
On Wed, 22 Nov 2017 19:13:09 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Nov 22, 2017 at 6:51 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > Hi Stefan,
> >
> > On Wed, 22 Nov 2017 17:43:35 +0100 (CET)
> > Stefan Wahren <stefan.wahren@i2se.com> wrote:
> >
> >> Hi Boris,
> >> if i boot Raspberry Pi 3 (ARM64, defconfig, linux-next-20171122) with sufficient CMA memory (32 MB), i'll get this warning during boot:
> >>
> >> [ 7.623510] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
> >> [ 7.632453] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
> >> [ 7.639707] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
> >> [ 7.647364] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
> >> [ 7.655451] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
> >> [ 7.663415] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
> >> [ 7.730580] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
> >> [ 7.743965] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
> >> [ 7.750841] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> >> [ 7.757620] [drm] Driver supports precise vblank timestamp query.
> >> [ 7.811775] ------------[ cut here ]------------
> >> [ 7.811780] refcount_t: increment on 0; use-after-free.
> >> [ 7.811881] WARNING: CPU: 2 PID: 2188 at lib/refcount.c:153 refcount_inc+0x44/0x50
> >> [ 7.811884] Modules linked in: vc4(+) cfg80211 cec drm_kms_helper smsc95xx usbnet drm rfkill brcmutil bcm2835_rng rng_core bcm2835_dma crc32_ce i2c_bcm2835 pwm_bcm2835 ip_tables x_tables ipv6
> >> [ 7.811950] CPU: 2 PID: 2188 Comm: systemd-udevd Not tainted 4.14.0-next-20171122 #1
> >> [ 7.811953] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
> >> [ 7.811958] task: ffff800036b91c00 task.stack: ffff00000d6f0000
> >> [ 7.811967] pstate: 20000005 (nzCv daif -PAN -UAO)
> >> [ 7.811974] pc : refcount_inc+0x44/0x50
> >> [ 7.811981] lr : refcount_inc+0x44/0x50
> >> [ 7.811984] sp : ffff00000d6f3300
> >> [ 7.811988] x29: ffff00000d6f3300 x28: 0000000000000000
> >> [ 7.811996] x27: 0000000000000003 x26: ffff800037107800
> >> [ 7.812004] x25: 0000000000000001 x24: ffff800035afdc00
> >> [ 7.812012] x23: 0000000000000000 x22: ffff800035dfa600
> >> [ 7.812020] x21: ffff800035afd9b0 x20: ffff800035afd9a4
> >> [ 7.812027] x19: 0000000000000000 x18: 0000000000000000
> >> [ 7.812034] x17: 0000000000000001 x16: 0000000000000019
> >> [ 7.812042] x15: 0000000000000001 x14: 00000000fffffff0
> >> [ 7.812049] x13: ffff0000090ae840 x12: ffff000008fa2d50
> >> [ 7.812057] x11: ffff000008fa2000 x10: ffff0000090ad000
> >> [ 7.812064] x9 : 0000000000000000 x8 : ffff0000090b5c2f
> >> [ 7.812072] x7 : 0000000000000000 x6 : 000000000015ee00
> >> [ 7.812079] x5 : 0000000000000000 x4 : 0000000000000000
> >> [ 7.812087] x3 : ffffffffffffffff x2 : 0000800030047000
> >> [ 7.812094] x1 : ffff800036b91c00 x0 : 000000000000002b
> >> [ 7.812102] Call trace:
> >> [ 7.812109] refcount_inc+0x44/0x50
> >> [ 7.812226] vc4_bo_inc_usecnt+0x84/0x88 [vc4]
> >> [ 7.812310] vc4_prepare_fb+0x40/0xf0 [vc4]
> >> [ 7.812460] drm_atomic_helper_prepare_planes+0x54/0xf0 [drm_kms_helper]
> >> [ 7.812543] vc4_atomic_commit+0x88/0x130 [vc4]
> >> [ 7.812868] drm_atomic_commit+0x48/0x68 [drm]
> >> [ 7.813002] restore_fbdev_mode_atomic+0x1d8/0x1e8 [drm_kms_helper]
> >> [ 7.813113] restore_fbdev_mode+0x28/0x160 [drm_kms_helper]
> >> [ 7.813223] drm_fb_helper_restore_fbdev_mode_unlocked.part.24+0x28/0x90 [drm_kms_helper]
> >> [ 7.813331] drm_fb_helper_set_par+0x54/0xa8 [drm_kms_helper]
> >> [ 7.813346] fbcon_init+0x4e8/0x538
> >> [ 7.813357] visual_init+0xb4/0x108
> >> [ 7.813366] do_bind_con_driver+0x1b8/0x3a0
> >> [ 7.813373] do_take_over_console+0x150/0x1d0
> >> [ 7.813380] do_fbcon_takeover+0x6c/0xf0
> >> [ 7.813387] fbcon_event_notify+0x8fc/0x928
> >> [ 7.813399] notifier_call_chain+0x50/0x90
> >> [ 7.813406] __blocking_notifier_call_chain+0x4c/0x90
> >> [ 7.813413] blocking_notifier_call_chain+0x14/0x20
> >> [ 7.813420] fb_notifier_call_chain+0x1c/0x28
> >> [ 7.813426] register_framebuffer+0x1d0/0x2d8
> >> [ 7.813533] __drm_fb_helper_initial_config_and_unlock+0x1e8/0x350 [drm_kms_helper]
> >> [ 7.813639] drm_fb_helper_initial_config+0x40/0x58 [drm_kms_helper]
> >> [ 7.813747] drm_fbdev_cma_init_with_funcs+0x88/0x158 [drm_kms_helper]
> >> [ 7.813855] drm_fbdev_cma_init+0x14/0x28 [drm_kms_helper]
> >> [ 7.813943] vc4_kms_load+0xa4/0xf0 [vc4]
> >> [ 7.814026] vc4_drm_bind+0x100/0x168 [vc4]
> >> [ 7.814038] try_to_bring_up_master+0x144/0x1a8
> >> [ 7.814044] component_master_add_with_match+0x9c/0xe0
> >> [ 7.814128] vc4_platform_drm_probe+0xb4/0xf0 [vc4]
> >> [ 7.814137] platform_drv_probe+0x58/0xc0
> >> [ 7.814146] driver_probe_device+0x224/0x308
> >> [ 7.814153] __driver_attach+0xac/0xb0
> >> [ 7.814161] bus_for_each_dev+0x64/0xa0
> >> [ 7.814169] driver_attach+0x20/0x28
> >> [ 7.814177] bus_add_driver+0x108/0x228
> >> [ 7.814184] driver_register+0x60/0xf8
> >> [ 7.814190] __platform_driver_register+0x40/0x48
> >> [ 7.814274] vc4_drm_register+0x38/0x1000 [vc4]
> >> [ 7.814283] do_one_initcall+0x38/0x120
> >> [ 7.814295] do_init_module+0x58/0x1b8
> >> [ 7.814304] load_module+0x1fa8/0x2280
> >> [ 7.814311] SyS_finit_module+0xc0/0xd0
> >> [ 7.814318] __sys_trace_return+0x0/0x4
> >> [ 7.814325] ---[ end trace 3348554eb91e19a1 ]---
> >
> > Looks like I didn't test this code with CONFIG_REFCOUNT_FULL enabled :-/.
> >
> > Anyway, can you try to apply the following diff and let me know if it
> > fixes the problem?
> >
> > Thanks,
> >
> > Boris
> >
> > --->8---
> > diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> > index 4ae45d7dac42..2decc8e2c79f 100644
> > --- a/drivers/gpu/drm/vc4/vc4_bo.c
> > +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> > @@ -637,7 +637,8 @@ int vc4_bo_inc_usecnt(struct vc4_bo *bo)
> > mutex_lock(&bo->madv_lock);
> > switch (bo->madv) {
> > case VC4_MADV_WILLNEED:
> > - refcount_inc(&bo->usecnt);
> > + if (!refcount_inc_not_zero(&bo->usecnt))
> > + refcount_set(&bo->usecnt, 1);
>
> This is racy.
Well, in this case it's not, because this section is protected by a
mutex.
> You need a refcount_inc_allow_zero (if that exists).
I searched for something like that, but it seems that there's no such
helper.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [BUG] drm: vc4: refcount_t: increment on 0; use-after-free.
2017-11-22 18:21 ` Boris Brezillon
@ 2017-11-23 7:57 ` Daniel Vetter
2017-11-25 20:57 ` Kees Cook
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2017-11-23 7:57 UTC (permalink / raw)
To: Boris Brezillon, Kees Cook
Cc: Stefan Wahren, David Airlie, dri-devel, Daniel Vetter
On Wed, Nov 22, 2017 at 07:21:00PM +0100, Boris Brezillon wrote:
> On Wed, 22 Nov 2017 19:13:09 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Wed, Nov 22, 2017 at 6:51 PM, Boris Brezillon
> > <boris.brezillon@free-electrons.com> wrote:
> > > Hi Stefan,
> > >
> > > On Wed, 22 Nov 2017 17:43:35 +0100 (CET)
> > > Stefan Wahren <stefan.wahren@i2se.com> wrote:
> > >
> > >> Hi Boris,
> > >> if i boot Raspberry Pi 3 (ARM64, defconfig, linux-next-20171122) with sufficient CMA memory (32 MB), i'll get this warning during boot:
> > >>
> > >> [ 7.623510] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
> > >> [ 7.632453] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
> > >> [ 7.639707] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
> > >> [ 7.647364] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
> > >> [ 7.655451] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
> > >> [ 7.663415] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
> > >> [ 7.730580] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
> > >> [ 7.743965] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
> > >> [ 7.750841] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> > >> [ 7.757620] [drm] Driver supports precise vblank timestamp query.
> > >> [ 7.811775] ------------[ cut here ]------------
> > >> [ 7.811780] refcount_t: increment on 0; use-after-free.
> > >> [ 7.811881] WARNING: CPU: 2 PID: 2188 at lib/refcount.c:153 refcount_inc+0x44/0x50
> > >> [ 7.811884] Modules linked in: vc4(+) cfg80211 cec drm_kms_helper smsc95xx usbnet drm rfkill brcmutil bcm2835_rng rng_core bcm2835_dma crc32_ce i2c_bcm2835 pwm_bcm2835 ip_tables x_tables ipv6
> > >> [ 7.811950] CPU: 2 PID: 2188 Comm: systemd-udevd Not tainted 4.14.0-next-20171122 #1
> > >> [ 7.811953] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
> > >> [ 7.811958] task: ffff800036b91c00 task.stack: ffff00000d6f0000
> > >> [ 7.811967] pstate: 20000005 (nzCv daif -PAN -UAO)
> > >> [ 7.811974] pc : refcount_inc+0x44/0x50
> > >> [ 7.811981] lr : refcount_inc+0x44/0x50
> > >> [ 7.811984] sp : ffff00000d6f3300
> > >> [ 7.811988] x29: ffff00000d6f3300 x28: 0000000000000000
> > >> [ 7.811996] x27: 0000000000000003 x26: ffff800037107800
> > >> [ 7.812004] x25: 0000000000000001 x24: ffff800035afdc00
> > >> [ 7.812012] x23: 0000000000000000 x22: ffff800035dfa600
> > >> [ 7.812020] x21: ffff800035afd9b0 x20: ffff800035afd9a4
> > >> [ 7.812027] x19: 0000000000000000 x18: 0000000000000000
> > >> [ 7.812034] x17: 0000000000000001 x16: 0000000000000019
> > >> [ 7.812042] x15: 0000000000000001 x14: 00000000fffffff0
> > >> [ 7.812049] x13: ffff0000090ae840 x12: ffff000008fa2d50
> > >> [ 7.812057] x11: ffff000008fa2000 x10: ffff0000090ad000
> > >> [ 7.812064] x9 : 0000000000000000 x8 : ffff0000090b5c2f
> > >> [ 7.812072] x7 : 0000000000000000 x6 : 000000000015ee00
> > >> [ 7.812079] x5 : 0000000000000000 x4 : 0000000000000000
> > >> [ 7.812087] x3 : ffffffffffffffff x2 : 0000800030047000
> > >> [ 7.812094] x1 : ffff800036b91c00 x0 : 000000000000002b
> > >> [ 7.812102] Call trace:
> > >> [ 7.812109] refcount_inc+0x44/0x50
> > >> [ 7.812226] vc4_bo_inc_usecnt+0x84/0x88 [vc4]
> > >> [ 7.812310] vc4_prepare_fb+0x40/0xf0 [vc4]
> > >> [ 7.812460] drm_atomic_helper_prepare_planes+0x54/0xf0 [drm_kms_helper]
> > >> [ 7.812543] vc4_atomic_commit+0x88/0x130 [vc4]
> > >> [ 7.812868] drm_atomic_commit+0x48/0x68 [drm]
> > >> [ 7.813002] restore_fbdev_mode_atomic+0x1d8/0x1e8 [drm_kms_helper]
> > >> [ 7.813113] restore_fbdev_mode+0x28/0x160 [drm_kms_helper]
> > >> [ 7.813223] drm_fb_helper_restore_fbdev_mode_unlocked.part.24+0x28/0x90 [drm_kms_helper]
> > >> [ 7.813331] drm_fb_helper_set_par+0x54/0xa8 [drm_kms_helper]
> > >> [ 7.813346] fbcon_init+0x4e8/0x538
> > >> [ 7.813357] visual_init+0xb4/0x108
> > >> [ 7.813366] do_bind_con_driver+0x1b8/0x3a0
> > >> [ 7.813373] do_take_over_console+0x150/0x1d0
> > >> [ 7.813380] do_fbcon_takeover+0x6c/0xf0
> > >> [ 7.813387] fbcon_event_notify+0x8fc/0x928
> > >> [ 7.813399] notifier_call_chain+0x50/0x90
> > >> [ 7.813406] __blocking_notifier_call_chain+0x4c/0x90
> > >> [ 7.813413] blocking_notifier_call_chain+0x14/0x20
> > >> [ 7.813420] fb_notifier_call_chain+0x1c/0x28
> > >> [ 7.813426] register_framebuffer+0x1d0/0x2d8
> > >> [ 7.813533] __drm_fb_helper_initial_config_and_unlock+0x1e8/0x350 [drm_kms_helper]
> > >> [ 7.813639] drm_fb_helper_initial_config+0x40/0x58 [drm_kms_helper]
> > >> [ 7.813747] drm_fbdev_cma_init_with_funcs+0x88/0x158 [drm_kms_helper]
> > >> [ 7.813855] drm_fbdev_cma_init+0x14/0x28 [drm_kms_helper]
> > >> [ 7.813943] vc4_kms_load+0xa4/0xf0 [vc4]
> > >> [ 7.814026] vc4_drm_bind+0x100/0x168 [vc4]
> > >> [ 7.814038] try_to_bring_up_master+0x144/0x1a8
> > >> [ 7.814044] component_master_add_with_match+0x9c/0xe0
> > >> [ 7.814128] vc4_platform_drm_probe+0xb4/0xf0 [vc4]
> > >> [ 7.814137] platform_drv_probe+0x58/0xc0
> > >> [ 7.814146] driver_probe_device+0x224/0x308
> > >> [ 7.814153] __driver_attach+0xac/0xb0
> > >> [ 7.814161] bus_for_each_dev+0x64/0xa0
> > >> [ 7.814169] driver_attach+0x20/0x28
> > >> [ 7.814177] bus_add_driver+0x108/0x228
> > >> [ 7.814184] driver_register+0x60/0xf8
> > >> [ 7.814190] __platform_driver_register+0x40/0x48
> > >> [ 7.814274] vc4_drm_register+0x38/0x1000 [vc4]
> > >> [ 7.814283] do_one_initcall+0x38/0x120
> > >> [ 7.814295] do_init_module+0x58/0x1b8
> > >> [ 7.814304] load_module+0x1fa8/0x2280
> > >> [ 7.814311] SyS_finit_module+0xc0/0xd0
> > >> [ 7.814318] __sys_trace_return+0x0/0x4
> > >> [ 7.814325] ---[ end trace 3348554eb91e19a1 ]---
> > >
> > > Looks like I didn't test this code with CONFIG_REFCOUNT_FULL enabled :-/.
> > >
> > > Anyway, can you try to apply the following diff and let me know if it
> > > fixes the problem?
> > >
> > > Thanks,
> > >
> > > Boris
> > >
> > > --->8---
> > > diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> > > index 4ae45d7dac42..2decc8e2c79f 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_bo.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> > > @@ -637,7 +637,8 @@ int vc4_bo_inc_usecnt(struct vc4_bo *bo)
> > > mutex_lock(&bo->madv_lock);
> > > switch (bo->madv) {
> > > case VC4_MADV_WILLNEED:
> > > - refcount_inc(&bo->usecnt);
> > > + if (!refcount_inc_not_zero(&bo->usecnt))
> > > + refcount_set(&bo->usecnt, 1);
> >
> > This is racy.
>
> Well, in this case it's not, because this section is protected by a
> mutex.
>
> > You need a refcount_inc_allow_zero (if that exists).
>
> I searched for something like that, but it seems that there's no such
> helper.
In that case I think the right thing is to switch to atomic_t. That means
no overflow protection, but if we don't have a "this can be 0, I know what
we're doing" style refcount, then I don't think it's suitable.
Adding Kees to confirm or clarify or ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [BUG] drm: vc4: refcount_t: increment on 0; use-after-free.
2017-11-23 7:57 ` Daniel Vetter
@ 2017-11-25 20:57 ` Kees Cook
2017-11-26 20:02 ` Boris Brezillon
0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2017-11-25 20:57 UTC (permalink / raw)
To: Daniel Vetter, Reshetova, Elena
Cc: Stefan Wahren, Boris Brezillon, David Airlie, dri-devel,
Daniel Vetter
On Wed, Nov 22, 2017 at 11:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Nov 22, 2017 at 07:21:00PM +0100, Boris Brezillon wrote:
>> On Wed, 22 Nov 2017 19:13:09 +0100
>> Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> > On Wed, Nov 22, 2017 at 6:51 PM, Boris Brezillon
>> > <boris.brezillon@free-electrons.com> wrote:
>> > > Hi Stefan,
>> > >
>> > > On Wed, 22 Nov 2017 17:43:35 +0100 (CET)
>> > > Stefan Wahren <stefan.wahren@i2se.com> wrote:
>> > >
>> > >> Hi Boris,
>> > >> if i boot Raspberry Pi 3 (ARM64, defconfig, linux-next-20171122) with sufficient CMA memory (32 MB), i'll get this warning during boot:
>> > >>
>> > >> [ 7.623510] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
>> > >> [ 7.632453] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
>> > >> [ 7.639707] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
>> > >> [ 7.647364] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
>> > >> [ 7.655451] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
>> > >> [ 7.663415] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
>> > >> [ 7.730580] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
>> > >> [ 7.743965] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
>> > >> [ 7.750841] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>> > >> [ 7.757620] [drm] Driver supports precise vblank timestamp query.
>> > >> [ 7.811775] ------------[ cut here ]------------
>> > >> [ 7.811780] refcount_t: increment on 0; use-after-free.
>> > >> [ 7.811881] WARNING: CPU: 2 PID: 2188 at lib/refcount.c:153 refcount_inc+0x44/0x50
>> > >> [ 7.811884] Modules linked in: vc4(+) cfg80211 cec drm_kms_helper smsc95xx usbnet drm rfkill brcmutil bcm2835_rng rng_core bcm2835_dma crc32_ce i2c_bcm2835 pwm_bcm2835 ip_tables x_tables ipv6
>> > >> [ 7.811950] CPU: 2 PID: 2188 Comm: systemd-udevd Not tainted 4.14.0-next-20171122 #1
>> > >> [ 7.811953] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
>> > >> [ 7.811958] task: ffff800036b91c00 task.stack: ffff00000d6f0000
>> > >> [ 7.811967] pstate: 20000005 (nzCv daif -PAN -UAO)
>> > >> [ 7.811974] pc : refcount_inc+0x44/0x50
>> > >> [ 7.811981] lr : refcount_inc+0x44/0x50
>> > >> [ 7.811984] sp : ffff00000d6f3300
>> > >> [ 7.811988] x29: ffff00000d6f3300 x28: 0000000000000000
>> > >> [ 7.811996] x27: 0000000000000003 x26: ffff800037107800
>> > >> [ 7.812004] x25: 0000000000000001 x24: ffff800035afdc00
>> > >> [ 7.812012] x23: 0000000000000000 x22: ffff800035dfa600
>> > >> [ 7.812020] x21: ffff800035afd9b0 x20: ffff800035afd9a4
>> > >> [ 7.812027] x19: 0000000000000000 x18: 0000000000000000
>> > >> [ 7.812034] x17: 0000000000000001 x16: 0000000000000019
>> > >> [ 7.812042] x15: 0000000000000001 x14: 00000000fffffff0
>> > >> [ 7.812049] x13: ffff0000090ae840 x12: ffff000008fa2d50
>> > >> [ 7.812057] x11: ffff000008fa2000 x10: ffff0000090ad000
>> > >> [ 7.812064] x9 : 0000000000000000 x8 : ffff0000090b5c2f
>> > >> [ 7.812072] x7 : 0000000000000000 x6 : 000000000015ee00
>> > >> [ 7.812079] x5 : 0000000000000000 x4 : 0000000000000000
>> > >> [ 7.812087] x3 : ffffffffffffffff x2 : 0000800030047000
>> > >> [ 7.812094] x1 : ffff800036b91c00 x0 : 000000000000002b
>> > >> [ 7.812102] Call trace:
>> > >> [ 7.812109] refcount_inc+0x44/0x50
>> > >> [ 7.812226] vc4_bo_inc_usecnt+0x84/0x88 [vc4]
>> > >> [ 7.812310] vc4_prepare_fb+0x40/0xf0 [vc4]
>> > >> [ 7.812460] drm_atomic_helper_prepare_planes+0x54/0xf0 [drm_kms_helper]
>> > >> [ 7.812543] vc4_atomic_commit+0x88/0x130 [vc4]
>> > >> [ 7.812868] drm_atomic_commit+0x48/0x68 [drm]
>> > >> [ 7.813002] restore_fbdev_mode_atomic+0x1d8/0x1e8 [drm_kms_helper]
>> > >> [ 7.813113] restore_fbdev_mode+0x28/0x160 [drm_kms_helper]
>> > >> [ 7.813223] drm_fb_helper_restore_fbdev_mode_unlocked.part.24+0x28/0x90 [drm_kms_helper]
>> > >> [ 7.813331] drm_fb_helper_set_par+0x54/0xa8 [drm_kms_helper]
>> > >> [ 7.813346] fbcon_init+0x4e8/0x538
>> > >> [ 7.813357] visual_init+0xb4/0x108
>> > >> [ 7.813366] do_bind_con_driver+0x1b8/0x3a0
>> > >> [ 7.813373] do_take_over_console+0x150/0x1d0
>> > >> [ 7.813380] do_fbcon_takeover+0x6c/0xf0
>> > >> [ 7.813387] fbcon_event_notify+0x8fc/0x928
>> > >> [ 7.813399] notifier_call_chain+0x50/0x90
>> > >> [ 7.813406] __blocking_notifier_call_chain+0x4c/0x90
>> > >> [ 7.813413] blocking_notifier_call_chain+0x14/0x20
>> > >> [ 7.813420] fb_notifier_call_chain+0x1c/0x28
>> > >> [ 7.813426] register_framebuffer+0x1d0/0x2d8
>> > >> [ 7.813533] __drm_fb_helper_initial_config_and_unlock+0x1e8/0x350 [drm_kms_helper]
>> > >> [ 7.813639] drm_fb_helper_initial_config+0x40/0x58 [drm_kms_helper]
>> > >> [ 7.813747] drm_fbdev_cma_init_with_funcs+0x88/0x158 [drm_kms_helper]
>> > >> [ 7.813855] drm_fbdev_cma_init+0x14/0x28 [drm_kms_helper]
>> > >> [ 7.813943] vc4_kms_load+0xa4/0xf0 [vc4]
>> > >> [ 7.814026] vc4_drm_bind+0x100/0x168 [vc4]
>> > >> [ 7.814038] try_to_bring_up_master+0x144/0x1a8
>> > >> [ 7.814044] component_master_add_with_match+0x9c/0xe0
>> > >> [ 7.814128] vc4_platform_drm_probe+0xb4/0xf0 [vc4]
>> > >> [ 7.814137] platform_drv_probe+0x58/0xc0
>> > >> [ 7.814146] driver_probe_device+0x224/0x308
>> > >> [ 7.814153] __driver_attach+0xac/0xb0
>> > >> [ 7.814161] bus_for_each_dev+0x64/0xa0
>> > >> [ 7.814169] driver_attach+0x20/0x28
>> > >> [ 7.814177] bus_add_driver+0x108/0x228
>> > >> [ 7.814184] driver_register+0x60/0xf8
>> > >> [ 7.814190] __platform_driver_register+0x40/0x48
>> > >> [ 7.814274] vc4_drm_register+0x38/0x1000 [vc4]
>> > >> [ 7.814283] do_one_initcall+0x38/0x120
>> > >> [ 7.814295] do_init_module+0x58/0x1b8
>> > >> [ 7.814304] load_module+0x1fa8/0x2280
>> > >> [ 7.814311] SyS_finit_module+0xc0/0xd0
>> > >> [ 7.814318] __sys_trace_return+0x0/0x4
>> > >> [ 7.814325] ---[ end trace 3348554eb91e19a1 ]---
>> > >
>> > > Looks like I didn't test this code with CONFIG_REFCOUNT_FULL enabled :-/.
>> > >
>> > > Anyway, can you try to apply the following diff and let me know if it
>> > > fixes the problem?
>> > >
>> > > Thanks,
>> > >
>> > > Boris
>> > >
>> > > --->8---
>> > > diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
>> > > index 4ae45d7dac42..2decc8e2c79f 100644
>> > > --- a/drivers/gpu/drm/vc4/vc4_bo.c
>> > > +++ b/drivers/gpu/drm/vc4/vc4_bo.c
>> > > @@ -637,7 +637,8 @@ int vc4_bo_inc_usecnt(struct vc4_bo *bo)
>> > > mutex_lock(&bo->madv_lock);
>> > > switch (bo->madv) {
>> > > case VC4_MADV_WILLNEED:
>> > > - refcount_inc(&bo->usecnt);
>> > > + if (!refcount_inc_not_zero(&bo->usecnt))
>> > > + refcount_set(&bo->usecnt, 1);
>> >
>> > This is racy.
>>
>> Well, in this case it's not, because this section is protected by a
>> mutex.
>>
>> > You need a refcount_inc_allow_zero (if that exists).
>>
>> I searched for something like that, but it seems that there's no such
>> helper.
>
> In that case I think the right thing is to switch to atomic_t. That means
> no overflow protection, but if we don't have a "this can be 0, I know what
> we're doing" style refcount, then I don't think it's suitable.
>
> Adding Kees to confirm or clarify or ...
Adding Elena for more details. IIRC there have been two cases:
1) refcount == 0 doesn't mean the object has been released. The
solution tends to be a +1 to the count everywhere.
2) refcount == 0 means the object was released, but the refcount gets
reused for the next allocation. The solution tends to be to use
refcount_set() in the new initialization instead of refcount_inc().
It's not clear to me if either apply here, though.
-Kees
--
Kees Cook
Pixel Security
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [BUG] drm: vc4: refcount_t: increment on 0; use-after-free.
2017-11-25 20:57 ` Kees Cook
@ 2017-11-26 20:02 ` Boris Brezillon
2017-11-27 1:40 ` Eric Anholt
2017-11-28 10:25 ` Reshetova, Elena
0 siblings, 2 replies; 10+ messages in thread
From: Boris Brezillon @ 2017-11-26 20:02 UTC (permalink / raw)
To: Kees Cook
Cc: Stefan Wahren, David Airlie, dri-devel, Daniel Vetter,
Reshetova, Elena
Hi Kees,
On Sat, 25 Nov 2017 12:57:04 -0800
Kees Cook <keescook@chromium.org> wrote:
> On Wed, Nov 22, 2017 at 11:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Nov 22, 2017 at 07:21:00PM +0100, Boris Brezillon wrote:
> >> On Wed, 22 Nov 2017 19:13:09 +0100
> >> Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> > On Wed, Nov 22, 2017 at 6:51 PM, Boris Brezillon
> >> > <boris.brezillon@free-electrons.com> wrote:
> >> > > Hi Stefan,
> >> > >
> >> > > On Wed, 22 Nov 2017 17:43:35 +0100 (CET)
> >> > > Stefan Wahren <stefan.wahren@i2se.com> wrote:
> >> > >
> >> > >> Hi Boris,
> >> > >> if i boot Raspberry Pi 3 (ARM64, defconfig, linux-next-20171122) with sufficient CMA memory (32 MB), i'll get this warning during boot:
> >> > >>
> >> > >> [ 7.623510] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
> >> > >> [ 7.632453] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
> >> > >> [ 7.639707] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
> >> > >> [ 7.647364] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
> >> > >> [ 7.655451] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
> >> > >> [ 7.663415] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
> >> > >> [ 7.730580] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
> >> > >> [ 7.743965] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
> >> > >> [ 7.750841] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> >> > >> [ 7.757620] [drm] Driver supports precise vblank timestamp query.
> >> > >> [ 7.811775] ------------[ cut here ]------------
> >> > >> [ 7.811780] refcount_t: increment on 0; use-after-free.
> >> > >> [ 7.811881] WARNING: CPU: 2 PID: 2188 at lib/refcount.c:153 refcount_inc+0x44/0x50
> >> > >> [ 7.811884] Modules linked in: vc4(+) cfg80211 cec drm_kms_helper smsc95xx usbnet drm rfkill brcmutil bcm2835_rng rng_core bcm2835_dma crc32_ce i2c_bcm2835 pwm_bcm2835 ip_tables x_tables ipv6
> >> > >> [ 7.811950] CPU: 2 PID: 2188 Comm: systemd-udevd Not tainted 4.14.0-next-20171122 #1
> >> > >> [ 7.811953] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
> >> > >> [ 7.811958] task: ffff800036b91c00 task.stack: ffff00000d6f0000
> >> > >> [ 7.811967] pstate: 20000005 (nzCv daif -PAN -UAO)
> >> > >> [ 7.811974] pc : refcount_inc+0x44/0x50
> >> > >> [ 7.811981] lr : refcount_inc+0x44/0x50
> >> > >> [ 7.811984] sp : ffff00000d6f3300
> >> > >> [ 7.811988] x29: ffff00000d6f3300 x28: 0000000000000000
> >> > >> [ 7.811996] x27: 0000000000000003 x26: ffff800037107800
> >> > >> [ 7.812004] x25: 0000000000000001 x24: ffff800035afdc00
> >> > >> [ 7.812012] x23: 0000000000000000 x22: ffff800035dfa600
> >> > >> [ 7.812020] x21: ffff800035afd9b0 x20: ffff800035afd9a4
> >> > >> [ 7.812027] x19: 0000000000000000 x18: 0000000000000000
> >> > >> [ 7.812034] x17: 0000000000000001 x16: 0000000000000019
> >> > >> [ 7.812042] x15: 0000000000000001 x14: 00000000fffffff0
> >> > >> [ 7.812049] x13: ffff0000090ae840 x12: ffff000008fa2d50
> >> > >> [ 7.812057] x11: ffff000008fa2000 x10: ffff0000090ad000
> >> > >> [ 7.812064] x9 : 0000000000000000 x8 : ffff0000090b5c2f
> >> > >> [ 7.812072] x7 : 0000000000000000 x6 : 000000000015ee00
> >> > >> [ 7.812079] x5 : 0000000000000000 x4 : 0000000000000000
> >> > >> [ 7.812087] x3 : ffffffffffffffff x2 : 0000800030047000
> >> > >> [ 7.812094] x1 : ffff800036b91c00 x0 : 000000000000002b
> >> > >> [ 7.812102] Call trace:
> >> > >> [ 7.812109] refcount_inc+0x44/0x50
> >> > >> [ 7.812226] vc4_bo_inc_usecnt+0x84/0x88 [vc4]
> >> > >> [ 7.812310] vc4_prepare_fb+0x40/0xf0 [vc4]
> >> > >> [ 7.812460] drm_atomic_helper_prepare_planes+0x54/0xf0 [drm_kms_helper]
> >> > >> [ 7.812543] vc4_atomic_commit+0x88/0x130 [vc4]
> >> > >> [ 7.812868] drm_atomic_commit+0x48/0x68 [drm]
> >> > >> [ 7.813002] restore_fbdev_mode_atomic+0x1d8/0x1e8 [drm_kms_helper]
> >> > >> [ 7.813113] restore_fbdev_mode+0x28/0x160 [drm_kms_helper]
> >> > >> [ 7.813223] drm_fb_helper_restore_fbdev_mode_unlocked.part.24+0x28/0x90 [drm_kms_helper]
> >> > >> [ 7.813331] drm_fb_helper_set_par+0x54/0xa8 [drm_kms_helper]
> >> > >> [ 7.813346] fbcon_init+0x4e8/0x538
> >> > >> [ 7.813357] visual_init+0xb4/0x108
> >> > >> [ 7.813366] do_bind_con_driver+0x1b8/0x3a0
> >> > >> [ 7.813373] do_take_over_console+0x150/0x1d0
> >> > >> [ 7.813380] do_fbcon_takeover+0x6c/0xf0
> >> > >> [ 7.813387] fbcon_event_notify+0x8fc/0x928
> >> > >> [ 7.813399] notifier_call_chain+0x50/0x90
> >> > >> [ 7.813406] __blocking_notifier_call_chain+0x4c/0x90
> >> > >> [ 7.813413] blocking_notifier_call_chain+0x14/0x20
> >> > >> [ 7.813420] fb_notifier_call_chain+0x1c/0x28
> >> > >> [ 7.813426] register_framebuffer+0x1d0/0x2d8
> >> > >> [ 7.813533] __drm_fb_helper_initial_config_and_unlock+0x1e8/0x350 [drm_kms_helper]
> >> > >> [ 7.813639] drm_fb_helper_initial_config+0x40/0x58 [drm_kms_helper]
> >> > >> [ 7.813747] drm_fbdev_cma_init_with_funcs+0x88/0x158 [drm_kms_helper]
> >> > >> [ 7.813855] drm_fbdev_cma_init+0x14/0x28 [drm_kms_helper]
> >> > >> [ 7.813943] vc4_kms_load+0xa4/0xf0 [vc4]
> >> > >> [ 7.814026] vc4_drm_bind+0x100/0x168 [vc4]
> >> > >> [ 7.814038] try_to_bring_up_master+0x144/0x1a8
> >> > >> [ 7.814044] component_master_add_with_match+0x9c/0xe0
> >> > >> [ 7.814128] vc4_platform_drm_probe+0xb4/0xf0 [vc4]
> >> > >> [ 7.814137] platform_drv_probe+0x58/0xc0
> >> > >> [ 7.814146] driver_probe_device+0x224/0x308
> >> > >> [ 7.814153] __driver_attach+0xac/0xb0
> >> > >> [ 7.814161] bus_for_each_dev+0x64/0xa0
> >> > >> [ 7.814169] driver_attach+0x20/0x28
> >> > >> [ 7.814177] bus_add_driver+0x108/0x228
> >> > >> [ 7.814184] driver_register+0x60/0xf8
> >> > >> [ 7.814190] __platform_driver_register+0x40/0x48
> >> > >> [ 7.814274] vc4_drm_register+0x38/0x1000 [vc4]
> >> > >> [ 7.814283] do_one_initcall+0x38/0x120
> >> > >> [ 7.814295] do_init_module+0x58/0x1b8
> >> > >> [ 7.814304] load_module+0x1fa8/0x2280
> >> > >> [ 7.814311] SyS_finit_module+0xc0/0xd0
> >> > >> [ 7.814318] __sys_trace_return+0x0/0x4
> >> > >> [ 7.814325] ---[ end trace 3348554eb91e19a1 ]---
> >> > >
> >> > > Looks like I didn't test this code with CONFIG_REFCOUNT_FULL enabled :-/.
> >> > >
> >> > > Anyway, can you try to apply the following diff and let me know if it
> >> > > fixes the problem?
> >> > >
> >> > > Thanks,
> >> > >
> >> > > Boris
> >> > >
> >> > > --->8---
> >> > > diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> >> > > index 4ae45d7dac42..2decc8e2c79f 100644
> >> > > --- a/drivers/gpu/drm/vc4/vc4_bo.c
> >> > > +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> >> > > @@ -637,7 +637,8 @@ int vc4_bo_inc_usecnt(struct vc4_bo *bo)
> >> > > mutex_lock(&bo->madv_lock);
> >> > > switch (bo->madv) {
> >> > > case VC4_MADV_WILLNEED:
> >> > > - refcount_inc(&bo->usecnt);
> >> > > + if (!refcount_inc_not_zero(&bo->usecnt))
> >> > > + refcount_set(&bo->usecnt, 1);
> >> >
> >> > This is racy.
> >>
> >> Well, in this case it's not, because this section is protected by a
> >> mutex.
> >>
> >> > You need a refcount_inc_allow_zero (if that exists).
> >>
> >> I searched for something like that, but it seems that there's no such
> >> helper.
> >
> > In that case I think the right thing is to switch to atomic_t. That means
> > no overflow protection, but if we don't have a "this can be 0, I know what
> > we're doing" style refcount, then I don't think it's suitable.
> >
> > Adding Kees to confirm or clarify or ...
>
> Adding Elena for more details. IIRC there have been two cases:
>
> 1) refcount == 0 doesn't mean the object has been released. The
> solution tends to be a +1 to the count everywhere.
>
> 2) refcount == 0 means the object was released, but the refcount gets
> reused for the next allocation. The solution tends to be to use
> refcount_set() in the new initialization instead of refcount_inc().
>
> It's not clear to me if either apply here, though.
I'd say #1, since ->usecnt is not controlling the VC4-BO object
lifetime. This being said, I'm not sure the +1 trick is really
appropriate. Indeed, we're using the
refcount_inc_not_zero()/refcount_dec_not_one() helpers to remove/put
the object from/in a
"mem-pointed-by-obj-can-be-freed-under-mem-pressure" pool (note the
object itself is not freed, just the memory portion that is pointed by
this object). If I apply +1 everywhere then I have no way to handle the
0 -> 1 and 1 -> 0 transitions differently (these become 1 -> 2 and 2 ->
1 transitions, and there's no functions called refcount_dec_not_two()
and refcount_inc_not_one()).
Anyway, if refcount_inc_allow_zero() is not option, maybe I should just
use an atomic_t object (as suggested by Daniel) or a plain unsigned
integer protected by the ->madv.lock (as suggested by Eric). Of course,
that means losing built-in counter-overflow checking and reworking a bit
the VC4-MADV logic, but that's not such a big problem.
Regards,
Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [BUG] drm: vc4: refcount_t: increment on 0; use-after-free.
2017-11-26 20:02 ` Boris Brezillon
@ 2017-11-27 1:40 ` Eric Anholt
2017-11-28 10:25 ` Reshetova, Elena
1 sibling, 0 replies; 10+ messages in thread
From: Eric Anholt @ 2017-11-27 1:40 UTC (permalink / raw)
To: Boris Brezillon, Kees Cook
Cc: Stefan Wahren, David Airlie, Reshetova, Elena, dri-devel,
Daniel Vetter
[-- Attachment #1.1: Type: text/plain, Size: 10090 bytes --]
Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> Hi Kees,
>
> On Sat, 25 Nov 2017 12:57:04 -0800
> Kees Cook <keescook@chromium.org> wrote:
>
>> On Wed, Nov 22, 2017 at 11:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Wed, Nov 22, 2017 at 07:21:00PM +0100, Boris Brezillon wrote:
>> >> On Wed, 22 Nov 2017 19:13:09 +0100
>> >> Daniel Vetter <daniel@ffwll.ch> wrote:
>> >>
>> >> > On Wed, Nov 22, 2017 at 6:51 PM, Boris Brezillon
>> >> > <boris.brezillon@free-electrons.com> wrote:
>> >> > > Hi Stefan,
>> >> > >
>> >> > > On Wed, 22 Nov 2017 17:43:35 +0100 (CET)
>> >> > > Stefan Wahren <stefan.wahren@i2se.com> wrote:
>> >> > >
>> >> > >> Hi Boris,
>> >> > >> if i boot Raspberry Pi 3 (ARM64, defconfig, linux-next-20171122) with sufficient CMA memory (32 MB), i'll get this warning during boot:
>> >> > >>
>> >> > >> [ 7.623510] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
>> >> > >> [ 7.632453] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
>> >> > >> [ 7.639707] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
>> >> > >> [ 7.647364] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
>> >> > >> [ 7.655451] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
>> >> > >> [ 7.663415] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
>> >> > >> [ 7.730580] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
>> >> > >> [ 7.743965] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
>> >> > >> [ 7.750841] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>> >> > >> [ 7.757620] [drm] Driver supports precise vblank timestamp query.
>> >> > >> [ 7.811775] ------------[ cut here ]------------
>> >> > >> [ 7.811780] refcount_t: increment on 0; use-after-free.
>> >> > >> [ 7.811881] WARNING: CPU: 2 PID: 2188 at lib/refcount.c:153 refcount_inc+0x44/0x50
>> >> > >> [ 7.811884] Modules linked in: vc4(+) cfg80211 cec drm_kms_helper smsc95xx usbnet drm rfkill brcmutil bcm2835_rng rng_core bcm2835_dma crc32_ce i2c_bcm2835 pwm_bcm2835 ip_tables x_tables ipv6
>> >> > >> [ 7.811950] CPU: 2 PID: 2188 Comm: systemd-udevd Not tainted 4.14.0-next-20171122 #1
>> >> > >> [ 7.811953] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
>> >> > >> [ 7.811958] task: ffff800036b91c00 task.stack: ffff00000d6f0000
>> >> > >> [ 7.811967] pstate: 20000005 (nzCv daif -PAN -UAO)
>> >> > >> [ 7.811974] pc : refcount_inc+0x44/0x50
>> >> > >> [ 7.811981] lr : refcount_inc+0x44/0x50
>> >> > >> [ 7.811984] sp : ffff00000d6f3300
>> >> > >> [ 7.811988] x29: ffff00000d6f3300 x28: 0000000000000000
>> >> > >> [ 7.811996] x27: 0000000000000003 x26: ffff800037107800
>> >> > >> [ 7.812004] x25: 0000000000000001 x24: ffff800035afdc00
>> >> > >> [ 7.812012] x23: 0000000000000000 x22: ffff800035dfa600
>> >> > >> [ 7.812020] x21: ffff800035afd9b0 x20: ffff800035afd9a4
>> >> > >> [ 7.812027] x19: 0000000000000000 x18: 0000000000000000
>> >> > >> [ 7.812034] x17: 0000000000000001 x16: 0000000000000019
>> >> > >> [ 7.812042] x15: 0000000000000001 x14: 00000000fffffff0
>> >> > >> [ 7.812049] x13: ffff0000090ae840 x12: ffff000008fa2d50
>> >> > >> [ 7.812057] x11: ffff000008fa2000 x10: ffff0000090ad000
>> >> > >> [ 7.812064] x9 : 0000000000000000 x8 : ffff0000090b5c2f
>> >> > >> [ 7.812072] x7 : 0000000000000000 x6 : 000000000015ee00
>> >> > >> [ 7.812079] x5 : 0000000000000000 x4 : 0000000000000000
>> >> > >> [ 7.812087] x3 : ffffffffffffffff x2 : 0000800030047000
>> >> > >> [ 7.812094] x1 : ffff800036b91c00 x0 : 000000000000002b
>> >> > >> [ 7.812102] Call trace:
>> >> > >> [ 7.812109] refcount_inc+0x44/0x50
>> >> > >> [ 7.812226] vc4_bo_inc_usecnt+0x84/0x88 [vc4]
>> >> > >> [ 7.812310] vc4_prepare_fb+0x40/0xf0 [vc4]
>> >> > >> [ 7.812460] drm_atomic_helper_prepare_planes+0x54/0xf0 [drm_kms_helper]
>> >> > >> [ 7.812543] vc4_atomic_commit+0x88/0x130 [vc4]
>> >> > >> [ 7.812868] drm_atomic_commit+0x48/0x68 [drm]
>> >> > >> [ 7.813002] restore_fbdev_mode_atomic+0x1d8/0x1e8 [drm_kms_helper]
>> >> > >> [ 7.813113] restore_fbdev_mode+0x28/0x160 [drm_kms_helper]
>> >> > >> [ 7.813223] drm_fb_helper_restore_fbdev_mode_unlocked.part.24+0x28/0x90 [drm_kms_helper]
>> >> > >> [ 7.813331] drm_fb_helper_set_par+0x54/0xa8 [drm_kms_helper]
>> >> > >> [ 7.813346] fbcon_init+0x4e8/0x538
>> >> > >> [ 7.813357] visual_init+0xb4/0x108
>> >> > >> [ 7.813366] do_bind_con_driver+0x1b8/0x3a0
>> >> > >> [ 7.813373] do_take_over_console+0x150/0x1d0
>> >> > >> [ 7.813380] do_fbcon_takeover+0x6c/0xf0
>> >> > >> [ 7.813387] fbcon_event_notify+0x8fc/0x928
>> >> > >> [ 7.813399] notifier_call_chain+0x50/0x90
>> >> > >> [ 7.813406] __blocking_notifier_call_chain+0x4c/0x90
>> >> > >> [ 7.813413] blocking_notifier_call_chain+0x14/0x20
>> >> > >> [ 7.813420] fb_notifier_call_chain+0x1c/0x28
>> >> > >> [ 7.813426] register_framebuffer+0x1d0/0x2d8
>> >> > >> [ 7.813533] __drm_fb_helper_initial_config_and_unlock+0x1e8/0x350 [drm_kms_helper]
>> >> > >> [ 7.813639] drm_fb_helper_initial_config+0x40/0x58 [drm_kms_helper]
>> >> > >> [ 7.813747] drm_fbdev_cma_init_with_funcs+0x88/0x158 [drm_kms_helper]
>> >> > >> [ 7.813855] drm_fbdev_cma_init+0x14/0x28 [drm_kms_helper]
>> >> > >> [ 7.813943] vc4_kms_load+0xa4/0xf0 [vc4]
>> >> > >> [ 7.814026] vc4_drm_bind+0x100/0x168 [vc4]
>> >> > >> [ 7.814038] try_to_bring_up_master+0x144/0x1a8
>> >> > >> [ 7.814044] component_master_add_with_match+0x9c/0xe0
>> >> > >> [ 7.814128] vc4_platform_drm_probe+0xb4/0xf0 [vc4]
>> >> > >> [ 7.814137] platform_drv_probe+0x58/0xc0
>> >> > >> [ 7.814146] driver_probe_device+0x224/0x308
>> >> > >> [ 7.814153] __driver_attach+0xac/0xb0
>> >> > >> [ 7.814161] bus_for_each_dev+0x64/0xa0
>> >> > >> [ 7.814169] driver_attach+0x20/0x28
>> >> > >> [ 7.814177] bus_add_driver+0x108/0x228
>> >> > >> [ 7.814184] driver_register+0x60/0xf8
>> >> > >> [ 7.814190] __platform_driver_register+0x40/0x48
>> >> > >> [ 7.814274] vc4_drm_register+0x38/0x1000 [vc4]
>> >> > >> [ 7.814283] do_one_initcall+0x38/0x120
>> >> > >> [ 7.814295] do_init_module+0x58/0x1b8
>> >> > >> [ 7.814304] load_module+0x1fa8/0x2280
>> >> > >> [ 7.814311] SyS_finit_module+0xc0/0xd0
>> >> > >> [ 7.814318] __sys_trace_return+0x0/0x4
>> >> > >> [ 7.814325] ---[ end trace 3348554eb91e19a1 ]---
>> >> > >
>> >> > > Looks like I didn't test this code with CONFIG_REFCOUNT_FULL enabled :-/.
>> >> > >
>> >> > > Anyway, can you try to apply the following diff and let me know if it
>> >> > > fixes the problem?
>> >> > >
>> >> > > Thanks,
>> >> > >
>> >> > > Boris
>> >> > >
>> >> > > --->8---
>> >> > > diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
>> >> > > index 4ae45d7dac42..2decc8e2c79f 100644
>> >> > > --- a/drivers/gpu/drm/vc4/vc4_bo.c
>> >> > > +++ b/drivers/gpu/drm/vc4/vc4_bo.c
>> >> > > @@ -637,7 +637,8 @@ int vc4_bo_inc_usecnt(struct vc4_bo *bo)
>> >> > > mutex_lock(&bo->madv_lock);
>> >> > > switch (bo->madv) {
>> >> > > case VC4_MADV_WILLNEED:
>> >> > > - refcount_inc(&bo->usecnt);
>> >> > > + if (!refcount_inc_not_zero(&bo->usecnt))
>> >> > > + refcount_set(&bo->usecnt, 1);
>> >> >
>> >> > This is racy.
>> >>
>> >> Well, in this case it's not, because this section is protected by a
>> >> mutex.
>> >>
>> >> > You need a refcount_inc_allow_zero (if that exists).
>> >>
>> >> I searched for something like that, but it seems that there's no such
>> >> helper.
>> >
>> > In that case I think the right thing is to switch to atomic_t. That means
>> > no overflow protection, but if we don't have a "this can be 0, I know what
>> > we're doing" style refcount, then I don't think it's suitable.
>> >
>> > Adding Kees to confirm or clarify or ...
>>
>> Adding Elena for more details. IIRC there have been two cases:
>>
>> 1) refcount == 0 doesn't mean the object has been released. The
>> solution tends to be a +1 to the count everywhere.
>>
>> 2) refcount == 0 means the object was released, but the refcount gets
>> reused for the next allocation. The solution tends to be to use
>> refcount_set() in the new initialization instead of refcount_inc().
>>
>> It's not clear to me if either apply here, though.
>
> I'd say #1, since ->usecnt is not controlling the VC4-BO object
> lifetime. This being said, I'm not sure the +1 trick is really
> appropriate. Indeed, we're using the
> refcount_inc_not_zero()/refcount_dec_not_one() helpers to remove/put
> the object from/in a
> "mem-pointed-by-obj-can-be-freed-under-mem-pressure" pool (note the
> object itself is not freed, just the memory portion that is pointed by
> this object). If I apply +1 everywhere then I have no way to handle the
> 0 -> 1 and 1 -> 0 transitions differently (these become 1 -> 2 and 2 ->
> 1 transitions, and there's no functions called refcount_dec_not_two()
> and refcount_inc_not_one()).
>
> Anyway, if refcount_inc_allow_zero() is not option, maybe I should just
> use an atomic_t object (as suggested by Daniel) or a plain unsigned
> integer protected by the ->madv.lock (as suggested by Eric). Of course,
> that means losing built-in counter-overflow checking and reworking a bit
> the VC4-MADV logic, but that's not such a big problem.
I was profiling minetest last week, and the madv lock was at least 10%
of the kernel overhead, since we need to grab it for each BO that goes
idle->busy and also each BO that goes busy->idle in a job. Given that
we don't reallocate out of the userspace BO cache until it goes idle,
that's potentially a lot of BOs per job, and we'd probably be better off
with the global bo_lock.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [BUG] drm: vc4: refcount_t: increment on 0; use-after-free.
2017-11-26 20:02 ` Boris Brezillon
2017-11-27 1:40 ` Eric Anholt
@ 2017-11-28 10:25 ` Reshetova, Elena
1 sibling, 0 replies; 10+ messages in thread
From: Reshetova, Elena @ 2017-11-28 10:25 UTC (permalink / raw)
To: Boris Brezillon, Kees Cook
Cc: Stefan Wahren, David Airlie, dri-devel, Vetter, Daniel
Hi and sorry for the delayed reply!
> Hi Kees,
>
> On Sat, 25 Nov 2017 12:57:04 -0800
> Kees Cook <keescook@chromium.org> wrote:
>
> > On Wed, Nov 22, 2017 at 11:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Wed, Nov 22, 2017 at 07:21:00PM +0100, Boris Brezillon wrote:
> > >> On Wed, 22 Nov 2017 19:13:09 +0100
> > >> Daniel Vetter <daniel@ffwll.ch> wrote:
> > >>
> > >> > On Wed, Nov 22, 2017 at 6:51 PM, Boris Brezillon
> > >> > <boris.brezillon@free-electrons.com> wrote:
> > >> > > Hi Stefan,
> > >> > >
> > >> > > On Wed, 22 Nov 2017 17:43:35 +0100 (CET)
> > >> > > Stefan Wahren <stefan.wahren@i2se.com> wrote:
> > >> > >
> > >> > >> Hi Boris,
> > >> > >> if i boot Raspberry Pi 3 (ARM64, defconfig, linux-next-20171122) with
> sufficient CMA memory (32 MB), i'll get this warning during boot:
> > >> > >>
> > >> > >> [ 7.623510] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops
> [vc4])
> > >> > >> [ 7.632453] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops
> [vc4])
> > >> > >> [ 7.639707] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops
> [vc4])
> > >> > >> [ 7.647364] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops
> vc4_crtc_ops [vc4])
> > >> > >> [ 7.655451] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops
> vc4_crtc_ops [vc4])
> > >> > >> [ 7.663415] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops
> vc4_crtc_ops [vc4])
> > >> > >> [ 7.730580] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops
> [vc4])
> > >> > >> [ 7.743965] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
> > >> > >> [ 7.750841] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> > >> > >> [ 7.757620] [drm] Driver supports precise vblank timestamp query.
> > >> > >> [ 7.811775] ------------[ cut here ]------------
> > >> > >> [ 7.811780] refcount_t: increment on 0; use-after-free.
> > >> > >> [ 7.811881] WARNING: CPU: 2 PID: 2188 at lib/refcount.c:153
> refcount_inc+0x44/0x50
> > >> > >> [ 7.811884] Modules linked in: vc4(+) cfg80211 cec drm_kms_helper
> smsc95xx usbnet drm rfkill brcmutil bcm2835_rng rng_core bcm2835_dma crc32_ce
> i2c_bcm2835 pwm_bcm2835 ip_tables x_tables ipv6
> > >> > >> [ 7.811950] CPU: 2 PID: 2188 Comm: systemd-udevd Not tainted 4.14.0-
> next-20171122 #1
> > >> > >> [ 7.811953] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
> > >> > >> [ 7.811958] task: ffff800036b91c00 task.stack: ffff00000d6f0000
> > >> > >> [ 7.811967] pstate: 20000005 (nzCv daif -PAN -UAO)
> > >> > >> [ 7.811974] pc : refcount_inc+0x44/0x50
> > >> > >> [ 7.811981] lr : refcount_inc+0x44/0x50
> > >> > >> [ 7.811984] sp : ffff00000d6f3300
> > >> > >> [ 7.811988] x29: ffff00000d6f3300 x28: 0000000000000000
> > >> > >> [ 7.811996] x27: 0000000000000003 x26: ffff800037107800
> > >> > >> [ 7.812004] x25: 0000000000000001 x24: ffff800035afdc00
> > >> > >> [ 7.812012] x23: 0000000000000000 x22: ffff800035dfa600
> > >> > >> [ 7.812020] x21: ffff800035afd9b0 x20: ffff800035afd9a4
> > >> > >> [ 7.812027] x19: 0000000000000000 x18: 0000000000000000
> > >> > >> [ 7.812034] x17: 0000000000000001 x16: 0000000000000019
> > >> > >> [ 7.812042] x15: 0000000000000001 x14: 00000000fffffff0
> > >> > >> [ 7.812049] x13: ffff0000090ae840 x12: ffff000008fa2d50
> > >> > >> [ 7.812057] x11: ffff000008fa2000 x10: ffff0000090ad000
> > >> > >> [ 7.812064] x9 : 0000000000000000 x8 : ffff0000090b5c2f
> > >> > >> [ 7.812072] x7 : 0000000000000000 x6 : 000000000015ee00
> > >> > >> [ 7.812079] x5 : 0000000000000000 x4 : 0000000000000000
> > >> > >> [ 7.812087] x3 : ffffffffffffffff x2 : 0000800030047000
> > >> > >> [ 7.812094] x1 : ffff800036b91c00 x0 : 000000000000002b
> > >> > >> [ 7.812102] Call trace:
> > >> > >> [ 7.812109] refcount_inc+0x44/0x50
> > >> > >> [ 7.812226] vc4_bo_inc_usecnt+0x84/0x88 [vc4]
> > >> > >> [ 7.812310] vc4_prepare_fb+0x40/0xf0 [vc4]
> > >> > >> [ 7.812460] drm_atomic_helper_prepare_planes+0x54/0xf0
> [drm_kms_helper]
> > >> > >> [ 7.812543] vc4_atomic_commit+0x88/0x130 [vc4]
> > >> > >> [ 7.812868] drm_atomic_commit+0x48/0x68 [drm]
> > >> > >> [ 7.813002] restore_fbdev_mode_atomic+0x1d8/0x1e8
> [drm_kms_helper]
> > >> > >> [ 7.813113] restore_fbdev_mode+0x28/0x160 [drm_kms_helper]
> > >> > >> [ 7.813223]
> drm_fb_helper_restore_fbdev_mode_unlocked.part.24+0x28/0x90
> [drm_kms_helper]
> > >> > >> [ 7.813331] drm_fb_helper_set_par+0x54/0xa8 [drm_kms_helper]
> > >> > >> [ 7.813346] fbcon_init+0x4e8/0x538
> > >> > >> [ 7.813357] visual_init+0xb4/0x108
> > >> > >> [ 7.813366] do_bind_con_driver+0x1b8/0x3a0
> > >> > >> [ 7.813373] do_take_over_console+0x150/0x1d0
> > >> > >> [ 7.813380] do_fbcon_takeover+0x6c/0xf0
> > >> > >> [ 7.813387] fbcon_event_notify+0x8fc/0x928
> > >> > >> [ 7.813399] notifier_call_chain+0x50/0x90
> > >> > >> [ 7.813406] __blocking_notifier_call_chain+0x4c/0x90
> > >> > >> [ 7.813413] blocking_notifier_call_chain+0x14/0x20
> > >> > >> [ 7.813420] fb_notifier_call_chain+0x1c/0x28
> > >> > >> [ 7.813426] register_framebuffer+0x1d0/0x2d8
> > >> > >> [ 7.813533] __drm_fb_helper_initial_config_and_unlock+0x1e8/0x350
> [drm_kms_helper]
> > >> > >> [ 7.813639] drm_fb_helper_initial_config+0x40/0x58 [drm_kms_helper]
> > >> > >> [ 7.813747] drm_fbdev_cma_init_with_funcs+0x88/0x158
> [drm_kms_helper]
> > >> > >> [ 7.813855] drm_fbdev_cma_init+0x14/0x28 [drm_kms_helper]
> > >> > >> [ 7.813943] vc4_kms_load+0xa4/0xf0 [vc4]
> > >> > >> [ 7.814026] vc4_drm_bind+0x100/0x168 [vc4]
> > >> > >> [ 7.814038] try_to_bring_up_master+0x144/0x1a8
> > >> > >> [ 7.814044] component_master_add_with_match+0x9c/0xe0
> > >> > >> [ 7.814128] vc4_platform_drm_probe+0xb4/0xf0 [vc4]
> > >> > >> [ 7.814137] platform_drv_probe+0x58/0xc0
> > >> > >> [ 7.814146] driver_probe_device+0x224/0x308
> > >> > >> [ 7.814153] __driver_attach+0xac/0xb0
> > >> > >> [ 7.814161] bus_for_each_dev+0x64/0xa0
> > >> > >> [ 7.814169] driver_attach+0x20/0x28
> > >> > >> [ 7.814177] bus_add_driver+0x108/0x228
> > >> > >> [ 7.814184] driver_register+0x60/0xf8
> > >> > >> [ 7.814190] __platform_driver_register+0x40/0x48
> > >> > >> [ 7.814274] vc4_drm_register+0x38/0x1000 [vc4]
> > >> > >> [ 7.814283] do_one_initcall+0x38/0x120
> > >> > >> [ 7.814295] do_init_module+0x58/0x1b8
> > >> > >> [ 7.814304] load_module+0x1fa8/0x2280
> > >> > >> [ 7.814311] SyS_finit_module+0xc0/0xd0
> > >> > >> [ 7.814318] __sys_trace_return+0x0/0x4
> > >> > >> [ 7.814325] ---[ end trace 3348554eb91e19a1 ]---
> > >> > >
> > >> > > Looks like I didn't test this code with CONFIG_REFCOUNT_FULL enabled :-/.
> > >> > >
> > >> > > Anyway, can you try to apply the following diff and let me know if it
> > >> > > fixes the problem?
> > >> > >
> > >> > > Thanks,
> > >> > >
> > >> > > Boris
> > >> > >
> > >> > > --->8---
> > >> > > diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> > >> > > index 4ae45d7dac42..2decc8e2c79f 100644
> > >> > > --- a/drivers/gpu/drm/vc4/vc4_bo.c
> > >> > > +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> > >> > > @@ -637,7 +637,8 @@ int vc4_bo_inc_usecnt(struct vc4_bo *bo)
> > >> > > mutex_lock(&bo->madv_lock);
> > >> > > switch (bo->madv) {
> > >> > > case VC4_MADV_WILLNEED:
> > >> > > - refcount_inc(&bo->usecnt);
> > >> > > + if (!refcount_inc_not_zero(&bo->usecnt))
> > >> > > + refcount_set(&bo->usecnt, 1);
> > >> >
> > >> > This is racy.
> > >>
> > >> Well, in this case it's not, because this section is protected by a
> > >> mutex.
> > >>
> > >> > You need a refcount_inc_allow_zero (if that exists).
> > >>
> > >> I searched for something like that, but it seems that there's no such
> > >> helper.
> > >
> > > In that case I think the right thing is to switch to atomic_t. That means
> > > no overflow protection, but if we don't have a "this can be 0, I know what
> > > we're doing" style refcount, then I don't think it's suitable.
> > >
> > > Adding Kees to confirm or clarify or ...
> >
> > Adding Elena for more details. IIRC there have been two cases:
> >
> > 1) refcount == 0 doesn't mean the object has been released. The
> > solution tends to be a +1 to the count everywhere.
> >
> > 2) refcount == 0 means the object was released, but the refcount gets
> > reused for the next allocation. The solution tends to be to use
> > refcount_set() in the new initialization instead of refcount_inc().
> >
> > It's not clear to me if either apply here, though.
>
> I'd say #1, since ->usecnt is not controlling the VC4-BO object
> lifetime. This being said, I'm not sure the +1 trick is really
> appropriate. Indeed, we're using the
> refcount_inc_not_zero()/refcount_dec_not_one() helpers to remove/put
> the object from/in a
> "mem-pointed-by-obj-can-be-freed-under-mem-pressure" pool (note the
> object itself is not freed, just the memory portion that is pointed by
> this object). If I apply +1 everywhere then I have no way to handle the
> 0 -> 1 and 1 -> 0 transitions differently (these become 1 -> 2 and 2 ->
> 1 transitions, and there's no functions called refcount_dec_not_two()
> and refcount_inc_not_one()).
>
> Anyway, if refcount_inc_allow_zero() is not option, maybe I should just
> use an atomic_t object (as suggested by Daniel) or a plain unsigned
> integer protected by the ->madv.lock (as suggested by Eric). Of course,
> that means losing built-in counter-overflow checking and reworking a bit
> the VC4-MADV logic, but that's not such a big problem.
So, yes, refcount_inc_allow_zero() is really not an option since the idea is to
clearly separate cases when a reference counter is initialized and when it is
incremented.
Kees outlined two common strategies on how we usually handle such cases,
but of course we need to look if it even make sense in each concrete case.
So, if you say that ->usecnt is not controlling an object lifetime (as in classical
refcounter notion), then maybe the correct way is to really revert the conversion and
go back to atomic_t. Overflow protection will be lost, but if you don't care
about it in this particular case and your usage pattern for this counter is different,
probably no need to make things more complex.
Best Regards,
Elena.
>
> Regards,
>
> Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-11-28 10:25 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-22 16:43 [BUG] drm: vc4: refcount_t: increment on 0; use-after-free Stefan Wahren
2017-11-22 17:51 ` Boris Brezillon
2017-11-22 18:13 ` Daniel Vetter
2017-11-22 18:21 ` Boris Brezillon
2017-11-23 7:57 ` Daniel Vetter
2017-11-25 20:57 ` Kees Cook
2017-11-26 20:02 ` Boris Brezillon
2017-11-27 1:40 ` Eric Anholt
2017-11-28 10:25 ` Reshetova, Elena
2017-11-22 18:17 ` Stefan Wahren
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.