tty/n_gsm.c: fix a memory leak in gsmld_open

PATCH In review xinhui.pan 3 participants 7 messages Jul 28, 2014 TTY layer and serial drivers

Share & tools

xinhui.pan

author

2014-07-28 08:16 UTC #0 lore raw

PATCH 1/1 tty/n_gsm.c: fix a memory leak in gsmld_open

If gsmld_attach_gsm fails, the gsm is not used anymore. tty core will not call gsmld_close to do the cleanup work. tty core just restore to the tty old ldisc.
That always causes memory leak.

Signed-off-by xinhui.pan <[email protected]>

Reported-by Peter Hurley <[email protected]>

---
 drivers/tty/n_gsm.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

drivers/tty/n_gsm.c +8 -1

index 81e7ccb..6cb1a6d 100644
@@ -2368,6 +2368,7 @@ static void gsmld_close(struct tty_struct *tty)
 static int gsmld_open(struct tty_struct *tty)
 {
 	struct gsm_mux *gsm;
+	int ret;
 
 	if (tty->ops->write == NULL)
 		return -EINVAL;

@@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
 
 	/* Attach the initial passive connection */
 	gsm->encoding = 1;
-	return gsmld_attach_gsm(tty, gsm);
+
+	ret = gsmld_attach_gsm(tty, gsm);
+	if (ret != 0) {
+		gsm_cleanup_mux(gsm);
+		mux_put(gsm);
+	}
+	return ret;
 }
 
 /**

Signature · 1 line

1.7.9.5

xinhui.pan

author

2014-07-28 08:24 UTC #1 lore raw

hi, All
thanks for reading. Below is my trigger.

----------------------------------------

#define WARN(x) do{		\
	KLOG_ERROR("XINHUI",x);	\
}while(0)

int main()

{
	KLOG_ERROR("XINHUI", "hello world :)\n");
	long fp[4];
	int i = 0;
	int next[4]={1,2,3,0};

	do{
		char path[64] = "dev/tty30";
		path[strlen(path) - 1] = '0' + i;

		fp[i] = open(path, O_RDWR);
		if (fp[i] == -1){
			WARN("mlk tty open fails\n");
			return 0;
		}
	}while(++i < 4);

	i = 0;
	do{
		int p = 21;
		int ret = ioctl(fp[i], TIOCSETD , &p);
		if (ret != 0) {
			WARN("mlk tty ioctl fails\n");
		}
		i = next[i];
	}while(1);

/* never run here, just ctrl^C */

	return 0 ;
}

----------------------------------------

without this patch, running my own tests.

my result:

PROCRANK:
.....
RAM: 1993076K total, 147968K free, 708K buffers, 108340K cached, 332K shmem, 1092232K slab AWESOME!

cat /proc/slabinfo
.....
kmalloc-2048 509700 509700 2048 16 8 : tunables 0 0 0 : slabdata 32285 32285 0
.....
AWESOME!!

thanks,

xinhui

Quoted 39 lines · xinhui.pan

If gsmld_attach_gsm fails, the gsm is not used anymore. tty core will not call gsmld_close to do the cleanup work. tty core just restore to the tty old ldisc.
That always causes memory leak.

Signed-off-by xinhui.pan <[email protected]>

Reported-by Peter Hurley <[email protected]>

---
 drivers/tty/n_gsm.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

drivers/tty/n_gsm.c +8 -1

index 81e7ccb..6cb1a6d 100644
@@ -2368,6 +2368,7 @@ static void gsmld_close(struct tty_struct *tty)
 static int gsmld_open(struct tty_struct *tty)
 {
 	struct gsm_mux *gsm;
+	int ret;
 
 	if (tty->ops->write == NULL)
 		return -EINVAL;

@@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
 
 	/* Attach the initial passive connection */
 	gsm->encoding = 1;
-	return gsmld_attach_gsm(tty, gsm);
+
+	ret = gsmld_attach_gsm(tty, gsm);
+	if (ret != 0) {
+		gsm_cleanup_mux(gsm);
+		mux_put(gsm);
+	}
+	return ret;
 }
 
 /**

Jiri Slaby

2014-07-28 08:49 UTC #2 lore raw

In reply to xinhui.pan

Quoted 4 lines · xinhui.pan

If gsmld_attach_gsm fails, the gsm is not used anymore. tty core will not call gsmld_close to do the cleanup work. tty core just restore to the tty old ldisc.
That always causes memory leak.

Nice catch!

drivers/tty/n_gsm.c static int gsmld_open(struct tty_struct *tty) · line 2390

Quoted 12 lines

drivers/tty/n_gsm.c +5 -1

@@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
 
 	/* Attach the initial passive connection */
 	gsm->encoding = 1;
