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

[Pan Xinhui <xinhuix.pan@intel.com>]

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@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.
>

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,
>