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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <551220B4.3050507@intel.com>
Date:	Wed, 25 Mar 2015 10:43:00 +0800
From:	Pan Xinhui <xinhuix.pan@...el.com>
To:	Jiri Slaby <jslaby@...e.cz>, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org
CC:	mnipxh@...il.com, yanmin_zhang@...ux.intel.com
Subject: Re: [PATCH] tty/n_gsm.c: fix a memory leak when gsmtty is removed

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