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@xxxxxxxxx>
> > ---
> >   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,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/