Hi Gustavo,
On 21 August 2012 19:50, Gustavo Padovan <gustavo@xxxxxxxxxxx> wrote:
> Hi Dean,
>
> * Dean Jenkins <djenkins@xxxxxxxxxx> [2012-08-11 19:47:10 +0100]:
>
>> A race condition exists between near simultaneous asynchronous
>> DLC data channel disconnection requests from the host and remote device.
>> This causes the socket layer to request a socket shutdown at the same time
>> the rfcomm core is processing the disconnect request from the remote
>> device.
>>
>> The socket layer retains a copy of a struct rfcomm_dlc d pointer.
>> The d pointer refers to a copy of a struct rfcomm_session.
>> When the socket layer thread performs a socket shutdown, the thread
>> may wait on a rfcomm lock in rfcomm_dlc_close(). This means that
>> whilst the thread waits, the rfcomm_session and/or rfcomm_dlc structures
>> pointed to by d maybe freed due to rfcomm core handling. Consequently,
>> when the rfcomm lock becomes available and the thread runs, a
>> malfunction could occur as a freed rfcomm_session pointer and/or a
>> freed d pointer will be erroneously reused.
>>
>> Therefore, after the rfcomm lock is acquired, check that the struct
>> rfcomm_session is still valid by searching the rfcomm session list.
>> If the session is valid then validate the d pointer by searching the
>> rfcomm session list of active DLCs for the rfcomm_dlc structure
>> pointed by d.
>>
>> If active DLCs exist when the rfcomm session is terminating,
>> avoid a memory leak of rfcomm_dlc structures by ensuring that
>> rfcomm_session_close() is used instead of rfcomm_session_del().
>>
>> Signed-off-by: Dean Jenkins <djenkins@xxxxxxxxxx>
>> ---
>> net/bluetooth/rfcomm/core.c | 29 ++++++++++++++++++++++++++---
>> 1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
>> index c7921fd..1a7db34 100644
>> --- a/net/bluetooth/rfcomm/core.c
>> +++ b/net/bluetooth/rfcomm/core.c
>> @@ -496,11 +496,34 @@ static int __rfcomm_dlc_close(struct rfcomm_dlc *d, int err)
>>
>> int rfcomm_dlc_close(struct rfcomm_dlc *d, int err)
>> {
>> - int r;
>> + int r = 0;
>> + struct rfcomm_dlc *d_list;
>> + struct rfcomm_session *s, *s_list;
>> + struct list_head *p, *n, *p2, *n2;
>>
>> rfcomm_lock();
>>
>> - r = __rfcomm_dlc_close(d, err);
>> + s = d->session;
>> + if (s) {
>
> Please invert the check here, and add a goto unlock;. There too many
> indentation levels here.
>
>> + /* check the session still exists after waiting on the mutex */
>> + list_for_each_safe(p, n, &session_list) {
>> + s_list = list_entry(p, struct rfcomm_session, list);
>
> please use list_for_each_entry(), the _safe version seems to not be needed.
>
>> + if (s == s_list) {
>> + /* check the dlc still exists */
>> + /* after waiting on the mutex */
>> + list_for_each_safe(p2, n2, &s->dlcs) {
>> + d_list = list_entry(p2,
>> + struct rfcomm_dlc,
>> + list);
>
>
> and here you can use rfcomm_dlc_get()
>
> Gustavo
Thanks for your feedback. Sorry for the delay getting back to you. I
blame gmail.
OK, I will take a look at redoing the patch as per your suggestion.
Thanks,
Regards,
Dean
--
Dean Jenkins
Embedded Software Engineer
Professional Services UK/EMEA
MontaVista Software, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
