Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf

2015-09-10 Thread Shawn Lin 

在 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(>lock);
 goto end;
 }
-   mutex_unlock(>lock);

 handle = ion_handle_create(client, buffer);
-   if (IS_ERR(handle))
+   if (IS_ERR(handle)) {
+   mutex_unlock(>lock);
 goto end;
+   }

-   mutex_lock(>lock);
 ret = ion_handle_add(client, handle);
 mutex_unlock(>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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf

2015-09-10 Thread 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(>lock);
 goto end;
 }
-   mutex_unlock(>lock);

 handle = ion_handle_create(client, buffer);
-   if (IS_ERR(handle))
+   if (IS_ERR(handle)) {
+   mutex_unlock(>lock);
 goto end;
+   }

-   mutex_lock(>lock);
 ret = ion_handle_add(client, handle);
 mutex_unlock(>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 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf

2015-09-10 Thread 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(>lock);
 goto end;
 }
-   mutex_unlock(>lock);

 handle = ion_handle_create(client, buffer);
-   if (IS_ERR(handle))
+   if (IS_ERR(handle)) {
+   mutex_unlock(>lock);
 goto end;
+   }

-   mutex_lock(>lock);
 ret = ion_handle_add(client, handle);
 mutex_unlock(>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 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf

2015-09-10 Thread Shawn Lin 

在 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(>lock);
 goto end;
 }
-   mutex_unlock(>lock);

 handle = ion_handle_create(client, buffer);
-   if (IS_ERR(handle))
+   if (IS_ERR(handle)) {
+   mutex_unlock(>lock);
 goto end;
+   }

-   mutex_lock(>lock);
 ret = ion_handle_add(client, handle);
 mutex_unlock(>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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf

2015-09-09 Thread Colin Cross 
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(>lock);
>> goto end;
>> }
>> -   mutex_unlock(>lock);
>>
>> handle = ion_handle_create(client, buffer);
>> -   if (IS_ERR(handle))
>> +   if (IS_ERR(handle)) {
>> +   mutex_unlock(>lock);
>> goto end;
>> +   }
>>
>> -   mutex_lock(>lock);
>> ret = ion_handle_add(client, handle);
>> mutex_unlock(>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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf

2015-09-09 Thread Laura Abbott 

(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(>lock);
goto end;
}
-   mutex_unlock(>lock);

handle = ion_handle_create(client, buffer);
-   if (IS_ERR(handle))
+   if (IS_ERR(handle)) {
+   mutex_unlock(>lock);
goto end;
+   }

-   mutex_lock(>lock);
ret = ion_handle_add(client, handle);
mutex_unlock(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: ion: fix corruption of ion_import_dma_buf

2015-09-09 Thread Shawn Lin 
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(>lock);
goto end;
}
-   mutex_unlock(>lock);
 
handle = ion_handle_create(client, buffer);
-   if (IS_ERR(handle))
+   if (IS_ERR(handle)) {
+   mutex_unlock(>lock);
goto end;
+   }
 
-   mutex_lock(>lock);
ret = ion_handle_add(client, handle);
mutex_unlock(>lock);
if (ret) {
-- 
2.3.7


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: ion: fix corruption of ion_import_dma_buf

2015-09-09 Thread Shawn Lin 
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(>lock);
goto end;
}
-   mutex_unlock(>lock);
 
handle = ion_handle_create(client, buffer);
-   if (IS_ERR(handle))
+   if (IS_ERR(handle)) {
+   mutex_unlock(>lock);
goto end;
+   }
 
-   mutex_lock(>lock);
ret = ion_handle_add(client, handle);
mutex_unlock(>lock);
if (ret) {
-- 
2.3.7


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf

2015-09-09 Thread Colin Cross 
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(>lock);
>> goto end;
>> }
>> -   mutex_unlock(>lock);
>>
>> handle = ion_handle_create(client, buffer);
>> -   if (IS_ERR(handle))
>> +   if (IS_ERR(handle)) {
>> +   mutex_unlock(>lock);
>> goto end;
>> +   }
>>
>> -   mutex_lock(>lock);
>> ret = ion_handle_add(client, handle);
>> mutex_unlock(>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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf

2015-09-09 Thread Laura Abbott 

(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(>lock);
goto end;
}
-   mutex_unlock(>lock);

handle = ion_handle_create(client, buffer);
-   if (IS_ERR(handle))
+   if (IS_ERR(handle)) {
+   mutex_unlock(>lock);
goto end;
+   }

-   mutex_lock(>lock);
ret = ion_handle_add(client, handle);
mutex_unlock(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/