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

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

[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 <xinhuix.pan@intel.com>
---
  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);
  	driver->ttys[tty->index] = NULL;
  }
  
-- 
1.9.1

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

[Pan Xinhui]

when gsmtty_remove put dlci, it will cause memory leak if
dlci->port's refcount is zero.
So we do the cleanup work in .cleanup callback instead.

dlci will be last put in two call chains.
1) gsmld_close -> gsm_cleanup_mux -> gsm_dlci_release -> dlci_put
2) gsmld_remove -> dlci_put
so there is a race. the memory leak depends on the race.

In call chain 2. we hit the memory leak. bellow comment tells.

release_tty -> tty_driver_remove_tty -> gsmtty_remove -> dlci_put -> tty_port_destructor (WARN_ON(port->itty) and return directly)
						|
						--> tty->port->itty = NULL;
			|
		tty_kref_put ---> release_one_tty -> gsmtty_cleanup (now we do the cleanup work here.)
So our patch fix it by doing the cleanup work after tty core did.

Signed-off-by: xinhui.pan <xinhuix.pan@intel.com>
---
  drivers/tty/n_gsm.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c434376..bce16e4 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3170,7 +3170,7 @@ 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 gsmtty_cleanup(struct tty_struct *tty)
  {
  	struct gsm_dlci *dlci = tty->driver_data;
  	struct gsm_mux *gsm = dlci->gsm;
@@ -3178,7 +3178,6 @@ static void gsmtty_remove(struct tty_driver *driver, struct tty_struct *tty)
  	dlci_put(dlci);
  	dlci_put(gsm->dlci[0]);
  	mux_put(gsm);
-	driver->ttys[tty->index] = NULL;
  }
  
  /* Virtual ttys for the demux */
@@ -3199,7 +3198,7 @@ static const struct tty_operations gsmtty_ops = {
  	.tiocmget		= gsmtty_tiocmget,
  	.tiocmset		= gsmtty_tiocmset,
  	.break_ctl		= gsmtty_break_ctl,
-	.remove			= gsmtty_remove,
+	.cleanup		= gsmtty_cleanup,
  };
  
  
-- 
1.9.1

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

[Greg KH]

On Wed, Mar 25, 2015 at 03:05:33PM +0800, Pan Xinhui wrote:
> when gsmtty_remove put dlci, it will cause memory leak if
> dlci->port's refcount is zero.
> So we do the cleanup work in .cleanup callback instead.
> 
> dlci will be last put in two call chains.
> 1) gsmld_close -> gsm_cleanup_mux -> gsm_dlci_release -> dlci_put
> 2) gsmld_remove -> dlci_put
> so there is a race. the memory leak depends on the race.
> 
> In call chain 2. we hit the memory leak. bellow comment tells.
> 
> release_tty -> tty_driver_remove_tty -> gsmtty_remove -> dlci_put -> tty_port_destructor (WARN_ON(port->itty) and return directly)
> 						|
> 						--> tty->port->itty = NULL;
> 			|
> 		tty_kref_put ---> release_one_tty -> gsmtty_cleanup (now we do the cleanup work here.)

That doesn't line up at all :(

> So our patch fix it by doing the cleanup work after tty core did.
> 
> Signed-off-by: xinhui.pan <xinhuix.pan@intel.com>

I need a real name here, "xinhui.pan" is not your real name, according
to your "From:" line, right?

Please fix up and resend.

And also include what changed from the previous version, this is much
different.

thanks,

greg k-h

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

[Jiri Slaby]

On 03/28/2015, 03:42 AM, Pan Xinhui wrote:
> when gsmtty_remove put dlci, it will cause memory leak if dlci->port's
> refcount is zero.
> So we do the cleanup work in .cleanup callback instead.
> 
> dlci will be last put in two call chains.
> 1) gsmld_close -> gsm_cleanup_mux -> gsm_dlci_release -> dlci_put
> 2) gsmld_remove -> dlci_put
> so there is a race. the memory leak depends on the race.
> 
> In call chain 2. we hit the memory leak. below comment tells.
> 
> release_tty -> tty_driver_remove_tty -> gsmtty_remove -> dlci_put ->
> tty_port_destructor (WARN_ON(port->itty) and return directly)
>                         |
>                tty->port->itty = NULL;
>                         |
>                tty_kref_put ---> release_one_tty -> gsmtty_cleanup
> (added by our patch)
> 
> So our patch fix the memory leak by doing the cleanup work after tty
> core did.
> 
> Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>

