tty/n_gsm.c: fix a memory leak when gsmtty is removed

PATCH In review Pan Xinhui 2 participants 3 messages Mar 23, 2015 – Mar 24, 2015 TTY layer and serial drivers

Share & tools

Pan Xinhui

author

2015-03-23 07:23 UTC #0 lore raw

PATCH 1/1 tty/n_gsm.c: fix a memory leak when gsmtty is removed

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 <[email protected]>

---
 drivers/tty/n_gsm.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

drivers/tty/n_gsm.c +14 -2

index c434376..50f4660 100644
@@ -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);
 	driver->ttys[tty->index] = NULL;
 }

Signature · 1 line

1.9.1

Jiri Slaby

2015-03-23 15:23 UTC #1 lore raw

Quoted 41 lines · Pan Xinhui

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 <[email protected]>

---
 drivers/tty/n_gsm.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

drivers/tty/n_gsm.c +6 -2

index c434376..50f4660 100644
@@ -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);

drivers/tty/n_gsm.c static int gsmtty_break_ctl(struct tty_struct · line 3195

Quoted 10 lines

drivers/tty/n_gsm.c +8 -0

     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,

Signature · 2 lines

js
suse labs

Pan Xinhui

author

2015-03-24 02:43 UTC #2 lore raw

hi, Jiri
thanks for your kind reply, and the nice advices. :)

Quoted 47 lines · Jiri Slaby

Quoted 46 lines · Pan Xinhui

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 <[email protected]>

---
  drivers/tty/n_gsm.c | 16 ++++++++++++++--
  1 file changed, 14 insertions(+), 2 deletions(-)

drivers/tty/n_gsm.c +9 -2

index c434376..50f4660 100644
@@ -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)

Quoted 9 lines

drivers/tty/n_gsm.c static int gsmtty_break_ctl(struct tty_struct *tty, int state)
Quoted 5 lines
drivers/tty/n_gsm.c +5 -0
+{
+    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.

Quoted 3 lines

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.

Quoted 6 lines

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

Quoted 2 lines

thanks,