From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752839AbbCWPXH (ORCPT ); Mon, 23 Mar 2015 11:23:07 -0400 Received: from mail-wi0-f173.google.com ([209.85.212.173]:38035 "EHLO mail-wi0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752541AbbCWPXF (ORCPT ); Mon, 23 Mar 2015 11:23:05 -0400 Message-ID: <55102FD5.5050605@suse.cz> Date: Mon, 23 Mar 2015 16:23:01 +0100 From: Jiri Slaby User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Pan Xinhui , 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> In-Reply-To: <551110AC.9000306@intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. If I am looking correctly the work can be moved from ->remove to ->cleanup, right? 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. thanks, -- js suse labs