From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753575AbbCXCnz (ORCPT ); Mon, 23 Mar 2015 22:43:55 -0400 Received: from mga03.intel.com ([134.134.136.65]:55192 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752258AbbCXCnw (ORCPT ); Mon, 23 Mar 2015 22:43:52 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,455,1422950400"; d="scan'208";a="696692917" Message-ID: <551220B4.3050507@intel.com> Date: Wed, 25 Mar 2015 10:43:00 +0800 From: Pan Xinhui User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Jiri Slaby , gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org CC: mnipxh@gmail.com, yanmin_zhang@linux.intel.com Subject: Re: [PATCH] tty/n_gsm.c: fix a memory leak when gsmtty is removed References: <551110AC.9000306@intel.com> <55102FD5.5050605@suse.cz> In-Reply-To: <55102FD5.5050605@suse.cz> 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 hi, Jiri thanks for your kind reply, and the nice advices. :) On 2015年03月23日 23:23, Jiri Slaby wrote: > On 03/24/2015, 08:22 AM, Pan Xinhui wrote: >> In gsmtty_remove, we will put dlci. when dlci's ref-count is zero, >> tty_port_destructor will be called, and it will check if port->itty is >> NULL. >> However port->itty will be set to NULL in release_tty after gsmtty_remove. >> that may cause memory leak. so we use queue_work to put the dlci later. >> >> Signed-off-by: xinhui.pan >> --- >> drivers/tty/n_gsm.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c >> index c434376..50f4660 100644 >> --- a/drivers/tty/n_gsm.c >> +++ b/drivers/tty/n_gsm.c >> @@ -135,6 +135,7 @@ struct gsm_dlci { >> #define DLCI_OPEN 2 /* SABM/UA complete */ >> #define DLCI_CLOSING 3 /* Sending DISC not seen UA/DM */ >> struct mutex mutex; >> + struct work_struct putself_work; >> >> /* Link layer */ >> spinlock_t lock; /* Protects the internal state */ >> @@ -3170,14 +3171,25 @@ static int gsmtty_break_ctl(struct tty_struct >> *tty, int state) >> return gsmtty_modem_update(dlci, encode); >> } >> >> -static void gsmtty_remove(struct tty_driver *driver, struct tty_struct >> *tty) >> +static void put_gsm_dlci(struct work_struct *work) >> { >> - struct gsm_dlci *dlci = tty->driver_data; >> + struct gsm_dlci *dlci = >> + container_of(work, struct gsm_dlci, putself_work); >> struct gsm_mux *gsm = dlci->gsm; >> >> + mutex_lock(&gsm->mutex); >> dlci_put(dlci); >> dlci_put(gsm->dlci[0]); >> + mutex_unlock(&gsm->mutex); >> mux_put(gsm); >> +} >> + >> +static void gsmtty_remove(struct tty_driver *driver, struct tty_struct >> *tty) >> +{ >> + struct gsm_dlci *dlci = tty->driver_data; >> + >> + INIT_WORK(&dlci->putself_work, put_gsm_dlci); >> + schedule_work(&dlci->putself_work) > > I am afraid you cannot guarantee it is "late enough" by this approach. > The work can be already running before itty is set to NULL. > you are right, we still cannot guarantee that. Although the test shows Okay. thanks for pointing out it. > If I am looking correctly the work can be moved from ->remove to > ->cleanup, right? > thanks for your advice. I will upload pathchV2 after a full test. > And it would be worth to add a Fixes line to the commit log. > dfabf7ffa30585 introduced this. > > And could you describe the scenario when it happens to the commit log > too? Like closing the other end first, before the tty. > currently dlci will be put by 1) gsmld_close --> gsm_cleanup_mux -> gsm_dlci_release -> dlci_put and 2) gsmld_remove -> dlci_put so there is a race. the memory leak depends on the race. Is my comment above that you need know? sorry for my poor English. thanks, xinhui > thanks, >