Fixes: dfabf7ffa30585
Acked-by: Jiri Slaby <jslaby@suse.cz>

> ---
> Changes in v2:
>  Don't use schedule_work to put dlci. Replace .remove with .cleanup
> callback.
> 
>  drivers/tty/n_gsm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index c434376..bce16e4 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -3170,7 +3170,7 @@ 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 gsmtty_cleanup(struct tty_struct *tty)
>  {
>      struct gsm_dlci *dlci = tty->driver_data;
>      struct gsm_mux *gsm = dlci->gsm;
> @@ -3178,7 +3178,6 @@ static void gsmtty_remove(struct tty_driver
> *driver, struct tty_struct *tty)
>      dlci_put(dlci);
>      dlci_put(gsm->dlci[0]);
>      mux_put(gsm);
> -    driver->ttys[tty->index] = NULL;
>  }
>  
>  /* Virtual ttys for the demux */
> @@ -3199,7 +3198,7 @@ static const struct tty_operations gsmtty_ops = {
>      .tiocmget        = gsmtty_tiocmget,
>      .tiocmset        = gsmtty_tiocmset,
>      .break_ctl        = gsmtty_break_ctl,
> -    .remove            = gsmtty_remove,
> +    .cleanup        = gsmtty_cleanup,
>  };
>  
>  


-- 
js
suse labs

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

[Pan Xinhui]

when gsmtty_remove put dlci, it will cause memory leak if dlci->port's refcount is zero.
So we do the cleanup work in .cleanup callback instead.

dlci will be last put in two call chains.
1) gsmld_close -> gsm_cleanup_mux -> gsm_dlci_release -> dlci_put
2) gsmld_remove -> dlci_put
so there is a race. the memory leak depends on the race.

In call chain 2. we hit the memory leak. below comment tells.

release_tty -> tty_driver_remove_tty -> gsmtty_remove -> dlci_put -> tty_port_destructor (WARN_ON(port->itty) and return directly)
                         |
                tty->port->itty = NULL;
                         |
                tty_kref_put ---> release_one_tty -> gsmtty_cleanup (added by our patch)

So our patch fix the memory leak by doing the cleanup work after tty core did.

Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
---
Changes in v2:
  Don't use schedule_work to put dlci. Replace .remove with .cleanup callback.

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

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c434376..bce16e4 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3170,7 +3170,7 @@ 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 gsmtty_cleanup(struct tty_struct *tty)
  {
  	struct gsm_dlci *dlci = tty->driver_data;
  	struct gsm_mux *gsm = dlci->gsm;
@@ -3178,7 +3178,6 @@ static void gsmtty_remove(struct tty_driver *driver, struct tty_struct *tty)
  	dlci_put(dlci);
  	dlci_put(gsm->dlci[0]);
  	mux_put(gsm);
-	driver->ttys[tty->index] = NULL;
  }
  
  /* Virtual ttys for the demux */
@@ -3199,7 +3198,7 @@ static const struct tty_operations gsmtty_ops = {
  	.tiocmget		= gsmtty_tiocmget,
  	.tiocmset		= gsmtty_tiocmset,
  	.break_ctl		= gsmtty_break_ctl,
-	.remove			= gsmtty_remove,
+	.cleanup		= gsmtty_cleanup,
  };
  
  
-- 
1.9.1

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

[Pan Xinhui]

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@intel.com>
>> ---
>>   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,
>

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

[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 <xinhuix.pan@intel.com>
---
  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);
  	driver->ttys[tty->index] = NULL;
  }
  
-- 
1.9.1

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

[Jiri Slaby]

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@intel.com>
> ---
>  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