* [PATCH] tty/n_gsm.c: fix a memory leak when gsmtty is removed
@ 2015-03-24 7:26 Pan Xinhui
2015-03-25 7:05 ` [PATCH v2] " Pan Xinhui
0 siblings, 1 reply; 5+ messages in thread
From: Pan Xinhui @ 2015-03-24 7:26 UTC (permalink / raw)
To: gregkh, jslaby, linux-kernel; +Cc: yanmin_zhang, mnipxh
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] tty/n_gsm.c: fix a memory leak when gsmtty is removed
2015-03-24 7:26 [PATCH] tty/n_gsm.c: fix a memory leak when gsmtty is removed Pan Xinhui
@ 2015-03-25 7:05 ` Pan Xinhui
2015-03-26 21:36 ` Greg KH
2015-03-28 2:42 ` [PATCH v2 RESEND] " Pan Xinhui
0 siblings, 2 replies; 5+ messages in thread
From: Pan Xinhui @ 2015-03-25 7:05 UTC (permalink / raw)
To: gregkh, jslaby, linux-kernel; +Cc: yanmin_zhang, mnipxh
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] tty/n_gsm.c: fix a memory leak when gsmtty is removed
2015-03-25 7:05 ` [PATCH v2] " Pan Xinhui
@ 2015-03-26 21:36 ` Greg KH
2015-03-28 2:42 ` [PATCH v2 RESEND] " Pan Xinhui
1 sibling, 0 replies; 5+ messages in thread
From: Greg KH @ 2015-03-26 21:36 UTC (permalink / raw)
To: Pan Xinhui; +Cc: jslaby, linux-kernel, yanmin_zhang, mnipxh
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 RESEND] tty/n_gsm.c: fix a memory leak when gsmtty is removed
2015-03-28 2:42 ` [PATCH v2 RESEND] " Pan Xinhui
@ 2015-03-27 8:51 ` Jiri Slaby
0 siblings, 0 replies; 5+ messages in thread
From: Jiri Slaby @ 2015-03-27 8:51 UTC (permalink / raw)
To: Pan Xinhui, gregkh, linux-kernel; +Cc: yanmin_zhang, mnipxh
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 RESEND] tty/n_gsm.c: fix a memory leak when gsmtty is removed
2015-03-25 7:05 ` [PATCH v2] " Pan Xinhui
2015-03-26 21:36 ` Greg KH
@ 2015-03-28 2:42 ` Pan Xinhui
2015-03-27 8:51 ` Jiri Slaby
1 sibling, 1 reply; 5+ messages in thread
From: Pan Xinhui @ 2015-03-28 2:42 UTC (permalink / raw)
To: gregkh, jslaby, linux-kernel; +Cc: yanmin_zhang, mnipxh
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-03-27 8:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-24 7:26 [PATCH] tty/n_gsm.c: fix a memory leak when gsmtty is removed Pan Xinhui
2015-03-25 7:05 ` [PATCH v2] " Pan Xinhui
2015-03-26 21:36 ` Greg KH
2015-03-28 2:42 ` [PATCH v2 RESEND] " Pan Xinhui
2015-03-27 8:51 ` Jiri Slaby
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.