Re: [PATCH] tty/n_gsm.c: fix a memory leak when gsmtty is removed

From: Pan Xinhui
Date: Mon Mar 23 2015 - 22:43:57 EST


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 <xinhuix.pan@xxxxxxxxx>
---
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,

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