Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf
在 2015/9/11 0:44, Laura Abbott 写道: On 09/09/2015 10:41 PM, Colin Cross wrote: On Wed, Sep 9, 2015 at 10:19 AM, Laura Abbott wrote: (adding Colin and John) On 09/09/2015 12:41 AM, Shawn Lin wrote: we found this issue but still exit in lastest kernel. Simply keep ion_handle_create under mutex_lock to avoid this race. WARNING: CPU: 2 PID: 2648 at drivers/staging/android/ion/ion.c:512 ion_handle_add+0xb4/0xc0() ion_handle_add: buffer already found. Modules linked in: iwlmvm iwlwifi mac80211 cfg80211 compat CPU: 2 PID: 2648 Comm: TimedEventQueue Tainted: GW3.14.0 #7 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9 811d7fd3 9a3efd88 0a58 812208a0 0200 80e128d4 80e128d4 8d4ae00c a8cd8600 a8cd8094 9a3efd74 80935e0e 0009 9a3efd6c 811d7fd3 9a3efd88 9a3efd9c Call Trace: [<80faf273>] dump_stack+0x48/0x69 [<80935dc9>] warn_slowpath_common+0x79/0x90 [<80e128d4>] ? ion_handle_add+0xb4/0xc0 [<80e128d4>] ? ion_handle_add+0xb4/0xc0 [<80935e0e>] warn_slowpath_fmt+0x2e/0x30 [<80e128d4>] ion_handle_add+0xb4/0xc0 [<80e144cc>] ion_import_dma_buf+0x8c/0x110 [<80c517c4>] reg_init+0x364/0x7d0 [<80993363>] ? futex_wait+0x123/0x210 [<80992e0e>] ? get_futex_key+0x16e/0x1e0 [<8099308f>] ? futex_wake+0x5f/0x120 [<80c51e19>] vpu_service_ioctl+0x1e9/0x500 [<80994aec>] ? do_futex+0xec/0x8e0 [<80971080>] ? prepare_to_wait_event+0xc0/0xc0 [<80c51c30>] ? reg_init+0x7d0/0x7d0 [<80a22562>] do_vfs_ioctl+0x2d2/0x4c0 [<80b198ad>] ? inode_has_perm.isra.41+0x2d/0x40 [<80b199cf>] ? file_has_perm+0x7f/0x90 [<80b1a5f7>] ? selinux_file_ioctl+0x47/0xf0 [<80a227a8>] SyS_ioctl+0x58/0x80 [<80fb45e8>] syscall_call+0x7/0x7 [<80fb>] ? mmc_do_calc_max_discard+0xab/0xe4 Fixes: 83271f626 ("ion: hold reference to handle...") Signed-off-by: Shawn Lin --- drivers/staging/android/ion/ion.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index eec878e..32e7b5c 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -1179,13 +1179,13 @@ struct ion_handle *ion_import_dma_buf(struct ion_client *client, int fd) mutex_unlock(&client->lock); goto end; } - mutex_unlock(&client->lock); handle = ion_handle_create(client, buffer); - if (IS_ERR(handle)) + if (IS_ERR(handle)) { + mutex_unlock(&client->lock); goto end; + } - mutex_lock(&client->lock); ret = ion_handle_add(client, handle); mutex_unlock(&client->lock); if (ret) { So the patch looks correct but the locking change there seems like it was added deliberately. Colin/John, do you remember why the locking for ion_import_dma_buf changed? Was there a deadlock condition somewhere? Thanks, Laura I can't see any reason to not hold the mutex across ion_handle_create. The patch that introduced the bug (83271f6262c91a49df325c52bec8f00f4de294ca, ion: hold reference to handle after ion_uhandle_get) required that the mutex not be held around the call to ion_handle_put, but didn't affect ion_handle_create. Thanks for confirming. With that, Thanks for reviewing this patch, Laura. :) Reviewed-by: Laura Abbott -- Best Regards Shawn Lin ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf
On 09/09/2015 10:41 PM, Colin Cross wrote: On Wed, Sep 9, 2015 at 10:19 AM, Laura Abbott wrote: (adding Colin and John) On 09/09/2015 12:41 AM, Shawn Lin wrote: we found this issue but still exit in lastest kernel. Simply keep ion_handle_create under mutex_lock to avoid this race. WARNING: CPU: 2 PID: 2648 at drivers/staging/android/ion/ion.c:512 ion_handle_add+0xb4/0xc0() ion_handle_add: buffer already found. Modules linked in: iwlmvm iwlwifi mac80211 cfg80211 compat CPU: 2 PID: 2648 Comm: TimedEventQueue Tainted: GW3.14.0 #7 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9 811d7fd3 9a3efd88 0a58 812208a0 0200 80e128d4 80e128d4 8d4ae00c a8cd8600 a8cd8094 9a3efd74 80935e0e 0009 9a3efd6c 811d7fd3 9a3efd88 9a3efd9c Call Trace: [<80faf273>] dump_stack+0x48/0x69 [<80935dc9>] warn_slowpath_common+0x79/0x90 [<80e128d4>] ? ion_handle_add+0xb4/0xc0 [<80e128d4>] ? ion_handle_add+0xb4/0xc0 [<80935e0e>] warn_slowpath_fmt+0x2e/0x30 [<80e128d4>] ion_handle_add+0xb4/0xc0 [<80e144cc>] ion_import_dma_buf+0x8c/0x110 [<80c517c4>] reg_init+0x364/0x7d0 [<80993363>] ? futex_wait+0x123/0x210 [<80992e0e>] ? get_futex_key+0x16e/0x1e0 [<8099308f>] ? futex_wake+0x5f/0x120 [<80c51e19>] vpu_service_ioctl+0x1e9/0x500 [<80994aec>] ? do_futex+0xec/0x8e0 [<80971080>] ? prepare_to_wait_event+0xc0/0xc0 [<80c51c30>] ? reg_init+0x7d0/0x7d0 [<80a22562>] do_vfs_ioctl+0x2d2/0x4c0 [<80b198ad>] ? inode_has_perm.isra.41+0x2d/0x40 [<80b199cf>] ? file_has_perm+0x7f/0x90 [<80b1a5f7>] ? selinux_file_ioctl+0x47/0xf0 [<80a227a8>] SyS_ioctl+0x58/0x80 [<80fb45e8>] syscall_call+0x7/0x7 [<80fb>] ? mmc_do_calc_max_discard+0xab/0xe4 Fixes: 83271f626 ("ion: hold reference to handle...") Signed-off-by: Shawn Lin --- drivers/staging/android/ion/ion.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index eec878e..32e7b5c 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -1179,13 +1179,13 @@ struct ion_handle *ion_import_dma_buf(struct ion_client *client, int fd) mutex_unlock(&client->lock); goto end; } - mutex_unlock(&client->lock); handle = ion_handle_create(client, buffer); - if (IS_ERR(handle)) + if (IS_ERR(handle)) { + mutex_unlock(&client->lock); goto end; + } - mutex_lock(&client->lock); ret = ion_handle_add(client, handle); mutex_unlock(&client->lock); if (ret) { So the patch looks correct but the locking change there seems like it was added deliberately. Colin/John, do you remember why the locking for ion_import_dma_buf changed? Was there a deadlock condition somewhere? Thanks, Laura I can't see any reason to not hold the mutex across ion_handle_create. The patch that introduced the bug (83271f6262c91a49df325c52bec8f00f4de294ca, ion: hold reference to handle after ion_uhandle_get) required that the mutex not be held around the call to ion_handle_put, but didn't affect ion_handle_create. Thanks for confirming. With that, Reviewed-by: Laura Abbott ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf
On Wed, Sep 9, 2015 at 10:19 AM, Laura Abbott wrote: > (adding Colin and John) > > > On 09/09/2015 12:41 AM, Shawn Lin wrote: >> >> we found this issue but still exit in lastest kernel. Simply >> keep ion_handle_create under mutex_lock to avoid this race. >> >> WARNING: CPU: 2 PID: 2648 at drivers/staging/android/ion/ion.c:512 >> ion_handle_add+0xb4/0xc0() >> ion_handle_add: buffer already found. >> Modules linked in: iwlmvm iwlwifi mac80211 cfg80211 compat >> CPU: 2 PID: 2648 Comm: TimedEventQueue Tainted: GW3.14.0 #7 >> 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9 811d7fd3 >> 9a3efd88 0a58 812208a0 0200 80e128d4 80e128d4 8d4ae00c a8cd8600 >> a8cd8094 9a3efd74 80935e0e 0009 9a3efd6c 811d7fd3 9a3efd88 9a3efd9c >> Call Trace: >>[<80faf273>] dump_stack+0x48/0x69 >>[<80935dc9>] warn_slowpath_common+0x79/0x90 >>[<80e128d4>] ? ion_handle_add+0xb4/0xc0 >>[<80e128d4>] ? ion_handle_add+0xb4/0xc0 >>[<80935e0e>] warn_slowpath_fmt+0x2e/0x30 >>[<80e128d4>] ion_handle_add+0xb4/0xc0 >>[<80e144cc>] ion_import_dma_buf+0x8c/0x110 >>[<80c517c4>] reg_init+0x364/0x7d0 >>[<80993363>] ? futex_wait+0x123/0x210 >>[<80992e0e>] ? get_futex_key+0x16e/0x1e0 >>[<8099308f>] ? futex_wake+0x5f/0x120 >>[<80c51e19>] vpu_service_ioctl+0x1e9/0x500 >>[<80994aec>] ? do_futex+0xec/0x8e0 >>[<80971080>] ? prepare_to_wait_event+0xc0/0xc0 >>[<80c51c30>] ? reg_init+0x7d0/0x7d0 >>[<80a22562>] do_vfs_ioctl+0x2d2/0x4c0 >>[<80b198ad>] ? inode_has_perm.isra.41+0x2d/0x40 >>[<80b199cf>] ? file_has_perm+0x7f/0x90 >>[<80b1a5f7>] ? selinux_file_ioctl+0x47/0xf0 >>[<80a227a8>] SyS_ioctl+0x58/0x80 >>[<80fb45e8>] syscall_call+0x7/0x7 >>[<80fb>] ? mmc_do_calc_max_discard+0xab/0xe4 >> >> Fixes: 83271f626 ("ion: hold reference to handle...") >> Signed-off-by: Shawn Lin >> --- >> >> drivers/staging/android/ion/ion.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/staging/android/ion/ion.c >> b/drivers/staging/android/ion/ion.c >> index eec878e..32e7b5c 100644 >> --- a/drivers/staging/android/ion/ion.c >> +++ b/drivers/staging/android/ion/ion.c >> @@ -1179,13 +1179,13 @@ struct ion_handle *ion_import_dma_buf(struct >> ion_client *client, int fd) >> mutex_unlock(&client->lock); >> goto end; >> } >> - mutex_unlock(&client->lock); >> >> handle = ion_handle_create(client, buffer); >> - if (IS_ERR(handle)) >> + if (IS_ERR(handle)) { >> + mutex_unlock(&client->lock); >> goto end; >> + } >> >> - mutex_lock(&client->lock); >> ret = ion_handle_add(client, handle); >> mutex_unlock(&client->lock); >> if (ret) { >> > > So the patch looks correct but the locking change there seems like it was > added > deliberately. Colin/John, do you remember why the locking for > ion_import_dma_buf > changed? Was there a deadlock condition somewhere? > > Thanks, > Laura I can't see any reason to not hold the mutex across ion_handle_create. The patch that introduced the bug (83271f6262c91a49df325c52bec8f00f4de294ca, ion: hold reference to handle after ion_uhandle_get) required that the mutex not be held around the call to ion_handle_put, but didn't affect ion_handle_create. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf
(adding Colin and John) On 09/09/2015 12:41 AM, Shawn Lin wrote: we found this issue but still exit in lastest kernel. Simply keep ion_handle_create under mutex_lock to avoid this race. WARNING: CPU: 2 PID: 2648 at drivers/staging/android/ion/ion.c:512 ion_handle_add+0xb4/0xc0() ion_handle_add: buffer already found. Modules linked in: iwlmvm iwlwifi mac80211 cfg80211 compat CPU: 2 PID: 2648 Comm: TimedEventQueue Tainted: GW3.14.0 #7 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9 811d7fd3 9a3efd88 0a58 812208a0 0200 80e128d4 80e128d4 8d4ae00c a8cd8600 a8cd8094 9a3efd74 80935e0e 0009 9a3efd6c 811d7fd3 9a3efd88 9a3efd9c Call Trace: [<80faf273>] dump_stack+0x48/0x69 [<80935dc9>] warn_slowpath_common+0x79/0x90 [<80e128d4>] ? ion_handle_add+0xb4/0xc0 [<80e128d4>] ? ion_handle_add+0xb4/0xc0 [<80935e0e>] warn_slowpath_fmt+0x2e/0x30 [<80e128d4>] ion_handle_add+0xb4/0xc0 [<80e144cc>] ion_import_dma_buf+0x8c/0x110 [<80c517c4>] reg_init+0x364/0x7d0 [<80993363>] ? futex_wait+0x123/0x210 [<80992e0e>] ? get_futex_key+0x16e/0x1e0 [<8099308f>] ? futex_wake+0x5f/0x120 [<80c51e19>] vpu_service_ioctl+0x1e9/0x500 [<80994aec>] ? do_futex+0xec/0x8e0 [<80971080>] ? prepare_to_wait_event+0xc0/0xc0 [<80c51c30>] ? reg_init+0x7d0/0x7d0 [<80a22562>] do_vfs_ioctl+0x2d2/0x4c0 [<80b198ad>] ? inode_has_perm.isra.41+0x2d/0x40 [<80b199cf>] ? file_has_perm+0x7f/0x90 [<80b1a5f7>] ? selinux_file_ioctl+0x47/0xf0 [<80a227a8>] SyS_ioctl+0x58/0x80 [<80fb45e8>] syscall_call+0x7/0x7 [<80fb>] ? mmc_do_calc_max_discard+0xab/0xe4 Fixes: 83271f626 ("ion: hold reference to handle...") Signed-off-by: Shawn Lin --- drivers/staging/android/ion/ion.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index eec878e..32e7b5c 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -1179,13 +1179,13 @@ struct ion_handle *ion_import_dma_buf(struct ion_client *client, int fd) mutex_unlock(&client->lock); goto end; } - mutex_unlock(&client->lock); handle = ion_handle_create(client, buffer); - if (IS_ERR(handle)) + if (IS_ERR(handle)) { + mutex_unlock(&client->lock); goto end; + } - mutex_lock(&client->lock); ret = ion_handle_add(client, handle); mutex_unlock(&client->lock); if (ret) { So the patch looks correct but the locking change there seems like it was added deliberately. Colin/John, do you remember why the locking for ion_import_dma_buf changed? Was there a deadlock condition somewhere? Thanks, Laura ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: ion: fix corruption of ion_import_dma_buf
we found this issue but still exit in lastest kernel. Simply keep ion_handle_create under mutex_lock to avoid this race. WARNING: CPU: 2 PID: 2648 at drivers/staging/android/ion/ion.c:512 ion_handle_add+0xb4/0xc0() ion_handle_add: buffer already found. Modules linked in: iwlmvm iwlwifi mac80211 cfg80211 compat CPU: 2 PID: 2648 Comm: TimedEventQueue Tainted: GW3.14.0 #7 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9 811d7fd3 9a3efd88 0a58 812208a0 0200 80e128d4 80e128d4 8d4ae00c a8cd8600 a8cd8094 9a3efd74 80935e0e 0009 9a3efd6c 811d7fd3 9a3efd88 9a3efd9c Call Trace: [<80faf273>] dump_stack+0x48/0x69 [<80935dc9>] warn_slowpath_common+0x79/0x90 [<80e128d4>] ? ion_handle_add+0xb4/0xc0 [<80e128d4>] ? ion_handle_add+0xb4/0xc0 [<80935e0e>] warn_slowpath_fmt+0x2e/0x30 [<80e128d4>] ion_handle_add+0xb4/0xc0 [<80e144cc>] ion_import_dma_buf+0x8c/0x110 [<80c517c4>] reg_init+0x364/0x7d0 [<80993363>] ? futex_wait+0x123/0x210 [<80992e0e>] ? get_futex_key+0x16e/0x1e0 [<8099308f>] ? futex_wake+0x5f/0x120 [<80c51e19>] vpu_service_ioctl+0x1e9/0x500 [<80994aec>] ? do_futex+0xec/0x8e0 [<80971080>] ? prepare_to_wait_event+0xc0/0xc0 [<80c51c30>] ? reg_init+0x7d0/0x7d0 [<80a22562>] do_vfs_ioctl+0x2d2/0x4c0 [<80b198ad>] ? inode_has_perm.isra.41+0x2d/0x40 [<80b199cf>] ? file_has_perm+0x7f/0x90 [<80b1a5f7>] ? selinux_file_ioctl+0x47/0xf0 [<80a227a8>] SyS_ioctl+0x58/0x80 [<80fb45e8>] syscall_call+0x7/0x7 [<80fb>] ? mmc_do_calc_max_discard+0xab/0xe4 Fixes: 83271f626 ("ion: hold reference to handle...") Signed-off-by: Shawn Lin --- drivers/staging/android/ion/ion.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index eec878e..32e7b5c 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -1179,13 +1179,13 @@ struct ion_handle *ion_import_dma_buf(struct ion_client *client, int fd) mutex_unlock(&client->lock); goto end; } - mutex_unlock(&client->lock); handle = ion_handle_create(client, buffer); - if (IS_ERR(handle)) + if (IS_ERR(handle)) { + mutex_unlock(&client->lock); goto end; + } - mutex_lock(&client->lock); ret = ion_handle_add(client, handle); mutex_unlock(&client->lock); if (ret) { -- 2.3.7 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel