From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751378AbbIJXvt (ORCPT ); Thu, 10 Sep 2015 19:51:49 -0400 Received: from lucky1.263xmail.com ([211.157.147.132]:48164 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750743AbbIJXvs (ORCPT ); Thu, 10 Sep 2015 19:51:48 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: shawn.lin@rock-chips.com X-FST-TO: devel@driverdev.osuosl.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: shawn.lin@rock-chips.com X-UNIQUE-TAG: <4a226962b1388122b753ca326bbc25c0> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf To: Laura Abbott , Colin Cross References: <1441784512-2534-1-git-send-email-shawn.lin@rock-chips.com> <55F06A11.1010605@redhat.com> <55F1B36C.1050605@redhat.com> Cc: shawn.lin@rock-chips.com, Greg Kroah-Hartman , =?UTF-8?Q?Arve_Hj=c3=b8nnev=c3=a5g?= , Riley Andrews , John Stultz , lkml , "devel@driverdev.osuosl.org" From: Shawn Lin Message-ID: <55F2177D.7000701@rock-chips.com> Date: Fri, 11 Sep 2015 07:51:25 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <55F1B36C.1050605@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 在 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: 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 >>>> --- >>>> >>>> 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