Race condition in minigbm drv_create/drv_destroy |
||||
Issue descriptionChrome Version: 61.0.3163.72 OS: 9765.49.0 USB flakiness causes Mimo touchscreen to suddenly disconnect and reconnect. Upon investigating, it seems like drv_destroy and drv_create are not thread-safe. Once Chrome realizes that Mimo is disconnected (due to flakiness), it calls drv_destroy, which is responsible of freeing most data structure that the udl backend uses. However, since it instantly reconnects, drv_create seems to run before drv_destroy does. This causes memory leak because udl_backend structs are malloced again without freeing first. More importantly, drv_destroy frees those structs (when it shouldn't) and forces DrmThread to undefined behavior. What steps will reproduce the problem? (1) Boot up CfM. (2) Observe flickery Mimo. (3) Disconnecting Displayport or HDMI causes Mimo to freeze or not come back up. What is the expected result? DisplayPort and HDMI hotpluggin don't cause any problems. Mimo doesn't flicker. What happens instead? Mimo flickers or freezes. I'm in the process of submitting a CL to fix this issue.
,
Sep 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/minigbm/+/96b7d461f7bfc979b8c538455bba718066ae01cb commit 96b7d461f7bfc979b8c538455bba718066ae01cb Author: Ege Mihmanli <egemih@google.com> Date: Thu Sep 21 08:08:00 2017 Avoid freeing then accessing driver backend format and usage combinations Due to USB flakiness issues, sometimes udl devices suddenly disconnect then reconnect. In such cases, we see that drv_create runs before drv_destroy can destroy the driver belonging to the disconnected udl device. Once drv_destroy runs, it frees structs frequently used by the newly created driver, most importantly drv->backend->combos->data which stores format and usage combinations for creating BOs. By nature, drv_destroy should not destroy data belonging to backend. Further, BO format and usage combinations are hardcoded in driver _init, so there is no point in it being freed in drv_destroy. This fix moves the combos array to struct drv so a new combos array can be created and destroyed in drv_create and drv_destroy. This eliminates the race condition by avoiding using the shared resource. BUG= crbug.com/766831 TEST=Boot up CfM, make sure ui and chrome logs are not complaining about scanout and framebuffer failures. Play around with Mimo touchscreen to make sure the display is not flickering. Change-Id: I53d57693574daeb5486cd5daca71f660ada635ef Reviewed-on: https://chromium-review.googlesource.com/674528 Commit-Ready: Ege Mihmanli <egemih@google.com> Tested-by: Ege Mihmanli <egemih@google.com> Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org> [modify] https://crrev.com/96b7d461f7bfc979b8c538455bba718066ae01cb/drv.c [modify] https://crrev.com/96b7d461f7bfc979b8c538455bba718066ae01cb/drv_priv.h [modify] https://crrev.com/96b7d461f7bfc979b8c538455bba718066ae01cb/i915.c [modify] https://crrev.com/96b7d461f7bfc979b8c538455bba718066ae01cb/rockchip.c [modify] https://crrev.com/96b7d461f7bfc979b8c538455bba718066ae01cb/helpers.c
,
Sep 25 2017
,
Sep 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/minigbm/+/23f2dcd926ccc218688fcaafc639fa035bff34f0 commit 23f2dcd926ccc218688fcaafc639fa035bff34f0 Author: Ege Mihmanli <egemih@google.com> Date: Tue Sep 26 18:43:21 2017 Avoid freeing then accessing driver backend format and usage combinations Due to USB flakiness issues, sometimes udl devices suddenly disconnect then reconnect. In such cases, we see that drv_create runs before drv_destroy can destroy the driver belonging to the disconnected udl device. Once drv_destroy runs, it frees structs frequently used by the newly created driver, most importantly drv->backend->combos->data which stores format and usage combinations for creating BOs. By nature, drv_destroy should not destroy data belonging to backend. Further, BO format and usage combinations are hardcoded in driver _init, so there is no point in it being freed in drv_destroy. This fix moves the combos array to struct drv so a new combos array can be created and destroyed in drv_create and drv_destroy. This eliminates the race condition by avoiding using the shared resource. BUG= crbug.com/766831 , crbug.com/767536 TEST=Boot up CfM, make sure ui and chrome logs are not complaining about scanout and framebuffer failures. Play around with Mimo touchscreen to make sure the display is not flickering. Change-Id: I53d57693574daeb5486cd5daca71f660ada635ef Reviewed-on: https://chromium-review.googlesource.com/674528 Commit-Ready: Ege Mihmanli <egemih@google.com> Tested-by: Ege Mihmanli <egemih@google.com> Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org> (cherry picked from commit 96b7d461f7bfc979b8c538455bba718066ae01cb) Reviewed-on: https://chromium-review.googlesource.com/684754 Commit-Queue: Ege Mihmanli <egemih@google.com> Reviewed-by: Zhongze Hu <frankhu@google.com> [modify] https://crrev.com/23f2dcd926ccc218688fcaafc639fa035bff34f0/drv.c [modify] https://crrev.com/23f2dcd926ccc218688fcaafc639fa035bff34f0/drv_priv.h [modify] https://crrev.com/23f2dcd926ccc218688fcaafc639fa035bff34f0/i915.c [modify] https://crrev.com/23f2dcd926ccc218688fcaafc639fa035bff34f0/rockchip.c [modify] https://crrev.com/23f2dcd926ccc218688fcaafc639fa035bff34f0/helpers.c
,
Sep 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/minigbm/+/80e96af939f3018b134f814e669bc30b33ef481f commit 80e96af939f3018b134f814e669bc30b33ef481f Author: Ege Mihmanli <egemih@google.com> Date: Wed Sep 27 00:08:33 2017 Avoid freeing then accessing driver backend format and usage combinations Due to USB flakiness issues, sometimes udl devices suddenly disconnect then reconnect. In such cases, we see that drv_create runs before drv_destroy can destroy the driver belonging to the disconnected udl device. Once drv_destroy runs, it frees structs frequently used by the newly created driver, most importantly drv->backend->combos->data which stores format and usage combinations for creating BOs. By nature, drv_destroy should not destroy data belonging to backend. Further, BO format and usage combinations are hardcoded in driver _init, so there is no point in it being freed in drv_destroy. This fix moves the combos array to struct drv so a new combos array can be created and destroyed in drv_create and drv_destroy. This eliminates the race condition by avoiding using the shared resource. BUG= crbug.com/766831 , crbug.com/767536 TEST=Boot up CfM, make sure ui and chrome logs are not complaining about scanout and framebuffer failures. Play around with Mimo touchscreen to make sure the display is not flickering. Change-Id: I53d57693574daeb5486cd5daca71f660ada635ef Reviewed-on: https://chromium-review.googlesource.com/674528 Commit-Ready: Ege Mihmanli <egemih@google.com> Tested-by: Ege Mihmanli <egemih@google.com> Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org> (cherry picked from commit 96b7d461f7bfc979b8c538455bba718066ae01cb) Reviewed-on: https://chromium-review.googlesource.com/685373 Commit-Queue: Ege Mihmanli <egemih@chromium.org> Tested-by: Ege Mihmanli <egemih@chromium.org> Reviewed-by: Zhongze Hu <frankhu@google.com> [modify] https://crrev.com/80e96af939f3018b134f814e669bc30b33ef481f/drv.c [modify] https://crrev.com/80e96af939f3018b134f814e669bc30b33ef481f/drv_priv.h [modify] https://crrev.com/80e96af939f3018b134f814e669bc30b33ef481f/i915.c [modify] https://crrev.com/80e96af939f3018b134f814e669bc30b33ef481f/rockchip.c [modify] https://crrev.com/80e96af939f3018b134f814e669bc30b33ef481f/helpers.c
,
Oct 9 2017
Issue 725526 has been merged into this issue. |
||||
►
Sign in to add a comment |
||||
Comment 1 by egemih@chromium.org
, Sep 20 2017