* [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open @ 2014-07-28 8:14 xinhui.pan 2014-07-28 8:23 ` xinhui.pan ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: xinhui.pan @ 2014-07-28 8:14 UTC (permalink / raw) To: Greg KH, Jiri Slaby; +Cc: Zhang, Yanmin, Peter Hurley, mnipxh, linux-kernel 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 <xinhuiX.pan@intel.com> Reported-by: Peter Hurley <peter@hurleysoftware.com> --- drivers/tty/n_gsm.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index 81e7ccb..6cb1a6d 100644 --- a/drivers/tty/n_gsm.c +++ b/drivers/tty/n_gsm.c @@ -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; } /** -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open 2014-07-28 8:14 [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open xinhui.pan @ 2014-07-28 8:23 ` xinhui.pan 2014-07-28 8:49 ` Jiri Slaby 2014-07-28 19:30 ` Peter Hurley 2 siblings, 0 replies; 7+ messages in thread From: xinhui.pan @ 2014-07-28 8:23 UTC (permalink / raw) To: Greg KH, Jiri Slaby; +Cc: Zhang, Yanmin, Peter Hurley, mnipxh, linux-kernel 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 于 2014年07月28日 16:14, 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 <xinhuiX.pan@intel.com> > Reported-by: Peter Hurley <peter@hurleysoftware.com> > --- > drivers/tty/n_gsm.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c > index 81e7ccb..6cb1a6d 100644 > --- a/drivers/tty/n_gsm.c > +++ b/drivers/tty/n_gsm.c > @@ -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; > } > > /** > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open 2014-07-28 8:14 [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open xinhui.pan 2014-07-28 8:23 ` xinhui.pan @ 2014-07-28 8:49 ` Jiri Slaby 2014-07-28 9:03 ` xinhui.pan 2014-07-28 19:30 ` Peter Hurley 2 siblings, 1 reply; 7+ messages in thread From: Jiri Slaby @ 2014-07-28 8:49 UTC (permalink / raw) To: xinhui.pan, Greg KH; +Cc: Zhang, Yanmin, Peter Hurley, mnipxh, linux-kernel On 07/28/2014 10:14 AM, xinhui.pan wrote: > 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! > --- a/drivers/tty/n_gsm.c > +++ b/drivers/tty/n_gsm.c > @@ -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, -- js suse labs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open 2014-07-28 8:49 ` Jiri Slaby @ 2014-07-28 9:03 ` xinhui.pan 2014-07-28 11:32 ` xinhui.pan 0 siblings, 1 reply; 7+ messages in thread From: xinhui.pan @ 2014-07-28 9:03 UTC (permalink / raw) To: Jiri Slaby, Greg KH; +Cc: Zhang, Yanmin, Peter Hurley, mnipxh, linux-kernel Hi, Jiri 于 2014年07月28日 16:49, Jiri Slaby 写道: > On 07/28/2014 10:14 AM, xinhui.pan wrote: >> 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! > >> --- a/drivers/tty/n_gsm.c >> +++ b/drivers/tty/n_gsm.c >> @@ -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 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 > thanks, > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open 2014-07-28 9:03 ` xinhui.pan @ 2014-07-28 11:32 ` xinhui.pan 2014-07-28 12:11 ` Jiri Slaby 0 siblings, 1 reply; 7+ messages in thread From: xinhui.pan @ 2014-07-28 11:32 UTC (permalink / raw) To: Jiri Slaby, Greg KH; +Cc: Zhang, Yanmin, Peter Hurley, mnipxh, linux-kernel 于 2014年07月28日 17:03, xinhui.pan 写道: > Hi, Jiri > > 于 2014年07月28日 16:49, Jiri Slaby 写道: >> On 07/28/2014 10:14 AM, xinhui.pan wrote: >>> 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! >> >>> --- a/drivers/tty/n_gsm.c >>> +++ b/drivers/tty/n_gsm.c >>> @@ -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 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 > thanks, > > xinhui > >> thanks, >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open 2014-07-28 11:32 ` xinhui.pan @ 2014-07-28 12:11 ` Jiri Slaby 0 siblings, 0 replies; 7+ messages in thread From: Jiri Slaby @ 2014-07-28 12:11 UTC (permalink / raw) To: xinhui.pan, Greg KH; +Cc: Zhang, Yanmin, Peter Hurley, mnipxh, linux-kernel On 07/28/2014 01:32 PM, xinhui.pan wrote: > 于 2014年07月28日 17:03, xinhui.pan 写道: >> Hi, Jiri >> >> 于 2014年07月28日 16:49, Jiri Slaby 写道: >>> On 07/28/2014 10:14 AM, xinhui.pan wrote: >>>> 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! >>> >>>> --- a/drivers/tty/n_gsm.c >>>> +++ b/drivers/tty/n_gsm.c >>>> @@ -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 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, -- js suse labs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open 2014-07-28 8:14 [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open xinhui.pan 2014-07-28 8:23 ` xinhui.pan 2014-07-28 8:49 ` Jiri Slaby @ 2014-07-28 19:30 ` Peter Hurley 2 siblings, 0 replies; 7+ messages in thread From: Peter Hurley @ 2014-07-28 19:30 UTC (permalink / raw) To: xinhui.pan; +Cc: Greg KH, Jiri Slaby, Zhang, Yanmin, mnipxh, linux-kernel On 07/28/2014 04:14 AM, xinhui.pan wrote: > 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 <xinhuiX.pan@intel.com> > Reported-by: Peter Hurley <peter@hurleysoftware.com> Looks good, thanks! ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-07-28 19:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-07-28 8:14 [PATCH] tty/n_gsm.c: fix a memory leak in gsmld_open xinhui.pan 2014-07-28 8:23 ` xinhui.pan 2014-07-28 8:49 ` Jiri Slaby 2014-07-28 9:03 ` xinhui.pan 2014-07-28 11:32 ` xinhui.pan 2014-07-28 12:11 ` Jiri Slaby 2014-07-28 19:30 ` Peter Hurley
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.