-	return gsmld_attach_gsm(tty, gsm);
+
+	ret = gsmld_attach_gsm(tty, gsm);
+	if (ret != 0) {
+		gsm_cleanup_mux(gsm);
+		mux_put(gsm);

It is quite illogical to put the mux here. It should be in gsmld_open. I.e. gsm_cleanup_mux here, mux_put there.

thanks,

Signature · 2 lines

js
suse labs

xinhui.pan

author

2014-07-28 09:03 UTC #3 lore raw

Hi, Jiri

Quoted 12 lines · Jiri Slaby

Quoted 4 lines · xinhui.pan
If gsmld_attach_gsm fails, the gsm is not used anymore. tty core will not call gsmld_close to do the cleanup work. tty core just restore to the tty old ldisc.
That always causes memory leak.
Nice catch!
Quoted 4 lines
drivers/tty/n_gsm.c +0 -0
@@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
 

Quoted 12 lines

drivers/tty/n_gsm.c static int gsmld_open(struct tty_struct *tty) · line 2390
Quoted 8 lines
drivers/tty/n_gsm.c +5 -1
 	/* Attach the initial passive connection */
 	gsm->encoding = 1;
-	return gsmld_attach_gsm(tty, gsm);
+
+	ret = gsmld_attach_gsm(tty, gsm);
+	if (ret != 0) {
+		gsm_cleanup_mux(gsm);
+		mux_put(gsm);
It is quite illogical to put the mux here. It should be in gsmld_open. I.e. gsm_cleanup_mux here, mux_put there.

Thanks for your reply :)
But I am a little confused with your comments, could you explain it when you are free? Sorry for my poor English.

thanks,

xinhui

Quoted 2 lines

thanks,

xinhui.pan

author

2014-07-28 11:32 UTC #4 lore raw

Quoted 28 lines · xinhui.pan

Hi, Jiri
Quoted 12 lines · Jiri Slaby
Quoted 4 lines · xinhui.pan
If gsmld_attach_gsm fails, the gsm is not used anymore. tty core will not call gsmld_close to do the cleanup work. tty core just restore to the tty old ldisc.
That always causes memory leak.
Nice catch!
Quoted 4 lines
drivers/tty/n_gsm.c +0 -0
@@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
 
Quoted 12 lines
drivers/tty/n_gsm.c static int gsmld_open(struct tty_struct *tty) · line 2390
Quoted 8 lines
drivers/tty/n_gsm.c +5 -1
 	/* Attach the initial passive connection */
 	gsm->encoding = 1;
-	return gsmld_attach_gsm(tty, gsm);
+
+	ret = gsmld_attach_gsm(tty, gsm);
+	if (ret != 0) {
+		gsm_cleanup_mux(gsm);
+		mux_put(gsm);
It is quite illogical to put the mux here. It should be in gsmld_open. I.e. gsm_cleanup_mux here, mux_put there.

Quoted 4 lines

Thanks for your reply :)
But I am a little confused with your comments, could you explain it when you are free? Sorry for my poor English.

hi, Jiri
I guess you want gsm_cleanup_mux() called in gsmld_attach_gsm(), just after gsm_activate_mux() fails?
Yes, that seems really make sence. :)
Thanks for pointing out it.

Let me explain my opinion. :)
Actually gsmld_attach_gsm results in two different effects when it fails.
(a)gsmld_attach_gsm -> gsm_activate_mux(gsm_mux[] is full,-EBUSY), then gsm_mux[] did not change, and gsm->num is invaild; (b)gsmld_attach_gsm -> gsm_activate_mux(return -ENOMEM), then gsm_mux[] changes, and gsm->num is vaild.

To be honest, I am not very clear about this. I even suspect this is done in such way on purpose. So I just keep the code what it is now. Let's handle the error in gsmld_open(). We can make sure that gsm is not used here and it's safe to cleanup and put it.

thanks,

xinhui

Quoted 6 lines

thanks,
xinhui
Quoted 2 lines
thanks,

Jiri Slaby

2014-07-28 12:11 UTC #5 lore raw

Quoted 29 lines · xinhui.pan

Quoted 28 lines · xinhui.pan
Hi, Jiri
Quoted 24 lines · Jiri Slaby
Quoted 4 lines · xinhui.pan
If gsmld_attach_gsm fails, the gsm is not used anymore. tty core will not call gsmld_close to do the cleanup work. tty core just restore to the tty old ldisc.
That always causes memory leak.
Nice catch!
drivers/tty/n_gsm.c static int gsmld_open(struct tty_struct *tty) · line 2390
Quoted 12 lines
drivers/tty/n_gsm.c +5 -1
@@ -2382,7 +2383,13 @@ static int gsmld_open(struct tty_struct *tty)
 
 	/* Attach the initial passive connection */
 	gsm->encoding = 1;
-	return gsmld_attach_gsm(tty, gsm);
+
+	ret = gsmld_attach_gsm(tty, gsm);
+	if (ret != 0) {
+		gsm_cleanup_mux(gsm);
+		mux_put(gsm);
It is quite illogical to put the mux here. It should be in gsmld_open. I.e. gsm_cleanup_mux here, mux_put there.

Quoted 8 lines

Quoted 4 lines
Thanks for your reply :)
But I am a little confused with your comments, could you explain it when you are free? Sorry for my poor English.
hi, Jiri
I guess you want gsm_cleanup_mux() called in gsmld_attach_gsm(), just after gsm_activate_mux() fails?
Yes, that seems really make sence. :)

Oh, I don't know what made me think you are changing gsmld_attach_gsm. I misread the patch.

So in the end, your patch looks fine to me.

thanks,

Signature · 2 lines

js
suse labs

Peter Hurley

2014-07-28 19:31 UTC #6 lore raw

In reply to xinhui.pan

Quoted 7 lines · xinhui.pan

If gsmld_attach_gsm fails, the gsm is not used anymore. tty core will not call gsmld_close to do the cleanup work. tty core just restore to the tty old ldisc.
That always causes memory leak.
Signed-off-by xinhui.pan <[email protected]>
Reported-by Peter Hurley <[email protected]>

Looks good, thanks!