Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1221288 > unrolled thread

[PATCH] staging: ion: fix corruption of ion_import_dma_buf

Started byShawn Lin <shawn.lin@rock-chips.com>
First post2015-09-09 09:50 +0200
Last post2015-09-11 02:00 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] staging: ion: fix corruption of ion_import_dma_buf Shawn Lin <shawn.lin@rock-chips.com> - 2015-09-09 09:50 +0200
    Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf Laura Abbott <labbott@redhat.com> - 2015-09-09 19:20 +0200
      Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf Colin Cross <ccross@android.com> - 2015-09-10 07:50 +0200
        Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf Laura Abbott <labbott@redhat.com> - 2015-09-10 18:50 +0200
          Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf Shawn Lin <shawn.lin@rock-chips.com> - 2015-09-11 02:00 +0200

#1221288 — [PATCH] staging: ion: fix corruption of ion_import_dma_buf

FromShawn Lin <shawn.lin@rock-chips.com>
Date2015-09-09 09:50 +0200
Subject[PATCH] staging: ion: fix corruption of ion_import_dma_buf
Message-ID<q6IiK-529-19@gated-at.bofh.it>
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: G        W    3.14.0 #7
 00000000 00000000 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9 811d7fd3
 9a3efd88 00000a58 812208a0 00000200 80e128d4 80e128d4 8d4ae00c a8cd8600
 a8cd8094 9a3efd74 80935e0e 00000009 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
  [<80fb0000>] ? mmc_do_calc_max_discard+0xab/0xe4

Fixes: 83271f626 ("ion: hold reference to handle...")
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 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


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

[toc] | [next] | [standalone]

#1221573

FromLaura Abbott <labbott@redhat.com>
Date2015-09-09 19:20 +0200
Message-ID<q6Rcl-150-1@gated-at.bofh.it>
In reply to#1221288
(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: G        W    3.14.0 #7
>   00000000 00000000 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9 811d7fd3
>   9a3efd88 00000a58 812208a0 00000200 80e128d4 80e128d4 8d4ae00c a8cd8600
>   a8cd8094 9a3efd74 80935e0e 00000009 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
>    [<80fb0000>] ? mmc_do_calc_max_discard+0xab/0xe4
>
> Fixes: 83271f626 ("ion: hold reference to handle...")
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
>   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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221925

FromColin Cross <ccross@android.com>
Date2015-09-10 07:50 +0200
Message-ID<q72U9-V0-1@gated-at.bofh.it>
In reply to#1221573
On Wed, Sep 9, 2015 at 10:19 AM, Laura Abbott <labbott@redhat.com> 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: G        W    3.14.0 #7
>>   00000000 00000000 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9 811d7fd3
>>   9a3efd88 00000a58 812208a0 00000200 80e128d4 80e128d4 8d4ae00c a8cd8600
>>   a8cd8094 9a3efd74 80935e0e 00000009 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
>>    [<80fb0000>] ? mmc_do_calc_max_discard+0xab/0xe4
>>
>> Fixes: 83271f626 ("ion: hold reference to handle...")
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>>   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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222308

FromLaura Abbott <labbott@redhat.com>
Date2015-09-10 18:50 +0200
Message-ID<q7dcS-79C-9@gated-at.bofh.it>
In reply to#1221925
On 09/09/2015 10:41 PM, Colin Cross wrote:
> On Wed, Sep 9, 2015 at 10:19 AM, Laura Abbott <labbott@redhat.com> 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: G        W    3.14.0 #7
>>>    00000000 00000000 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9 811d7fd3
>>>    9a3efd88 00000a58 812208a0 00000200 80e128d4 80e128d4 8d4ae00c a8cd8600
>>>    a8cd8094 9a3efd74 80935e0e 00000009 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
>>>     [<80fb0000>] ? mmc_do_calc_max_discard+0xab/0xe4
>>>
>>> Fixes: 83271f626 ("ion: hold reference to handle...")
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>> ---
>>>
>>>    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 <labbott@redhat.com>

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

[toc] | [prev] | [next] | [standalone]

#1222436

FromShawn Lin <shawn.lin@rock-chips.com>
Date2015-09-11 02:00 +0200
Message-ID<q7jV0-9E-3@gated-at.bofh.it>
In reply to#1222308
在 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 <labbott@redhat.com> 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: G        W    3.14.0 #7
>>>>    00000000 00000000 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9
>>>> 811d7fd3
>>>>    9a3efd88 00000a58 812208a0 00000200 80e128d4 80e128d4 8d4ae00c
>>>> a8cd8600
>>>>    a8cd8094 9a3efd74 80935e0e 00000009 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
>>>>     [<80fb0000>] ? mmc_do_calc_max_discard+0xab/0xe4
>>>>
>>>> Fixes: 83271f626 ("ion: hold reference to handle...")
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>> ---
>>>>
>>>>    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 <labbott@redhat.com>
>
>
>
>


-- 
Best Regards
Shawn Lin

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web