From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752560AbcAEGfn (ORCPT ); Tue, 5 Jan 2016 01:35:43 -0500 Received: from e35.co.us.ibm.com ([32.97.110.153]:34028 "EHLO e35.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750787AbcAEGfk (ORCPT ); Tue, 5 Jan 2016 01:35:40 -0500 X-IBM-Helo: d03dlp03.boulder.ibm.com X-IBM-MailFrom: xinhui.pan@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Subject: Re: [PATCH resend ] tty/n_gsm.c: use gsm->num to remove mux itself from gsm_mux[] To: xinhui , One Thousand Gnomes References: <566E6AD3.1060003@linux.vnet.ibm.com> <20151214154035.15ebb75a@lxorguk.ukuu.org.uk> <398d6e8b.12b08.151b53d2775.Coremail.mnipxh@163.com> Cc: "linux-kernel@vger.kernel.org" , Greg Kroah-Hartman , Jiri Slaby , "yanmin_zhang@linux.intel.com" From: Pan Xinhui Message-ID: <568B6424.4010702@linux.vnet.ibm.com> Date: Tue, 5 Jan 2016 14:35:16 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <398d6e8b.12b08.151b53d2775.Coremail.mnipxh@163.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16010506-0013-0000-0000-00001B918433 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Alan thanks for your reply :) On 2015/12/18 21:17, xinhui wrote: > hi, Alan > this is xinhui. My eyes got badly hurt, and i am ooo this whole week and next coming week. sorry for late responce. > I just review the codes in my mind. gsm ioctl callback might change gsm->num, so you are right. > i still have many confusion. but tears came out several times:( when i am back, i will reply you again. > > thx > xinhui > > > > On 2015-12-14 23:40 , One Thousand Gnomes Wrote: > > On Mon, 14 Dec 2015 15:08:03 +0800 > Pan Xinhui wrote: > >> From: Pan Xinhui >> >> There is one filed gsm->num to store mux's index of gsm_mux[]. So use >> gsm->num to remove itself from gsm_mux[] instead of the for-loop >> traverse in gsm_cleanup_mux(). >> >> Signed-off-by: Pan Xinhui >> --- >> drivers/tty/n_gsm.c | 14 +++++--------- >> 1 files changed, 5 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c >> index 9aff371..cf28054 100644 >> --- a/drivers/tty/n_gsm.c >> +++ b/drivers/tty/n_gsm.c >> @@ -2037,18 +2037,14 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm) >> >> gsm->dead = 1; >> >> - spin_lock(&gsm_mux_lock); >> - for (i = 0; i < MAX_MUX; i++) { >> - if (gsm_mux[i] == gsm) { >> - gsm_mux[i] = NULL; >> - break; >> - } >> - } >> - spin_unlock(&gsm_mux_lock); >> /* open failed before registering => nothing to do */ >> - if (i == MAX_MUX) >> + if (gsm_mux[gsm->num] != gsm) >> return; >> >> + spin_lock(&gsm_mux_lock); >> + gsm_mux[gsm->num] = NULL; >> + spin_unlock(&gsm_mux_lock); > > Its a highly theoretical and probably impossible corner case but I can't > help thinking the lock should be held for the if () as well as NULLing > this out. > yes, gsm_mux[] must be touched with gsm_mux_lock held. I am still wondering if it's possible that two gsm_cleanup_mux() run on the same mux. seems gsmld_config() -> gsm_cleanup_mux() might have race with gsmld_detach_gsm() -> gsm_cleanup_mux(). what's more, we need make sure gsm_mux[gsm->num] == gsm, as if there is a new mux put into gsm_mux[], we might NULL this new mux out. here is one possible race. CPUA CPUB CPUC in cleanup() in cleanup() in activate() if (gsm_mux[gsm->num] != gsm) if (gsm_mux[gsm->num] != gsm) .. ... spin_lock(&gsm_mux_lock); gsm_mux[gsm->num] = NULL; spin_unlock(&gsm_mux_lock); spin_lock(&gsm_mux_lock); ... gsm->num = i; gsm_mux[i] = gsm; ... spin_unlock(&gsm_mux_lock); spin_lock(&gsm_mux_lock); gsm_mux[gsm->num] = NULL;//this NULLing might cause BUGS!! spin_unlock(&gsm_mux_lock); I will send out patch V2 to avoid any possible race. thanks for pointing it out. thanks xinhui > Alan >