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

[Jiri Slaby <jslaby@suse.cz>]

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